Should never be reached failure in WebCore::RenderFlexibleBox::alignChildren
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Aug 2016 14:45:43 +0000 (14:45 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 28 Aug 2016 14:45:43 +0000 (14:45 +0000)
https://bugs.webkit.org/show_bug.cgi?id=151591
<rdar://problem/27711829>

Reviewed by Darin Adler.

Source/WebCore:

The align-self and align-items CSS properties were originally defined in the
Flexbible Box specification. The new CSS Box Alignment specification tries
to generalize them so they can be used by other layout models, as it's  the
case of the GridLayout spec.

Since we have implemented the Grid Layout spec behind a runtime flag, we should
do the same with the new syntax of these properties.

Test: css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html

* css/CSSParser.cpp:
(WebCore::isValidKeywordPropertyAndValue):
(WebCore::isKeywordPropertyID):
(WebCore::CSSParser::parseValue):
* rendering/RenderFlexibleBox.cpp:
(WebCore::RenderFlexibleBox::alignChildren):

LayoutTests:

Test to verify that align-self and align-items CSS properties use the old
syntax when the Grid Layout feature is not enabled.

* css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt: Added.
* css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@205102 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt [new file with mode: 0644]
LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/css/parser/CSSParser.cpp
Source/WebCore/rendering/RenderFlexibleBox.cpp

index 9e68d05..f12f06d 100644 (file)
@@ -1,3 +1,17 @@
+2016-08-28  Javier Fernandez  <jfernandez@igalia.com>
+
+        Should never be reached failure in WebCore::RenderFlexibleBox::alignChildren
+        https://bugs.webkit.org/show_bug.cgi?id=151591
+        <rdar://problem/27711829>
+
+        Reviewed by Darin Adler.
+
+        Test to verify that align-self and align-items CSS properties use the old
+        syntax when the Grid Layout feature is not enabled.
+
+        * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt: Added.
+        * css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html: Added.
+
 2016-08-28  Frederic Wang  <fwang@igalia.com>
 
         Improve parsing of the menclose notation attribute value
diff --git a/LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt b/LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled-expected.txt
new file mode 100644 (file)
index 0000000..f883b2e
--- /dev/null
@@ -0,0 +1,57 @@
+Test to verify that the new alignment values are parsed as invalid if Grid Layout is disabled and in any case they do not cause a crash because assertions in flexbox layout code.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+
+New alignment values should be invalid when grid layout is disabled
+
+Testing Self-Alignment values.
+PASS alignSelf is 'flex-start'
+PASS alignSelf is 'flex-start'
+PASS alignSelf is 'flex-start'
+PASS alignSelf is 'flex-start'
+PASS alignSelf is 'flex-start'
+PASS alignSelf is 'flex-start'
+
+Testing Default-Alignment values.
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+PASS alignItems is 'flex-end'
+PASS alignSelf is 'flex-end'
+
+Even when grid layout is enabled, new values should not violate assertions in FlexibleBox layout logic.
+
+Testing Self-Alignment values.
+PASS alignSelf is 'start unsafe'
+PASS alignSelf is 'start'
+PASS alignSelf is 'end'
+PASS alignSelf is 'flex-start safe'
+PASS alignSelf is 'self-start'
+PASS alignSelf is 'self-end'
+
+Testing Default-Alignment values.
+PASS alignItems is 'start unsafe'
+PASS alignSelf is 'start unsafe'
+PASS alignItems is 'start'
+PASS alignSelf is 'start'
+PASS alignItems is 'end'
+PASS alignSelf is 'end'
+PASS alignItems is 'flex-start safe'
+PASS alignSelf is 'flex-start safe'
+PASS alignItems is 'self-start'
+PASS alignSelf is 'self-start'
+PASS alignItems is 'self-end'
+PASS alignSelf is 'self-end'
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html b/LayoutTests/css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html
new file mode 100644 (file)
index 0000000..8751fef
--- /dev/null
@@ -0,0 +1,73 @@
+<!DOCTYPE html>
+<div id="flexContainer" style="display: flex">
+    <div id="flexItem"></div>
+</div>
+<script src="../../resources/js-test.js"></script>
+<script>
+description('Test to verify that the new alignment values are parsed as invalid if Grid Layout is disabled and in any case they do not cause a crash because assertions in flexbox layout code.');
+
+function checkAlignSelfValue(value, computedValue, gridEnabled)
+{
+    item.style.webkitAlignSelf = value;
+    alignSelf = getComputedStyle(item, '').getPropertyValue('-webkit-align-self');
+    if (gridEnabled)
+        shouldBe("alignSelf", computedValue);
+    else
+        shouldBe("alignSelf", "'flex-start'");
+}
+
+function checkAlignItemsValue(value, computedValue, gridEnabled)
+{
+    container.style.webkitAlignItems = value;
+    alignItems = getComputedStyle(container, '').getPropertyValue('-webkit-align-items');
+    alignSelf = getComputedStyle(item, '').getPropertyValue('-webkit-align-self');
+    if (gridEnabled) {
+        shouldBe("alignItems", computedValue);
+        shouldBe("alignSelf", computedValue);
+    } else {
+        shouldBe("alignItems", "'flex-end'");
+        shouldBe("alignSelf", "'flex-end'");
+    }
+}
+
+function checkAlignmentValues(gridEnabled)
+{
+    if (window.internals)
+        internals.setCSSGridLayoutEnabled(gridEnabled);
+
+    debug('<br>Testing Self-Alignment values.');
+    checkAlignSelfValue("start unsafe", "'start unsafe'", gridEnabled)
+    checkAlignSelfValue("start", "'start'", gridEnabled)
+    checkAlignSelfValue("end", "'end'", gridEnabled)
+    checkAlignSelfValue("flex-start safe", "'flex-start safe'", gridEnabled)
+    checkAlignSelfValue("self-start", "'self-start'", gridEnabled)
+    checkAlignSelfValue("self-end", "'self-end'", gridEnabled)
+
+    item.style.webkitAlignSelf = "auto";
+
+    debug('<br>Testing Default-Alignment values.');
+    checkAlignItemsValue("start unsafe", "'start unsafe'", gridEnabled)
+    checkAlignItemsValue("start", "'start'", gridEnabled)
+    checkAlignItemsValue("end", "'end'", gridEnabled)
+    checkAlignItemsValue("flex-start safe", "'flex-start safe'", gridEnabled)
+    checkAlignItemsValue("self-start", "'self-start'", gridEnabled)
+    checkAlignItemsValue("self-end", "'self-end'", gridEnabled)
+
+    item.style.webkitAlignSelf = "flex-start";
+}
+
+var container = document.getElementById("flexContainer");
+var item = document.getElementById("flexItem");
+
+container.style.webkitAlignItems = "flex-end";
+item.style.webkitAlignSelf = "flex-start";
+var alignSelf = "flex-start";
+var alignItems = "flex-start";
+
+debug('<br>New alignment values should be invalid when grid layout is disabled');
+checkAlignmentValues(false);
+
+debug('<br>Even when grid layout is enabled, new values should not violate assertions in FlexibleBox layout logic.');
+checkAlignmentValues(true);
+
+</script>
index 7f9d089..6a9ed54 100644 (file)
@@ -1,3 +1,28 @@
+2016-08-28  Javier Fernandez  <jfernandez@igalia.com>
+
+        Should never be reached failure in WebCore::RenderFlexibleBox::alignChildren
+        https://bugs.webkit.org/show_bug.cgi?id=151591
+        <rdar://problem/27711829>
+
+        Reviewed by Darin Adler.
+
+        The align-self and align-items CSS properties were originally defined in the
+        Flexbible Box specification. The new CSS Box Alignment specification tries
+        to generalize them so they can be used by other layout models, as it's  the
+        case of the GridLayout spec.
+
+        Since we have implemented the Grid Layout spec behind a runtime flag, we should
+        do the same with the new syntax of these properties.
+
+        Test: css3/flexbox/new-alignment-values-invalid-if-grid-not-enabled.html
+
+        * css/CSSParser.cpp:
+        (WebCore::isValidKeywordPropertyAndValue):
+        (WebCore::isKeywordPropertyID):
+        (WebCore::CSSParser::parseValue):
+        * rendering/RenderFlexibleBox.cpp:
+        (WebCore::RenderFlexibleBox::alignChildren):
+
 2016-08-28  Frederic Wang  <fwang@igalia.com>
 
         Improve parsing of the menclose notation attribute value
index 0f62a38..e078695 100644 (file)
@@ -819,6 +819,18 @@ static inline bool isValidKeywordPropertyAndValue(CSSPropertyID propertyId, int
         if (valueID == CSSValueAuto || valueID == CSSValueBalance)
             return true;
         break;
+    case CSSPropertyAlignItems:
+        // FIXME: Per CSS alignment, this property should accept the same arguments as 'justify-self' so we should share its parsing code.
+        // FIXME: For now, we will do it behind the GRID_LAYOUT compile flag.
+        if (valueID == CSSValueFlexStart || valueID == CSSValueFlexEnd || valueID == CSSValueCenter || valueID == CSSValueBaseline || valueID == CSSValueStretch)
+            return true;
+        break;
+    case CSSPropertyAlignSelf:
+        // FIXME: Per CSS alignment, this property should accept the same arguments as 'justify-self' so we should share its parsing code.
+        // FIXME: For now, we will do it behind the GRID_LAYOUT compile flag.
+        if (valueID == CSSValueAuto || valueID == CSSValueFlexStart || valueID == CSSValueFlexEnd || valueID == CSSValueCenter || valueID == CSSValueBaseline || valueID == CSSValueStretch)
+            return true;
+        break;
     case CSSPropertyFlexDirection:
         if (valueID == CSSValueRow || valueID == CSSValueRowReverse || valueID == CSSValueColumn || valueID == CSSValueColumnReverse)
             return true;
@@ -1143,6 +1155,9 @@ static inline bool isKeywordPropertyID(CSSPropertyID propertyId)
     case CSSPropertyFontVariantCaps:
     case CSSPropertyFontVariantAlternates:
         return true;
+    case CSSPropertyAlignItems:
+    case CSSPropertyAlignSelf:
+        return !RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled();
     default:
         return false;
     }
@@ -2723,8 +2738,10 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         parsedValue = parseContentDistributionOverflowPosition();
         break;
     case CSSPropertyJustifySelf:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
     case CSSPropertyJustifyItems:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         if (parseLegacyPosition(propId, important))
             return true;
         m_valueList->setCurrentIndex(0);
@@ -3143,9 +3160,11 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         parsedValue = parseContentDistributionOverflowPosition();
         break;
     case CSSPropertyAlignSelf:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
 
     case CSSPropertyAlignItems:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
     case CSSPropertyBorderBottomStyle:
     case CSSPropertyBorderCollapse:
index f0f39b0..ab18c26 100644 (file)
@@ -34,6 +34,7 @@
 #include "LayoutRepainter.h"
 #include "RenderLayer.h"
 #include "RenderView.h"
+#include "RuntimeEnabledFeatures.h"
 #include <limits>
 #include <wtf/MathExtras.h>
 
@@ -1397,6 +1398,8 @@ void RenderFlexibleBox::alignChildren(const Vector<LineContext>& lineContexts)
                 // yet for FlexibleBox.
                 // Defaulting to Stretch for now, as it what most of FlexBox based renders
                 // expect as default.
+                ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
+                FALLTHROUGH;
             case ItemPositionStretch: {
                 applyStretchAlignmentToChild(*child, lineCrossAxisExtent);
                 // Since wrap-reverse flips cross start and cross end, strech children should be aligned with the cross end.
@@ -1431,6 +1434,8 @@ void RenderFlexibleBox::alignChildren(const Vector<LineContext>& lineContexts)
             case ItemPositionRight:
                 // FIXME: https://webkit.org/b/135460 - The extended grammar is not supported
                 // yet for FlexibleBox.
+                ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
+                break;
             default:
                 ASSERT_NOT_REACHED();
                 break;