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`](328ec7dc4d/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 3a82d2b1a8 (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

@@ -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 <gtest/gtest.h>
 #include <yoga/Yoga.h>
+#include <array>
 
 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<YGEdge, 6> 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<YGEdge, 6> 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<YGEdge, 2> 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);
+    }
+  }
+}

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 <gtest/gtest.h>
 #include <yoga/Yoga.h>
+#include <array>
 
 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<YGEdge, 6> 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<YGEdge, 6> 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<YGEdge, 2> 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);
+    }
+  }
+}

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;
   }