From 86c39b5e4fe42344c44910e5c9a6d0de22241fad Mon Sep 17 00:00:00 2001 From: Andrew Rasmussen Date: Fri, 1 May 2015 12:16:47 -0700 Subject: [PATCH] Revert "Ensure that flex children adopt their parent's cross-axis min dimension." This reverts commit b8316413b310643ea6555a015b3903f4fde1104e. This was causing issues for existing components built with React Native.. we need to investigate more in the future. --- src/Layout.c | 8 +- src/Layout.js | 8 +- src/__tests__/Layout-test.c | 125 ++++++++-------- src/__tests__/Layout-test.js | 13 +- .../com/facebook/csslayout/LayoutEngine.java | 8 +- .../facebook/csslayout/LayoutEngineTest.java | 133 ++++++++---------- 6 files changed, 132 insertions(+), 163 deletions(-) diff --git a/src/Layout.c b/src/Layout.c index ffb376cd..82831b9a 100644 --- a/src/Layout.c +++ b/src/Layout.c @@ -505,9 +505,9 @@ static void layoutNodeImpl(css_node_t *node, float parentMaxWidth) { child = node->get_child(node->context, i); float nextContentDim = 0; - // If it's a flexible child, accumulate the size that the child potentially - // contributes to the row - if (isFlex(child)) { + // It only makes sense to consider a child flexible if we have a computed + // dimension for the node-> + if (!isUndefined(node->layout.dimensions[dim[mainAxis]]) && isFlex(child)) { flexibleChildrenCount++; totalFlexible += getFlex(child); @@ -573,7 +573,7 @@ static void layoutNodeImpl(css_node_t *node, float parentMaxWidth) { if (!isUndefined(node->layout.dimensions[dim[mainAxis]])) { remainingMainDim = definedMainDim - mainContentDim; } else { - remainingMainDim = boundAxis(node, mainAxis, fmaxf(mainContentDim, 0)) - mainContentDim; + remainingMainDim = fmaxf(mainContentDim, 0) - mainContentDim; } // If there are flexible children in the mix, they are going to fill the diff --git a/src/Layout.js b/src/Layout.js index 34505c1e..cd48d6ce 100755 --- a/src/Layout.js +++ b/src/Layout.js @@ -396,9 +396,9 @@ var computeLayout = (function() { child = node.children[i]; var/*float*/ nextContentDim = 0; - // If it's a flexible child, accumulate the size that the child potentially - // contributes to the row - if (isFlex(child)) { + // It only makes sense to consider a child flexible if we have a computed + // dimension for the node. + if (!isUndefined(node.layout[dim[mainAxis]]) && isFlex(child)) { flexibleChildrenCount++; totalFlexible += getFlex(child); @@ -464,7 +464,7 @@ var computeLayout = (function() { if (!isUndefined(node.layout[dim[mainAxis]])) { remainingMainDim = definedMainDim - mainContentDim; } else { - remainingMainDim = boundAxis(node, mainAxis, fmaxf(mainContentDim, 0)) - mainContentDim; + remainingMainDim = fmaxf(mainContentDim, 0) - mainContentDim; } // If there are flexible children in the mix, they are going to fill the diff --git a/src/__tests__/Layout-test.c b/src/__tests__/Layout-test.c index 7f860c75..c1bb8ff5 100644 --- a/src/__tests__/Layout-test.c +++ b/src/__tests__/Layout-test.c @@ -4688,73 +4688,6 @@ int main() test("should layout node with position absolute, top and left and min bounds", root_node, root_layout); } - { - css_node_t *root_node = new_test_css_node(); - { - css_node_t *node_0 = root_node; - node_0->style.minDimensions[CSS_HEIGHT] = 800; - init_css_node_children(node_0, 1); - { - css_node_t *node_1; - node_1 = node_0->get_child(node_0->context, 0); - node_1->style.flex = 1; - } - } - - css_node_t *root_layout = new_test_css_node(); - { - css_node_t *node_0 = root_layout; - node_0->layout.position[CSS_TOP] = 0; - node_0->layout.position[CSS_LEFT] = 0; - node_0->layout.dimensions[CSS_WIDTH] = 0; - node_0->layout.dimensions[CSS_HEIGHT] = 800; - init_css_node_children(node_0, 1); - { - css_node_t *node_1; - node_1 = node_0->get_child(node_0->context, 0); - node_1->layout.position[CSS_TOP] = 0; - node_1->layout.position[CSS_LEFT] = 0; - node_1->layout.dimensions[CSS_WIDTH] = 0; - node_1->layout.dimensions[CSS_HEIGHT] = 800; - } - } - - test("should layout minHeight with a flex child", root_node, root_layout); - } - - { - css_node_t *root_node = new_test_css_node(); - { - css_node_t *node_0 = root_node; - node_0->style.minDimensions[CSS_HEIGHT] = 800; - init_css_node_children(node_0, 1); - { - css_node_t *node_1; - node_1 = node_0->get_child(node_0->context, 0); - } - } - - css_node_t *root_layout = new_test_css_node(); - { - css_node_t *node_0 = root_layout; - node_0->layout.position[CSS_TOP] = 0; - node_0->layout.position[CSS_LEFT] = 0; - node_0->layout.dimensions[CSS_WIDTH] = 0; - node_0->layout.dimensions[CSS_HEIGHT] = 800; - init_css_node_children(node_0, 1); - { - css_node_t *node_1; - node_1 = node_0->get_child(node_0->context, 0); - node_1->layout.position[CSS_TOP] = 0; - node_1->layout.position[CSS_LEFT] = 0; - node_1->layout.dimensions[CSS_WIDTH] = 0; - node_1->layout.dimensions[CSS_HEIGHT] = 0; - } - } - - test("should layout minHeight without a flex child", root_node, root_layout); - } - { css_node_t *root_node = new_test_css_node(); { @@ -4882,6 +4815,64 @@ int main() test("should layout absolutely positioned node with absolutely positioned padded and bordered parent", root_node, root_layout); } + + { + css_node_t *root_node = new_test_css_node(); + { + css_node_t *node_0 = root_node; + node_0->style.dimensions[CSS_WIDTH] = 400; + node_0->style.dimensions[CSS_HEIGHT] = 400; + init_css_node_children(node_0, 1); + { + css_node_t *node_1; + node_1 = node_0->get_child(node_0->context, 0); + node_1->style.flex = 1; + node_1->style.padding[CSS_LEFT] = 10; + node_1->style.padding[CSS_TOP] = 10; + node_1->style.padding[CSS_RIGHT] = 10; + node_1->style.padding[CSS_BOTTOM] = 10; + init_css_node_children(node_1, 1); + { + css_node_t *node_2; + node_2 = node_1->get_child(node_1->context, 0); + node_2->style.position_type = CSS_POSITION_ABSOLUTE; + node_2->style.position[CSS_LEFT] = 10; + node_2->style.position[CSS_TOP] = 10; + node_2->style.position[CSS_RIGHT] = 10; + node_2->style.position[CSS_BOTTOM] = 10; + } + } + } + + css_node_t *root_layout = new_test_css_node(); + { + css_node_t *node_0 = root_layout; + node_0->layout.position[CSS_TOP] = 0; + node_0->layout.position[CSS_LEFT] = 0; + node_0->layout.dimensions[CSS_WIDTH] = 400; + node_0->layout.dimensions[CSS_HEIGHT] = 400; + init_css_node_children(node_0, 1); + { + css_node_t *node_1; + node_1 = node_0->get_child(node_0->context, 0); + node_1->layout.position[CSS_TOP] = 0; + node_1->layout.position[CSS_LEFT] = 0; + node_1->layout.dimensions[CSS_WIDTH] = 400; + node_1->layout.dimensions[CSS_HEIGHT] = 400; + init_css_node_children(node_1, 1); + { + css_node_t *node_2; + node_2 = node_1->get_child(node_1->context, 0); + node_2->layout.position[CSS_TOP] = 10; + node_2->layout.position[CSS_LEFT] = 10; + node_2->layout.dimensions[CSS_WIDTH] = 380; + node_2->layout.dimensions[CSS_HEIGHT] = 380; + } + } + } + + test("should layout absolutely positioned node with padded flex 1 parent", root_node, root_layout); + } /** END_GENERATED **/ return tests_finished(); } diff --git a/src/__tests__/Layout-test.js b/src/__tests__/Layout-test.js index a44044dc..f0bb41e4 100755 --- a/src/__tests__/Layout-test.js +++ b/src/__tests__/Layout-test.js @@ -1513,7 +1513,7 @@ describe('Layout', function() { ); }); - it('should layout minHeight with a flex child', function() { + xit('should layout minHeight with a flex child', function() { testLayout( {style: {minHeight: 800}, children: [ {style: {flex: 1}} @@ -1524,17 +1524,6 @@ describe('Layout', function() { ); }); - it('should layout minHeight without a flex child', function() { - testLayout( - {style: {minHeight: 800}, children: [ - {style: {}} - ]}, - {width: 0, height: 800, top: 0, left: 0, children: [ - {width: 0, height: 0, top: 0, left: 0} - ]} - ); - }); - xit('should layout node with a nested sibling child with width', function() { testLayout( {style: {}, children: [ diff --git a/src/java/src/com/facebook/csslayout/LayoutEngine.java b/src/java/src/com/facebook/csslayout/LayoutEngine.java index 623d024a..5d77fbbc 100644 --- a/src/java/src/com/facebook/csslayout/LayoutEngine.java +++ b/src/java/src/com/facebook/csslayout/LayoutEngine.java @@ -450,9 +450,9 @@ public class LayoutEngine { child = node.getChildAt(i); float nextContentDim = 0; - // If it's a flexible child, accumulate the size that the child potentially - // contributes to the row - if (isFlex(child)) { + // It only makes sense to consider a child flexible if we have a computed + // dimension for the node. + if (!CSSConstants.isUndefined(getLayoutDimension(node, getDim(mainAxis))) && isFlex(child)) { flexibleChildrenCount++; totalFlexible = totalFlexible + getFlex(child); @@ -518,7 +518,7 @@ public class LayoutEngine { if (!CSSConstants.isUndefined(getLayoutDimension(node, getDim(mainAxis)))) { remainingMainDim = definedMainDim - mainContentDim; } else { - remainingMainDim = boundAxis(node, mainAxis, Math.max(mainContentDim, 0)) - mainContentDim; + remainingMainDim = Math.max(mainContentDim, 0) - mainContentDim; } // If there are flexible children in the mix, they are going to fill the diff --git a/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java b/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java index 52ba50b2..ce0e890c 100644 --- a/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java +++ b/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java @@ -5005,77 +5005,6 @@ public class LayoutEngineTest { @Test public void testCase120() - { - TestCSSNode root_node = new TestCSSNode(); - { - TestCSSNode node_0 = root_node; - node_0.style.minHeight = 800; - addChildren(node_0, 1); - { - TestCSSNode node_1; - node_1 = node_0.getChildAt(0); - node_1.style.flex = 1; - } - } - - TestCSSNode root_layout = new TestCSSNode(); - { - TestCSSNode node_0 = root_layout; - node_0.layout.y = 0; - node_0.layout.x = 0; - node_0.layout.width = 0; - node_0.layout.height = 800; - addChildren(node_0, 1); - { - TestCSSNode node_1; - node_1 = node_0.getChildAt(0); - node_1.layout.y = 0; - node_1.layout.x = 0; - node_1.layout.width = 0; - node_1.layout.height = 800; - } - } - - test("should layout minHeight with a flex child", root_node, root_layout); - } - - @Test - public void testCase121() - { - TestCSSNode root_node = new TestCSSNode(); - { - TestCSSNode node_0 = root_node; - node_0.style.minHeight = 800; - addChildren(node_0, 1); - { - TestCSSNode node_1; - node_1 = node_0.getChildAt(0); - } - } - - TestCSSNode root_layout = new TestCSSNode(); - { - TestCSSNode node_0 = root_layout; - node_0.layout.y = 0; - node_0.layout.x = 0; - node_0.layout.width = 0; - node_0.layout.height = 800; - addChildren(node_0, 1); - { - TestCSSNode node_1; - node_1 = node_0.getChildAt(0); - node_1.layout.y = 0; - node_1.layout.x = 0; - node_1.layout.width = 0; - node_1.layout.height = 0; - } - } - - test("should layout minHeight without a flex child", root_node, root_layout); - } - - @Test - public void testCase122() { TestCSSNode root_node = new TestCSSNode(); { @@ -5139,7 +5068,7 @@ public class LayoutEngineTest { } @Test - public void testCase123() + public void testCase121() { TestCSSNode root_node = new TestCSSNode(); { @@ -5205,5 +5134,65 @@ public class LayoutEngineTest { test("should layout absolutely positioned node with absolutely positioned padded and bordered parent", root_node, root_layout); } + + @Test + public void testCase122() + { + TestCSSNode root_node = new TestCSSNode(); + { + TestCSSNode node_0 = root_node; + node_0.style.width = 400; + node_0.style.height = 400; + addChildren(node_0, 1); + { + TestCSSNode node_1; + node_1 = node_0.getChildAt(0); + node_1.style.flex = 1; + node_1.style.padding[Spacing.LEFT] = 10; + node_1.style.padding[Spacing.TOP] = 10; + node_1.style.padding[Spacing.RIGHT] = 10; + node_1.style.padding[Spacing.BOTTOM] = 10; + addChildren(node_1, 1); + { + TestCSSNode node_2; + node_2 = node_1.getChildAt(0); + node_2.style.positionType = CSSPositionType.ABSOLUTE; + node_2.style.positionLeft = 10; + node_2.style.positionTop = 10; + node_2.style.positionRight = 10; + node_2.style.positionBottom = 10; + } + } + } + + TestCSSNode root_layout = new TestCSSNode(); + { + TestCSSNode node_0 = root_layout; + node_0.layout.y = 0; + node_0.layout.x = 0; + node_0.layout.width = 400; + node_0.layout.height = 400; + addChildren(node_0, 1); + { + TestCSSNode node_1; + node_1 = node_0.getChildAt(0); + node_1.layout.y = 0; + node_1.layout.x = 0; + node_1.layout.width = 400; + node_1.layout.height = 400; + addChildren(node_1, 1); + { + TestCSSNode node_2; + node_2 = node_1.getChildAt(0); + node_2.layout.y = 10; + node_2.layout.x = 10; + node_2.layout.width = 380; + node_2.layout.height = 380; + } + } + } + + test("should layout absolutely positioned node with padded flex 1 parent", root_node, root_layout); + } /** END_GENERATED **/ }