Fix CSSStyleValue.parse() / parseAll() for shorthand CSS properties

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

Reviewed by Antti Koivisto.

Fix CSSStyleValue.parse() / parseAll() for shorthand CSS properties:
- https://drafts.css-houdini.org/css-typed-om/#dom-cssstylevalue-parse
- https://drafts.css-houdini.org/css-typed-om/#dom-cssstylevalue-parseall

* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parse.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parseAll-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parseAll.html:
* LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-serialization/cssStyleValue-string-expected.txt:
* Source/WebCore/css/typedom/CSSStyleValueFactory.cpp:
(WebCore::CSSStyleValueFactory::extractShorthandCSSValues):
(WebCore::CSSStyleValueFactory::parseStyleValue):
* Source/WebCore/css/typedom/CSSStyleValueFactory.h:
* Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp:
(WebCore::MainThreadStylePropertyMapReadOnly::shorthandPropertyValue const):

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

LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-objects/parse.html

@@ -32,6 +32,7 @@
   const result = CSSStyleValue.parse('margin', '1px 2px 3px 4px');
   assert_true(result instanceof CSSStyleValue,
               'Result must be a subclass of CSSStyleValue');
+  assert_equals(result.toString(), '1px 2px 3px 4px');
 }, 'CSSStyleValue.parse() with a valid shorthand property returns a CSSStyleValue');
 
 test(() => {

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

@@ -2,6 +2,6 @@
 PASS CSSStyleValue.parseAll() with a valid property returns a list with a single CSSStyleValue
 PASS CSSStyleValue.parseAll() is not case sensitive
 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 shorthand property returns a CSSStyleValue
 PASS CSSStyleValue.parseAll() with a valid custom property returns a list with a single CSSStyleValue
 

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

@@ -38,6 +38,7 @@
   assert_equals(result.length, 1, 'Result must be a list with one element');
   assert_true(result[0] instanceof CSSStyleValue,
               'Only element in result must be a subclass of CSSStyleValue');
+  assert_equals(result[0].toString(), '1px 2px 3px 4px');
 }, 'CSSStyleValue.parseAll() with a valid shorthand property returns a CSSStyleValue');
 
 test(() => {

LayoutTests/imported/w3c/web-platform-tests/css/css-typed-om/stylevalue-serialization/cssStyleValue-string-expected.txt

@@ -1,4 +1,4 @@
 
 PASS CSSStyleValue parsed from string serializes to given string
-FAIL Shorthand CSSStyleValue parsed from string serializes to given string assert_equals: expected "blue" but got "initial"
+PASS Shorthand CSSStyleValue parsed from string serializes to given string
 

Source/WebCore/css/typedom/CSSStyleValueFactory.cpp

@@ -55,6 +55,37 @@
 
 namespace WebCore {
 
+ExceptionOr<RefPtr<CSSStyleValue>> CSSStyleValueFactory::constructStyleValueForShorthandProperty(CSSPropertyID propertyID, const Function<RefPtr<CSSValue>(CSSPropertyID)>& propertyValue, Document* document)
+{
+    auto shorthand = shorthandForProperty(propertyID);
+    Vector<Ref<CSSValue>> values;
+    for (auto& longhand : shorthand) {
+        RefPtr value = propertyValue(longhand);
+        if (!value)
+            return nullptr;
+        if (value->isImplicitInitialValue())
+            continue;
+        values.append(value.releaseNonNull());
+        // VariableReference set from the shorthand, value will be the same for all longhands.
+        if (is<CSSPendingSubstitutionValue>(values.last().get()))
+            break;
+    }
+    if (values.isEmpty())
+        return nullptr;
+
+    if (values.size() == 1) {
+        auto reifiedValue = reifyValue(values[0].copyRef(), document);
+        if (reifiedValue.hasException())
+            return reifiedValue.releaseException();
+        return { reifiedValue.releaseReturnValue() };
+    }
+
+    auto valueList = CSSValueList::createSpaceSeparated();
+    for (auto& value : values)
+        valueList->append(WTFMove(value));
+    return { CSSStyleValue::create(WTFMove(valueList)) };
+}
+
 ExceptionOr<RefPtr<CSSValue>> CSSStyleValueFactory::extractCSSValue(const CSSPropertyID& propertyID, const String& cssText)
 {
     auto styleDeclaration = MutableStyleProperties::create();
@@ -68,7 +99,7 @@ ExceptionOr<RefPtr<CSSValue>> CSSStyleValueFactory::extractCSSValue(const CSSPro
     return styleDeclaration->getPropertyCSSValue(propertyID);
 }
 
-ExceptionOr<void> CSSStyleValueFactory::extractShorthandCSSValues(Vector<Ref<CSSValue>>& cssValues, const CSSPropertyID& propertyID, const String& cssText)
+ExceptionOr<RefPtr<CSSStyleValue>> CSSStyleValueFactory::extractShorthandCSSValues(const CSSPropertyID& propertyID, const String& cssText)
 {
     auto styleDeclaration = MutableStyleProperties::create();
 
@@ -78,12 +109,9 @@ ExceptionOr<void> CSSStyleValueFactory::extractShorthandCSSValues(Vector<Ref<CSS
     if (parseResult == CSSParser::ParseResult::Error)
         return Exception { TypeError, makeString(cssText, " cannot be parsed.")};
 
-    auto shorthand = shorthandForProperty(propertyID);
-    for (auto longhand : shorthand) {
-        if (auto cssValue = styleDeclaration->getPropertyCSSValue(longhand))
-            cssValues.append(cssValue.releaseNonNull());
-    }
-    return { };
+    return constructStyleValueForShorthandProperty(propertyID, [&](auto longhandPropertyID) {
+        return styleDeclaration->getPropertyCSSValue(longhandPropertyID);
+    });
 }
 
 ExceptionOr<void> CSSStyleValueFactory::extractCustomCSSValues(Vector<Ref<CSSValue>>& cssValues, const AtomString& customPropertyName, const String& cssText)
@@ -124,26 +152,29 @@ ExceptionOr<Vector<Ref<CSSStyleValue>>> CSSStyleValueFactory::parseStyleValue(co
         if (propertyID == CSSPropertyInvalid)
             return Exception { TypeError, "Property String is not a valid CSS property."_s };
 
-
         if (isShorthandCSSProperty(propertyID)) {
-            auto result = extractShorthandCSSValues(cssValues, propertyID, cssText);
-            if (result.hasException())
-                return result.releaseException();
-        } else {
-            auto result = extractCSSValue(propertyID, cssText);
+            auto result = extractShorthandCSSValues(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));
-                    }
+            auto cssValue = result.releaseReturnValue();
+            if (!cssValue)
+                return Vector<Ref<CSSStyleValue>> { };
+            return Vector { cssValue.releaseNonNull() };
+        }
+
+        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());
             }
+            if (cssValues.isEmpty())
+                cssValues.append(cssValue.releaseNonNull());
         }
     }
 

