BREAKING - Fix unconstraint sizing in main axis

Emil Sjolander · Facebook Github Bot 6 · commit 0a9b6bedb312 · 2016-09-26T06:13:56.000-07:00
Summary:
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
diff --git a/Libraries/Components/ScrollView/ScrollView.js b/Libraries/Components/ScrollView/ScrollView.js
@@ -529,7 +529,7 @@ const ScrollView = React.createClass({
         return React.cloneElement(
           refreshControl,
           {style: props.style},
-          <ScrollViewClass {...props} style={baseStyle} ref={this._setScrollViewRef}>
+          <ScrollViewClass {...props} ref={this._setScrollViewRef}>
             {contentContainer}
           </ScrollViewClass>
         );
@@ -545,12 +545,14 @@ const ScrollView = React.createClass({
 
 const styles = StyleSheet.create({
   baseVertical: {
-    flex: 1,
+    flexGrow: 1,
     flexDirection: 'column',
+    overflow: 'scroll',
   },
   baseHorizontal: {
-    flex: 1,
+    flexGrow: 1,
     flexDirection: 'row',
+    overflow: 'scroll',
   },
   contentContainerHorizontal: {
     flexDirection: 'row',
diff --git a/Libraries/Components/View/ViewStylePropTypes.js b/Libraries/Components/View/ViewStylePropTypes.js
@@ -43,7 +43,6 @@ var ViewStylePropTypes = {
   borderBottomWidth: ReactPropTypes.number,
   borderLeftWidth: ReactPropTypes.number,
   opacity: ReactPropTypes.number,
-  overflow: ReactPropTypes.oneOf(['visible', 'hidden']),
   /**
    * (Android-only) Sets the elevation of a view, using Android's underlying
    * [elevation API](https://developer.android.com/training/material/shadows-clipping.html#Elevation).
diff --git a/Libraries/StyleSheet/LayoutPropTypes.js b/Libraries/StyleSheet/LayoutPropTypes.js
@@ -328,6 +328,19 @@ var LayoutPropTypes = {
     'stretch'
   ]),
 
+  /** `overflow` controls how a children are measured and displayed.
+   *  `overflow: hidden` causes views to be clipped while `overflow: scroll`
+   *  causes views to be measured independently of their parents main axis.`
+   *  It works like `overflow` in CSS (default: visible).
+   *  See https://developer.mozilla.org/en/docs/Web/CSS/overflow
+   *  for more details.
+   */
+  overflow: ReactPropTypes.oneOf([
+    'visible',
+    'hidden',
+    'scroll',
+  ]),
+
   /** In React Native `flex` does not work the same way that it does in CSS.
    *  `flex` is a number rather than a string, and it works
    *  according to the `css-layout` library
diff --git a/React/Base/RCTConvert.m b/React/Base/RCTConvert.m
@@ -612,7 +612,8 @@ + (NSPropertyList)NSPropertyList:(id)json
 
 RCT_ENUM_CONVERTER(CSSOverflow, (@{
   @"hidden": @(CSSOverflowHidden),
-  @"visible": @(CSSOverflowVisible)
+  @"visible": @(CSSOverflowVisible),
+  @"scroll": @(CSSOverflowScroll),
 }), CSSOverflowVisible, intValue)
 
 RCT_ENUM_CONVERTER(CSSFlexDirection, (@{
diff --git a/React/CSSLayout/CSSLayout.c b/React/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/React/CSSLayout/CSSLayout.h b/React/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/React/Views/RCTScrollViewManager.m b/React/Views/RCTScrollViewManager.m
@@ -11,6 +11,7 @@
 
 #import "RCTBridge.h"
 #import "RCTScrollView.h"
+#import "RCTShadowView.h"
 #import "RCTUIManager.h"
 
 @interface RCTScrollView (Private)
@@ -79,6 +80,15 @@ - (UIView *)view
 RCT_EXPORT_VIEW_PROPERTY(onMomentumScrollEnd, RCTDirectEventBlock)
 RCT_EXPORT_VIEW_PROPERTY(onScrollAnimationEnd, RCTDirectEventBlock)
 
+// overflow is used both in css-layout as well as by reac-native. In css-layout
+// we always want to treat overflow as scroll but depending on what the overflow
+// is set to from js we want to clip drawing or not. This piece of code ensures
+// that css-layout is always treating the contents of a scroll container as
+// overflow: 'scroll'.
+RCT_CUSTOM_SHADOW_PROPERTY(overflow, CSSOverflow, RCTShadowView) {
+  view.overflow = CSSOverflowScroll;
+}
+
 RCT_EXPORT_METHOD(getContentSize:(nonnull NSNumber *)reactTag
                   callback:(RCTResponseSenderBlock)callback)
 {
diff --git a/React/Views/RCTViewManager.h b/React/Views/RCTViewManager.h
@@ -117,4 +117,19 @@ RCT_REMAP_VIEW_PROPERTY(name, __custom__, type)         \
 #define RCT_EXPORT_SHADOW_PROPERTY(name, type) \
 + (NSArray<NSString *> *)propConfigShadow_##name { return @[@#type]; }
 
+/**
+ * This macro maps a named property to an arbitrary key path in the shadow view.
+ */
+#define RCT_REMAP_SHADOW_PROPERTY(name, keyPath, type) \
++ (NSArray<NSString *> *)propConfigShadow_##name { return @[@#type, @#keyPath]; }
+
+/**
+ * This macro can be used when you need to provide custom logic for setting
+ * shadow view properties. The macro should be followed by a method body, which can
+ * refer to "json", "view" and "defaultView" to implement the required logic.
+ */
+#define RCT_CUSTOM_SHADOW_PROPERTY(name, type, viewClass) \
+RCT_REMAP_SHADOW_PROPERTY(name, __custom__, type)         \
+- (void)set_##name:(id)json forShadowView:(viewClass *)view withDefaultView:(viewClass *)defaultView
+
 @end
diff --git a/React/Views/RCTViewManager.m b/React/Views/RCTViewManager.m
@@ -122,7 +122,7 @@ - (RCTViewManagerUIBlock)uiBlockToAmendWithShadowViewRegistry:(__unused NSDictio
 RCT_CUSTOM_VIEW_PROPERTY(overflow, CSSOverflow, RCTView)
 {
   if (json) {
-    view.clipsToBounds = [RCTConvert CSSOverflow:json] == CSSOverflowHidden;
+    view.clipsToBounds = [RCTConvert CSSOverflow:json] != CSSOverflowVisible;
   } else {
     view.clipsToBounds = defaultView.clipsToBounds;
   }
diff --git a/ReactAndroid/src/main/java/com/facebook/csslayout/CSSLayout.java b/ReactAndroid/src/main/java/com/facebook/csslayout/CSSLayout.java
@@ -51,7 +51,7 @@ public void resetResult() {
     Arrays.fill(dimensions, CSSConstants.UNDEFINED);
     direction = CSSDirection.LTR;
 
-    computedFlexBasis = 0;
+    computedFlexBasis = CSSConstants.UNDEFINED;
 
     generationCount = 0;
     lastParentDirection = null;
diff --git a/ReactAndroid/src/main/java/com/facebook/csslayout/CSSNode.java b/ReactAndroid/src/main/java/com/facebook/csslayout/CSSNode.java
@@ -181,6 +181,7 @@ public void dirty() {
     }
 
     mLayoutState = LayoutState.DIRTY;
+    layout.computedFlexBasis = CSSConstants.UNDEFINED;
 
     if (mParent != null) {
       mParent.dirty();
diff --git a/ReactAndroid/src/main/java/com/facebook/csslayout/CSSOverflow.java b/ReactAndroid/src/main/java/com/facebook/csslayout/CSSOverflow.java
@@ -12,4 +12,5 @@
 public enum CSSOverflow {
   VISIBLE,
   HIDDEN,
+  SCROLL,
 }
diff --git a/ReactAndroid/src/main/java/com/facebook/csslayout/LayoutEngine.java b/ReactAndroid/src/main/java/com/facebook/csslayout/LayoutEngine.java
@@ -695,9 +695,9 @@ private static void layoutNodeImpl(
           // 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 @@ private static void layoutNodeImpl(
             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/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java
@@ -10,6 +10,7 @@
 import com.facebook.csslayout.CSSConstants;
 import com.facebook.csslayout.CSSFlexDirection;
 import com.facebook.csslayout.CSSJustify;
+import com.facebook.csslayout.CSSOverflow;
 import com.facebook.csslayout.CSSPositionType;
 import com.facebook.csslayout.CSSWrap;
 import com.facebook.react.uimanager.annotations.ReactProp;
@@ -102,6 +103,12 @@ public void setJustifyContent(@Nullable String justifyContent) {
             justifyContent.toUpperCase(Locale.US).replace("-", "_")));
   }
 
+  @ReactProp(name = ViewProps.OVERFLOW)
+  public void setOverflow(@Nullable String overflow) {
+    setOverflow(overflow == null ? CSSOverflow.VISIBLE : CSSOverflow.valueOf(
+            overflow.toUpperCase(Locale.US).replace("-", "_")));
+  }
+
   @ReactPropGroup(names = {
       ViewProps.MARGIN,
       ViewProps.MARGIN_VERTICAL,
diff --git a/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/ViewProps.java
@@ -26,6 +26,7 @@ public class ViewProps {
   // !!! Keep in sync with LAYOUT_ONLY_PROPS below
   public static final String ALIGN_ITEMS = "alignItems";
   public static final String ALIGN_SELF = "alignSelf";
+  public static final String OVERFLOW = "overflow";
   public static final String BOTTOM = "bottom";
   public static final String COLLAPSABLE = "collapsable";
   public static final String FLEX = "flex";
@@ -113,6 +114,7 @@ public class ViewProps {
             FLEX_DIRECTION,
             FLEX_WRAP,
             JUSTIFY_CONTENT,
+            OVERFLOW,
 
             /* position */
             POSITION,

Original file line number	Diff line number	Diff line change
`@@ -122,7 +122,7 @@ - (RCTViewManagerUIBlock)uiBlockToAmendWithShadowViewRegistry:(__unused NSDictio`
`122`	`122`	`RCT_CUSTOM_VIEW_PROPERTY(overflow, CSSOverflow, RCTView)`
`123`	`123`	`{`
`124`	`124`	`if (json) {`
`125`		`- view.clipsToBounds = [RCTConvert CSSOverflow:json] == CSSOverflowHidden;`
	`125`	`+ view.clipsToBounds = [RCTConvert CSSOverflow:json] != CSSOverflowVisible;`
`126`	`126`	`} else {`
`127`	`127`	`view.clipsToBounds = defaultView.clipsToBounds;`
`128`	`128`	`}`