[css-align] Initial values are parsed as invalid for some Alignment properties
authorjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Sep 2016 11:46:57 +0000 (11:46 +0000)
committerjfernandez@igalia.com <jfernandez@igalia.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Sep 2016 11:46:57 +0000 (11:46 +0000)
https://bugs.webkit.org/show_bug.cgi?id=161303

Reviewed by Darin Adler.

Source/WebCore:

Due to the implementation of the new CSS Box Alignment specification,
some properties have now new values allowed, which are not valid
according to the Flexible Box Layout specification.

In r205102 we have get back the keywordID parsing, originally implemented for
the Flexbible Box Layout specification. Even though the new valued would be
parsed as invalid when they are set, the 'initial' values will be assigned
in any case.

This patch verifies that the 'initial' values depend on whether the Grid
Layout is enabled or not and verifying such values are parsed as valid.

Additionally, it gets back as well they keywordID parsing for the Content
Alignment properties (align-content and justify-content). This required to
touch a bit the StyleBuilderConverter logic, since we will have to deal with
either the complex CSSContentDistributionValue complex or the  simpler
CSSPrimitiveValue.

Test: fast/css/ensure-flexbox-compatibility-with-initial-values.html

* css/StyleBuilderConverter.h:
(WebCore::StyleBuilderConverter::convertContentAlignmentData): Handling a primitive value if Grid Layout is not enabled.
* css/parser/CSSParser.cpp:
(WebCore::isValidKeywordPropertyAndValue): Simpler parsing of alignment properties if Grid Layout is not enabled.
(WebCore::isKeywordPropertyID): Alignment properties are defined as keyword if Grid Layout is no enabled.
(WebCore::CSSParser::parseValue): Assert Grid Layout is enabled when using the complex parsing.
* rendering/style/RenderStyle.cpp:
(WebCore::RenderStyle::isCSSGridLayoutEnabled): Checking out the Grid Layout runtime flags.
* rendering/style/RenderStyle.h:
(WebCore::RenderStyle::initialDefaultAlignment): Initial value will depend on whether Grid Layout is enabled or not.
(WebCore::RenderStyle::initialContentAlignment): Initial value will depend on whether Grid Layout is enabled or not.

LayoutTests:

Test to verify the "initial" values of the CSS Box Alignment properties
are parsed as valid independently of whether Grid Layout is enabled or not.

* fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: Added.
* fast/css/ensure-flexbox-compatibility-with-initial-values.html: Added.
* fast/css/resources/alignment-parsing-utils.js:
(checkSupportedValues):

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

LayoutTests/ChangeLog
LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html [new file with mode: 0644]
LayoutTests/fast/css/resources/alignment-parsing-utils.js
Source/WebCore/ChangeLog
Source/WebCore/css/StyleBuilderConverter.h
Source/WebCore/css/parser/CSSParser.cpp
Source/WebCore/rendering/RenderFlexibleBox.cpp
Source/WebCore/rendering/style/RenderStyle.cpp
Source/WebCore/rendering/style/RenderStyle.h

index 84ef754..a65c33e 100644 (file)
@@ -1,3 +1,18 @@
+2016-09-12  Javier Fernandez  <jfernandez@igalia.com>
+
+        [css-align] Initial values are parsed as invalid for some Alignment properties
+        https://bugs.webkit.org/show_bug.cgi?id=161303
+
+        Reviewed by Darin Adler.
+
+        Test to verify the "initial" values of the CSS Box Alignment properties
+        are parsed as valid independently of whether Grid Layout is enabled or not.
+
+        * fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt: Added.
+        * fast/css/ensure-flexbox-compatibility-with-initial-values.html: Added.
+        * fast/css/resources/alignment-parsing-utils.js:
+        (checkSupportedValues):
+
 2016-09-11  Chris Dumez  <cdumez@apple.com>
 
         HTMLTrackElement.kind's invalid value default should be the metadata state
