From 15d5cb0169c83b4b66b93ba5eafe7240f870ef0e Mon Sep 17 00:00:00 2001 From: Emil Sjolander Date: Mon, 26 Sep 2016 06:11:49 -0700 Subject: [PATCH] BREAKING - Fix unconstraint sizing in main axis Summary: @public This fixes measuring of items in the main axis of a container. Previously items were in a lot of cases measured with UNSPECIFIED instead of AT_MOST. This was to support scrolling containers. The correct way to handle scrolling containers is to instead provide them with their own overflow value to activate this behavior. This is also similar to how the web works. This is a breaking change. Most of your layouts will continue to function as before however some of them might not. Typically this is due to having a `flex: 1` style where it is currently a no-op due to being measured with an undefined size but after this change it may collapse your component to take zero size due to the implicit `flexBasis: 0` now being correctly treated. Removing the bad `flex: 1` style or changing it to `flexGrow: 1` should solve most if not all layout issues your see after this diff. Reviewed By: majak Differential Revision: D3876927 fbshipit-source-id: 81ea1c9d6574dd4564a3333f1b3617cf84b4022f --- CSSLayout/CSSLayout.c | 34 +- CSSLayout/CSSLayout.h | 1 + java/com/facebook/csslayout/CSSLayout.java | 2 +- java/com/facebook/csslayout/CSSNode.java | 1 + java/com/facebook/csslayout/CSSOverflow.java | 1 + java/com/facebook/csslayout/LayoutEngine.java | 26 +- tests/CSSLayoutMeasureModeTest.cpp | 294 ++++++++++++++++++ 7 files changed, 329 insertions(+), 30 deletions(-) create mode 100644 tests/CSSLayoutMeasureModeTest.cpp diff --git a/CSSLayout/CSSLayout.c b/CSSLayout/CSSLayout.c index 0b8748f1..dd55ed3f 100644 --- a/CSSLayout/CSSLayout.c +++ b/CSSLayout/CSSLayout.c @@ -183,6 +183,7 @@ void CSSNodeInit(const CSSNodeRef node) { // Such that the comparison is always going to be false node->layout.lastParentDirection = (CSSDirection) -1; node->layout.nextCachedMeasurementsIndex = 0; + node->layout.computedFlexBasis = CSSUndefined; node->layout.measuredDimensions[CSSDimensionWidth] = CSSUndefined; node->layout.measuredDimensions[CSSDimensionHeight] = CSSUndefined; @@ -193,6 +194,7 @@ void CSSNodeInit(const CSSNodeRef node) { void _CSSNodeMarkDirty(const CSSNodeRef node) { if (!node->isDirty) { node->isDirty = true; + node->layout.computedFlexBasis = CSSUndefined; if (node->parent) { _CSSNodeMarkDirty(node->parent); } @@ -451,6 +453,8 @@ _CSSNodePrint(const CSSNodeRef node, const CSSPrintOptions options, const uint32 printf("overflow: 'hidden', "); } else if (node->style.overflow == CSSOverflowVisible) { printf("overflow: 'visible', "); + } else if (node->style.overflow == CSSOverflowScroll) { + printf("overflow: 'scroll', "); } if (eqFour(node->style.margin)) { @@ -1117,8 +1121,10 @@ static void layoutNodeImpl(const CSSNodeRef node, getPaddingAndBorderAxis(child, CSSFlexDirectionColumn)); } else if (!CSSValueIsUndefined(child->style.flexBasis) && !CSSValueIsUndefined(availableInnerMainDim)) { - child->layout.computedFlexBasis = - fmaxf(child->style.flexBasis, getPaddingAndBorderAxis(child, mainAxis)); + if (CSSValueIsUndefined(child->layout.computedFlexBasis)) { + child->layout.computedFlexBasis = + fmaxf(child->style.flexBasis, getPaddingAndBorderAxis(child, mainAxis)); + } } else { // Compute the flex basis and hypothetical main size (i.e. the clamped // flex basis). @@ -1138,21 +1144,19 @@ static void layoutNodeImpl(const CSSNodeRef node, childHeightMeasureMode = CSSMeasureModeExactly; } - // According to the spec, if the main size is not definite and the - // child's inline axis is parallel to the main axis (i.e. it's - // horizontal), the child should be sized using "UNDEFINED" in - // the main size. Otherwise use "AT_MOST" in the cross axis. - if (!isMainAxisRow && CSSValueIsUndefined(childWidth) && - !CSSValueIsUndefined(availableInnerWidth)) { - childWidth = availableInnerWidth; - childWidthMeasureMode = CSSMeasureModeAtMost; - } - // The W3C spec doesn't say anything about the 'overflow' property, // but all major browsers appear to implement the following logic. - if (node->style.overflow == CSSOverflowHidden) { - if (isMainAxisRow && CSSValueIsUndefined(childHeight) && - !CSSValueIsUndefined(availableInnerHeight)) { + if ((!isMainAxisRow && node->style.overflow == CSSOverflowScroll) || + node->style.overflow != CSSOverflowScroll) { + if (CSSValueIsUndefined(childWidth) && !CSSValueIsUndefined(availableInnerWidth)) { + childWidth = availableInnerWidth; + childWidthMeasureMode = CSSMeasureModeAtMost; + } + } + + if ((isMainAxisRow && node->style.overflow == CSSOverflowScroll) || + node->style.overflow != CSSOverflowScroll) { + if (CSSValueIsUndefined(childHeight) && !CSSValueIsUndefined(availableInnerHeight)) { childHeight = availableInnerHeight; childHeightMeasureMode = CSSMeasureModeAtMost; } diff --git a/CSSLayout/CSSLayout.h b/CSSLayout/CSSLayout.h index f3353fdf..37c24d93 100644 --- a/CSSLayout/CSSLayout.h +++ b/CSSLayout/CSSLayout.h @@ -55,6 +55,7 @@ typedef enum CSSJustify { typedef enum CSSOverflow { CSSOverflowVisible, CSSOverflowHidden, + CSSOverflowScroll, } CSSOverflow; // Note: auto is only a valid value for alignSelf. It is NOT a valid value for diff --git a/java/com/facebook/csslayout/CSSLayout.java b/java/com/facebook/csslayout/CSSLayout.java index a77e2930..11f9f65f 100644 --- a/java/com/facebook/csslayout/CSSLayout.java +++ b/java/com/facebook/csslayout/CSSLayout.java @@ -51,7 +51,7 @@ public class CSSLayout { Arrays.fill(dimensions, CSSConstants.UNDEFINED); direction = CSSDirection.LTR; - computedFlexBasis = 0; + computedFlexBasis = CSSConstants.UNDEFINED; generationCount = 0; lastParentDirection = null; diff --git a/java/com/facebook/csslayout/CSSNode.java b/java/com/facebook/csslayout/CSSNode.java index 9e1bc07b..ae9f07b9 100644 --- a/java/com/facebook/csslayout/CSSNode.java +++ b/java/com/facebook/csslayout/CSSNode.java @@ -181,6 +181,7 @@ public class CSSNode implements CSSNodeAPI { } mLayoutState = LayoutState.DIRTY; + layout.computedFlexBasis = CSSConstants.UNDEFINED; if (mParent != null) { mParent.dirty(); diff --git a/java/com/facebook/csslayout/CSSOverflow.java b/java/com/facebook/csslayout/CSSOverflow.java index 29957a9a..9aae8225 100644 --- a/java/com/facebook/csslayout/CSSOverflow.java +++ b/java/com/facebook/csslayout/CSSOverflow.java @@ -12,4 +12,5 @@ package com.facebook.csslayout; public enum CSSOverflow { VISIBLE, HIDDEN, + SCROLL, } diff --git a/java/com/facebook/csslayout/LayoutEngine.java b/java/com/facebook/csslayout/LayoutEngine.java index b8edf8f9..7497fd2f 100644 --- a/java/com/facebook/csslayout/LayoutEngine.java +++ b/java/com/facebook/csslayout/LayoutEngine.java @@ -695,9 +695,9 @@ public class LayoutEngine { // The height is definite, so use that as the flex basis. child.layout.computedFlexBasis = Math.max(child.style.dimensions[DIMENSION_HEIGHT], ((child.style.padding.getWithFallback(leadingSpacing[CSS_FLEX_DIRECTION_COLUMN], leading[CSS_FLEX_DIRECTION_COLUMN]) + child.style.border.getWithFallback(leadingSpacing[CSS_FLEX_DIRECTION_COLUMN], leading[CSS_FLEX_DIRECTION_COLUMN])) + (child.style.padding.getWithFallback(trailingSpacing[CSS_FLEX_DIRECTION_COLUMN], trailing[CSS_FLEX_DIRECTION_COLUMN]) + child.style.border.getWithFallback(trailingSpacing[CSS_FLEX_DIRECTION_COLUMN], trailing[CSS_FLEX_DIRECTION_COLUMN])))); } else if (!isFlexBasisAuto(child) && !Float.isNaN(availableInnerMainDim)) { - - // If the basis isn't 'auto', it is assumed to be zero. - child.layout.computedFlexBasis = Math.max(child.style.flexBasis, ((child.style.padding.getWithFallback(leadingSpacing[mainAxis], leading[mainAxis]) + child.style.border.getWithFallback(leadingSpacing[mainAxis], leading[mainAxis])) + (child.style.padding.getWithFallback(trailingSpacing[mainAxis], trailing[mainAxis]) + child.style.border.getWithFallback(trailingSpacing[mainAxis], trailing[mainAxis])))); + if (Float.isNaN(child.layout.computedFlexBasis)) { + child.layout.computedFlexBasis = Math.max(child.style.flexBasis, ((child.style.padding.getWithFallback(leadingSpacing[mainAxis], leading[mainAxis]) + child.style.border.getWithFallback(leadingSpacing[mainAxis], leading[mainAxis])) + (child.style.padding.getWithFallback(trailingSpacing[mainAxis], trailing[mainAxis]) + child.style.border.getWithFallback(trailingSpacing[mainAxis], trailing[mainAxis])))); + } } else { // Compute the flex basis and hypothetical main size (i.e. the clamped flex basis). @@ -715,19 +715,17 @@ public class LayoutEngine { childHeightMeasureMode = CSSMeasureMode.EXACTLY; } - // According to the spec, if the main size is not definite and the - // child's inline axis is parallel to the main axis (i.e. it's - // horizontal), the child should be sized using "UNDEFINED" in - // the main size. Otherwise use "AT_MOST" in the cross axis. - if (!isMainAxisRow && Float.isNaN(childWidth) && !Float.isNaN(availableInnerWidth)) { - childWidth = availableInnerWidth; - childWidthMeasureMode = CSSMeasureMode.AT_MOST; - } - // The W3C spec doesn't say anything about the 'overflow' property, // but all major browsers appear to implement the following logic. - if (node.style.overflow == CSSOverflow.HIDDEN) { - if (isMainAxisRow && Float.isNaN(childHeight) && !Float.isNaN(availableInnerHeight)) { + if ((!isMainAxisRow && node.style.overflow == CSSOverflow.SCROLL) || node.style.overflow != CSSOverflow.SCROLL) { + if (Float.isNaN(childWidth) && !Float.isNaN(availableInnerWidth)) { + childWidth = availableInnerWidth; + childWidthMeasureMode = CSSMeasureMode.AT_MOST; + } + } + + if ((isMainAxisRow && node.style.overflow == CSSOverflow.SCROLL) || node.style.overflow != CSSOverflow.SCROLL) { + if (Float.isNaN(childHeight) && !Float.isNaN(availableInnerHeight)) { childHeight = availableInnerHeight; childHeightMeasureMode = CSSMeasureMode.AT_MOST; } diff --git a/tests/CSSLayoutMeasureModeTest.cpp b/tests/CSSLayoutMeasureModeTest.cpp new file mode 100644 index 00000000..27fd4240 --- /dev/null +++ b/tests/CSSLayoutMeasureModeTest.cpp @@ -0,0 +1,294 @@ +/** + * Copyright (c) 2014-present, Facebook, Inc. + * All rights reserved. + * + * This source code is licensed under the BSD-style license found in the + * LICENSE file in the root directory of this source tree. An additional grant + * of patent rights can be found in the PATENTS file in the same directory. + */ + +#include +#include + +struct _MeasureConstraint { + float width; + CSSMeasureMode widthMode; + float height; + CSSMeasureMode heightMode; +}; + +struct _MeasureConstraintList { + uint32_t length; + struct _MeasureConstraint *constraints; +}; + +static CSSSize _measure(void *context, + float width, + CSSMeasureMode widthMode, + float height, + CSSMeasureMode heightMode) { + struct _MeasureConstraintList *constraintList = (struct _MeasureConstraintList *)context; + struct _MeasureConstraint *constraints = constraintList->constraints; + uint32_t currentIndex = constraintList->length; + (&constraints[currentIndex])->width = width; + (&constraints[currentIndex])->widthMode = widthMode; + (&constraints[currentIndex])->height = height; + (&constraints[currentIndex])->heightMode = heightMode; + constraintList->length = currentIndex + 1; + + return CSSSize { + .width = widthMode == CSSMeasureModeUndefined ? 10 : width, + .height = heightMode == CSSMeasureModeUndefined ? 10 : width, + }; +} + +TEST(CSSLayoutTest, exactly_measure_stretched_child_column) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].width); + ASSERT_EQ(CSSMeasureModeExactly, constraintList.constraints[0].widthMode); +} + +TEST(CSSLayoutTest, exactly_measure_stretched_child_row) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetFlexDirection(root, CSSFlexDirectionRow); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeExactly, constraintList.constraints[0].heightMode); +} + +TEST(CSSLayoutTest, at_most_main_axis_column) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].heightMode); +} + +TEST(CSSLayoutTest, at_most_cross_axis_column) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetAlignItems(root, CSSAlignFlexStart); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].width); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].widthMode); +} + +TEST(CSSLayoutTest, at_most_main_axis_row) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetFlexDirection(root, CSSFlexDirectionRow); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].width); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].widthMode); +} + +TEST(CSSLayoutTest, at_most_cross_axis_row) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetFlexDirection(root, CSSFlexDirectionRow); + CSSNodeStyleSetAlignItems(root, CSSAlignFlexStart); + CSSNodeStyleSetWidth(root, 100); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].heightMode); +} + +TEST(CSSLayoutTest, flex_child) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeStyleSetFlexGrow(root_child0, 1); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(2, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].heightMode); + + ASSERT_EQ(100, constraintList.constraints[1].height); + ASSERT_EQ(CSSMeasureModeExactly, constraintList.constraints[1].heightMode); +} + +TEST(CSSLayoutTest, flex_child_with_flex_basis) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetHeight(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeStyleSetFlexGrow(root_child0, 1); + CSSNodeStyleSetFlexBasis(root_child0, 0); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeExactly, constraintList.constraints[0].heightMode); +} + +TEST(CSSLayoutTest, overflow_scroll_column) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetAlignItems(root, CSSAlignFlexStart); + CSSNodeStyleSetOverflow(root, CSSOverflowScroll); + CSSNodeStyleSetHeight(root, 100); + CSSNodeStyleSetWidth(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_EQ(100, constraintList.constraints[0].width); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].widthMode); + + ASSERT_TRUE(CSSValueIsUndefined(constraintList.constraints[0].height)); + ASSERT_EQ(CSSMeasureModeUndefined, constraintList.constraints[0].heightMode); +} + +TEST(CSSLayoutTest, overflow_scroll_row) { + struct _MeasureConstraintList constraintList = _MeasureConstraintList { + .length = 0, + .constraints = (struct _MeasureConstraint *) malloc(10 * sizeof(struct _MeasureConstraint)), + }; + + const CSSNodeRef root = CSSNodeNew(); + CSSNodeStyleSetAlignItems(root, CSSAlignFlexStart); + CSSNodeStyleSetFlexDirection(root, CSSFlexDirectionRow); + CSSNodeStyleSetOverflow(root, CSSOverflowScroll); + CSSNodeStyleSetHeight(root, 100); + CSSNodeStyleSetWidth(root, 100); + + const CSSNodeRef root_child0 = CSSNodeNew(); + CSSNodeSetContext(root_child0, &constraintList); + CSSNodeSetMeasureFunc(root_child0, _measure); + CSSNodeInsertChild(root, root_child0, 0); + + CSSNodeCalculateLayout(root, CSSUndefined, CSSUndefined, CSSDirectionLTR); + + ASSERT_EQ(1, constraintList.length); + + ASSERT_TRUE(CSSValueIsUndefined(constraintList.constraints[0].width)); + ASSERT_EQ(CSSMeasureModeUndefined, constraintList.constraints[0].widthMode); + + ASSERT_EQ(100, constraintList.constraints[0].height); + ASSERT_EQ(CSSMeasureModeAtMost, constraintList.constraints[0].heightMode); +} +