Skip to content

Commit

Permalink
Fixup hack for flex line size calculation (facebook#39433)
Browse files Browse the repository at this point in the history
Summary:
X-link: facebook/yoga#1380


Back when rolling out flex gap, we encountered a bug where gap was added to the end of the main axis when a size was not specified.

During flex line justification/sizing, we calculate the amount of space that should be in between children. We erroneously add this, even after the last child element.

For `justify-content`, this space between children is derived from free space along the axis. The only time we have free space is if we had a dimension/dimension constraint already set on the parent. In this case, the extra space added to the end of the flex line is never noticed, becasue we bound `maxLineMainDim` to container dimension constraints at the end of layout, and the error doesn't effect how any children are positioned or sized.

`betweenMainDim` may still be set for `gap` even if we don't have a sized parent, which makes the extra space propagated to `maxLineMainDim` effect parent size.

Because we were in a code freeze, I opted to have us go with a solution just effecting flex gap, instead of the right one, in case there were any side effects. This cleans up the code to use the right calculation everywhere, and fixes a separate bug, where `endOfLineIndex` and `startOfLineIndex` may not be the last/first in the line if they are out of the layout flow (absolutely positioned, or display: none_

See the original conversation on facebook/yoga#1188

Reviewed By: javache

Differential Revision: D49260049
  • Loading branch information
NickGerleman authored and facebook-github-bot committed Sep 14, 2023
1 parent 3e7a873 commit 4c4425a
Showing 1 changed file with 6 additions and 7 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -1306,11 +1306,6 @@ static void YGJustifyMainAxis(
const auto child = node->getChild(i);
const Style& childStyle = child->getStyle();
const LayoutResults& childLayout = child->getLayout();
const bool isLastChild = i == flexLine.endOfLineIndex - 1;
// remove the gap if it is the last element of the line
if (isLastChild) {
betweenMainDim -= gap;
}
if (childStyle.display() == YGDisplayNone) {
continue;
}
Expand Down Expand Up @@ -1344,6 +1339,10 @@ static void YGJustifyMainAxis(
leadingEdge(mainAxis));
}

if (child != flexLine.itemsInFlow.back()) {
flexLine.layout.mainDim += betweenMainDim;
}

if (child->marginTrailingValue(mainAxis).unit == YGUnitAuto) {
flexLine.layout.mainDim += flexLine.layout.remainingFreeSpace /
static_cast<float>(numberOfAutoMarginsOnCurrentLine);
Expand All @@ -1354,14 +1353,14 @@ static void YGJustifyMainAxis(
// If we skipped the flex step, then we can't rely on the measuredDims
// because they weren't computed. This means we can't call
// dimensionWithMargin.
flexLine.layout.mainDim += betweenMainDim +
flexLine.layout.mainDim +=
child->getMarginForAxis(mainAxis, availableInnerWidth).unwrap() +
childLayout.computedFlexBasis.unwrap();
flexLine.layout.crossDim = availableInnerCrossDim;
} else {
// The main dimension is the sum of all the elements dimension plus
// the spacing.
flexLine.layout.mainDim += betweenMainDim +
flexLine.layout.mainDim +=
dimensionWithMargin(child, mainAxis, availableInnerWidth);

if (isNodeBaselineLayout) {
Expand Down

0 comments on commit 4c4425a

Please sign in to comment.