diff --git a/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt b/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values-expected.txt
new file mode 100644 (file)
index 0000000..74e9fd8
--- /dev/null
@@ -0,0 +1,45 @@
+Test to verify initial values of alignment properties are backward-comaptible with flexbox implementation.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+
+Verifying initial values are supported when grid is ENABLED.
+PASS CSS.supports('align-items', 'normal') is true
+PASS CSS.supports('align-self', 'normal') is true
+PASS CSS.supports('align-content', 'normal') is true
+PASS CSS.supports('justify-content', 'normal') is true
+PASS CSS.supports('align-items', 'normal') is true
+PASS CSS.supports('align-self', 'normal') is true
+PASS CSS.supports('align-content', 'normal') is true
+PASS CSS.supports('justify-content', 'normal') is true
+PASS CSS.supports('align-items', 'normal') is true
+PASS CSS.supports('align-self', 'normal') is true
+PASS CSS.supports('align-content', 'normal') is true
+PASS CSS.supports('justify-content', 'normal') is true
+PASS CSS.supports('align-items', 'normal') is true
+PASS CSS.supports('align-self', 'normal') is true
+PASS CSS.supports('align-content', 'normal') is true
+PASS CSS.supports('justify-content', 'normal') is true
+
+Verifying initial values are supported when grid is DISABLED.
+PASS CSS.supports('align-items', 'stretch') is true
+PASS CSS.supports('align-self', 'stretch') is true
+PASS CSS.supports('align-content', 'flex-start') is true
+PASS CSS.supports('justify-content', 'flex-start') is true
+PASS CSS.supports('align-items', 'stretch') is true
+PASS CSS.supports('align-self', 'stretch') is true
+PASS CSS.supports('align-content', 'flex-start') is true
+PASS CSS.supports('justify-content', 'flex-start') is true
+PASS CSS.supports('align-items', 'stretch') is true
+PASS CSS.supports('align-self', 'stretch') is true
+PASS CSS.supports('align-content', 'flex-start') is true
+PASS CSS.supports('justify-content', 'flex-start') is true
+PASS CSS.supports('align-items', 'stretch') is true
+PASS CSS.supports('align-self', 'stretch') is true
+PASS CSS.supports('align-content', 'flex-start') is true
+PASS CSS.supports('justify-content', 'flex-start') is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html b/LayoutTests/fast/css/ensure-flexbox-compatibility-with-initial-values.html
new file mode 100644 (file)
index 0000000..b50ca36
--- /dev/null
@@ -0,0 +1,66 @@
+<!DOCTYPE html>
+<script src="../../resources/js-test.js"></script>
+<script src="resources/alignment-parsing-utils.js"></script>
+<body>
+<script>
+description('Test to verify initial values of alignment properties are backward-comaptible with flexbox implementation.');
+
+function setInitialValues(element)
+{
+    element.style.alignItems = "initial";
+    element.style.alignContent = "initial";
+    element.style.justifyContent = "initial";
+}
+
+function checkSupportedInitialValues(element)
+{
+    checkSupportedValues(element.id, "align-items");
+    checkSupportedValues(element.id, "align-self");
+    checkSupportedValues(element.id, "align-content");
+    checkSupportedValues(element.id, "justify-content");
+}
+
+function checkInitialValues(gridEnabled)
+{
+    if (window.internals)
+        window.internals.setCSSGridLayoutEnabled(gridEnabled);
+
+    var root = document.createElement("div");
+    document.body.appendChild(root);
+
+    var container = document.createElement("div");
+    container.id = "container";
+    root.appendChild(container);
+    var item = document.createElement("div");
+    item.id = "item";
+    container.appendChild(item);
+
+    var flexContainer = document.createElement("div");
+    flexContainer.id = "flexContainer";
+    flexContainer.style.display = "flex";
+    root.appendChild(flexContainer);
+    var flexItem = document.createElement("div");
+    flexItem.id = "flexItem";
+    flexContainer.appendChild(flexItem);
+
+    setInitialValues(root);
+    setInitialValues(container);
+    setInitialValues(item);
+    setInitialValues(flexContainer);
+    setInitialValues(flexItem);
+
+    checkSupportedInitialValues(container);
+    checkSupportedInitialValues(item);
+    checkSupportedInitialValues(flexContainer);
+    checkSupportedInitialValues(flexItem);
+
+    document.body.removeChild(root);
+}
+
+debug('<br>Verifying initial values are supported when grid is ENABLED.');
+checkInitialValues(true);
+
+debug('<br>Verifying initial values are supported when grid is DISABLED.');
+checkInitialValues(false);
+</script>
+</body>
index c8d6b27..c8866b0 100644 (file)
@@ -46,3 +46,9 @@ function checkLegacyValues(property, propertyID, value)
     parentElement.appendChild(element);
     checkValues(element, property, propertyID, "", value);
 }
