From 4d18fe628efe94d8216b2d997263ee0d2835d831 Mon Sep 17 00:00:00 2001 From: Joe Vilches Date: Wed, 11 Oct 2023 17:49:09 -0700 Subject: [PATCH] Fix issue where marginStart and marginEnd were not working with rowReverse flex direction (#1420) Summary: X-link: https://github.com/facebook/litho/pull/962 X-link: https://github.com/facebook/react-native/pull/40804 Pull Request resolved: https://github.com/facebook/yoga/pull/1420 This stack is ultimately aiming to solve https://github.com/facebook/yoga/issues/1208 **The problem** Turns out that we do not even check direction when determining which edge is the leading (start) and trailing (end) edges. This is not how web does it as the start/end is based on the writing direction NOT the flex direction: https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_flexible_box_layout/Basic_concepts_of_flexbox#start_and_end_lines. While web does not have marginStart and marginEnd, they do have margin-inline-start/end which relies on the writing mode to determine the "start"/"end": https://developer.mozilla.org/en-US/docs/Web/CSS/margin-inline-start. This means that if you do something like ``` export default function Playground(props: Props): React.Node { return ( ); } const styles = StyleSheet.create({ container: { marginEnd: 100, flexDirection: 'row-reverse', backgroundColor: 'red', display: 'flex', width: 100, height: 100, }, item: { backgroundColor: 'blue', width: 10, }, }); ``` You get {F1116264350} As you can see the margin gets applied to the left edge even thought the direction is ltr and it should be applied to the right edge. **The solution** I ended up fixing this by creating a new `leadingLayoutEdge` and `trailingLayoutEdge` function that take the flex direction as well as the direction. Based on the errata, the a few functions will use these new functions to determine which `YGEdge` is the starting/ending. You might be wondering why I did not put this logic inside of `leadingEdge(flexDirection)` / `trailingEdge(flexDirection)` since other areas could potentially have the same bug like `getLeadingPadding`. These functions are a bit overloaded and there are cases where we actually want to use the flexDirection to get the edge in question. For example, many of the calls to `setLayoutPosition` in `CalculateLayout.cpp` call `leadingEdge()` / `trailingEdge()` to set the proper position for cases like row-reverse where items need to line up in a different direction. Differential Revision: D50140503 fbshipit-source-id: a120a32f010eca5edc91a5f970356b8d3dbc1ebd --- yoga/algorithm/CalculateLayout.cpp | 56 +++++++++++++++--------- yoga/algorithm/FlexDirection.h | 20 +++++++++ yoga/node/Node.cpp | 70 +++++++++++++++++++++++------- yoga/node/Node.h | 17 +++++++- 4 files changed, 125 insertions(+), 38 deletions(-) diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp index d2d56b7d20..7afeba3681 100644 --- a/yoga/algorithm/CalculateLayout.cpp +++ b/yoga/algorithm/CalculateLayout.cpp @@ -52,10 +52,11 @@ bool calculateLayoutInternal( static inline float dimensionWithMargin( const yoga::Node* const node, const FlexDirection axis, + const Direction direction, const float widthSize) { return node->getLayout().measuredDimension(dimension(axis)) + - (node->getLeadingMargin(axis, widthSize) + - node->getTrailingMargin(axis, widthSize)); + (node->getLeadingMargin(axis, direction, widthSize) + + node->getTrailingMargin(axis, direction, widthSize)); } static inline bool styleDefinesDimension( @@ -453,7 +454,8 @@ static void layoutAbsoluteChild( node->getLayout().measuredDimension(dimension(mainAxis)) - child->getLayout().measuredDimension(dimension(mainAxis)) - node->getTrailingBorder(mainAxis) - - child->getTrailingMargin(mainAxis, isMainAxisRow ? width : height) - + child->getTrailingMargin( + mainAxis, direction, isMainAxisRow ? width : height) - child->getTrailingPosition( mainAxis, isMainAxisRow ? width : height), leadingEdge(mainAxis)); @@ -483,6 +485,7 @@ static void layoutAbsoluteChild( node->getLeadingBorder(mainAxis) + child->getLeadingMargin( mainAxis, + direction, node->getLayout().measuredDimension(dimension(mainAxis))), leadingEdge(mainAxis)); } @@ -494,7 +497,7 @@ static void layoutAbsoluteChild( child->getLayout().measuredDimension(dimension(crossAxis)) - node->getTrailingBorder(crossAxis) - child->getTrailingMargin( - crossAxis, isMainAxisRow ? height : width) - + crossAxis, direction, isMainAxisRow ? height : width) - child->getTrailingPosition( crossAxis, isMainAxisRow ? height : width), leadingEdge(crossAxis)); @@ -526,6 +529,7 @@ static void layoutAbsoluteChild( node->getLeadingBorder(crossAxis) + child->getLeadingMargin( crossAxis, + direction, node->getLayout().measuredDimension(dimension(crossAxis))), leadingEdge(crossAxis)); } @@ -1186,6 +1190,7 @@ static void justifyMainAxis( const size_t startOfLineIndex, const FlexDirection mainAxis, const FlexDirection crossAxis, + const Direction direction, const MeasureMode measureModeMainDim, const MeasureMode measureModeCrossDim, const float mainAxisownerSize, @@ -1303,7 +1308,8 @@ static void justifyMainAxis( child->setLayoutPosition( child->getLeadingPosition(mainAxis, availableInnerMainDim) + node->getLeadingBorder(mainAxis) + - child->getLeadingMargin(mainAxis, availableInnerWidth), + child->getLeadingMargin( + mainAxis, direction, availableInnerWidth), leadingEdge(mainAxis)); } } else { @@ -1344,15 +1350,15 @@ static void justifyMainAxis( } else { // The main dimension is the sum of all the elements dimension plus // the spacing. - flexLine.layout.mainDim += - dimensionWithMargin(child, mainAxis, availableInnerWidth); + flexLine.layout.mainDim += dimensionWithMargin( + child, mainAxis, direction, availableInnerWidth); if (isNodeBaselineLayout) { // If the child is baseline aligned then the cross dimension is // calculated by adding maxAscent and maxDescent from the baseline. const float ascent = calculateBaseline(child) + child->getLeadingMargin( - FlexDirection::Column, availableInnerWidth); + FlexDirection::Column, direction, availableInnerWidth); const float descent = child->getLayout().measuredDimension(Dimension::Height) + child->getMarginForAxis( @@ -1369,7 +1375,8 @@ static void justifyMainAxis( // when the items are not baseline aligned flexLine.layout.crossDim = yoga::maxOrDefined( flexLine.layout.crossDim, - dimensionWithMargin(child, crossAxis, availableInnerWidth)); + dimensionWithMargin( + child, crossAxis, direction, availableInnerWidth)); } } } else if (performLayout) { @@ -1498,16 +1505,16 @@ static void calculateLayoutImpl( const YGEdge endEdge = direction == Direction::LTR ? YGEdgeRight : YGEdgeLeft; const float marginRowLeading = - node->getLeadingMargin(flexRowDirection, ownerWidth); + node->getLeadingMargin(flexRowDirection, direction, ownerWidth); node->setLayoutMargin(marginRowLeading, startEdge); const float marginRowTrailing = - node->getTrailingMargin(flexRowDirection, ownerWidth); + node->getTrailingMargin(flexRowDirection, direction, ownerWidth); node->setLayoutMargin(marginRowTrailing, endEdge); const float marginColumnLeading = - node->getLeadingMargin(flexColumnDirection, ownerWidth); + node->getLeadingMargin(flexColumnDirection, direction, ownerWidth); node->setLayoutMargin(marginColumnLeading, YGEdgeTop); const float marginColumnTrailing = - node->getTrailingMargin(flexColumnDirection, ownerWidth); + node->getTrailingMargin(flexColumnDirection, direction, ownerWidth); node->setLayoutMargin(marginColumnTrailing, YGEdgeBottom); const float marginAxisRow = marginRowLeading + marginRowTrailing; @@ -1793,6 +1800,7 @@ static void calculateLayoutImpl( startOfLineIndex, mainAxis, crossAxis, + direction, measureModeMainDim, measureModeCrossDim, mainAxisownerSize, @@ -1849,7 +1857,8 @@ static void calculateLayoutImpl( child->setLayoutPosition( child->getLeadingPosition(crossAxis, availableInnerCrossDim) + node->getLeadingBorder(crossAxis) + - child->getLeadingMargin(crossAxis, availableInnerWidth), + child->getLeadingMargin( + crossAxis, direction, availableInnerWidth), leadingEdge(crossAxis)); } // If leading position is not defined or calculations result in Nan, @@ -1859,7 +1868,8 @@ static void calculateLayoutImpl( child->getLayout().position[leadingEdge(crossAxis)])) { child->setLayoutPosition( node->getLeadingBorder(crossAxis) + - child->getLeadingMargin(crossAxis, availableInnerWidth), + child->getLeadingMargin( + crossAxis, direction, availableInnerWidth), leadingEdge(crossAxis)); } } else { @@ -1945,8 +1955,10 @@ static void calculateLayoutImpl( generationCount); } } else { - const float remainingCrossDim = containerCrossAxis - - dimensionWithMargin(child, crossAxis, availableInnerWidth); + const float remainingCrossDim = + containerCrossAxis - + dimensionWithMargin( + child, crossAxis, direction, availableInnerWidth); if (child->marginLeadingValue(crossAxis).unit == YGUnitAuto && child->marginTrailingValue(crossAxis).unit == YGUnitAuto) { @@ -2053,7 +2065,7 @@ static void calculateLayoutImpl( if (resolveChildAlignment(node, child) == Align::Baseline) { const float ascent = calculateBaseline(child) + child->getLeadingMargin( - FlexDirection::Column, availableInnerWidth); + FlexDirection::Column, direction, availableInnerWidth); const float descent = child->getLayout().measuredDimension(Dimension::Height) + child->getMarginForAxis( @@ -2083,7 +2095,8 @@ static void calculateLayoutImpl( case Align::FlexStart: { child->setLayoutPosition( currentLead + - child->getLeadingMargin(crossAxis, availableInnerWidth), + child->getLeadingMargin( + crossAxis, direction, availableInnerWidth), leadingEdge(crossAxis)); break; } @@ -2091,7 +2104,7 @@ static void calculateLayoutImpl( child->setLayoutPosition( currentLead + lineHeight - child->getTrailingMargin( - crossAxis, availableInnerWidth) - + crossAxis, direction, availableInnerWidth) - child->getLayout().measuredDimension( dimension(crossAxis)), leadingEdge(crossAxis)); @@ -2109,7 +2122,8 @@ static void calculateLayoutImpl( case Align::Stretch: { child->setLayoutPosition( currentLead + - child->getLeadingMargin(crossAxis, availableInnerWidth), + child->getLeadingMargin( + crossAxis, direction, availableInnerWidth), leadingEdge(crossAxis)); // Remeasure child with the line height as it as been only diff --git a/yoga/algorithm/FlexDirection.h b/yoga/algorithm/FlexDirection.h index 010bfed409..7dd2b4cce7 100644 --- a/yoga/algorithm/FlexDirection.h +++ b/yoga/algorithm/FlexDirection.h @@ -77,6 +77,26 @@ inline YGEdge trailingEdge(const FlexDirection flexDirection) { fatalWithMessage("Invalid FlexDirection"); } +inline YGEdge leadingLayoutEdge( + const FlexDirection flexDirection, + const Direction direction) { + if (isRow(flexDirection)) { + return direction == Direction::RTL ? YGEdgeRight : YGEdgeLeft; + } + + return YGEdgeTop; +} + +inline YGEdge trailingLayoutEdge( + const FlexDirection flexDirection, + const Direction direction) { + if (isRow(flexDirection)) { + return direction == Direction::RTL ? YGEdgeLeft : YGEdgeRight; + } + + return YGEdgeBottom; +} + inline Dimension dimension(const FlexDirection flexDirection) { switch (flexDirection) { case FlexDirection::Column: diff --git a/yoga/node/Node.cpp b/yoga/node/Node.cpp index d84aac542f..c7cee82930 100644 --- a/yoga/node/Node.cpp +++ b/yoga/node/Node.cpp @@ -83,6 +83,22 @@ CompactValue Node::computeEdgeValueForColumn( } } +YGEdge Node::getLeadingLayoutEdgeUsingErrata( + FlexDirection flexDirection, + Direction direction) const { + return hasErrata(Errata::StartingEndingEdgeFromFlexDirection) + ? leadingEdge(flexDirection) + : leadingLayoutEdge(flexDirection, direction); +} + +YGEdge Node::getTrailingLayoutEdgeUsingErrata( + FlexDirection flexDirection, + Direction direction) const { + return hasErrata(Errata::StartingEndingEdgeFromFlexDirection) + ? trailingEdge(flexDirection) + : trailingLayoutEdge(flexDirection, direction); +} + bool Node::isLeadingPositionDefined(FlexDirection axis) const { auto leadingPosition = isRow(axis) ? computeEdgeValueForRow( @@ -117,18 +133,26 @@ float Node::getTrailingPosition(FlexDirection axis, float axisSize) const { return resolveValue(trailingPosition, axisSize).unwrapOrDefault(0.0f); } -float Node::getLeadingMargin(FlexDirection axis, float widthSize) const { +float Node::getLeadingMargin( + FlexDirection axis, + Direction direction, + float widthSize) const { + const YGEdge startEdge = getLeadingLayoutEdgeUsingErrata(axis, direction); auto leadingMargin = isRow(axis) - ? computeEdgeValueForRow(style_.margin(), YGEdgeStart, leadingEdge(axis)) - : computeEdgeValueForColumn(style_.margin(), leadingEdge(axis)); + ? computeEdgeValueForRow(style_.margin(), YGEdgeStart, startEdge) + : computeEdgeValueForColumn(style_.margin(), startEdge); return resolveValue(leadingMargin, widthSize).unwrapOrDefault(0.0f); } -float Node::getTrailingMargin(FlexDirection axis, float widthSize) const { +float Node::getTrailingMargin( + FlexDirection axis, + Direction direction, + float widthSize) const { + const YGEdge endEdge = getTrailingLayoutEdgeUsingErrata(axis, direction); auto trailingMargin = isRow(axis) - ? computeEdgeValueForRow(style_.margin(), YGEdgeEnd, trailingEdge(axis)) - : computeEdgeValueForColumn(style_.margin(), trailingEdge(axis)); + ? computeEdgeValueForRow(style_.margin(), YGEdgeEnd, endEdge) + : computeEdgeValueForColumn(style_.margin(), endEdge); return resolveValue(trailingMargin, widthSize).unwrapOrDefault(0.0f); } @@ -176,7 +200,10 @@ float Node::getTrailingPaddingAndBorder(FlexDirection axis, float widthSize) } float Node::getMarginForAxis(FlexDirection axis, float widthSize) const { - return getLeadingMargin(axis, widthSize) + getTrailingMargin(axis, widthSize); + // The total margin for a given axis does not depend on the direction + // so hardcoding LTR here to avoid piping direction to this function + return getLeadingMargin(axis, Direction::LTR, widthSize) + + getTrailingMargin(axis, Direction::LTR, widthSize); } float Node::getGapForAxis(FlexDirection axis) const { @@ -360,18 +387,31 @@ void Node::setPosition( const float relativePositionMain = relativePosition(mainAxis, mainSize); const float relativePositionCross = relativePosition(crossAxis, crossSize); + const YGEdge mainAxisLeadingEdge = + getLeadingLayoutEdgeUsingErrata(mainAxis, direction); + const YGEdge mainAxisTrailingEdge = + getTrailingLayoutEdgeUsingErrata(mainAxis, direction); + const YGEdge crossAxisLeadingEdge = + getLeadingLayoutEdgeUsingErrata(crossAxis, direction); + const YGEdge crossAxisTrailingEdge = + getTrailingLayoutEdgeUsingErrata(crossAxis, direction); + setLayoutPosition( - (getLeadingMargin(mainAxis, ownerWidth) + relativePositionMain), - leadingEdge(mainAxis)); + (getLeadingMargin(mainAxis, direction, ownerWidth) + + relativePositionMain), + mainAxisLeadingEdge); setLayoutPosition( - (getTrailingMargin(mainAxis, ownerWidth) + relativePositionMain), - trailingEdge(mainAxis)); + (getTrailingMargin(mainAxis, direction, ownerWidth) + + relativePositionMain), + mainAxisTrailingEdge); setLayoutPosition( - (getLeadingMargin(crossAxis, ownerWidth) + relativePositionCross), - leadingEdge(crossAxis)); + (getLeadingMargin(crossAxis, direction, ownerWidth) + + relativePositionCross), + crossAxisLeadingEdge); setLayoutPosition( - (getTrailingMargin(crossAxis, ownerWidth) + relativePositionCross), - trailingEdge(crossAxis)); + (getTrailingMargin(crossAxis, direction, ownerWidth) + + relativePositionCross), + crossAxisTrailingEdge); } YGValue Node::marginLeadingValue(FlexDirection axis) const { diff --git a/yoga/node/Node.h b/yoga/node/Node.h index bf1f89f332..4aaf0f7b3c 100644 --- a/yoga/node/Node.h +++ b/yoga/node/Node.h @@ -50,6 +50,13 @@ class YG_EXPORT Node : public ::YGNode { float relativePosition(FlexDirection axis, const float axisSize) const; + YGEdge getLeadingLayoutEdgeUsingErrata( + FlexDirection flexDirection, + Direction direction) const; + YGEdge getTrailingLayoutEdgeUsingErrata( + FlexDirection flexDirection, + Direction direction) const; + void useWebDefaults() { style_.flexDirection() = FlexDirection::Row; style_.alignContent() = Align::Stretch; @@ -193,8 +200,14 @@ class YG_EXPORT Node : public ::YGNode { bool isTrailingPosDefined(FlexDirection axis) const; float getLeadingPosition(FlexDirection axis, float axisSize) const; float getTrailingPosition(FlexDirection axis, float axisSize) const; - float getLeadingMargin(FlexDirection axis, float widthSize) const; - float getTrailingMargin(FlexDirection axis, float widthSize) const; + float getLeadingMargin( + FlexDirection axis, + Direction direction, + float widthSize) const; + float getTrailingMargin( + FlexDirection axis, + Direction direction, + float widthSize) const; float getLeadingBorder(FlexDirection flexDirection) const; float getTrailingBorder(FlexDirection flexDirection) const; float getLeadingPadding(FlexDirection axis, float widthSize) const;