Fix handling of negative flex gap (#1405)

Summary: X-link: facebook/react-native#39596 Pull Request resolved: #1405 I noticed that we weren't clamping negative flex gap values to zero. This fixes that bug. Reviewed By: rshest Differential Revision: D49530494 fbshipit-source-id: aa87547419e92d733487360db4c8937f9a9e757f
facebook · Sep 26, 2023 · 13253b9 · 13253b9
1 parent 0c16cd6
commit 13253b9
Show file tree

Hide file tree

Showing 6 changed files with 105 additions and 14 deletions.
diff --git a/gentest/gentest.js b/gentest/gentest.js
@@ -701,7 +701,7 @@ function calculateTree(root, roundToPixelGrid) {
 
 function getYogaStyle(node) {
   // TODO: Relying on computed style means we cannot test shorthand props like
-  // "padding", "margin", "gap".
+  // "padding", "margin", "gap", or negative values.
   return [
     'direction',
     'flex-direction',

diff --git a/tests/FlexGapTest.cpp b/tests/FlexGapTest.cpp
@@ -0,0 +1,93 @@
+/*
+ * Copyright (c) Meta Platforms, Inc. and affiliates.
+ *
+ * This source code is licensed under the MIT license found in the
+ * LICENSE file in the root directory of this source tree.
+ */
+
+#include <gtest/gtest.h>
+#include <yoga/Yoga.h>
+
+// TODO: move this to a fixture based test once it supports parsing negative
+// values
+TEST(FlexGap, gap_negative_value) {
+  const YGConfigRef config = YGConfigNew();
+
+  const YGNodeRef root = YGNodeNewWithConfig(config);
+  YGNodeStyleSetFlexDirection(root, YGFlexDirectionRow);
+  YGNodeStyleSetGap(root, YGGutterAll, -20);
+  YGNodeStyleSetHeight(root, 200);
+
+  const YGNodeRef root_child0 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child0, 20);
+  YGNodeInsertChild(root, root_child0, 0);
+
+  const YGNodeRef root_child1 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child1, 20);
+  YGNodeInsertChild(root, root_child1, 1);
+
+  const YGNodeRef root_child2 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child2, 20);
+  YGNodeInsertChild(root, root_child2, 2);
+
+  const YGNodeRef root_child3 = YGNodeNewWithConfig(config);
+  YGNodeStyleSetWidth(root_child3, 20);
+  YGNodeInsertChild(root, root_child3, 3);
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionLTR);
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
+  ASSERT_FLOAT_EQ(80, YGNodeLayoutGetWidth(root));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child0));
+
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetLeft(root_child1));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child1));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child1));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child1));
+
+  ASSERT_FLOAT_EQ(40, YGNodeLayoutGetLeft(root_child2));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child2));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child2));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child2));
+
+  ASSERT_FLOAT_EQ(60, YGNodeLayoutGetLeft(root_child3));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child3));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child3));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child3));
+
+  YGNodeCalculateLayout(root, YGUndefined, YGUndefined, YGDirectionRTL);
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root));
+  ASSERT_FLOAT_EQ(80, YGNodeLayoutGetWidth(root));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root));
+
+  ASSERT_FLOAT_EQ(60, YGNodeLayoutGetLeft(root_child0));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child0));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child0));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child0));
+
+  ASSERT_FLOAT_EQ(40, YGNodeLayoutGetLeft(root_child1));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child1));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child1));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child1));
+
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetLeft(root_child2));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child2));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child2));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child2));
+
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetLeft(root_child3));
+  ASSERT_FLOAT_EQ(0, YGNodeLayoutGetTop(root_child3));
+  ASSERT_FLOAT_EQ(20, YGNodeLayoutGetWidth(root_child3));
+  ASSERT_FLOAT_EQ(200, YGNodeLayoutGetHeight(root_child3));
+
+  YGNodeFreeRecursive(root);
+
+  YGConfigFree(config);
+}
diff --git a/yoga/algorithm/CalculateLayout.cpp b/yoga/algorithm/CalculateLayout.cpp
@@ -1215,7 +1215,7 @@ static void justifyMainAxis(
       node->getLeadingPaddingAndBorder(mainAxis, ownerWidth).unwrap();
   const float trailingPaddingAndBorderMain =
       node->getTrailingPaddingAndBorder(mainAxis, ownerWidth).unwrap();
-  const float gap = node->getGapForAxis(mainAxis, ownerWidth).unwrap();
+  const float gap = node->getGapForAxis(mainAxis, ownerWidth);
   // If we are using "at most" rules in the main axis, make sure that
   // remainingFreeSpace is 0 when min main dimension is not given
   if (measureModeMainDim == MeasureMode::AtMost &&
@@ -1666,8 +1666,7 @@ static void calculateLayoutImpl(
       generationCount);
 
   if (childCount > 1) {
-    totalMainDim +=
-        node->getGapForAxis(mainAxis, availableInnerCrossDim).unwrap() *
+    totalMainDim += node->getGapForAxis(mainAxis, availableInnerCrossDim) *
         static_cast<float>(childCount - 1);
   }
 
@@ -1692,7 +1691,7 @@ static void calculateLayoutImpl(
   float totalLineCrossDim = 0;
 
   const float crossAxisGap =
-      node->getGapForAxis(crossAxis, availableInnerCrossDim).unwrap();
+      node->getGapForAxis(crossAxis, availableInnerCrossDim);
 
   // Max main dimension of all the lines.
   float maxLineMainDim = 0;

diff --git a/yoga/algorithm/FlexLine.cpp b/yoga/algorithm/FlexLine.cpp
@@ -33,7 +33,7 @@ FlexLine calculateFlexLine(
   const FlexDirection mainAxis = resolveDirection(
       node->getStyle().flexDirection(), node->resolveDirection(ownerDirection));
   const bool isNodeFlexWrap = node->getStyle().flexWrap() != Wrap::NoWrap;
-  const float gap = node->getGapForAxis(mainAxis, availableInnerWidth).unwrap();
+  const float gap = node->getGapForAxis(mainAxis, availableInnerWidth);
 
   // Add items to the current line until it's full or we run out of items.
   for (; endOfLineIndex < node->getChildren().size(); endOfLineIndex++) {

diff --git a/yoga/node/Node.cpp b/yoga/node/Node.cpp
@@ -202,13 +202,13 @@ FloatOptional Node::getMarginForAxis(
   return getLeadingMargin(axis, widthSize) + getTrailingMargin(axis, widthSize);
 }
 
-FloatOptional Node::getGapForAxis(
-    const FlexDirection axis,
-    const float widthSize) const {
+float Node::getGapForAxis(const FlexDirection axis, const float widthSize)
+    const {
   auto gap = isRow(axis)
-      ? computeColumnGap(style_.gap(), CompactValue::ofZero())
-      : computeRowGap(style_.gap(), CompactValue::ofZero());
-  return yoga::resolveValue(gap, widthSize);
+      ? computeColumnGap(style_.gap(), CompactValue::ofUndefined())
+      : computeRowGap(style_.gap(), CompactValue::ofUndefined());
+  auto resolvedGap = yoga::resolveValue(gap, widthSize);
+  return maxOrDefined(resolvedGap.unwrap(), 0);
 }
 
 YGSize Node::measure(

diff --git a/yoga/node/Node.h b/yoga/node/Node.h
@@ -231,8 +231,7 @@ class YG_EXPORT Node : public ::YGNode {
   FloatOptional getMarginForAxis(
       const FlexDirection axis,
       const float widthSize) const;
-  FloatOptional getGapForAxis(const FlexDirection axis, const float widthSize)
-      const;
+  float getGapForAxis(const FlexDirection axis, const float widthSize) const;
   // Setters
 
   void setContext(void* context) {