+
+function checkSupportedValues(elementID, property)
+{
+    var value = eval("window.getComputedStyle(" + elementID + " , '').getPropertyValue('" + property + "')");
+    shouldBeTrue("CSS.supports('" + property + "', '" + value + "')");
+}
index 823789f..7ec8240 100644 (file)
@@ -1,3 +1,42 @@
+2016-09-12  Javier Fernandez  <jfernandez@igalia.com>
+
+        [css-align] Initial values are parsed as invalid for some Alignment properties
+        https://bugs.webkit.org/show_bug.cgi?id=161303
+
+        Reviewed by Darin Adler.
+
+        Due to the implementation of the new CSS Box Alignment specification,
+        some properties have now new values allowed, which are not valid
+        according to the Flexible Box Layout specification.
+
+        In r205102 we have get back the keywordID parsing, originally implemented for
+        the Flexbible Box Layout specification. Even though the new valued would be
+        parsed as invalid when they are set, the 'initial' values will be assigned
+        in any case.
+
+        This patch verifies that the 'initial' values depend on whether the Grid
+        Layout is enabled or not and verifying such values are parsed as valid.
+
+        Additionally, it gets back as well they keywordID parsing for the Content
+        Alignment properties (align-content and justify-content). This required to
+        touch a bit the StyleBuilderConverter logic, since we will have to deal with
+        either the complex CSSContentDistributionValue complex or the  simpler
+        CSSPrimitiveValue.
+
+        Test: fast/css/ensure-flexbox-compatibility-with-initial-values.html
+
+        * css/StyleBuilderConverter.h:
+        (WebCore::StyleBuilderConverter::convertContentAlignmentData): Handling a primitive value if Grid Layout is not enabled.
+        * css/parser/CSSParser.cpp:
+        (WebCore::isValidKeywordPropertyAndValue): Simpler parsing of alignment properties if Grid Layout is not enabled.
+        (WebCore::isKeywordPropertyID): Alignment properties are defined as keyword if Grid Layout is no enabled.
+        (WebCore::CSSParser::parseValue): Assert Grid Layout is enabled when using the complex parsing.
+        * rendering/style/RenderStyle.cpp:
+        (WebCore::RenderStyle::isCSSGridLayoutEnabled): Checking out the Grid Layout runtime flags.
+        * rendering/style/RenderStyle.h:
+        (WebCore::RenderStyle::initialDefaultAlignment): Initial value will depend on whether Grid Layout is enabled or not.
+        (WebCore::RenderStyle::initialContentAlignment): Initial value will depend on whether Grid Layout is enabled or not.
+
 2016-09-12  Chris Dumez  <cdumez@apple.com>
 
         ol.start may return incorrect value for reversed lists when not explicitly set
index cf4fd72..a5a6096 100644 (file)
@@ -46,6 +46,7 @@
 #include "LengthRepeat.h"
 #include "Pair.h"
 #include "QuotesData.h"
+#include "RuntimeEnabledFeatures.h"
 #include "SVGURIReference.h"
 #include "Settings.h"
 #include "StyleResolver.h"
