Calling CSSStyleValue.parseAll() on a list-valued CSS property should…

… split its value list

https://bugs.webkit.org/show_bug.cgi?id=247063

Reviewed by Antti Koivisto.

Calling CSSStyleValue.parseAll() on a list-valued CSS property [1] should split its value list:
- https://drafts.css-houdini.org/css-typed-om/#parse-a-cssstylevalue (Step 4)
- https://drafts.css-houdini.org/css-typed-om/#subdivide-into-iterations

List-valued properties now indicate their separator in CSSProperties.json so that makeprop.pl
can property generate CSSProperty::listValuedPropertySeparator(), on which
CSSProperty::isListValuedProperty() relies. Right now, we only make use of the fact that a
property is list-valued or not. However, we will eventually need to know which separator its
uses in order to implement other parts of the CSS Typed OM specification.

[1] https://drafts.css-houdini.org/css-typed-om/#list-valued-properties

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parseAll-expected.txt:
* Source/WebCore/css/CSSProperties.json:
* Source/WebCore/css/CSSProperty.h:
(WebCore::CSSProperty::isListValuedProperty):
* Source/WebCore/css/makeprop.pl:
(addProperty):
* Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:
(WebCore::CSSStyleValueFactory::extractCSSValue):
(WebCore::CSSStyleValueFactory::parseStyleValue):
(WebCore::CSSStyleValueFactory::extractCSSValues): Deleted.
* Source/WebCore/css/typedom/CSSStyleValueFactory.h:

Canonical link: https://commits.webkit.org/256070@main

LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parseAll-expected.txt

@@ -1,7 +1,7 @@
 
 PASS CSSStyleValue.parseAll() with a valid property returns a list with a single CSSStyleValue
 PASS CSSStyleValue.parseAll() is not case sensitive
-FAIL CSSStyleValue.parseAll() with a valid list-valued property returns a list with a single CSSStyleValue assert_equals: Result must be a list with three elements expected 3 but got 1
+PASS CSSStyleValue.parseAll() with a valid list-valued property returns a list with a single CSSStyleValue
 FAIL CSSStyleValue.parseAll() with a valid shorthand property returns a CSSStyleValue assert_equals: Result must be a list with one element expected 1 but got 4
 PASS CSSStyleValue.parseAll() with a valid custom property returns a list with a single CSSStyleValue
 

Source/WebCore/css/CSSProperties.json

@@ -125,6 +125,10 @@
         "Indicates that this CSS property is a FillLayer property. It must have",
         "corresponding methods on the FillLayer class.",
         "",
+        "* separator:",
+        "Indicates separator for list-valued CSS property (populates",
+        "CSSProperty::listPropertySeparator().",
+        "",
         "* skip-builder:",
         "Ignore this property in the StyleBuilder.",
         "",
@@ -1013,7 +1017,8 @@
                 "aliases": [
                     "-webkit-animation-delay"
                 ],
-                "name-for-methods": "Delay"
+                "name-for-methods": "Delay",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1032,7 +1037,8 @@
                 "aliases": [
                     "-webkit-animation-direction"
                 ],
-                "name-for-methods": "Direction"
+                "name-for-methods": "Direction",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1045,7 +1051,8 @@
                 "aliases": [
                     "-webkit-animation-duration"
                 ],
-                "name-for-methods": "Duration"
+                "name-for-methods": "Duration",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1064,7 +1071,8 @@
                 "aliases": [
                     "-webkit-animation-fill-mode"
                 ],
-                "name-for-methods": "FillMode"
+                "name-for-methods": "FillMode",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1077,7 +1085,8 @@
                 "aliases": [
                     "-webkit-animation-iteration-count"
                 ],
-                "name-for-methods": "IterationCount"
+                "name-for-methods": "IterationCount",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1090,7 +1099,8 @@
                 "aliases": [
                     "-webkit-animation-name"
                 ],
-                "name-for-methods": "Name"
+                "name-for-methods": "Name",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1107,7 +1117,8 @@
                 "aliases": [
                     "-webkit-animation-play-state"
                 ],
-                "name-for-methods": "PlayState"
+                "name-for-methods": "PlayState",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1120,7 +1131,8 @@
                 "aliases": [
                     "-webkit-animation-timing-function"
                 ],
