Fix issue where % width would be wrong if physical and relative paddi…

…ng defined on parent (facebook#1662) Summary: X-link: facebook/react-native#44791 This should fix facebook#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
joevilches · Jun 5, 2024 · 7c45e58 · 7c45e58
1 parent fb53cb7
commit 7c45e58
Show file tree

Hide file tree

Showing 7 changed files with 212 additions and 25 deletions.
diff --git a/gentest/fixtures/YGPaddingTest.html b/gentest/fixtures/YGPaddingTest.html
@@ -13,10 +13,17 @@
   <div style="height: 10px;"></div>
 </div>
 
-<div id="padding_center_child" style="width: 100px; height: 100px; padding-start: 10px; padding-top: 10; padding-end: 20px; padding-bottom: 20px; align-items: center; justify-content: center;">
+<div id="padding_center_child"
+  style="width: 100px; height: 100px; padding-start: 10px; padding-top: 10; padding-end: 20px; padding-bottom: 20px; align-items: center; justify-content: center;">
   <div style="height: 10px; width: 10px;"></div>
 </div>
 
-<div id="child_with_padding_align_end" style="width: 200px; height: 200px; justify-content: flex-end; align-items: flex-end;">
+<div id="child_with_padding_align_end"
+  style="width: 200px; height: 200px; justify-content: flex-end; align-items: flex-end;">
   <div style="width: 100px; height: 100px; padding: 20px;"></div>
 </div>
+
+<div id="physical_and_relative_edge_defined"
+  style="width: 200px; height: 200px; padding-left: 20px; padding-end: 50px;">
+  <div style="width: 100%; height: 50px;"></div>
+</div>
diff --git a/java/tests/com/facebook/yoga/YGPaddingTest.java b/java/tests/com/facebook/yoga/YGPaddingTest.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<<176b6a98b0b08ef3f3fd20289e8fd089>>
+ * @generated SignedSource<<e7b2dc11dc748322103e0c8d1196dc64>>
  * generated by gentest/gentest-driver.ts from gentest/fixtures/YGPaddingTest.html
  */
 
@@ -273,6 +273,48 @@ public void test_child_with_padding_align_end() {
     assertEquals(100f, root_child0.getLayoutHeight(), 0.0f);
   }
 
+  @Test
+  public void test_physical_and_relative_edge_defined() {
+    YogaConfig config = YogaConfigFactory.create();
+
+    final YogaNode root = createNode(config);
+    root.setPositionType(YogaPositionType.ABSOLUTE);
+    root.setPadding(YogaEdge.LEFT, 20);
+    root.setPadding(YogaEdge.END, 50);
+    root.setWidth(200f);
+    root.setHeight(200f);
+
+    final YogaNode root_child0 = createNode(config);
+    root_child0.setWidthPercent(100f);
+    root_child0.setHeight(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(200f, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(20f, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0.getLayoutY(), 0.0f);
+    assertEquals(130f, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, 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(200f, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(50f, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0.getLayoutY(), 0.0f);
+    assertEquals(150f, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0.getLayoutHeight(), 0.0f);
+  }
+
   private YogaNode createNode(YogaConfig config) {
     return mNodeFactory.create(config);
   }

diff --git a/javascript/tests/generated/YGPaddingTest.test.ts b/javascript/tests/generated/YGPaddingTest.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<<a14ac3da5d4dc41e829f7820349c288a>>
+ * @generated SignedSource<<688d750852e836fc6403042d89d5ab51>>
  * generated by gentest/gentest-driver.ts from gentest/fixtures/YGPaddingTest.html
  */
 
@@ -303,3 +303,50 @@ test('child_with_padding_align_end', () => {
     config.free();
   }
 });
+test('physical_and_relative_edge_defined', () => {
+  const config = Yoga.Config.create();
+  let root;
+
+  try {
+    root = Yoga.Node.create(config);
+    root.setPositionType(PositionType.Absolute);
+    root.setPadding(Edge.Left, 20);
+    root.setPadding(Edge.End, 50);
+    root.setWidth(200);
+    root.setHeight(200);
+
+    const root_child0 = Yoga.Node.create(config);
+    root_child0.setWidth("100%");
+    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(200);
+
+    expect(root_child0.getComputedLeft()).toBe(20);
+    expect(root_child0.getComputedTop()).toBe(0);
+    expect(root_child0.getComputedWidth()).toBe(130);
+    expect(root_child0.getComputedHeight()).toBe(50);
+
+    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(200);
+
+    expect(root_child0.getComputedLeft()).toBe(50);
+    expect(root_child0.getComputedTop()).toBe(0);
+    expect(root_child0.getComputedWidth()).toBe(150);
+    expect(root_child0.getComputedHeight()).toBe(50);
+  } finally {
+    if (typeof root !== 'undefined') {
+      root.freeRecursive();
+    }
+
+    config.free();
+  }
+});
diff --git a/tests/generated/YGPaddingTest.cpp b/tests/generated/YGPaddingTest.cpp
@@ -5,7 +5,7 @@
  * LICENSE file in the root directory of this source tree.
  *
  * clang-format off
- * @generated SignedSource<<32fe835176961bebfe905dfd47874f5c>>
+ * @generated SignedSource<<f1bb0b5643b1db2ff78e5c327f45f0f3>>
  * generated by gentest/gentest-driver.ts from gentest/fixtures/YGPaddingTest.html
  */
 
@@ -264,3 +264,46 @@ TEST(YogaTest, child_with_padding_align_end) {
 
   YGConfigFree(config);
 }
+
+TEST(YogaTest, physical_and_relative_edge_defined) {
+  const YGConfigRef config = YGConfigNew();
+
+  const YGNodeRef root = YGNodeNewWithConfig(config);
+  YGNodeStyleSetPositionType(root, YGPositionTypeAbsolute);
+  YGNodeStyleSetPadding(root, YGEdgeLeft, 20);
+  YGNodeStyleSetPadding(root, YGEdgeEnd, 50);
+  YGNodeStyleSetWidth(root, 200);
+  YGNodeStyleSetHeight(root, 200);
+
+  const YGNodeRef root_child0 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidthPercent(root_child0, 100);
+  YGNodeStyleSetHeight(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(200, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(130, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(50, 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(200, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(150, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0));
+
+  YGNodeFreeRecursive(root);
+
+  YGConfigFree(config);
+}
diff --git a/yoga/algorithm/AbsoluteLayout.cpp b/yoga/algorithm/AbsoluteLayout.cpp
@@ -342,6 +342,7 @@ void layoutAbsoluteChild(
       childWidth = boundAxis(
           child,
           FlexDirection::Row,
+          direction,
           childWidth,
           containingBlockWidth,
           containingBlockWidth);
@@ -373,6 +374,7 @@ void layoutAbsoluteChild(
       childHeight = boundAxis(
           child,
           FlexDirection::Column,
+          direction,
           childHeight,
           containingBlockHeight,
           containingBlockWidth);

diff --git a/yoga/algorithm/BoundAxis.h b/yoga/algorithm/BoundAxis.h
@@ -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(
@@ -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