@@ -1253,13 +1254,33 @@ inline StyleSelfAlignmentData StyleBuilderConverter::convertSelfOrDefaultAlignme
 inline StyleContentAlignmentData StyleBuilderConverter::convertContentAlignmentData(StyleResolver&, CSSValue& value)
 {
     StyleContentAlignmentData alignmentData = RenderStyle::initialContentAlignment();
-    auto& contentValue = downcast<CSSContentDistributionValue>(value);
-    if (contentValue.distribution()->getValueID() != CSSValueInvalid)
-        alignmentData.setDistribution(contentValue.distribution().get());
-    if (contentValue.position()->getValueID() != CSSValueInvalid)
-        alignmentData.setPosition(contentValue.position().get());
-    if (contentValue.overflow()->getValueID() != CSSValueInvalid)
-        alignmentData.setOverflow(contentValue.overflow().get());
+#if ENABLE(CSS_GRID_LAYOUT)
+    if (RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled()) {
+        auto& contentValue = downcast<CSSContentDistributionValue>(value);
+        if (contentValue.distribution()->getValueID() != CSSValueInvalid)
+            alignmentData.setDistribution(contentValue.distribution().get());
+        if (contentValue.position()->getValueID() != CSSValueInvalid)
+            alignmentData.setPosition(contentValue.position().get());
+        if (contentValue.overflow()->getValueID() != CSSValueInvalid)
+            alignmentData.setOverflow(contentValue.overflow().get());
+        return alignmentData;
+    }
+#endif
+    auto& primitiveValue = downcast<CSSPrimitiveValue>(value);
+    switch (primitiveValue.getValueID()) {
+    case CSSValueStretch:
+    case CSSValueSpaceBetween:
+    case CSSValueSpaceAround:
+        alignmentData.setDistribution(primitiveValue);
+        break;
+    case CSSValueFlexStart:
+    case CSSValueFlexEnd:
+    case CSSValueCenter:
+        alignmentData.setPosition(primitiveValue);
+        break;
+    default:
+        ASSERT_NOT_REACHED();
+    }
     return alignmentData;
 }
 
index 6dfaf48..bc0dd54 100644 (file)
@@ -819,6 +819,10 @@ static inline bool isValidKeywordPropertyAndValue(CSSPropertyID propertyId, int
         if (valueID == CSSValueAuto || valueID == CSSValueBalance)
             return true;
         break;
+    case CSSPropertyAlignContent:
+        // FIXME: Per CSS alignment, this property should accept an optional <overflow-position>. We should share this parsing code with 'justify-self'.
+        // FIXME: For now, we will do it behind the GRID_LAYOUT compile flag.
+        return valueID == CSSValueFlexStart || valueID == CSSValueFlexEnd || valueID == CSSValueCenter || valueID == CSSValueSpaceBetween || valueID == CSSValueSpaceAround || valueID == CSSValueStretch;
     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.
@@ -839,6 +843,10 @@ static inline bool isValidKeywordPropertyAndValue(CSSPropertyID propertyId, int
         if (valueID == CSSValueNowrap || valueID == CSSValueWrap || valueID == CSSValueWrapReverse)
              return true;
         break;
+    case CSSPropertyJustifyContent:
+        // FIXME: Per CSS alignment, this property should accept an optional <overflow-position>. We should share this parsing code with 'justify-self'.
+        // FIXME: For now, we will do it behind the GRID_LAYOUT compile flag.
+        return valueID == CSSValueFlexStart || valueID == CSSValueFlexEnd || valueID == CSSValueCenter || valueID == CSSValueSpaceBetween || valueID == CSSValueSpaceAround;
     case CSSPropertyWebkitFontKerning:
         if (valueID == CSSValueAuto || valueID == CSSValueNormal || valueID == CSSValueNone)
             return true;
@@ -1155,6 +1163,8 @@ static inline bool isKeywordPropertyID(CSSPropertyID propertyId)
     case CSSPropertyFontVariantCaps:
     case CSSPropertyFontVariantAlternates:
         return true;
+    case CSSPropertyJustifyContent:
+    case CSSPropertyAlignContent:
     case CSSPropertyAlignItems:
     case CSSPropertyAlignSelf:
 #if ENABLE(CSS_GRID_LAYOUT)
@@ -2738,10 +2748,11 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         }
         return false;
     }
+#if ENABLE(CSS_GRID_LAYOUT)
     case CSSPropertyJustifyContent:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         parsedValue = parseContentDistributionOverflowPosition();
         break;
-#if ENABLE(CSS_GRID_LAYOUT)
     case CSSPropertyJustifySelf:
         if (!isCSSGridLayoutEnabled())
             return false;
