Skip to content

Commit

Permalink
Fix issue where % width would be wrong if physical and relative paddi…
Browse files Browse the repository at this point in the history
…ng defined on parent

Summary:
This should fix facebook/yoga#1657. Rather insidious bug but we had code like

```
  // The total padding/border for a given axis does not depend on the direction
  // so hardcoding LTR here to avoid piping direction to this function
  return node->style().computeInlineStartPaddingAndBorder(
             axis, Direction::LTR, widthSize) +
      node->style().computeInlineEndPaddingAndBorder(
          axis, Direction::LTR, widthSize);
```

That comment is NOT true if someone sets both the physical edge and relative edge. So like paddingLeft and paddingEnd for RTL. This diff simply pipes the direction to that spot to use instead of hardcoding LTR. Every file changed is just to pipe `direction`.

Differential Revision: D58169843
  • Loading branch information
joevilches authored and facebook-github-bot committed Jun 5, 2024
1 parent 2d9e54b commit 00c4a3f
Show file tree
Hide file tree
Showing 3 changed files with 68 additions and 20 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -342,6 +342,7 @@ void layoutAbsoluteChild(
childWidth = boundAxis(
child,
FlexDirection::Row,
direction,
childWidth,
containingBlockWidth,
containingBlockWidth);
Expand Down Expand Up @@ -373,6 +374,7 @@ void layoutAbsoluteChild(
childHeight = boundAxis(
child,
FlexDirection::Column,
direction,
childHeight,
containingBlockHeight,
containingBlockWidth);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,12 @@ namespace facebook::yoga {
inline float paddingAndBorderForAxis(
const yoga::Node* const node,
const FlexDirection axis,
const Direction direction,
const float widthSize) {
// The total padding/border for a given axis does not depend on the direction
// so hardcoding LTR here to avoid piping direction to this function
return node->style().computeInlineStartPaddingAndBorder(
axis, Direction::LTR, widthSize) +
axis, direction, widthSize) +
node->style().computeInlineEndPaddingAndBorder(
axis, Direction::LTR, widthSize);
axis, direction, widthSize);
}

inline FloatOptional boundAxisWithinMinAndMax(
Expand Down Expand Up @@ -60,13 +59,14 @@ inline FloatOptional boundAxisWithinMinAndMax(
inline float boundAxis(
const yoga::Node* const node,
const FlexDirection axis,
const Direction direction,
const float value,
const float axisSize,
const float widthSize) {
return yoga::maxOrDefined(
boundAxisWithinMinAndMax(node, axis, FloatOptional{value}, axisSize)
.unwrap(),
paddingAndBorderForAxis(node, axis, widthSize));
paddingAndBorderForAxis(node, axis, direction, widthSize));
}

} // namespace facebook::yoga
Original file line number Diff line number Diff line change
Expand Up @@ -97,23 +97,25 @@ static void computeFlexBasisForChild(
(child->getConfig()->isExperimentalFeatureEnabled(
ExperimentalFeature::WebFlexBasis) &&
child->getLayout().computedFlexBasisGeneration != generationCount)) {
const FloatOptional paddingAndBorder =
FloatOptional(paddingAndBorderForAxis(child, mainAxis, ownerWidth));
const FloatOptional paddingAndBorder = FloatOptional(
paddingAndBorderForAxis(child, mainAxis, direction, ownerWidth));
child->setLayoutComputedFlexBasis(
yoga::maxOrDefined(resolvedFlexBasis, paddingAndBorder));
}
} else if (isMainAxisRow && isRowStyleDimDefined) {
// The width is definite, so use that as the flex basis.
const FloatOptional paddingAndBorder = FloatOptional(
paddingAndBorderForAxis(child, FlexDirection::Row, ownerWidth));
const FloatOptional paddingAndBorder =
FloatOptional(paddingAndBorderForAxis(
child, FlexDirection::Row, direction, ownerWidth));

child->setLayoutComputedFlexBasis(yoga::maxOrDefined(
child->getResolvedDimension(Dimension::Width).resolve(ownerWidth),
paddingAndBorder));
} else if (!isMainAxisRow && isColumnStyleDimDefined) {
// The height is definite, so use that as the flex basis.
const FloatOptional paddingAndBorder = FloatOptional(
paddingAndBorderForAxis(child, FlexDirection::Column, ownerWidth));
const FloatOptional paddingAndBorder =
FloatOptional(paddingAndBorderForAxis(
child, FlexDirection::Column, direction, ownerWidth));
child->setLayoutComputedFlexBasis(yoga::maxOrDefined(
child->getResolvedDimension(Dimension::Height).resolve(ownerHeight),
paddingAndBorder));
Expand Down Expand Up @@ -244,13 +246,14 @@ static void computeFlexBasisForChild(

child->setLayoutComputedFlexBasis(FloatOptional(yoga::maxOrDefined(
child->getLayout().measuredDimension(dimension(mainAxis)),
paddingAndBorderForAxis(child, mainAxis, ownerWidth))));
paddingAndBorderForAxis(child, mainAxis, direction, ownerWidth))));
}
child->setLayoutComputedFlexBasisGeneration(generationCount);
}

