Fix flex within max size if max size is not constraint to

Summary: Previous fix for flex in max size constraint was not entirely correct and was missing a test case for the time when the max constraint is not applied. This diff addresses that.

Reviewed By: gkassabli

Differential Revision: D4162104

fbshipit-source-id: 08feba6cb4e789c9aa12179e2cdeadc66b011841

CSSLayout/CSSLayout.c

@@ -289,7 +289,8 @@ void CSSNodeSetMeasureFunc(const CSSNodeRef node, CSSMeasureFunc measureFunc) {
   if (measureFunc == NULL) {
     node->measure = NULL;
   } else {
-    CSS_ASSERT(CSSNodeChildCount(node) == 0, "Cannot set measure function: Nodes with measure functions cannot have children.");
+    CSS_ASSERT(CSSNodeChildCount(node) == 0,
+               "Cannot set measure function: Nodes with measure functions cannot have children.");
     node->measure = measureFunc;
   }
 }
@@ -897,6 +898,23 @@ static float getRelativePosition(const CSSNodeRef node, const CSSFlexDirection a
                                          : -getTrailingPosition(node, axis);
 }
 
+static void constrainMaxSizeForMode(const float maxSize, CSSMeasureMode *mode, float *size) {
+  switch (*mode) {
+    case CSSMeasureModeExactly:
+    case CSSMeasureModeAtMost:
+      *size = (CSSValueIsUndefined(maxSize) || *size < maxSize) ? *size : maxSize;
+      break;
+    case CSSMeasureModeUndefined:
+      if (!CSSValueIsUndefined(maxSize)) {
+        *mode = CSSMeasureModeAtMost;
+        *size = maxSize;
+      }
+      break;
+    case CSSMeasureModeCount:
+      break;
+  }
+}
+
 static void setPosition(const CSSNodeRef node, const CSSDirection direction) {
   const CSSFlexDirection mainAxis = resolveAxis(node->style.flexDirection, direction);
   const CSSFlexDirection crossAxis = getCrossFlexDirection(mainAxis, direction);
@@ -996,15 +1014,12 @@ static void computeChildFlexBasis(const CSSNodeRef node,
       childHeightMeasureMode = CSSMeasureModeExactly;
     }
 
-    if (!CSSValueIsUndefined(child->style.maxDimensions[CSSDimensionWidth])) {
+    constrainMaxSizeForMode(child->style.maxDimensions[CSSDimensionWidth],
-      childWidth = child->style.maxDimensions[CSSDimensionWidth];
+                            &childWidthMeasureMode,
-      childWidthMeasureMode = CSSMeasureModeAtMost;
+                            &childWidth);
-    }
+    constrainMaxSizeForMode(child->style.maxDimensions[CSSDimensionHeight],
-
+                            &childHeightMeasureMode,
-    if (!CSSValueIsUndefined(child->style.maxDimensions[CSSDimensionHeight])) {
+                            &childHeight);
-      childHeight = child->style.maxDimensions[CSSDimensionHeight];
-      childHeightMeasureMode = CSSMeasureModeAtMost;
-    }
 
     // Measure the child
     layoutNodeInternal(child,
@@ -1738,15 +1753,12 @@ static void layoutNodeImpl(const CSSNodeRef node,
           }
         }
 
-        if (!CSSValueIsUndefined(currentRelativeChild->style.maxDimensions[CSSDimensionWidth])) {
+        constrainMaxSizeForMode(currentRelativeChild->style.maxDimensions[CSSDimensionWidth],
-          childWidth = currentRelativeChild->style.maxDimensions[CSSDimensionWidth];
+                                &childWidthMeasureMode,
-          childWidthMeasureMode = CSSMeasureModeAtMost;
+                                &childWidth);
-        }
+        constrainMaxSizeForMode(currentRelativeChild->style.maxDimensions[CSSDimensionHeight],
-
+                                &childHeightMeasureMode,
-        if (!CSSValueIsUndefined(currentRelativeChild->style.maxDimensions[CSSDimensionHeight])) {
+                                &childHeight);
-          childHeight = currentRelativeChild->style.maxDimensions[CSSDimensionHeight];
-          childHeightMeasureMode = CSSMeasureModeAtMost;
-        }
 
         const bool requiresStretchLayout =
             !isStyleDimDefined(currentRelativeChild, crossAxis) &&
@@ -1858,7 +1870,8 @@ static void layoutNodeImpl(const CSSNodeRef node,
             crossDim = fmaxf(crossDim, getDimWithMargin(child, crossAxis));
           }
         } else if (performLayout) {
-          child->layout.position[pos[mainAxis]] += getLeadingBorder(node, mainAxis) + leadingMainDim;
+          child->layout.position[pos[mainAxis]] +=
+              getLeadingBorder(node, mainAxis) + leadingMainDim;
         }
       }
     }
