Fixup hack for flex line size calculation (#1380)

Summary: X-link: facebook/react-native#39433 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 usually never noticed, because 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. There was at least one screenshot test where this issue showed up though, and I was able to add a slightly different repro where we may have free space without a definite dimension by enforcing a min dimension and not stretching. {F1091401183} The new reference is correct, and looking back at diffs, is what this seemed to originally look like when added three years ago. Seems like there may have been a potential regression, but I didn't spot anything suspicious when I looked around the code history. `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 #1188 Reviewed By: javache Differential Revision: D49260049
facebook · Sep 15, 2023 · 4bb0df4 · 4bb0df4
1 parent f9c2c27
commit 4bb0df4
Show file tree

Hide file tree

Showing 7 changed files with 239 additions and 13 deletions.
diff --git a/gentest/Gemfile b/gentest/Gemfile
@@ -1,4 +1,4 @@
 source "https://rubygems.org"
 
 gem 'watir', '~>7.2.0'
-gem 'webdrivers', '~> 5.2.0'
+gem 'webdrivers', '~> 5.3.0'
diff --git a/gentest/Gemfile.lock b/gentest/Gemfile.lock
@@ -1,9 +1,9 @@
 GEM
   remote: https://rubygems.org/
   specs:
-    nokogiri (1.15.2-arm64-darwin)
+    nokogiri (1.15.4-arm64-darwin)
       racc (~> 1.4)
-    racc (1.7.0)
+    racc (1.7.1)
     regexp_parser (2.8.1)
     rexml (3.2.5)
     rubyzip (2.3.2)
@@ -14,18 +14,18 @@ GEM
     watir (7.2.2)
       regexp_parser (>= 1.2, < 3)
       selenium-webdriver (~> 4.2)
-    webdrivers (5.2.0)
+    webdrivers (5.3.1)
       nokogiri (~> 1.6)
       rubyzip (>= 1.3.0)
-      selenium-webdriver (~> 4.0)
+      selenium-webdriver (~> 4.0, < 4.11)
     websocket (1.2.9)
 
 PLATFORMS
   arm64-darwin-22
 
 DEPENDENCIES
   watir (~> 7.2.0)
-  webdrivers (~> 5.2.0)
+  webdrivers (~> 5.3.0)
 
 BUNDLED WITH
    2.4.10
diff --git a/gentest/fixtures/YGJustifyContentTest.html b/gentest/fixtures/YGJustifyContentTest.html
@@ -101,3 +101,10 @@
         </div>
     </div>
 </div>
+
+<div id="justify_content_space_between_indefinite_container_dim_with_free_space" style="width: 300px; height: 300x; align-items: center;">
+  <div style="flex-direction: row; min-width: 200px; justify-content: space-between;">
+      <div style="width: 50px; height: 50px;"></div>
+      <div style="width: 50px; height: 50px;"></div>
+  </div>
+</div>
diff --git a/java/tests/com/facebook/yoga/YGJustifyContentTest.java b/java/tests/com/facebook/yoga/YGJustifyContentTest.java
@@ -1167,6 +1167,77 @@ public void test_justify_content_min_width_with_padding_child_width_lower_than_p
     assertEquals(100f, root_child0_child0_child0.getLayoutHeight(), 0.0f);
   }
 
