DaddyFrosty/yoga
1
0
Fork 0
Code Issues 96 Pull Requests 31 Actions Packages Projects Releases 33 Wiki Activity

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

Closed
intergalacticspacehighway wants to merge 3 commits from fix/remove-gap-last-element into main
pull from: fix/remove-gap-last-element
merge into: DaddyFrosty:main
Conversation 15 Commits 3 Files Changed 1 +5
1 changed files with 5 additions and 0 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
yoga/Yoga.cpp
View File

@@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(
const YGNodeRef child = node->getChild(i);
jacobp100 commented 2022-12-09 03:35:47 -08:00 (Migrated from github.com)
Review
Copy Link

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 commented 2022-12-09 03:36:11 -08:00 (Migrated from github.com)
Review
Copy Link

@NickGerleman does that make sense to you?

@NickGerleman does that make sense to you?
intergalacticspacehighway commented 2022-12-09 05:48:56 -08:00 (Migrated from github.com)
Review
Copy Link

@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 commented 2022-12-09 06:20:41 -08:00 (Migrated from github.com)
Review
Copy Link

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 commented 2022-12-09 06:48:45 -08:00 (Migrated from github.com)
Review
Copy Link

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?

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 commented 2022-12-09 07:25:47 -08:00 (Migrated from github.com)
Review
Copy Link

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 07:21:26 -08:00 (Migrated from github.com)
Review
Copy Link

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.

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.
jacobp100 commented 2022-12-13 07:40:23 -08:00 (Migrated from github.com)
Review
Copy Link

@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 😅
const YGStyle& childStyle = child->getStyle();
const YGLayout childLayout = child->getLayout();
const bool isLastChild = i == collectedFlexItemsValues.endOfLineIndex - 1;
// remove the gap if it is the last element of the line
if (isLastChild) {
betweenMainDim -= gap;
}
if (childStyle.display() == YGDisplayNone) {
continue;
}