static void measureNodeWithMeasureFunc(
yoga::Node* const node,
const Direction direction,
float availableWidth,
float availableHeight,
const SizingMode widthSizingMode,
Expand Down Expand Up @@ -292,12 +295,18 @@ static void measureNodeWithMeasureFunc(
// Don't bother sizing the text if both dimensions are already defined.
node->setLayoutMeasuredDimension(
boundAxis(
node, FlexDirection::Row, availableWidth, ownerWidth, ownerWidth),
node,
FlexDirection::Row,
direction,
availableWidth,
ownerWidth,
ownerWidth),
Dimension::Width);
node->setLayoutMeasuredDimension(
boundAxis(
node,
FlexDirection::Column,
direction,
availableHeight,
ownerHeight,
ownerWidth),
Expand Down Expand Up @@ -330,6 +339,7 @@ static void measureNodeWithMeasureFunc(
boundAxis(
node,
FlexDirection::Row,
direction,
(widthSizingMode == SizingMode::MaxContent ||
widthSizingMode == SizingMode::FitContent)
? measuredSize.width + paddingAndBorderAxisRow
Expand All @@ -342,6 +352,7 @@ static void measureNodeWithMeasureFunc(
boundAxis(
node,
FlexDirection::Column,
direction,
(heightSizingMode == SizingMode::MaxContent ||
heightSizingMode == SizingMode::FitContent)
? measuredSize.height + paddingAndBorderAxisColumn
Expand All @@ -356,6 +367,7 @@ static void measureNodeWithMeasureFunc(
// or the minimum size as indicated by the padding and border sizes.
static void measureNodeWithoutChildren(
yoga::Node* const node,
const Direction direction,
const float availableWidth,
const float availableHeight,
const SizingMode widthSizingMode,
Expand All @@ -372,7 +384,8 @@ static void measureNodeWithoutChildren(
layout.border(PhysicalEdge::Left) + layout.border(PhysicalEdge::Right);
}
node->setLayoutMeasuredDimension(
boundAxis(node, FlexDirection::Row, width, ownerWidth, ownerWidth),
boundAxis(
node, FlexDirection::Row, direction, width, ownerWidth, ownerWidth),
Dimension::Width);

float height = availableHeight;
Expand All @@ -383,12 +396,19 @@ static void measureNodeWithoutChildren(
layout.border(PhysicalEdge::Top) + layout.border(PhysicalEdge::Bottom);
}
node->setLayoutMeasuredDimension(
boundAxis(node, FlexDirection::Column, height, ownerHeight, ownerWidth),
boundAxis(
node,
FlexDirection::Column,
direction,
height,
ownerHeight,
ownerWidth),
Dimension::Height);
}

static bool measureNodeWithFixedSize(
yoga::Node* const node,
const Direction direction,
const float availableWidth,
const float availableHeight,
const SizingMode widthSizingMode,
Expand All @@ -405,6 +425,7 @@ static bool measureNodeWithFixedSize(
boundAxis(
node,
FlexDirection::Row,
direction,
yoga::isUndefined(availableWidth) ||
(widthSizingMode == SizingMode::FitContent &&
availableWidth < 0.0f)
Expand All @@ -418,6 +439,7 @@ static bool measureNodeWithFixedSize(
boundAxis(
node,
FlexDirection::Column,
direction,
yoga::isUndefined(availableHeight) ||
(heightSizingMode == SizingMode::FitContent &&
availableHeight < 0.0f)
Expand Down Expand Up @@ -619,6 +641,7 @@ static float distributeFreeSpaceSecondPass(
updatedMainSize = boundAxis(
currentLineChild,
mainAxis,
direction,
childSize,
availableInnerMainDim,
availableInnerWidth);
Expand All @@ -633,6 +656,7 @@ static float distributeFreeSpaceSecondPass(
updatedMainSize = boundAxis(
currentLineChild,
mainAxis,
direction,
childFlexBasis +
flexLine.layout.remainingFreeSpace /
flexLine.layout.totalFlexGrowFactors * flexGrowFactor,
Expand Down Expand Up @@ -756,6 +780,7 @@ static float distributeFreeSpaceSecondPass(
// is removed from the remaingfreespace.
static void distributeFreeSpaceFirstPass(
FlexLine& flexLine,
const Direction direction,
const FlexDirection mainAxis,
const float mainAxisownerSize,
const float availableInnerMainDim,
Expand Down Expand Up @@ -788,6 +813,7 @@ static void distributeFreeSpaceFirstPass(
boundMainSize = boundAxis(
currentLineChild,
mainAxis,
direction,
baseMainSize,
availableInnerMainDim,
availableInnerWidth);
Expand Down Expand Up @@ -816,6 +842,7 @@ static void distributeFreeSpaceFirstPass(
boundMainSize = boundAxis(
currentLineChild,
mainAxis,
direction,
baseMainSize,
availableInnerMainDim,
availableInnerWidth);
Expand Down Expand Up @@ -878,6 +905,7 @@ static void resolveFlexibleLength(
// First pass: detect the flex items whose min/max constraints trigger
distributeFreeSpaceFirstPass(
flexLine,
direction,
mainAxis,
mainAxisownerSize,
availableInnerMainDim,
Expand Down Expand Up @@ -1261,6 +1289,7 @@ static void calculateLayoutImpl(
if (node->hasMeasureFunc()) {
measureNodeWithMeasureFunc(
node,
direction,
availableWidth - marginAxisRow,
availableHeight - marginAxisColumn,
widthSizingMode,
Expand All @@ -1276,6 +1305,7 @@ static void calculateLayoutImpl(
if (childCount == 0) {
measureNodeWithoutChildren(
node,
direction,
availableWidth - marginAxisRow,
availableHeight - marginAxisColumn,
widthSizingMode,
Expand All @@ -1290,6 +1320,7 @@ static void calculateLayoutImpl(
if (!performLayout &&
measureNodeWithFixedSize(
node,
direction,
availableWidth - marginAxisRow,
availableHeight - marginAxisColumn,
widthSizingMode,
Expand All @@ -1316,9 +1347,9 @@ static void calculateLayoutImpl(
const float crossAxisownerSize = isMainAxisRow ? ownerHeight : ownerWidth;

const float paddingAndBorderAxisMain =
paddingAndBorderForAxis(node, mainAxis, ownerWidth);
paddingAndBorderForAxis(node, mainAxis, direction, ownerWidth);
const float paddingAndBorderAxisCross =
paddingAndBorderForAxis(node, crossAxis, ownerWidth);
paddingAndBorderForAxis(node, crossAxis, direction, ownerWidth);
const float leadingPaddingAndBorderCross =
node->style().computeFlexStartPaddingAndBorder(
crossAxis, direction, ownerWidth);
Expand Down Expand Up @@ -1539,6 +1570,7 @@ static void calculateLayoutImpl(
boundAxis(
node,
crossAxis,
direction,
flexLine.layout.crossDim + paddingAndBorderAxisCross,
crossAxisownerSize,
ownerWidth) -
Expand All @@ -1559,6 +1591,7 @@ static void calculateLayoutImpl(
boundAxis(
node,
crossAxis,
direction,
flexLine.layout.crossDim + paddingAndBorderAxisCross,
crossAxisownerSize,
ownerWidth) -
Expand Down Expand Up @@ -1733,8 +1766,13 @@ static void calculateLayoutImpl(
.unwrap()
: totalLineCrossDim + paddingAndBorderAxisCross;

const float innerCrossDim =
boundAxis(node, crossAxis, unclampedCrossDim, ownerHeight, ownerWidth) -
const float innerCrossDim = boundAxis(
node,
crossAxis,
direction,
unclampedCrossDim,
ownerHeight,
ownerWidth) -
paddingAndBorderAxisCross;

const float remainingAlignContentDim = innerCrossDim - totalLineCrossDim;
Expand Down Expand Up @@ -1935,6 +1973,7 @@ static void calculateLayoutImpl(
boundAxis(
node,
FlexDirection::Row,
direction,
availableWidth - marginAxisRow,
ownerWidth,
ownerWidth),
Expand All @@ -1944,6 +1983,7 @@ static void calculateLayoutImpl(
boundAxis(
node,
FlexDirection::Column,
direction,
availableHeight - marginAxisColumn,
ownerHeight,
ownerWidth),
Expand All @@ -1958,7 +1998,12 @@ static void calculateLayoutImpl(
// doesn't go below the padding and border amount.
node->setLayoutMeasuredDimension(
boundAxis(
node, mainAxis, maxLineMainDim, mainAxisownerSize, ownerWidth),
node,
mainAxis,
direction,
maxLineMainDim,
mainAxisownerSize,
ownerWidth),
dimension(mainAxis));

} else if (
Expand Down Expand Up @@ -1987,6 +2032,7 @@ static void calculateLayoutImpl(
boundAxis(
node,
crossAxis,
direction,
totalLineCrossDim + paddingAndBorderAxisCross,
crossAxisownerSize,
ownerWidth),
Expand Down

0 comments on commit 00c4a3f

Please sign in to comment.