From 5c711a7076f4d38d4bb6241c0934ed1d41e9ccb9 Mon Sep 17 00:00:00 2001 From: Adam Comella Date: Fri, 12 Oct 2018 15:04:54 -0700 Subject: [PATCH] When paddingStart is 0, it should override paddingHorizontal (#816) Summary: Fixes #815 Imagine a node with this style: `{ paddingHorizontal: 10, paddingStart: 0 }`. After running layout on this node, we expect its computed `paddingStart` to be `0`. However, it is actually `10`. Consider the expression `paddingEdgeStart.getValue() > 0.0f` in [`getLeadingPadding`](https://github.com/facebook/yoga/blob/328ec7dc4d104b42b836d5ccebff04033d045133/yoga/YGNode.cpp#L461). Why is `0` handled like a negative number rather than a positive number? I suspect this should be `>=` so `0` is handled like the positive numbers (this is how `getTrailingPadding` works). It looks like https://github.com/facebook/yoga/commit/3a82d2b1a8f5b65a2c3bd1407552d3ed2226238e?diff=unified&w=1#diff-07b4949bf42749fde386e769ff08a124 changed the operator from `>=` to `>` in `getLeadingPadding`. I suspect it was a mistake. `getTrailingPadding` still uses `>=`. I manually verified this using the code in #815 and added some unit tests to catch this bug and other similar issues. Adam Comella Microsoft Corp. Pull Request resolved: https://github.com/facebook/yoga/pull/816 Reviewed By: priteshrnandgaonkar Differential Revision: D10282617 Pulled By: shergin fbshipit-source-id: 2ab2874ae39d9454308a020a960ace85573fe777 --- tests/YGComputedMarginTest.cpp | 97 ++++++++++++++++++++++++++++++-- tests/YGComputedPaddingTest.cpp | 99 +++++++++++++++++++++++++++++++-- yoga/YGNode.cpp | 4 +- 3 files changed, 188 insertions(+), 12 deletions(-) diff --git a/tests/YGComputedMarginTest.cpp b/tests/YGComputedMarginTest.cpp index 860f58d3..619a2444 100644 --- a/tests/YGComputedMarginTest.cpp +++ b/tests/YGComputedMarginTest.cpp @@ -1,12 +1,13 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the LICENSE + * file in the root directory of this source tree. * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. */ - #include #include +#include TEST(YogaTest, computed_layout_margin) { const YGNodeRef root = YGNodeNew(); @@ -26,3 +27,89 @@ TEST(YogaTest, computed_layout_margin) { YGNodeFreeRecursive(root); } + +TEST(YogaTest, margin_side_overrides_horizontal_and_vertical) { + const std::array edges = {{YGEdgeTop, + YGEdgeBottom, + YGEdgeStart, + YGEdgeEnd, + YGEdgeLeft, + YGEdgeRight}}; + + for (float edgeValue = 0; edgeValue < 2; ++edgeValue) { + for (const auto& edge : edges) { + YGEdge horizontalOrVertical = edge == YGEdgeTop || edge == YGEdgeBottom + ? YGEdgeVertical + : YGEdgeHorizontal; + + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetMargin(root, horizontalOrVertical, 10); + YGNodeStyleSetMargin(root, edge, edgeValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + ASSERT_FLOAT_EQ(edgeValue, YGNodeLayoutGetMargin(root, edge)); + + YGNodeFreeRecursive(root); + } + } +} + +TEST(YogaTest, margin_side_overrides_all) { + const std::array edges = {{YGEdgeTop, + YGEdgeBottom, + YGEdgeStart, + YGEdgeEnd, + YGEdgeLeft, + YGEdgeRight}}; + + for (float edgeValue = 0; edgeValue < 2; ++edgeValue) { + for (const auto& edge : edges) { + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetMargin(root, YGEdgeAll, 10); + YGNodeStyleSetMargin(root, edge, edgeValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + ASSERT_FLOAT_EQ(edgeValue, YGNodeLayoutGetMargin(root, edge)); + + YGNodeFreeRecursive(root); + } + } +} + +TEST(YogaTest, margin_horizontal_and_vertical_overrides_all) { + const std::array directions = {{YGEdgeHorizontal, YGEdgeVertical}}; + + for (float directionValue = 0; directionValue < 2; ++directionValue) { + for (const auto& direction : directions) { + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetMargin(root, YGEdgeAll, 10); + YGNodeStyleSetMargin(root, direction, directionValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + if (direction == YGEdgeVertical) { + ASSERT_FLOAT_EQ(directionValue, YGNodeLayoutGetMargin(root, YGEdgeTop)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetMargin(root, YGEdgeBottom)); + } else { + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetMargin(root, YGEdgeStart)); + ASSERT_FLOAT_EQ(directionValue, YGNodeLayoutGetMargin(root, YGEdgeEnd)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetMargin(root, YGEdgeLeft)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetMargin(root, YGEdgeRight)); + } + + YGNodeFreeRecursive(root); + } + } +} diff --git a/tests/YGComputedPaddingTest.cpp b/tests/YGComputedPaddingTest.cpp index 038a1a7a..e40542c7 100644 --- a/tests/YGComputedPaddingTest.cpp +++ b/tests/YGComputedPaddingTest.cpp @@ -1,12 +1,13 @@ -/** - * Copyright (c) Facebook, Inc. and its affiliates. +/* + * Copyright (c) 2018-present, Facebook, Inc. + * + * This source code is licensed under the MIT license found in the LICENSE + * file in the root directory of this source tree. * - * This source code is licensed under the MIT license found in the - * LICENSE file in the root directory of this source tree. */ - #include #include +#include TEST(YogaTest, computed_layout_padding) { const YGNodeRef root = YGNodeNew(); @@ -26,3 +27,91 @@ TEST(YogaTest, computed_layout_padding) { YGNodeFreeRecursive(root); } + +TEST(YogaTest, padding_side_overrides_horizontal_and_vertical) { + const std::array edges = {{YGEdgeTop, + YGEdgeBottom, + YGEdgeStart, + YGEdgeEnd, + YGEdgeLeft, + YGEdgeRight}}; + + for (float edgeValue = 0; edgeValue < 2; ++edgeValue) { + for (const auto& edge : edges) { + YGEdge horizontalOrVertical = edge == YGEdgeTop || edge == YGEdgeBottom + ? YGEdgeVertical + : YGEdgeHorizontal; + + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetPadding(root, horizontalOrVertical, 10); + YGNodeStyleSetPadding(root, edge, edgeValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + ASSERT_FLOAT_EQ(edgeValue, YGNodeLayoutGetPadding(root, edge)); + + YGNodeFreeRecursive(root); + } + } +} + +TEST(YogaTest, padding_side_overrides_all) { + const std::array edges = {{YGEdgeTop, + YGEdgeBottom, + YGEdgeStart, + YGEdgeEnd, + YGEdgeLeft, + YGEdgeRight}}; + + for (float edgeValue = 0; edgeValue < 2; ++edgeValue) { + for (const auto& edge : edges) { + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetPadding(root, YGEdgeAll, 10); + YGNodeStyleSetPadding(root, edge, edgeValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + ASSERT_FLOAT_EQ(edgeValue, YGNodeLayoutGetPadding(root, edge)); + + YGNodeFreeRecursive(root); + } + } +} + +TEST(YogaTest, padding_horizontal_and_vertical_overrides_all) { + const std::array directions = {{YGEdgeHorizontal, YGEdgeVertical}}; + + for (float directionValue = 0; directionValue < 2; ++directionValue) { + for (const auto& direction : directions) { + const YGNodeRef root = YGNodeNew(); + YGNodeStyleSetWidth(root, 100); + YGNodeStyleSetHeight(root, 100); + YGNodeStyleSetPadding(root, YGEdgeAll, 10); + YGNodeStyleSetPadding(root, direction, directionValue); + + YGNodeCalculateLayout(root, 100, 100, YGDirectionLTR); + + if (direction == YGEdgeVertical) { + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeTop)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeBottom)); + } else { + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeStart)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeEnd)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeLeft)); + ASSERT_FLOAT_EQ( + directionValue, YGNodeLayoutGetPadding(root, YGEdgeRight)); + } + + YGNodeFreeRecursive(root); + } + } +} diff --git a/yoga/YGNode.cpp b/yoga/YGNode.cpp index 987474de..76f662e7 100644 --- a/yoga/YGNode.cpp +++ b/yoga/YGNode.cpp @@ -1,5 +1,5 @@ /* - * Copyright (c) Facebook, Inc. and its affiliates. + * Copyright (c) 2018-present, Facebook, Inc. * * This source code is licensed under the MIT license found in the LICENSE * file in the root directory of this source tree. @@ -458,7 +458,7 @@ YGFloatOptional YGNode::getLeadingPadding( YGResolveValue(style_.padding[YGEdgeStart], widthSize); if (YGFlexDirectionIsRow(axis) && style_.padding[YGEdgeStart].unit != YGUnitUndefined && - !paddingEdgeStart.isUndefined() && paddingEdgeStart.getValue() > 0.0f) { + !paddingEdgeStart.isUndefined() && paddingEdgeStart.getValue() >= 0.0f) { return paddingEdgeStart; }