fix: remove gap if its last element in line

[intergalacticspacehighway]

Fixes - facebook/react-native#35553

Approach

We're using betweenMainDim to add gap between items in main axis. This is resulting in increased main axis dimension of the container as it gets added even for the last element. One solution is to keep using it and subtract the gap when last element is reached.

Aside

Mutating this value feels weird, but I think betweenMainDim gets initialized for every line so should be fine? I did some manual tests to verify. I tried running tests but I'll have to downgrade the java version. Let me know if anything fails. Thanks! 🙏

[jacobp100]

This is a good catch! I was looking for ages what the culprit might be.

I'm still trying to fully grok this step - but my feeling is the betweenMainDim should never be applied on the first child. I.e. on these lines:-

yoga/yoga/Yoga.cpp

Lines 2593 to 2601 in 60dd791

	collectedFlexItemsValues.mainDim += betweenMainDim +
	child->getMarginForAxis(mainAxis, availableInnerWidth).unwrap() +
	childLayout.computedFlexBasis.unwrap();
	collectedFlexItemsValues.crossDim = availableInnerCrossDim;
	} else {
	// The main dimension is the sum of all the elements dimension plus
	// the spacing.
	collectedFlexItemsValues.mainDim += betweenMainDim +
	YGNodeDimWithMargin(child, mainAxis, availableInnerWidth);

I think there was always a bug here, and collectedFlexItemsValues.mainDim would be too large in the case any sort of justification. It would include one more betweenMainDim than it should have.

However, I think we got away with it because either justification is only going to do something (i.e. set betweenMainDim to something bigger than zero) when the container has its size defined - and when its size is defined, we probably don't use collectedFlexItemsValues.mainDim.

So in the case you did justify-content: space-between and the container only gets sized by its contents, the children wouldn't be justified, and betweenMainDim would be zero.

In the case it had an explicit size and betweenMainDim was not zero, the explicit size was used instead, so it never really mattered that the mainDim was too large.

Maybe we could do const float appliedBetweenMainDim = i != 0 ? betweenMainDim : 0

[jacobp100]

@NickGerleman does that make sense to you?

[intergalacticspacehighway]

@jacobp100 that makes sense to me!
But collectedFlexItemsValues.mainDim is also used to layout child items here. If we don't add betweenMainDim for the first item, there won't be a gap between first and second child. Also, verified by testing it.
I am trying to run tests but getting below error on running buck test //:yoga
No build file at BUCK when resolving target //:yoga.

[jacobp100]

Ah yeah - that makes sense. Maybe here (before the child is moved) we could add:-

if (i != 0) {
  collectedFlexItemsValues.mainDim += betweenMainDim;
}

Then remove that from the mainDim calculations below

There's an issue around this code where auto margins turn negative. But assuming the fix gets merged, we need to be careful that if you have a gap and an auto margin, the gap is still applied. I.e. these two views should never be less than 10px apart

<View style={{ flexDirection: 'row', gap: 10 }}>
  <View style={{ marginRight: 'auto' }} />
  <View />
</View>

[intergalacticspacehighway]

okay, yeah. Just wanted to try a simple fix without touching too many things 😅. Should be easier to make changes once we have tests running. Do you think there's an issue with the approach of removing the gap for the last item?

[jacobp100]

Just added #1189 - I really just wanna see what happens after running on CI

Did the stuff I said about the how we were always over-shooting the mainDim make sense? Assuming it doesn't cause extra issues to fix, it'd be nice to fix so we're less likely to see regressions later

[NickGerleman]

Reading through this, is this a correct summary?

1. betweenMainDim controls space between items (both gap, and justifications which add space between children)
2. We iterate through the children in the line, adding its dimension and gap to the line dimensions (then padding/border at the end)
   1. This is wrong with gap, since it will add extra trailing dimension
   2. We think the same issue might apply to justification adding extra to line dimensions (it adds for all children), but there is only free space when the parent has a defined size, in which case we may not use the calculated dimension to begin with
3. So, the question is whether to change the summing logic specific to gap, or to also apply to the existing justification code as well

We can run through an example, where a container has size 100px, we have two children of size 25px, no gap, and YGJustifySpaceEvenly. We would expect spacing to take a total of 50px extra in total then. Per-item, we would add leadingMainDim and betweenMainDim of the following:

        // leadingMainDim = 50 / (2 + 1) = 16.67
        leadingMainDim = collectedFlexItemsValues.remainingFreeSpace /
            (collectedFlexItemsValues.itemsOnLine + 1);
        // betweenMainDim = 16.67
        betweenMainDim += leadingMainDim;
        // combined: 33.33 px/item (66.66px of spacing total)

So I am seeing the same issue where we are over-counting betweenMainDim during justification.

Separately, if we have YGJustifyCenter, we undercount by quite a bit since we only sum the leading gap, but never the trailing gap.

In the block @intergalacticspacehighway mentions, we position the item based on the accumulated offset, and the leading space for the current item. So, positioning the second item correctly would assume betweenMainDim, has already been added. So I agree that omitting it for the first child does not seem to be correct. edit: I see, we are adding to the main dim before positioning in the proposed change.

I think fixing up this logic to be more consistently correct would be worth doing, but also I think it might be a little risky unless we can super-conclusively determine the incorrect collectedFlexItemsValues.mainDim would never come into place during justification. And given the current timing around the holidays, I would be weary to make that sort of change at this specific moment.

So, I think instead, we should stick with the fix @intergalacticspacehighway has for now of not adding gap to betweenMainDim when we are the last item. Though, as a matter of style, I agree with the note in the description that mutating the value is a bit unclear. So I think it would be better to instead have separate variables for justification gap, and gap derived gap, so that we could declare betweenMainDim once in the inner scope.

[jacobp100]

@NickGerleman My understanding is:-

Previously, betweenMainDim acted as a gap, but was only used for justification purposes

We were adding we were adding a total betweenMainDim * children.length to mainDim - but since this is a distance between children, we should add a total betweenMainDim * (children.length - 1).

I believe this was never noticed this bug because for justification to do anything - meaning betweenMainDim != 0 - the container needs an explicit main axis size. So even though mainDim was too big, it was just ignored

When the container did not have an explicit axis size, betweenMainDim was 0, so adding one too many betweenMainDims, did not have an effect

The gap property leveraged betweenMainDim, so now in the case that a container does not have an explicit axis size, it's possible for betweenMainDim to be greater than zero, and suddenly it does matter

I think the real fix is to not include betweenMainDim for the last child at all - not just subtract the gap portion of betweenMainDim for the last child

Hope that makes sense 😅

[inobelar]

@intergalacticspacehighway or @NickGerleman Can you please also add test case for this scenario (mentioned here facebook/react-native#35553 (comment) ) ?

[NickGerleman]

Sorry for the delays here. I will look through this today. Though +1 for adding more fixtures which cover this scenario. The test generator should work in OSS (just... not the running the UTs part).

[NickGerleman]

I tried running tests but I'll have to downgrade the java version. Let me know if anything fails.

The UTs are broken in OSS. The BUCK logic for the Java ones kept on breaking, and Buck in OSS in general hasn't gone well. But this week I will be adding a way to run the C++ UTs via the CMake/GTest build which should hopefully be relatively pain-free (and would test the same things, minus the JNI bindings).

[NickGerleman]

Okay, I left a giant dump of information in the one of the threads, but plan of action-wise, I would propose:

1. Merge the less invasive change now which only impacts gap
   1. Let's add some fixtures too for the case of a parent with size based on children (all the current ones seem to be a parent of fixed dimensions)
2. Merge the fix which also covers existing behavior once we're in the new year (e.g. Update Yoga.cpp #1189)
   1. Though I think YGJustifyCenter would still be wrong after

[NickGerleman]

Going to import this and see if I can add some tests, then later today I will work on exposing the GTest UTs to OSS.

[facebook-github-bot]

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

[facebook-github-bot]

@NickGerleman merged this pull request in ba27f9d.