@@ -1924,8 +1937,8 @@ static void layoutNodeImpl(const CSSNodeRef node,
 
             float childWidth;
             float childHeight;
-            CSSMeasureMode childWidthMeasureMode;
+            CSSMeasureMode childWidthMeasureMode = CSSMeasureModeExactly;
-            CSSMeasureMode childHeightMeasureMode;
+            CSSMeasureMode childHeightMeasureMode = CSSMeasureModeExactly;
 
             if (isMainAxisRow) {
               childHeight = crossDim;
@@ -1937,13 +1950,12 @@ static void layoutNodeImpl(const CSSNodeRef node,
                             getMarginAxis(child, CSSFlexDirectionColumn);
             }
 
-            if (!CSSValueIsUndefined(child->style.maxDimensions[CSSDimensionWidth])) {
+            constrainMaxSizeForMode(child->style.maxDimensions[CSSDimensionWidth],
-              childWidth = child->style.maxDimensions[CSSDimensionWidth];
+                                    &childWidthMeasureMode,
-            }
+                                    &childWidth);
-
+            constrainMaxSizeForMode(child->style.maxDimensions[CSSDimensionHeight],
-            if (!CSSValueIsUndefined(child->style.maxDimensions[CSSDimensionHeight])) {
+                                    &childHeightMeasureMode,
-              childHeight = child->style.maxDimensions[CSSDimensionHeight];
+                                    &childHeight);
-            }
 
             // If the child defines a definite size for its cross axis, there's
             // no need to stretch.
@@ -1952,6 +1964,7 @@ static void layoutNodeImpl(const CSSNodeRef node,
                   CSSValueIsUndefined(childWidth) ? CSSMeasureModeUndefined : CSSMeasureModeExactly;
               childHeightMeasureMode = CSSValueIsUndefined(childHeight) ? CSSMeasureModeUndefined
                                                                         : CSSMeasureModeExactly;
+
               layoutNodeInternal(child,
                                  childWidth,
                                  childHeight,

csharp/tests/Facebook.CSSLayout/CSSLayoutMinMaxDimensionTest.cs

@@ -47,6 +47,12 @@
     <div style="height: 20px; flex-grow: 1;"></div>
   </div>
 </div>
+
+<div id="flex_grow_within_constrained_max_width" style="width: 200px; height: 100px;">
+  <div style="flex-direction: row; max-width: 300px;">
+    <div style="height: 20px; flex-grow: 1;"></div>
+  </div>
+</div>
  *
  */
 
@@ -442,5 +448,58 @@ namespace Facebook.CSSLayout
             Assert.AreEqual(20, root_child0_child0.LayoutHeight);
         }
 
+        [Test]
+        public void Test_flex_grow_within_constrained_max_width()
+        {
+            CSSNode root = new CSSNode();
+            root.StyleWidth = 200;
+            root.StyleHeight = 100;
+
+            CSSNode root_child0 = new CSSNode();
+            root_child0.FlexDirection = CSSFlexDirection.Row;
+            root_child0.StyleMaxWidth = 300;
+            root.Insert(0, root_child0);
+
+            CSSNode root_child0_child0 = new CSSNode();
+            root_child0_child0.FlexGrow = 1;
+            root_child0_child0.StyleHeight = 20;
+            root_child0.Insert(0, root_child0_child0);
+            root.StyleDirection = CSSDirection.LeftToRight;
+            root.CalculateLayout();
+
+            Assert.AreEqual(0, root.LayoutX);
+            Assert.AreEqual(0, root.LayoutY);
+            Assert.AreEqual(200, root.LayoutWidth);
+            Assert.AreEqual(100, root.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0.LayoutX);
+            Assert.AreEqual(0, root_child0.LayoutY);
+            Assert.AreEqual(200, root_child0.LayoutWidth);
+            Assert.AreEqual(20, root_child0.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0_child0.LayoutX);
+            Assert.AreEqual(0, root_child0_child0.LayoutY);
+            Assert.AreEqual(200, root_child0_child0.LayoutWidth);
+            Assert.AreEqual(20, root_child0_child0.LayoutHeight);
+
+            root.StyleDirection = CSSDirection.RightToLeft;
+            root.CalculateLayout();
+
+            Assert.AreEqual(0, root.LayoutX);
+            Assert.AreEqual(0, root.LayoutY);
+            Assert.AreEqual(200, root.LayoutWidth);
+            Assert.AreEqual(100, root.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0.LayoutX);
+            Assert.AreEqual(0, root_child0.LayoutY);
+            Assert.AreEqual(200, root_child0.LayoutWidth);
+            Assert.AreEqual(20, root_child0.LayoutHeight);
+
+            Assert.AreEqual(0, root_child0_child0.LayoutX);
+            Assert.AreEqual(0, root_child0_child0.LayoutY);
+            Assert.AreEqual(200, root_child0_child0.LayoutWidth);
+            Assert.AreEqual(20, root_child0_child0.LayoutHeight);
+        }
+
     }
 }

gentest/fixtures/CSSLayoutMinMaxDimensionTest.html

@@ -35,3 +35,9 @@
     <div style="height: 20px; flex-grow: 1;"></div>
   </div>
 </div>
+
+<div id="flex_grow_within_constrained_max_width" style="width: 200px; height: 100px;">
+  <div style="flex-direction: row; max-width: 300px;">
+    <div style="height: 20px; flex-grow: 1;"></div>
+  </div>
+</div>

java/tests/com/facebook/csslayout/CSSLayoutMinMaxDimensionTest.java

@@ -47,6 +47,12 @@
     <div style="height: 20px; flex-grow: 1;"></div>
   </div>
 </div>
+
+<div id="flex_grow_within_constrained_max_width" style="width: 200px; height: 100px;">
+  <div style="flex-direction: row; max-width: 300px;">
+    <div style="height: 20px; flex-grow: 1;"></div>
+  </div>
+</div>
  *
  */
 
@@ -433,4 +439,56 @@ public class CSSLayoutMinMaxDimensionTest {
     assertEquals(20, root_child0_child0.getLayoutHeight(), 0.0f);
   }
 
+  @Test
+  public void test_flex_grow_within_constrained_max_width() {
+    final CSSNode root = new CSSNode();
+    root.setStyleWidth(200);
+    root.setStyleHeight(100);
+
+    final CSSNode root_child0 = new CSSNode();
+    root_child0.setFlexDirection(CSSFlexDirection.ROW);
+    root_child0.setStyleMaxWidth(300);
+    root.addChildAt(root_child0, 0);
+
+    final CSSNode root_child0_child0 = new CSSNode();
+    root_child0_child0.setFlexGrow(1);
+    root_child0_child0.setStyleHeight(20);
+    root_child0.addChildAt(root_child0_child0, 0);
+    root.setDirection(CSSDirection.LTR);
+    root.calculateLayout(null);
+
+    assertEquals(0, root.getLayoutX(), 0.0f);
+    assertEquals(0, root.getLayoutY(), 0.0f);
+    assertEquals(200, root.getLayoutWidth(), 0.0f);
+    assertEquals(100, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0.getLayoutY(), 0.0f);
+    assertEquals(200, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(20, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(200, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(20, root_child0_child0.getLayoutHeight(), 0.0f);
+
+    root.setDirection(CSSDirection.RTL);
+    root.calculateLayout(null);
+
+    assertEquals(0, root.getLayoutX(), 0.0f);
+    assertEquals(0, root.getLayoutY(), 0.0f);
+    assertEquals(200, root.getLayoutWidth(), 0.0f);
+    assertEquals(100, root.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0.getLayoutY(), 0.0f);
+    assertEquals(200, root_child0.getLayoutWidth(), 0.0f);
+    assertEquals(20, root_child0.getLayoutHeight(), 0.0f);
+
+    assertEquals(0, root_child0_child0.getLayoutX(), 0.0f);
+    assertEquals(0, root_child0_child0.getLayoutY(), 0.0f);
+    assertEquals(200, root_child0_child0.getLayoutWidth(), 0.0f);
+    assertEquals(20, root_child0_child0.getLayoutHeight(), 0.0f);
+  }
+
 }

tests/CSSLayoutMinMaxDimensionTest.cpp

@@ -47,6 +47,12 @@
     <div style="height: 20px; flex-grow: 1;"></div>
   </div>
 </div>
+
+<div id="flex_grow_within_constrained_max_width" style="width: 200px; height: 100px;">
+  <div style="flex-direction: row; max-width: 300px;">
+    <div style="height: 20px; flex-grow: 1;"></div>
+  </div>
+</div>
  *
  */
 
@@ -420,3 +426,54 @@ TEST(CSSLayoutTest, flex_grow_within_max_width) {
 
   CSSNodeFreeRecursive(root);
 }
+
+TEST(CSSLayoutTest, flex_grow_within_constrained_max_width) {
+  const CSSNodeRef root = CSSNodeNew();
+  CSSNodeStyleSetWidth(root, 200);
+  CSSNodeStyleSetHeight(root, 100);
+
+  const CSSNodeRef root_child0 = CSSNodeNew();
+  CSSNodeStyleSetFlexDirection(root_child0, CSSFlexDirectionRow);
+  CSSNodeStyleSetMaxWidth(root_child0, 300);
+  CSSNodeInsertChild(root, root_child0, 0);
+
+  const CSSNodeRef root_child0_child0 = CSSNodeNew();
+  CSSNodeStyleSetFlexGrow(root_child0_child0, 1);
+  CSSNodeStyleSetHeight(root_child0_child0, 20);
+  CSSNodeInsertChild(root_child0, root_child0_child0, 0);
+  CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR);
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetHeight(root));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root_child0));
+  ASSERT_EQ(20, CSSNodeLayoutGetHeight(root_child0));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0_child0));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_EQ(20, CSSNodeLayoutGetHeight(root_child0_child0));
+
+  CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionRTL);
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root));
+  ASSERT_EQ(100, CSSNodeLayoutGetHeight(root));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root_child0));
+  ASSERT_EQ(20, CSSNodeLayoutGetHeight(root_child0));
+
+  ASSERT_EQ(0, CSSNodeLayoutGetLeft(root_child0_child0));
+  ASSERT_EQ(0, CSSNodeLayoutGetTop(root_child0_child0));
+  ASSERT_EQ(200, CSSNodeLayoutGetWidth(root_child0_child0));
+  ASSERT_EQ(20, CSSNodeLayoutGetHeight(root_child0_child0));
+
+  CSSNodeFreeRecursive(root);
+}