From 289b62732bb39931c47136e8c54ca8528a1da73e Mon Sep 17 00:00:00 2001 From: Joe Vilches Date: Mon, 10 Jun 2024 18:25:19 -0700 Subject: [PATCH] Fix issue with alternating flex direction and percent postions (#1663) Summary: X-link: https://github.com/facebook/react-native/pull/44792 Pull Request resolved: https://github.com/facebook/yoga/pull/1663 Fixing https://github.com/facebook/yoga/issues/1658. We had a problem where if a child had a different flex direction than its parent, and it also set a position as a percent, it would look at the wrong axis to evaluate the percent. What was happening was we were passing in the container's mainAxis size and crossAxis size to use to evaluate the position size if it was a percent. However, we matched these sizes with the main/cross axis of the child - which is wrong if the flex direction is different. I changed it so that the function just takes in ownerWidth and ownerHeight then calls isRow to determine which one to use for the main/cross axis position. This reduces the ambiguity quite a bit imo. Changelog: [Internal] Reviewed By: NickGerleman Differential Revision: D58172416 fbshipit-source-id: eafd8069e03493fc56c41a76879d1ad9b7e9236d --- gentest/fixtures/YGFlexDirectionTest.html | 5 ++ .../facebook/yoga/YGFlexDirectionTest.java | 45 ++++++++++++++++- .../generated/YGFlexDirectionTest.test.ts | 50 ++++++++++++++++++- tests/generated/YGFlexDirectionTest.cpp | 46 ++++++++++++++++- yoga/algorithm/CalculateLayout.cpp | 9 +--- yoga/node/Node.cpp | 17 ++++--- yoga/node/Node.h | 6 +-- 7 files changed, 156 insertions(+), 22 deletions(-) diff --git a/gentest/fixtures/YGFlexDirectionTest.html b/gentest/fixtures/YGFlexDirectionTest.html index 70f7c067..14e7e3ef 100644 --- a/gentest/fixtures/YGFlexDirectionTest.html +++ b/gentest/fixtures/YGFlexDirectionTest.html @@ -400,3 +400,8 @@
+ +
+
+
+
diff --git a/java/tests/com/facebook/yoga/YGFlexDirectionTest.java b/java/tests/com/facebook/yoga/YGFlexDirectionTest.java index 2e8561ee..c711e9ab 100644 --- a/java/tests/com/facebook/yoga/YGFlexDirectionTest.java +++ b/java/tests/com/facebook/yoga/YGFlexDirectionTest.java @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<4007f83eb3e84f3ee3fcf46d6d7be3bc>> + * @generated SignedSource<<5bb2b698f40c9c95737a77fff84f7700>> * generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html */ @@ -4242,6 +4242,49 @@ public class YGFlexDirectionTest { assertEquals(100f, root_child0_child2.getLayoutHeight(), 0.0f); } + @Test + public void test_flex_direction_alternating_with_percent() { + YogaConfig config = YogaConfigFactory.create(); + + final YogaNode root = createNode(config); + root.setPositionType(YogaPositionType.ABSOLUTE); + root.setWidth(200f); + root.setHeight(300f); + + final YogaNode root_child0 = createNode(config); + root_child0.setFlexDirection(YogaFlexDirection.ROW); + root_child0.setPositionPercent(YogaEdge.LEFT, 10f); + root_child0.setPositionPercent(YogaEdge.TOP, 10f); + root_child0.setWidthPercent(50f); + root_child0.setHeightPercent(50f); + root.addChildAt(root_child0, 0); + root.setDirection(YogaDirection.LTR); + root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED); + + assertEquals(0f, root.getLayoutX(), 0.0f); + assertEquals(0f, root.getLayoutY(), 0.0f); + assertEquals(200f, root.getLayoutWidth(), 0.0f); + assertEquals(300f, root.getLayoutHeight(), 0.0f); + + assertEquals(20f, root_child0.getLayoutX(), 0.0f); + assertEquals(30f, root_child0.getLayoutY(), 0.0f); + assertEquals(100f, root_child0.getLayoutWidth(), 0.0f); + assertEquals(150f, root_child0.getLayoutHeight(), 0.0f); + + root.setDirection(YogaDirection.RTL); + root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED); + + assertEquals(0f, root.getLayoutX(), 0.0f); + assertEquals(0f, root.getLayoutY(), 0.0f); + assertEquals(200f, root.getLayoutWidth(), 0.0f); + assertEquals(300f, root.getLayoutHeight(), 0.0f); + + assertEquals(120f, root_child0.getLayoutX(), 0.0f); + assertEquals(30f, root_child0.getLayoutY(), 0.0f); + assertEquals(100f, root_child0.getLayoutWidth(), 0.0f); + assertEquals(150f, root_child0.getLayoutHeight(), 0.0f); + } + private YogaNode createNode(YogaConfig config) { return mNodeFactory.create(config); } diff --git a/javascript/tests/generated/YGFlexDirectionTest.test.ts b/javascript/tests/generated/YGFlexDirectionTest.test.ts index 05955433..51b90d88 100644 --- a/javascript/tests/generated/YGFlexDirectionTest.test.ts +++ b/javascript/tests/generated/YGFlexDirectionTest.test.ts @@ -4,7 +4,7 @@ * This source code is licensed under the MIT license found in the * LICENSE file in the root directory of this source tree. * - * @generated SignedSource<<61e2e5c148d45c0bbb6bc886991bf3b9>> + * @generated SignedSource<<5aa80de7d8424e35b5b1b205ff44f4cc>> * generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html */ @@ -4510,3 +4510,51 @@ test('flex_direction_row_reverse_inner_padding_end', () => { config.free(); } }); +test('flex_direction_alternating_with_percent', () => { + const config = Yoga.Config.create(); + let root; + + try { + root = Yoga.Node.create(config); + root.setPositionType(PositionType.Absolute); + root.setWidth(200); + root.setHeight(300); + + const root_child0 = Yoga.Node.create(config); + root_child0.setFlexDirection(FlexDirection.Row); + root_child0.setPosition(Edge.Left, "10%"); + root_child0.setPosition(Edge.Top, "10%"); + root_child0.setWidth("50%"); + root_child0.setHeight("50%"); + root.insertChild(root_child0, 0); + root.calculateLayout(undefined, undefined, Direction.LTR); + + expect(root.getComputedLeft()).toBe(0); + expect(root.getComputedTop()).toBe(0); + expect(root.getComputedWidth()).toBe(200); + expect(root.getComputedHeight()).toBe(300); + + expect(root_child0.getComputedLeft()).toBe(20); + expect(root_child0.getComputedTop()).toBe(30); + expect(root_child0.getComputedWidth()).toBe(100); + expect(root_child0.getComputedHeight()).toBe(150); + + root.calculateLayout(undefined, undefined, Direction.RTL); + + expect(root.getComputedLeft()).toBe(0); + expect(root.getComputedTop()).toBe(0); + expect(root.getComputedWidth()).toBe(200); + expect(root.getComputedHeight()).toBe(300); + + expect(root_child0.getComputedLeft()).toBe(120); + expect(root_child0.getComputedTop()).toBe(30); + expect(root_child0.getComputedWidth()).toBe(100); + expect(root_child0.getComputedHeight()).toBe(150); + } finally { + if (typeof root !== 'undefined') { + root.freeRecursive(); + } + + config.free(); + } +}); diff --git a/tests/generated/YGFlexDirectionTest.cpp b/tests/generated/YGFlexDirectionTest.cpp index e64ef909..05ff7abc 100644 --- a/tests/generated/YGFlexDirectionTest.cpp +++ b/tests/generated/YGFlexDirectionTest.cpp @@ -5,7 +5,7 @@ * LICENSE file in the root directory of this source tree. * * clang-format off - * @generated SignedSource<<4a6a69c4e9bfda6dc73198791f553e13>> + * @generated SignedSource<<552f1533812daf0793244bbc8c465e17>> * generated by gentest/gentest-driver.ts from gentest/fixtures/YGFlexDirectionTest.html */ @@ -4283,3 +4283,47 @@ TEST(YogaTest, flex_direction_row_reverse_inner_padding_end) { YGConfigFree(config); } + +TEST(YogaTest, flex_direction_alternating_with_percent) { + const YGConfigRef config = YGConfigNew(); + + const YGNodeRef root = YGNodeNewWithConfig(config); + YGNodeStyleSetPositionType(root, YGPositionTypeAbsolute); + YGNodeStyleSetWidth(root, 200); + YGNodeStyleSetHeight(root, 300); + + const YGNodeRef root_child0 = YGNodeNewWithConfig(config); + YGNodeStyleSetFlexDirection(root_child0, YGFlexDirectionRow); + YGNodeStyleSetPositionPercent(root_child0, YGEdgeLeft, 10); + YGNodeStyleSetPositionPercent(root_child0, YGEdgeTop, 10); + YGNodeStyleSetWidthPercent(root_child0, 50); + YGNodeStyleSetHeightPercent(root_child0, 50); + YGNodeInsertChild(root, root_child0, 0); + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR); + + ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root)); + ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root)); + ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root)); + ASSERT_FLOAT_EQ(300, YGNodeLayoutGetHeight(root)); + + ASSERT_FLOAT_EQ(20, YGNodeLayoutGetLeft(root_child0)); + ASSERT_FLOAT_EQ(30, YGNodeLayoutGetTop(root_child0)); + ASSERT_FLOAT_EQ(100, YGNodeLayoutGetWidth(root_child0)); + ASSERT_FLOAT_EQ(150, YGNodeLayoutGetHeight(root_child0)); + + YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionRTL); + + ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root)); + ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root)); + ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root)); + ASSERT_FLOAT_EQ(300, YGNodeLayoutGetHeight(root)); + + ASSERT_FLOAT_EQ(120, YGNodeLayoutGetLeft(root_child0)); + ASSERT_FLOAT_EQ(30, YGNodeLayoutGetTop(root_child0)); + ASSERT_FLOAT_EQ(100, YGNodeLayoutGetWidth(root_child0)); + ASSERT_FLOAT_EQ(150, YGNodeLayoutGetHeight(root_child0)); + + YGNodeFreeRecursive(root); + + YGConfigFree(config); +} diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp index b54eeeec..58394a45 100644 --- a/yoga/algorithm/CalculateLayout.cpp +++ b/yoga/algorithm/CalculateLayout.cpp @@ -545,12 +545,8 @@ static float computeFlexBasisForChildren( if (performLayout) { // Set the initial position (relative to the owner). const Direction childDirection = child->resolveDirection(direction); - const float mainDim = - isRow(mainAxis) ? availableInnerWidth : availableInnerHeight; - const float crossDim = - isRow(mainAxis) ? availableInnerHeight : availableInnerWidth; child->setPosition( - childDirection, mainDim, crossDim, availableInnerWidth); + childDirection, availableInnerWidth, availableInnerHeight); } if (child->style().positionType() == PositionType::Absolute) { @@ -2388,8 +2384,7 @@ void calculateLayout( markerData, 0, // tree root gCurrentGenerationCount.load(std::memory_order_relaxed))) { - node->setPosition( - node->getLayout().direction(), ownerWidth, ownerHeight, ownerWidth); + node->setPosition(node->getLayout().direction(), ownerWidth, ownerHeight); roundLayoutResultsToPixelGrid(node, 0.0f, 0.0f); } diff --git a/yoga/node/Node.cpp b/yoga/node/Node.cpp index 31ad09f8..abda52f5 100644 --- a/yoga/node/Node.cpp +++ b/yoga/node/Node.cpp @@ -230,9 +230,8 @@ float Node::relativePosition( void Node::setPosition( const Direction direction, - const float mainSize, - const float crossSize, - const float ownerWidth) { + const float ownerWidth, + const float ownerHeight) { /* Root nodes should be always layouted as LTR, so we don't return negative * values. */ const Direction directionRespectingRoot = @@ -244,10 +243,14 @@ void Node::setPosition( // In the case of position static these are just 0. See: // https://www.w3.org/TR/css-position-3/#valdef-position-static - const float relativePositionMain = - relativePosition(mainAxis, directionRespectingRoot, mainSize); - const float relativePositionCross = - relativePosition(crossAxis, directionRespectingRoot, crossSize); + const float relativePositionMain = relativePosition( + mainAxis, + directionRespectingRoot, + isRow(mainAxis) ? ownerWidth : ownerHeight); + const float relativePositionCross = relativePosition( + crossAxis, + directionRespectingRoot, + isRow(mainAxis) ? ownerHeight : ownerWidth); const auto mainAxisLeadingEdge = inlineStartEdge(mainAxis, direction); const auto mainAxisTrailingEdge = inlineEndEdge(mainAxis, direction); diff --git a/yoga/node/Node.h b/yoga/node/Node.h index cd029b40..06175b8e 100644 --- a/yoga/node/Node.h +++ b/yoga/node/Node.h @@ -229,11 +229,7 @@ class YG_EXPORT Node : public ::YGNode { void setLayoutBorder(float border, PhysicalEdge edge); void setLayoutPadding(float padding, PhysicalEdge edge); void setLayoutPosition(float position, PhysicalEdge edge); - void setPosition( - Direction direction, - float mainSize, - float crossSize, - float ownerWidth); + void setPosition(Direction direction, float ownerWidth, float ownerHeight); // Other methods Style::Length resolveFlexBasisPtr() const;