Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from
Closed

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

Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbeede8
fix: remove gap if it's last element in line
intergalacticspacehighway Dec 9, 2022
60dd791
nit
intergalacticspacehighway Dec 9, 2022
97be2ce
nit: clang format
intergalacticspacehighway Dec 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions yoga/Yoga.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2540,6 +2540,11 @@ static void YGJustifyMainAxis(
const YGNodeRef child = node->getChild(i);
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;
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@NickGerleman does that make sense to you?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link

@jacobp100 jacobp100 Dec 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@NickGerleman NickGerleman Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

@jacobp100 jacobp100 Dec 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

if (childStyle.display() == YGDisplayNone) {
continue;
}
Expand Down