-                "name-for-methods": "TimingFunction"
+                "name-for-methods": "TimingFunction",
+                "separator": ","
             },
             "specification": {
                 "category": "css-animations",
@@ -1154,7 +1166,8 @@
             ],
             "codegen-properties": {
                 "name-for-methods": "Attachment",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": ","
             },
             "specification": {
                 "category": "css-backgrounds",
@@ -1182,7 +1195,8 @@
             ],
             "codegen-properties": {
                 "name-for-methods": "BlendMode",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": ","
             },
             "specification": {
                 "category": "css-compositing",
@@ -1203,7 +1217,8 @@
             "codegen-properties": {
                 "related-property": "-webkit-background-clip",
                 "name-for-methods": "Clip",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": " "
             },
             "specification": {
                 "category": "css-backgrounds",
@@ -1223,7 +1238,8 @@
         "background-image": {
             "codegen-properties": {
                 "name-for-methods": "Image",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": " "
             },
             "specification": {
                 "category": "css-backgrounds",
@@ -1234,7 +1250,8 @@
             "codegen-properties": {
                 "related-property": "-webkit-background-origin",
                 "name-for-methods": "Origin",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": " "
             },
             "specification": {
                 "category": "css-backgrounds",
@@ -1286,7 +1303,8 @@
         "background-size": {
             "codegen-properties": {
                 "name-for-methods": "Size",
-                "fill-layer-property": true
+                "fill-layer-property": true,
+                "separator": " "
             },
             "specification": {
                 "category": "css-backgrounds",
@@ -2483,7 +2501,8 @@
         },
         "content": {
             "codegen-properties": {
-                "custom": "All"
+                "custom": "All",
+                "separator": ","
             },
             "specification": {
                 "category": "css-content",
@@ -4945,7 +4964,8 @@
                 "aliases": [
                     "-webkit-transition-delay"
                 ],
-                "name-for-methods": "Delay"
+                "name-for-methods": "Delay",
+                "separator": ","
             },
             "specification": {
                 "category": "css-transitions",
@@ -4958,7 +4978,8 @@
                 "aliases": [
                     "-webkit-transition-duration"
                 ],
-                "name-for-methods": "Duration"
+                "name-for-methods": "Duration",
+                "separator": ","
             },
             "specification": {
                 "category": "css-transitions",
@@ -6196,7 +6217,8 @@
         },
         "grid-auto-columns": {
             "codegen-properties": {
-                "converter": "GridTrackSizeList"
+                "converter": "GridTrackSizeList",
+                "separator": " "
             },
             "specification": {
                 "category": "css-grid",
@@ -6205,7 +6227,8 @@
         },
         "grid-auto-rows": {
             "codegen-properties": {
-                "converter": "GridTrackSizeList"
+                "converter": "GridTrackSizeList",
+                "separator": " "
             },
             "specification": {
                 "category": "css-grid",

Source/WebCore/css/CSSProperty.h

@@ -87,6 +87,8 @@ class CSSProperty {
     static bool areInSameLogicalPropertyGroupWithDifferentMappingLogic(CSSPropertyID, CSSPropertyID);
     static bool isDescriptorOnly(CSSPropertyID);
     static bool isColorProperty(CSSPropertyID);
+    static UChar listValuedPropertySeparator(CSSPropertyID);
+    static bool isListValuedProperty(CSSPropertyID propertyID) { return !!listValuedPropertySeparator(propertyID); }
 
     const StylePropertyMetadata& metadata() const { return m_metadata; }
 

Source/WebCore/css/makeprop.pl

@@ -72,6 +72,7 @@
 my %nameIsDeferred;
 my %nameIsInherited;
 my %namePriorityShouldSink;
+my %nameListPropertySeparator;
 my %logicalPropertyGroups;
 my %logicalPropertyGroupResolvers = (
     "logical" => {
@@ -325,6 +326,8 @@ ($$)
                     $settingsFlags{$name} = $codegenProperties->{"settings-flag"};
                 } elsif ($codegenOptionName eq "color-property") {
                     $nameIsColorProperty{$name} = 1;
+                } elsif ($codegenOptionName eq "separator") {
+                    $nameListPropertySeparator{$name} = $codegenProperties->{"separator"};;
                 } elsif ($codegenOptionName eq "logical-property-group") {
                     die "Shorthand property $name can't belong to a logical property group\n" if exists $codegenProperties->{"longhands"};
                     $nameIsDeferred{$name} = 1;
@@ -735,6 +738,21 @@ sub sortByDescendingPriorityAndName
     }
 }
 
+UChar CSSProperty::listValuedPropertySeparator(CSSPropertyID id)
+{
+    switch (id) {
+EOF
+for my $name (sort keys %nameListPropertySeparator) {
+    print GPERF "    case CSSPropertyID::CSSProperty" . $nameToId{$name} . ":\n";
+    print GPERF "        return '" . substr($nameListPropertySeparator{$name}, 0, 1) . "';\n";
+}
+print GPERF << "EOF";
+    default:
+        break;
+    }
+    return '\\0';
+}
+
 bool CSSProperty::isDirectionAwareProperty(CSSPropertyID id)
 {
     switch (id) {

Source/WebCore/css/typedom/CSSStyleValueFactory.cpp

@@ -55,7 +55,7 @@
 
 namespace WebCore {
 
-ExceptionOr<void> CSSStyleValueFactory::extractCSSValues(Vector<Ref<CSSValue>>& cssValues, const CSSPropertyID& propertyID, const String& cssText)
+ExceptionOr<RefPtr<CSSValue>> CSSStyleValueFactory::extractCSSValue(const CSSPropertyID& propertyID, const String& cssText)
 {
     auto styleDeclaration = MutableStyleProperties::create();
 
@@ -65,10 +65,7 @@ ExceptionOr<void> CSSStyleValueFactory::extractCSSValues(Vector<Ref<CSSValue>>&
     if (parseResult == CSSParser::ParseResult::Error)
         return Exception { TypeError, makeString(cssText, " cannot be parsed.")};
 
-    if (auto cssValue = styleDeclaration->getPropertyCSSValue(propertyID))
-        cssValues.append(cssValue.releaseNonNull());
-
-    return { };
+    return styleDeclaration->getPropertyCSSValue(propertyID);
 }
 
 ExceptionOr<void> CSSStyleValueFactory::extractShorthandCSSValues(Vector<Ref<CSSValue>>& cssValues, const CSSPropertyID& propertyID, const String& cssText)
@@ -133,9 +130,20 @@ ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValueFactory::parseStyleValue(co
             if (result.hasException())
                 return result.releaseException();
         } else {
-            auto result = extractCSSValues(cssValues, propertyID, cssText);
+            auto result = extractCSSValue(propertyID, cssText);
             if (result.hasException())
                 return result.releaseException();
+            if (auto cssValue = result.releaseReturnValue()) {
+                // https://drafts.css-houdini.org/css-typed-om/#subdivide-into-iterations
+                if (CSSProperty::isListValuedProperty(propertyID)) {
+                    if (auto* valueList = dynamicDowncast<CSSValueList>(*cssValue)) {
+                        for (size_t i = 0, length = valueList->length(); i < length; ++i)
+                            cssValues.append(*valueList->item(i));
+                    }
+                }
+                if (cssValues.isEmpty())
+                    cssValues.append(cssValue.releaseNonNull());
+            }
         }
     }
 

Source/WebCore/css/typedom/CSSStyleValueFactory.h

@@ -42,17 +42,16 @@ class StylePropertyShorthand;
 
 class CSSStyleValueFactory {
 public:
-
     static ExceptionOr<Ref<CSSStyleValue>> reifyValue(Ref<CSSValue>, Document* = nullptr);
-
-    static ExceptionOr<void> extractCSSValues(Vector<Ref<CSSValue>>&, const CSSPropertyID&, const String&);
-    static ExceptionOr<void> extractShorthandCSSValues(Vector<Ref<CSSValue>>&, const CSSPropertyID&, const String&);
-    static ExceptionOr<void> extractCustomCSSValues(Vector<Ref<CSSValue>>&, const AtomString&, const String&);
-
     static ExceptionOr<Vector<Ref<CSSStyleValue>>> parseStyleValue(const AtomString&, const String&, bool);
 
 protected:
     CSSStyleValueFactory() = delete;
+
+private:
+    static ExceptionOr<RefPtr<CSSValue>> extractCSSValue(const CSSPropertyID&, const String&);
+    static ExceptionOr<void> extractShorthandCSSValues(Vector<Ref<CSSValue>>&, const CSSPropertyID&, const String&);
+    static ExceptionOr<void> extractCustomCSSValues(Vector<Ref<CSSValue>>&, const AtomString&, const String&);
 };
 
 } // namespace WebCore

Tools/Scripts/webkitpy/style/checkers/jsonchecker.py

@@ -305,6 +305,7 @@ def check_codegen_properties(self, property_name, codegen_properties):
             'no-default-color': self.validate_boolean,
             'related-property': self.validate_string,
             'runtime-flag': self.validate_string,
+            'separator': self.validate_string,
             'setter': self.validate_string,
             'settings-flag': self.validate_string,
             'sink-priority': self.validate_boolean,