+  @Test
+  public void test_justify_content_space_between_indefinite_container_dim_with_free_space() {
+    YogaConfig config = YogaConfigFactory.create();
+    config.setExperimentalFeatureEnabled(YogaExperimentalFeature.ABSOLUTE_PERCENTAGE_AGAINST_PADDING_EDGE, true);
+
+    final YogaNode root = createNode(config);
+    root.setAlignItems(YogaAlign.CENTER);
+    root.setWidth(300f);
+
+    final YogaNode root_child0 = createNode(config);
+    root_child0.setFlexDirection(YogaFlexDirection.ROW);
+    root_child0.setJustifyContent(YogaJustify.SPACE_BETWEEN);
+    root_child0.setMinWidth(200f);
+    root.addChildAt(root_child0, 0);
+
+    final YogaNode root_child0_child0 = createNode(config);
+    root_child0_child0.setWidth(50f);
+    root_child0_child0.setHeight(50f);
+    root_child0.addChildAt(root_child0_child0, 0);
+
+    final YogaNode root_child0_child1 = createNode(config);
+    root_child0_child1.setWidth(50f);
+    root_child0_child1.setHeight(50f);
+    root_child0.addChildAt(root_child0_child1, 1);
+    root.setDirection(YogaDirection.LTR);
+    root.calculateLayout(YogaConstants.UNDEFINED, YogaConstants.UNDEFINED);
+
+    assertEquals(0f, root.getLayoutX(), 0.0f);
+    assertEquals(0f, root.getLayoutY(), 0.0f);
+    assertEquals(300f, root.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(50f, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0.getLayoutY(), 0.0f);
+    assertEquals(200f, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0f, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(50f, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(150f, root_child0_child1.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0_child1.getLayoutY(), 0.0f);
+    assertEquals(50f, root_child0_child1.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0_child1.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(300f, root.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(50f, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0.getLayoutY(), 0.0f);
+    assertEquals(200f, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(150f, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(50f, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0f, root_child0_child1.getLayoutX(), 0.0f);
+    assertEquals(0f, root_child0_child1.getLayoutY(), 0.0f);
+    assertEquals(50f, root_child0_child1.getLayoutWidth(), 0.0f);
+    assertEquals(50f, root_child0_child1.getLayoutHeight(), 0.0f);
+  }
+
   private YogaNode createNode(YogaConfig config) {
     return mNodeFactory.create(config);
   }

diff --git a/javascript/tests/generated/YGJustifyContentTest.test.ts b/javascript/tests/generated/YGJustifyContentTest.test.ts
@@ -1275,3 +1275,80 @@ test('justify_content_min_width_with_padding_child_width_lower_than_parent', ()
     config.free();
   }
 });
+test('justify_content_space_between_indefinite_container_dim_with_free_space', () => {
+  const config = Yoga.Config.create();
+  let root;
+
+  config.setExperimentalFeatureEnabled(ExperimentalFeature.AbsolutePercentageAgainstPaddingEdge, true);
+
+  try {
+    root = Yoga.Node.create(config);
+    root.setAlignItems(Align.Center);
+    root.setWidth(300);
+
+    const root_child0 = Yoga.Node.create(config);
+    root_child0.setFlexDirection(FlexDirection.Row);
+    root_child0.setJustifyContent(Justify.SpaceBetween);
+    root_child0.setMinWidth(200);
+    root.insertChild(root_child0, 0);
+
+    const root_child0_child0 = Yoga.Node.create(config);
+    root_child0_child0.setWidth(50);
+    root_child0_child0.setHeight(50);
+    root_child0.insertChild(root_child0_child0, 0);
+
+    const root_child0_child1 = Yoga.Node.create(config);
+    root_child0_child1.setWidth(50);
+    root_child0_child1.setHeight(50);
+    root_child0.insertChild(root_child0_child1, 1);
+    root.calculateLayout(undefined, undefined, Direction.LTR);
+
+    expect(root.getComputedLeft()).toBe(0);
+    expect(root.getComputedTop()).toBe(0);
+    expect(root.getComputedWidth()).toBe(300);
+    expect(root.getComputedHeight()).toBe(50);
+
+    expect(root_child0.getComputedLeft()).toBe(50);
+    expect(root_child0.getComputedTop()).toBe(0);
+    expect(root_child0.getComputedWidth()).toBe(200);
+    expect(root_child0.getComputedHeight()).toBe(50);
+
+    expect(root_child0_child0.getComputedLeft()).toBe(0);
+    expect(root_child0_child0.getComputedTop()).toBe(0);
+    expect(root_child0_child0.getComputedWidth()).toBe(50);
+    expect(root_child0_child0.getComputedHeight()).toBe(50);
+
+    expect(root_child0_child1.getComputedLeft()).toBe(150);
+    expect(root_child0_child1.getComputedTop()).toBe(0);
+    expect(root_child0_child1.getComputedWidth()).toBe(50);
+    expect(root_child0_child1.getComputedHeight()).toBe(50);
+
+    root.calculateLayout(undefined, undefined, Direction.RTL);
+
+    expect(root.getComputedLeft()).toBe(0);
+    expect(root.getComputedTop()).toBe(0);
+    expect(root.getComputedWidth()).toBe(300);
+    expect(root.getComputedHeight()).toBe(50);
+
+    expect(root_child0.getComputedLeft()).toBe(50);
+    expect(root_child0.getComputedTop()).toBe(0);
+    expect(root_child0.getComputedWidth()).toBe(200);
+    expect(root_child0.getComputedHeight()).toBe(50);
+
+    expect(root_child0_child0.getComputedLeft()).toBe(150);
+    expect(root_child0_child0.getComputedTop()).toBe(0);
+    expect(root_child0_child0.getComputedWidth()).toBe(50);
+    expect(root_child0_child0.getComputedHeight()).toBe(50);
+
+    expect(root_child0_child1.getComputedLeft()).toBe(0);
+    expect(root_child0_child1.getComputedTop()).toBe(0);
+    expect(root_child0_child1.getComputedWidth()).toBe(50);
+    expect(root_child0_child1.getComputedHeight()).toBe(50);
+  } finally {
+    if (typeof root !== 'undefined') {
+      root.freeRecursive();
+    }
+
+    config.free();
+  }
+});
diff --git a/tests/generated/YGJustifyContentTest.cpp b/tests/generated/YGJustifyContentTest.cpp
@@ -1170,3 +1170,75 @@ TEST(YogaTest, justify_content_min_width_with_padding_child_width_lower_than_par
 
   YGConfigFree(config);
 }
+
+TEST(YogaTest, justify_content_space_between_indefinite_container_dim_with_free_space) {
+  const YGConfigRef config = YGConfigNew();
+  YGConfigSetExperimentalFeatureEnabled(config, YGExperimentalFeatureAbsolutePercentageAgainstPaddingEdge, true);
+
+  const YGNodeRef root = YGNodeNewWithConfig(config);
+  YGNodeStyleSetAlignItems(root, YGAlignCenter);
+  YGNodeStyleSetWidth(root, 300);
+
+  const YGNodeRef root_child0 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetFlexDirection(root_child0, YGFlexDirectionRow);
+  YGNodeStyleSetJustifyContent(root_child0, YGJustifySpaceBetween);
+  YGNodeStyleSetMinWidth(root_child0, 200);
+  YGNodeInsertChild(root, root_child0, 0);
+
+  const YGNodeRef root_child0_child0 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child0_child0, 50);
+  YGNodeStyleSetHeight(root_child0_child0, 50);
+  YGNodeInsertChild(root_child0, root_child0_child0, 0);
+
+  const YGNodeRef root_child0_child1 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child0_child1, 50);
+  YGNodeStyleSetHeight(root_child0_child1, 50);
+  YGNodeInsertChild(root_child0, root_child0_child1, 1);
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
+  ASSERT_FLOAT_EQ(300, YGNodeLayoutGetWidth(root));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0));
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0_child0));
+
+  ASSERT_FLOAT_EQ(150, YGNodeLayoutGetLeft(root_child0_child1));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0_child1));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetWidth(root_child0_child1));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0_child1));
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionRTL);
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
+  ASSERT_FLOAT_EQ(300, YGNodeLayoutGetWidth(root));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0));
+
+  ASSERT_FLOAT_EQ(150, YGNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0_child0));
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root_child0_child1));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0_child1));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetWidth(root_child0_child1));
+  ASSERT_FLOAT_EQ(50, YGNodeLayoutGetHeight(root_child0_child1));
+
+  YGNodeFreeRecursive(root);
+
+  YGConfigFree(config);
+}
diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp
@@ -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;
     }
@@ -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);
@@ -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) {