From 0a9b6bedb312eba22c5bc11498b1cc41363e5f27 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: 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 --- Libraries/Components/ScrollView/ScrollView.js | 8 +++-- .../Components/View/ViewStylePropTypes.js | 1 - Libraries/StyleSheet/LayoutPropTypes.js | 13 +++++++ React/Base/RCTConvert.m | 3 +- React/CSSLayout/CSSLayout.c | 34 +++++++++++-------- React/CSSLayout/CSSLayout.h | 1 + React/Views/RCTScrollViewManager.m | 10 ++++++ React/Views/RCTViewManager.h | 15 ++++++++ React/Views/RCTViewManager.m | 2 +- .../com/facebook/csslayout/CSSLayout.java | 2 +- .../java/com/facebook/csslayout/CSSNode.java | 1 + .../com/facebook/csslayout/CSSOverflow.java | 1 + .../com/facebook/csslayout/LayoutEngine.java | 26 +++++++------- .../react/uimanager/LayoutShadowNode.java | 7 ++++ .../facebook/react/uimanager/ViewProps.java | 2 ++ 15 files changed, 90 insertions(+), 36 deletions(-) diff --git a/Libraries/Components/ScrollView/ScrollView.js b/Libraries/Components/ScrollView/ScrollView.js index 342c414cd..611440b3a 100644 --- 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}, - + {contentContainer} ); @@ -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 index 136ab5c50..9990b9971 100644 --- 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 index 3ac6aff64..ea1020ad9 100644 --- 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 index a27afb59e..6b1997605 100644 --- a/React/Base/RCTConvert.m +++ b/React/Base/RCTConvert.m @@ -612,7 +612,8 @@ RCT_ENUM_CONVERTER(css_backface_visibility_t, (@{ 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 index 0b8748f14..dd55ed3fe 100644 --- 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 index f3353fdf0..37c24d935 100644 --- 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 index 9eb99a7c5..2b2a23641 100644 --- 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 @@ RCT_EXPORT_VIEW_PROPERTY(onMomentumScrollBegin, RCTDirectEventBlock) 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 index 71df14862..14bb93964 100644 --- 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 *)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 *)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 index ea8b92d8b..012200d1f 100644 --- a/React/Views/RCTViewManager.m +++ b/React/Views/RCTViewManager.m @@ -122,7 +122,7 @@ RCT_REMAP_VIEW_PROPERTY(shadowRadius, layer.shadowRadius, CGFloat) 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 index a77e2930e..11f9f65f7 100644 --- 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 class CSSLayout { 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 index 9e1bc07be..ae9f07b92 100644 --- 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 class CSSNode implements CSSNodeAPI { } 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 index 29957a9a7..9aae82250 100644 --- a/ReactAndroid/src/main/java/com/facebook/csslayout/CSSOverflow.java +++ b/ReactAndroid/src/main/java/com/facebook/csslayout/CSSOverflow.java @@ -12,4 +12,5 @@ package com.facebook.csslayout; 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 index b8edf8f97..7497fd2f1 100644 --- a/ReactAndroid/src/main/java/com/facebook/csslayout/LayoutEngine.java +++ b/ReactAndroid/src/main/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/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java b/ReactAndroid/src/main/java/com/facebook/react/uimanager/LayoutShadowNode.java index 710153085..ae5828323 100644 --- 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.CSSAlign; 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 class LayoutShadowNode extends ReactShadowNode { 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 index 469e0c708..ef9c9956e 100644 --- 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,