Source/WebCore/css/typedom/CSSStyleValueFactory.h

@@ -44,13 +44,14 @@ class CSSStyleValueFactory {
 public:
     static ExceptionOr<Ref<CSSStyleValue>> reifyValue(Ref<CSSValue>, Document* = nullptr);
     static ExceptionOr<Vector<Ref<CSSStyleValue>>> parseStyleValue(const AtomString&, const String&, bool);
+    static ExceptionOr<RefPtr<CSSStyleValue>> constructStyleValueForShorthandProperty(CSSPropertyID, const Function<RefPtr<CSSValue>(CSSPropertyID)>& propertyValue, Document* = nullptr);
 
 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<RefPtr<CSSStyleValue>> extractShorthandCSSValues(const CSSPropertyID&, const String&);
     static ExceptionOr<void> extractCustomCSSValues(Vector<Ref<CSSValue>>&, const AtomString&, const String&);
 };
 

Source/WebCore/css/typedom/MainThreadStylePropertyMapReadOnly.cpp

@@ -30,6 +30,7 @@
 #include "CSSPropertyNames.h"
 #include "CSSPropertyParser.h"
 #include "CSSStyleValue.h"
+#include "CSSStyleValueFactory.h"
 #include "CSSUnparsedValue.h"
 #include "CSSValueList.h"
 #include "CSSVariableData.h"
@@ -105,29 +106,10 @@ ExceptionOr<bool> MainThreadStylePropertyMapReadOnly::has(ScriptExecutionContext
 
 RefPtr<CSSStyleValue> MainThreadStylePropertyMapReadOnly::shorthandPropertyValue(Document& document, CSSPropertyID propertyID) const
 {
-    auto shorthand = shorthandForProperty(propertyID);
-    Vector<Ref<CSSValue>> values;
-    for (auto& longhand : shorthand) {
-        RefPtr value = propertyValue(longhand);
-        if (!value)
-            return nullptr;
-        if (value->isImplicitInitialValue())
-            continue;
-        // VariableReference set from the shorthand.
-        if (is<CSSPendingSubstitutionValue>(*value))
-            return CSSUnparsedValue::create(downcast<CSSPendingSubstitutionValue>(*value).shorthandValue().data().tokenRange());
-        values.append(value.releaseNonNull());
-    }
-    if (values.isEmpty())
-        return nullptr;
-
-    if (values.size() == 1)
-        return reifyValue(values[0].ptr(), document);
-
-    auto valueList = CSSValueList::createSpaceSeparated();
-    for (auto& value : values)
-        valueList->append(WTFMove(value));
-    return CSSStyleValue::create(WTFMove(valueList));
+    auto result = CSSStyleValueFactory::constructStyleValueForShorthandProperty(propertyID, [this](auto longhandPropertyID) {
+        return propertyValue(longhandPropertyID);
+    }, &document);
+    return result.hasException() ? nullptr : RefPtr<CSSStyleValue> { result.releaseReturnValue() };
 }
 
 } // namespace WebCore