From 8f6a96adbcb2b0389f948b8113d8b7cb8991bb2d Mon Sep 17 00:00:00 2001 From: Russell Keith-Magee Date: Sun, 22 Mar 2015 14:36:16 +0800 Subject: [PATCH] Normalized C and Java definition of isDimDefined. The JavaScript implementation of isDimDefined contains a check to ensure that the dimension value is positive; the C and Java versions did not have this check. As a result, a negative style value for 'width' (such as that used by the "should layout node with negative width" test) would have different layout under the C/Java implementation to the JavaScript implementation. This was hidden because the C/Java transpilers filtered out any negative instantiation values from the test suite. In effect, the negative value tests weren't running on the C/Java implementation. This patch removes the negative value filter from the transpiler, and makes the isDimDefined definition consistent between the three implementations. --- src/Layout.c | 3 +- src/__tests__/Layout-test.c | 2 + .../com/facebook/csslayout/LayoutEngine.java | 3 +- .../facebook/csslayout/LayoutEngineTest.java | 2 + src/transpile.js | 44 +++++++++---------- 5 files changed, 29 insertions(+), 25 deletions(-) diff --git a/src/Layout.c b/src/Layout.c index b31e7e3b..832c2d34 100644 --- a/src/Layout.c +++ b/src/Layout.c @@ -287,7 +287,8 @@ static float getDimWithMargin(css_node_t *node, css_flex_direction_t axis) { } static bool isDimDefined(css_node_t *node, css_flex_direction_t axis) { - return !isUndefined(node->style.dimensions[dim[axis]]); + float value = node->style.dimensions[dim[axis]]; + return !isUndefined(value) && value > 0.0; } static bool isPosDefined(css_node_t *node, css_position_t position) { diff --git a/src/__tests__/Layout-test.c b/src/__tests__/Layout-test.c index 888f6d90..65558774 100644 --- a/src/__tests__/Layout-test.c +++ b/src/__tests__/Layout-test.c @@ -2405,6 +2405,7 @@ int main() css_node_t *root_node = new_test_css_node(); { css_node_t *node_0 = root_node; + node_0->style.dimensions[CSS_WIDTH] = -31; init_css_node_children(node_0, 1); { css_node_t *node_1; @@ -3109,6 +3110,7 @@ int main() css_node_t *node_1; node_1 = node_0->get_child(node_0->context, 0); node_1->style.align_self = CSS_ALIGN_FLEX_START; + node_1->style.flex = -2.5; node_1 = node_0->get_child(node_0->context, 1); node_1->style.align_self = CSS_ALIGN_FLEX_START; node_1->style.flex = 0; diff --git a/src/java/src/com/facebook/csslayout/LayoutEngine.java b/src/java/src/com/facebook/csslayout/LayoutEngine.java index 83558abf..79e520f6 100644 --- a/src/java/src/com/facebook/csslayout/LayoutEngine.java +++ b/src/java/src/com/facebook/csslayout/LayoutEngine.java @@ -116,7 +116,8 @@ public class LayoutEngine { } private static boolean isDimDefined(CSSNode node, CSSFlexDirection axis) { - return !CSSConstants.isUndefined(getStyleDimension(node, getDim(axis))); + float value = getStyleDimension(node, getDim(axis)); + return !CSSConstants.isUndefined(value) && value > 0.0; } private static boolean isPosDefined(CSSNode node, PositionIndex position) { diff --git a/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java b/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java index b34ee12d..b32dc12d 100644 --- a/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java +++ b/src/java/tests/com/facebook/csslayout/LayoutEngineTest.java @@ -2603,6 +2603,7 @@ public class LayoutEngineTest { TestCSSNode root_node = new TestCSSNode(); { TestCSSNode node_0 = root_node; + node_0.style.width = -31; addChildren(node_0, 1); { TestCSSNode node_1; @@ -3341,6 +3342,7 @@ public class LayoutEngineTest { TestCSSNode node_1; node_1 = node_0.getChildAt(0); node_1.style.alignSelf = CSSAlign.FLEX_START; + node_1.style.flex = -2.5f; node_1 = node_0.getChildAt(1); node_1.style.alignSelf = CSSAlign.FLEX_START; node_1.style.flex = 0; diff --git a/src/transpile.js b/src/transpile.js index 9dbdc6af..7fab697f 100644 --- a/src/transpile.js +++ b/src/transpile.js @@ -86,24 +86,22 @@ function printLayout(test) { } } - function addFloat(positive, node, jsKey, cKey) { + function addFloat(node, jsKey, cKey) { if (jsKey in node.style) { - if (positive !== 'positive' || node.style[jsKey] >= 0) { - addStyle(cKey + ' = ' + node.style[jsKey] + ';'); - } + addStyle(cKey + ' = ' + node.style[jsKey] + ';'); } } - function addSpacing(positive, node, spacing, suffix) { - addFloat(positive, node, spacing + suffix, spacing + '[CSS_LEFT]'); - addFloat(positive, node, spacing + suffix, spacing + '[CSS_TOP]'); - addFloat(positive, node, spacing + suffix, spacing + '[CSS_RIGHT]'); - addFloat(positive, node, spacing + suffix, spacing + '[CSS_BOTTOM]'); + function addSpacing(node, spacing, suffix) { + addFloat(node, spacing + suffix, spacing + '[CSS_LEFT]'); + addFloat(node, spacing + suffix, spacing + '[CSS_TOP]'); + addFloat(node, spacing + suffix, spacing + '[CSS_RIGHT]'); + addFloat(node, spacing + suffix, spacing + '[CSS_BOTTOM]'); - addFloat(positive, node, spacing + 'Left' + suffix, spacing + '[CSS_LEFT]'); - addFloat(positive, node, spacing + 'Top' + suffix, spacing + '[CSS_TOP]'); - addFloat(positive, node, spacing + 'Right' + suffix, spacing + '[CSS_RIGHT]'); - addFloat(positive, node, spacing + 'Bottom' + suffix, spacing + '[CSS_BOTTOM]'); + addFloat(node, spacing + 'Left' + suffix, spacing + '[CSS_LEFT]'); + addFloat(node, spacing + 'Top' + suffix, spacing + '[CSS_TOP]'); + addFloat(node, spacing + 'Right' + suffix, spacing + '[CSS_RIGHT]'); + addFloat(node, spacing + 'Bottom' + suffix, spacing + '[CSS_BOTTOM]'); } function addMeasure(node) { @@ -144,16 +142,16 @@ function printLayout(test) { 'nowrap': 'CSS_NOWRAP', 'wrap': 'CSS_WRAP' }); - addFloat('positive', node, 'flex', 'flex'); - addFloat('positive', node, 'width', 'dimensions[CSS_WIDTH]'); - addFloat('positive', node, 'height', 'dimensions[CSS_HEIGHT]'); - addSpacing('all', node, 'margin', ''); - addSpacing('positive', node, 'padding', ''); - addSpacing('positive', node, 'border', 'Width'); - addFloat('all', node, 'left', 'position[CSS_LEFT]'); - addFloat('all', node, 'top', 'position[CSS_TOP]'); - addFloat('all', node, 'right', 'position[CSS_RIGHT]'); - addFloat('all', node, 'bottom', 'position[CSS_BOTTOM]'); + addFloat(node, 'flex', 'flex'); + addFloat(node, 'width', 'dimensions[CSS_WIDTH]'); + addFloat(node, 'height', 'dimensions[CSS_HEIGHT]'); + addSpacing(node, 'margin', ''); + addSpacing(node, 'padding', ''); + addSpacing(node, 'border', 'Width'); + addFloat(node, 'left', 'position[CSS_LEFT]'); + addFloat(node, 'top', 'position[CSS_TOP]'); + addFloat(node, 'right', 'position[CSS_RIGHT]'); + addFloat(node, 'bottom', 'position[CSS_BOTTOM]'); addMeasure(node); if (node.children) {