From b03a821884d7982c7064991a3d59d1322e2de43b Mon Sep 17 00:00:00 2001 From: Nick Gerleman Date: Sat, 30 Sep 2023 21:09:13 -0700 Subject: [PATCH] Fix handling of negative flex gap (#1405) Summary: X-link: https://github.com/facebook/react-native/pull/39596 Pull Request resolved: https://github.com/facebook/yoga/pull/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: 069db7312f72a085c5c4b01ead7bc66a353a07e5 --- gentest/gentest.js | 2 +- tests/FlexGapTest.cpp | 93 ++++++++++++++++++++++++++++++ yoga/algorithm/CalculateLayout.cpp | 7 +-- yoga/algorithm/FlexLine.cpp | 2 +- yoga/node/Node.cpp | 12 ++-- yoga/node/Node.h | 3 +- 6 files changed, 105 insertions(+), 14 deletions(-) create mode 100644 tests/FlexGapTest.cpp diff --git a/gentest/gentest.js b/gentest/gentest.js index 83fd4ee2..8cca9004 100755 --- 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 new file mode 100644 index 00000000..9699145a --- /dev/null +++ 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 +#include + +// 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 index 1d0b2d09..384de713 100644 --- 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(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 index f254a7a5..aab71aa2 100644 --- 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 index 908e1aeb..782c6838 100644 --- 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 index 2c891596..09c05ebe 100644 --- 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) {