fix: remove gap if its last element in line #1188

2022-12-08T18:42:46-08:00

intergalacticspacehighway commented

2022-12-08 18:42:46 -08:00

(Migrated from github.com)

Fixes - https://github.com/facebook/react-native/issues/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! 🙏

Fixes - https://github.com/facebook/react-native/issues/35553 ## Approach We're using `betweenMainDim` to add [gap between](https://github.com/intergalacticspacehighway/yoga/blob/bbeede82d36cd89657faf385aaa704f45443c326/yoga/Yoga.cpp#L2495) items in main axis. This is resulting in increased [main axis](https://github.com/intergalacticspacehighway/yoga/blob/bbeede82d36cd89657faf385aaa704f45443c326/yoga/Yoga.cpp#L2598) 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! 🙏

🎉 1

jacobp100 (Migrated from github.com) reviewed 2022-12-09 03:35:47 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

jacobp100 (Migrated from github.com) commented

2022-12-09 03:35:47 -08:00

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:-

60dd791dd8/yoga/Yoga.cpp (L2593-L2601)

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

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:- https://github.com/facebook/yoga/blob/60dd791dd8d56e0fdb978ac3357030c200a2c130/yoga/Yoga.cpp#L2593-L2601 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 (Migrated from github.com) reviewed 2022-12-09 03:36:12 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

jacobp100 (Migrated from github.com) commented

2022-12-09 03:36:11 -08:00

@NickGerleman does that make sense to you?

inobelar commented

2022-12-09 04:10:56 -08:00

(Migrated from github.com)

@intergalacticspacehighway or @NickGerleman Can you please also add test case for this scenario (mentioned here https://github.com/facebook/react-native/issues/35553#issuecomment-1342582700 ) ?

intergalacticspacehighway (Migrated from github.com) reviewed 2022-12-09 05:48:56 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

intergalacticspacehighway (Migrated from github.com) commented

2022-12-09 05:48:56 -08:00

@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 that makes sense to me! But `collectedFlexItemsValues.mainDim` is also used to layout child items [here](https://github.com/facebook/yoga/blob/60dd791dd8d56e0fdb978ac3357030c200a2c130/yoga/Yoga.cpp#L2575). 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 (Migrated from github.com) reviewed 2022-12-09 06:20:41 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

jacobp100 (Migrated from github.com) commented

2022-12-09 06:20:41 -08:00

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>

Ah yeah - that makes sense. Maybe [here](https://github.com/facebook/yoga/blob/60dd791dd8d56e0fdb978ac3357030c200a2c130/yoga/Yoga.cpp#L2574) (before the child is moved) we could add:- ```c if (i != 0) { collectedFlexItemsValues.mainDim += betweenMainDim; } ``` Then remove that from the `mainDim` calculations below [There's an issue](https://github.com/facebook/yoga/pull/1014) 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 (Migrated from github.com) reviewed 2022-12-09 06:48:45 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

intergalacticspacehighway (Migrated from github.com) commented

2022-12-09 06:48:45 -08:00

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 (Migrated from github.com) reviewed 2022-12-09 07:25:48 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

jacobp100 (Migrated from github.com) commented

2022-12-09 07:25:47 -08:00

Just added https://github.com/facebook/yoga/pull/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

Just added https://github.com/facebook/yoga/pull/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 commented

2022-12-13 06:07:55 -08:00

(Migrated from github.com)

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 (Migrated from github.com) reviewed 2022-12-13 07:21:27 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

NickGerleman (Migrated from github.com) commented

2022-12-13 07:21:26 -08:00

Reading through this, is this a correct summary?

betweenMainDim controls space between items (both gap, and justifications which add space between children)
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
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.

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: ```cpp // 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.

NickGerleman commented

2022-12-13 07:22:42 -08:00

(Migrated from github.com)

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).

> 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 commented

2022-12-13 07:36:10 -08:00

(Migrated from github.com)

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

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)
Merge the fix which also covers existing behavior once we're in the new year (e.g. #1189)
1. Though I think YGJustifyCenter would still be wrong after

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. #1189) 1. Though I think `YGJustifyCenter` would still be wrong after

👍 2

jacobp100 (Migrated from github.com) reviewed 2022-12-13 07:40:24 -08:00

yoga/Yoga.cpp

						
				@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(

				    const YGNodeRef child = node->getChild(i);

jacobp100 (Migrated from github.com) commented

2022-12-13 07:40:23 -08:00

@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 😅

@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 `betweenMainDim`s, 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 😅

NickGerleman commented

2022-12-15 11:49:58 -08:00

(Migrated from github.com)

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 commented

2022-12-15 11:50:22 -08:00

(Migrated from github.com)

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

@NickGerleman has imported this pull request. If you are a Meta employee, you can view this diff [on Phabricator](https://www.internalfb.com/diff/D42078162).

facebook-github-bot commented

2022-12-15 16:33:12 -08:00

(Migrated from github.com)

@NickGerleman merged this pull request in facebook/yoga@ba27f9d1ec.

@NickGerleman merged this pull request in facebook/yoga@ba27f9d1ecfa7518019845b84b035d3d4a2a6658.

Pull request closed

This pull request cannot be reopened because the branch was deleted.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: DaddyFrosty/yoga#1188

				`@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(`
				`const YGNodeRef child = node->getChild(i);`