@@ -3162,10 +3173,11 @@ bool CSSParser::parseValue(CSSPropertyID propId, bool important)
         parsedValue = parseImageResolution();
         break;
 #endif
+#if ENABLE(CSS_GRID_LAYOUT)
     case CSSPropertyAlignContent:
+        ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         parsedValue = parseContentDistributionOverflowPosition();
         break;
-#if ENABLE(CSS_GRID_LAYOUT)
     case CSSPropertyAlignSelf:
         ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
         return parseItemPositionOverflowPosition(propId, important);
index ab18c26..49ca2ac 100644 (file)
@@ -1398,7 +1398,11 @@ 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.
+#if ENABLE(CSS_GRID_LAYOUT)
                 ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
+#else
+                ASSERT_NOT_REACHED();
+#endif
                 FALLTHROUGH;
             case ItemPositionStretch: {
                 applyStretchAlignmentToChild(*child, lineCrossAxisExtent);
@@ -1434,7 +1438,11 @@ void RenderFlexibleBox::alignChildren(const Vector<LineContext>& lineContexts)
             case ItemPositionRight:
                 // FIXME: https://webkit.org/b/135460 - The extended grammar is not supported
                 // yet for FlexibleBox.
+#if ENABLE(CSS_GRID_LAYOUT)
                 ASSERT(RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled());
+#else
+                ASSERT_NOT_REACHED();
+#endif
                 break;
             default:
                 ASSERT_NOT_REACHED();
index c7f3105..35409a1 100644 (file)
@@ -37,6 +37,7 @@
 #include "QuotesData.h"
 #include "RenderObject.h"
 #include "RenderTheme.h"
+#include "RuntimeEnabledFeatures.h"
 #include "ScaleTransformOperation.h"
 #include "ShadowData.h"
 #include "StyleImage.h"
@@ -190,6 +191,15 @@ RenderStyle::~RenderStyle()
 #endif
 }
 
+bool RenderStyle::isCSSGridLayoutEnabled()
+{
+#if ENABLE(CSS_GRID_LAYOUT)
+    return RuntimeEnabledFeatures::sharedFeatures().isCSSGridLayoutEnabled();
+#else
+    return false;
+#endif
+}
+
 static StyleSelfAlignmentData resolvedSelfAlignment(const StyleSelfAlignmentData& value, ItemPosition normalValueBehavior)
 {
     ASSERT(value.position() != ItemPositionAuto);
index 960c93d..3c05a7c 100644 (file)
@@ -2001,8 +2001,8 @@ public:
     static Length initialFlexBasis() { return Length(Auto); }
     static int initialOrder() { return 0; }
     static StyleSelfAlignmentData initialSelfAlignment() { return StyleSelfAlignmentData(ItemPositionAuto, OverflowAlignmentDefault); }
-    static StyleSelfAlignmentData initialDefaultAlignment() { return StyleSelfAlignmentData(ItemPositionNormal, OverflowAlignmentDefault); }
-    static StyleContentAlignmentData initialContentAlignment() { return StyleContentAlignmentData(ContentPositionNormal, ContentDistributionDefault, OverflowAlignmentDefault); }
+    static StyleSelfAlignmentData initialDefaultAlignment() { return StyleSelfAlignmentData(isCSSGridLayoutEnabled() ? ItemPositionNormal : ItemPositionStretch, OverflowAlignmentDefault); }
+    static StyleContentAlignmentData initialContentAlignment() { return StyleContentAlignmentData(isCSSGridLayoutEnabled() ? ContentPositionNormal : ContentPositionFlexStart, ContentDistributionDefault, OverflowAlignmentDefault); }
     static EFlexDirection initialFlexDirection() { return FlowRow; }
     static EFlexWrap initialFlexWrap() { return FlexNoWrap; }
     static int initialMarqueeLoopCount() { return -1; }
@@ -2066,6 +2066,8 @@ public:
     static QuotesData* initialQuotes() { return nullptr; }
     static const AtomicString& initialContentAltText() { return emptyAtom; }
 
+    static bool isCSSGridLayoutEnabled();
+
     static WillChangeData* initialWillChange() { return nullptr; }
 
 #if ENABLE(TOUCH_EVENTS)