[@Property] Handle unit cycles correctly

https://bugs.webkit.org/show_bug.cgi?id=247212
rdar://101974818

Reviewed by Simon Fraser.

https://drafts.css-houdini.org/css-properties-values-api/#dependency-cycles

A registered custom property with <length> syntax may use units that depend on another property.
This can create a cycle:

div {
  --my-font-size: 10em;
  font-size: var(--my-font-size);
}

This makes both properties invalid and has to be handled similarly to cycles between custom properties.

The patch also contains some general cleanups to the custom property resolution code.

* LayoutTests/TestExpectations:
* LayoutTests/css-custom-properties-api/crash.html:
* LayoutTests/css-custom-properties-api/cycles.html:
* LayoutTests/css-custom-properties-api/inherits.html:

Update expected results to match the current spec (and Blink).
Registered typed custom properties behave like 'unset' when invalid.

* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/unit-cycles-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/var-reference-registered-properties-cycles-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-substitution-variable-declaration-expected.txt:
* Source/WebCore/css/CSSVariableReferenceValue.cpp:
(WebCore::CSSVariableReferenceValue::resolveVariableFallback):
(WebCore::CSSVariableReferenceValue::resolveVariableReference):
(WebCore::CSSVariableReferenceValue::resolveTokenRange):
(WebCore::CSSVariableReferenceValue::resolveVariableReferences const):
(WebCore::resolveVariableFallback): Deleted.
(WebCore::resolveVariableReference): Deleted.
(WebCore::resolveTokenRange): Deleted.

Turn static functions into private members.

* Source/WebCore/css/CSSVariableReferenceValue.h:
* Source/WebCore/css/parser/CSSParser.cpp:
(WebCore::CSSParser::parseValueWithVariableReferences):

Factor into two separate functions.

(WebCore::CSSParser::parseCustomPropertyValueWithVariableReferences):

Detect unit cycles and mark the associated normal property being part of a cycle.

* Source/WebCore/css/parser/CSSParser.h:
* Source/WebCore/style/PropertyCascade.h:
(WebCore::Style::PropertyCascade::customProperty const):

Return a reference instead of a copy.

* Source/WebCore/style/StyleBuilder.cpp:
(WebCore::Style::Builder::applyNonHighPriorityProperties):

Move custom property resolution after all other properties. Any dependent custom properties
will be already resolved by properties using them. This will just resolve the unused ones.

(WebCore::Style::Builder::applyPropertiesImpl):
(WebCore::Style::Builder::applyCustomProperty):

Remove the looping over the link match enum values. Custom properties are not legal is :visited and we anyway keep
only one version in RenderStyle.
Return invalid or unset value based on property registration status.

(WebCore::Style::Builder::applyProperty):

'unset' behaves differently if the property is registered as inherited or not.

(WebCore::Style::Builder::resolveVariableReferences):

Check for unit cycle.

(WebCore::Style::Builder::resolveCustomPropertyValueWithVariableReferences):
(WebCore::Style::Builder::resolveValue): Deleted.
(WebCore::Style::Builder::resolvedVariableValue): Deleted.
* Source/WebCore/style/StyleBuilder.h:
* Source/WebCore/style/StyleBuilderState.h:
(WebCore::Style::BuilderState::inProgressProperties const):
(WebCore::Style::BuilderState::inUnitCycleProperties):

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

LayoutTests/TestExpectations

@@ -5935,9 +5935,6 @@ imported/w3c/web-platform-tests/user-timing/idlharness-shadowrealm.window.html [
 imported/w3c/web-platform-tests/import-maps/acquiring/modulepreload.html [ Skip ]
 imported/w3c/web-platform-tests/import-maps/acquiring/modulepreload-link-header.html [ Skip ]
 
-# Crashes since StylePropertyMap::set() was implemented and this test started running.
-webkit.org/b/247212 imported/w3c/web-platform-tests/css/css-properties-values-api/unit-cycles.html [ Skip ]
-
 # WPT meta name="variant" is not supported. And iframe 'load' event has a bug.
 imported/w3c/web-platform-tests/import-maps/data-driven/resolving.html [ Skip ]
 

LayoutTests/css-custom-properties-api/crash.html

@@ -48,15 +48,15 @@ <h2 id=test1>This is text</h2>
 test(function() {
   inlineStyle.setProperty('--baz', '   40px');
   assert_equals(computedStyle.getPropertyValue('--baz'), '40px');
-  assert_equals(computedStyle.getPropertyValue('--foo'), '');
-  assert_equals(computedStyle.getPropertyValue('--bar'), '');
+  assert_equals(computedStyle.getPropertyValue('--foo'), '200px');
+  assert_equals(computedStyle.getPropertyValue('--bar'), '200px');
   assert_equals(computedStyle.getPropertyValue('--baz'), '40px');
-  assert_equals(computedStyle.getPropertyValue('font-size'), '30px');
+  assert_equals(computedStyle.getPropertyValue('font-size'), '200px');
   inlineStyle.removeProperty('--baz');
   assert_equals(computedStyle.getPropertyValue('--baz'), '200px');
-  assert_equals(computedStyle.getPropertyValue('--foo'), '');
-  assert_equals(computedStyle.getPropertyValue('--bar'), '');
-  assert_equals(computedStyle.getPropertyValue('font-size'), '30px');
+  assert_equals(computedStyle.getPropertyValue('--foo'), '200px');
+  assert_equals(computedStyle.getPropertyValue('--bar'), '200px');
+  assert_equals(computedStyle.getPropertyValue('font-size'), '200px');
   assert_equals(computedStyle.getPropertyValue('--baz'), '200px');
 }, "Setting the inline style is handled correctly when registered");
 

LayoutTests/css-custom-properties-api/cycles.html

@@ -92,11 +92,11 @@
   test_prop('parent1', '--b', '');
 }, "JS Attributes are valid for element 1");
 test(function() {
-  test_prop('child2', 'width', '160px');
-  test_prop('child2', '--a', '160px');
+  test_prop('child2', 'width', '200px');
+  test_prop('child2', '--a', '200px');
   test_prop('child2', '--b', '');
   test_prop('child2', 'font-size', '16px');
-  test_prop('parent2', '--a', '160px');
+  test_prop('parent2', '--a', '200px');
   test_prop('parent2', '--b', '');
   test_prop('parent2', 'font-size', '16px');
 }, "JS Attributes are valid for element 2");
@@ -110,8 +110,8 @@
   test_prop('parent3', 'font-size', '10px');
 }, "JS Attributes are valid for element 3");
 test(function() {
-  test_prop('child4', 'width', '100px');
-  test_prop('child4', '--a', '100px');
+  test_prop('child4', 'width', '200px');
+  test_prop('child4', '--a', '200px');
   test_prop('child4', '--b', '10em');
   test_prop('child4', 'font-size', '10px');
   test_prop('parent4', '--a', '200px');

LayoutTests/css-custom-properties-api/inherits.html

@@ -346,19 +346,19 @@
   test_prop('child6', '--my-custom-prop2', '200px');
 }, "JS Attributes are valid for element 6");
 test(function() {
-  test_prop('child7', 'width', '300px');
-  test_prop('child7', '--my-custom-prop', '');
-  test_prop('child7', '--my-custom-prop2', '');
+  test_prop('child7', 'width', '200px');
+  test_prop('child7', '--my-custom-prop', '100px');
+  test_prop('child7', '--my-custom-prop2', '200px');
 }, "JS Attributes are valid for element 7");
 test(function() {
-  test_prop('child8', 'width', '300px');
-  test_prop('child8', '--my-custom-prop', '');
-  test_prop('child8', '--my-custom-prop2', '');
+  test_prop('child8', 'width', '200px');
+  test_prop('child8', '--my-custom-prop', '200px');
+  test_prop('child8', '--my-custom-prop2', '200px');
 }, "JS Attributes are valid for element 8");
 test(function() {
-  test_prop('child9', 'width', '300px');
-  test_prop('child9', '--my-custom-prop', '');
-  test_prop('child9', '--my-custom-prop2', '');
+  test_prop('child9', 'width', '200px');
+  test_prop('child9', '--my-custom-prop', '100px');
+  test_prop('child9', '--my-custom-prop2', '200px');
 }, "JS Attributes are valid for element 9");
 test(function() {
   test_prop('child10', 'width', '110px');

LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/unit-cycles-expected.txt

@@ -1,20 +1,26 @@
 
-FAIL Non-font-dependent variables can be used in font-size assert_equals: expected "42px" but got "16px"
-FAIL Lengths with em units may not be referenced from font-size assert_equals: expected "" but got "32px"
-FAIL Lengths with ex units may not be referenced from font-size assert_equals: expected "" but got "14.359375px"
-FAIL Lengths with ch units may not be referenced from font-size assert_equals: expected "" but got "16px"
-FAIL Lengths with rem units may be referenced from font-size on non-root element assert_equals: expected "32px" but got "16px"
-FAIL Lengths with rem units may not be referenced from font-size on root element assert_equals: expected "" but got "32px"
+PASS Non-font-dependent variables can be used in font-size
+PASS Lengths with em units may not be referenced from font-size
+PASS Lengths with ex units may not be referenced from font-size
+PASS Lengths with ch units may not be referenced from font-size
+PASS Lengths with lh units may not be referenced from font-size
+PASS Lengths with rem units may be referenced from font-size on non-root element
+FAIL Lengths with rem units may not be referenced from font-size on root element assert_equals: expected "16px" but got "32px"
+PASS Lengths with lh units may not be referenced from line-height
 PASS Fallback may not use font-relative units
+PASS Fallback may not use line-height-relative units
 PASS Fallback not triggered while inside em unit cycle
 PASS Fallback not triggered while inside ex unit cycle
 PASS Fallback not triggered while inside ch unit cycle
-PASS Fallback not triggered while inside rem unit cycle on root element
-FAIL Lengths with em units are detected via var references assert_equals: expected "" but got "160px"
-FAIL Lengths with ex units are detected via var references assert_equals: expected "" but got "71.796875px"
-FAIL Lengths with ch units are detected via var references assert_equals: expected "" but got "80px"
-FAIL Lengths with rem units are detected via var references assert_equals: expected "" but got "160px"
-FAIL Inherited lengths with em units may be used assert_equals: expected "64px" but got "16px"
-FAIL Inherited lengths with ex units may be used assert_equals: expected "28.71875px" but got "16px"
-FAIL Inherited lengths with ch units may be used assert_equals: expected "32px" but got "16px"
+FAIL Fallback not triggered while inside rem unit cycle on root element assert_equals: expected "16px" but got "32px"
+PASS Fallback not triggered while inside lh unit cycle
+PASS Lengths with em units are detected via var references
+PASS Lengths with ex units are detected via var references
+PASS Lengths with ch units are detected via var references
+FAIL Lengths with rem units are detected via var references assert_equals: expected "16px" but got "160px"
+PASS Lengths with lh units are detected via var references
+PASS Inherited lengths with em units may be used
+PASS Inherited lengths with ex units may be used
+PASS Inherited lengths with ch units may be used
+PASS Inherited lengths with lh units may be used
 

LayoutTests/imported/w3c/web-platform-tests/css/css-properties-values-api/var-reference-registered-properties-cycles-expected.txt

@@ -1,7 +1,7 @@
 
-FAIL A var() cycle between two registered properties is handled correctly. assert_equals: expected "1px" but got ""
-FAIL A var() cycle between a registered properties and an unregistered property is handled correctly. assert_equals: expected "1px" but got ""
+PASS A var() cycle between two registered properties is handled correctly.
+PASS A var() cycle between a registered properties and an unregistered property is handled correctly.
 PASS A var() cycle between a two unregistered properties is handled correctly.
-FAIL A var() cycle between a syntax:'*' property and an unregistered property is handled correctly. assert_equals: expected "" but got "circle"
-FAIL Custom properties with universal syntax become guaranteed-invalid when invalid at computed-value time assert_equals: expected "" but got "foo"
+PASS A var() cycle between a syntax:'*' property and an unregistered property is handled correctly.
+PASS Custom properties with universal syntax become guaranteed-invalid when invalid at computed-value time
 

LayoutTests/imported/w3c/web-platform-tests/css/css-variables/variable-substitution-variable-declaration-expected.txt

@@ -29,5 +29,5 @@ PASS target9 --varB
 PASS target9 --varC
 PASS target10 --varA
 PASS target10 --varB
-FAIL target10 --varC assert_equals: expected "" but got "another good one"
+PASS target10 --varC
 

Source/WebCore/css/CSSVariableReferenceValue.cpp

@@ -40,8 +40,6 @@
 
 namespace WebCore {
 
-static bool resolveTokenRange(CSSParserTokenRange, Vector<CSSParserToken>&, Style::BuilderState&);
-
 CSSVariableReferenceValue::CSSVariableReferenceValue(Ref<CSSVariableData>&& data, const CSSParserContext& context)
     : CSSValue(VariableReferenceClass)
     , m_data(WTFMove(data))
@@ -71,16 +69,16 @@ String CSSVariableReferenceValue::customCSSText() const
     return m_stringValue;
 }
 
-static bool resolveVariableFallback(CSSParserTokenRange range, Vector<CSSParserToken>& result, Style::BuilderState& builderState)
+bool CSSVariableReferenceValue::resolveVariableFallback(CSSParserTokenRange range, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
 {
     if (range.atEnd())
         return false;
     ASSERT(range.peek().type() == CommaToken);
     range.consumeIncludingWhitespace();
-    return resolveTokenRange(range, result, builderState);
+    return resolveTokenRange(range, tokens, builderState);
 }
 
-static bool resolveVariableReference(CSSParserTokenRange range, CSSValueID functionId, Vector<CSSParserToken>& result, Style::BuilderState& builderState)
+bool CSSVariableReferenceValue::resolveVariableReference(CSSParserTokenRange range, CSSValueID functionId, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
 {
     ASSERT(functionId == CSSValueVar || functionId == CSSValueEnv);
 
@@ -95,50 +93,56 @@ static bool resolveVariableReference(CSSParserTokenRange range, CSSValueID funct
     builderState.builder().applyCustomProperty(variableName);
 
     // Apply fallback to detect cycles
-    Vector<CSSParserToken> fallbackResult;
-    bool fallbackReturn = resolveVariableFallback(CSSParserTokenRange(range), fallbackResult, builderState);
+    Vector<CSSParserToken> fallbackTokens;
+    bool fallbackSuccess = resolveVariableFallback(CSSParserTokenRange(range), fallbackTokens, builderState);
+
+    auto* property = [&]() -> const CSSCustomPropertyValue* {
+        if (functionId == CSSValueEnv)
+            return builderState.document().constantProperties().values().get(variableName);
 
-    auto* property = functionId == CSSValueVar
-        ? style.getCustomProperty(variableName)
-        : builderState.document().constantProperties().values().get(variableName);
+        auto* customProperty = style.getCustomProperty(variableName);
+        if (customProperty)
+            return customProperty;
 
-    if (!property || property->isUnset()) {
         auto* registered = builderState.document().customPropertyRegistry().get(variableName);
-        if (registered && registered->initialValue())
-            property = registered->initialValue();
-    }
+        return registered && registered->initialValue() ? registered->initialValue() : nullptr;
+    }();
 
     if (!property || property->isInvalid()) {
-        if (fallbackResult.size() > CSSVariableReferenceValue::maxSubstitutionTokens)
+        if (fallbackTokens.size() > maxSubstitutionTokens)
             return false;
 
-        if (fallbackReturn)
-            result.appendVector(fallbackResult);
-        return fallbackReturn;
+        if (fallbackSuccess) {
+            tokens.appendVector(fallbackTokens);
+            return true;
+        }
+        return false;
     }
 
     ASSERT(property->isResolved());
-    if (property->tokens().size() > CSSVariableReferenceValue::maxSubstitutionTokens)
+    if (property->tokens().size() > maxSubstitutionTokens)
         return false;
 
-    result.appendVector(property->tokens());
+    tokens.appendVector(property->tokens());
     return true;
 }
 
-static bool resolveTokenRange(CSSParserTokenRange range, Vector<CSSParserToken>& result, Style::BuilderState& builderState)
+bool CSSVariableReferenceValue::resolveTokenRange(CSSParserTokenRange range, Vector<CSSParserToken>& tokens, Style::BuilderState& builderState)
 {
     bool success = true;
     while (!range.atEnd()) {
         auto functionId = range.peek().functionId();
-        if (functionId == CSSValueVar || functionId == CSSValueEnv)
-            success &= resolveVariableReference(range.consumeBlock(), functionId, result, builderState);
-        else
-            result.append(range.consume());
+        if (functionId == CSSValueVar || functionId == CSSValueEnv) {
+            if (!resolveVariableReference(range.consumeBlock(), functionId, tokens, builderState))
+                success = false;
+            continue;
+        }
+        tokens.append(range.consume());
     }
     return success;
 }
 
-RefPtr<CSSVariableData> CSSVariableReferenceValue::resolveVariableReferences(Style::BuilderState& builderState) const
+RefPtr<CSSVariableData> CSSVariableReferenceValue::resolveVariableReferences(Style::BuilderState& builderState) const 
 {
     Vector<CSSParserToken> resolvedTokens;
     if (!resolveTokenRange(m_data->tokenRange(), resolvedTokens, builderState))

Source/WebCore/css/CSSVariableReferenceValue.h

@@ -61,6 +61,10 @@ class CSSVariableReferenceValue : public CSSValue {
 private:
     explicit CSSVariableReferenceValue(Ref<CSSVariableData>&&, const CSSParserContext&);
 
+    static bool resolveTokenRange(CSSParserTokenRange, Vector<CSSParserToken>&, Style::BuilderState&);
+    static bool resolveVariableReference(CSSParserTokenRange, CSSValueID, Vector<CSSParserToken>&, Style::BuilderState&);
+    static bool resolveVariableFallback(CSSParserTokenRange, Vector<CSSParserToken>&, Style::BuilderState&);
+
     Ref<CSSVariableData> m_data;
     mutable String m_stringValue;
     const CSSParserContext m_context;

Source/WebCore/css/parser/CSSParser.cpp

@@ -193,8 +193,6 @@ void CSSParser::parseDeclarationForInspector(const CSSParserContext& context, co
 
 RefPtr<CSSValue> CSSParser::parseValueWithVariableReferences(CSSPropertyID propID, const CSSValue& value, Style::BuilderState& builderState)
 {
-    ASSERT((propID == CSSPropertyCustom && value.isCustomPropertyValue()) || (propID != CSSPropertyCustom && !value.isCustomPropertyValue()));
-
     if (is<CSSPendingSubstitutionValue>(value)) {
         // FIXME: Should have a resolvedShorthands cache to stop this from being done over and over for each longhand value.
         auto& substitution = downcast<CSSPendingSubstitutionValue>(value);
@@ -217,30 +215,37 @@ RefPtr<CSSValue> CSSParser::parseValueWithVariableReferences(CSSPropertyID propI
         return nullptr;
     }
 
-    if (value.isVariableReferenceValue()) {
-        const CSSVariableReferenceValue& valueWithReferences = downcast<CSSVariableReferenceValue>(value);
-        auto resolvedData = valueWithReferences.resolveVariableReferences(builderState);
-        if (!resolvedData)
-            return nullptr;
-        return CSSPropertyParser::parseSingleValue(propID, resolvedData->tokens(), valueWithReferences.context());
-    }
+    const CSSVariableReferenceValue& valueWithReferences = downcast<CSSVariableReferenceValue>(value);
+    auto resolvedData = valueWithReferences.resolveVariableReferences(builderState);
+    if (!resolvedData)
+        return nullptr;
 
+    return CSSPropertyParser::parseSingleValue(propID, resolvedData->tokens(), valueWithReferences.context());
+}
+
+RefPtr<CSSCustomPropertyValue> CSSParser::parseCustomPropertyValueWithVariableReferences(const CSSCustomPropertyValue& value, Style::BuilderState& builderState)
+{
     const auto& customPropValue = downcast<CSSCustomPropertyValue>(value);
     const auto& valueWithReferences = std::get<Ref<CSSVariableReferenceValue>>(customPropValue.value()).get();
 
     auto& name = downcast<CSSCustomPropertyValue>(value).name();
     auto* registered = builderState.document().customPropertyRegistry().get(name);
     auto& syntax = registered ? registered->syntax : CSSCustomPropertySyntax::universal();
+
     auto resolvedData = valueWithReferences.resolveVariableReferences(builderState);
     if (!resolvedData)
         return nullptr;
 
-    // FIXME handle REM cycles.
     HashSet<CSSPropertyID> dependencies;
     CSSPropertyParser::collectParsedCustomPropertyValueDependencies(syntax, false, dependencies, resolvedData->tokens(), valueWithReferences.context());
 
-    for (auto id : dependencies)
-        builderState.builder().applyProperty(id);
+    // https://drafts.css-houdini.org/css-properties-values-api/#dependency-cycles
+    for (auto id : dependencies) {
+        if (builderState.inProgressProperties().get(id)) {
+            builderState.inUnitCycleProperties().set(id);
+            return nullptr;
+        }
+    }
 
     return CSSPropertyParser::parseTypedCustomPropertyValue(AtomString { name }, syntax, resolvedData->tokens(), builderState, valueWithReferences.context());
 }

Source/WebCore/css/parser/CSSParser.h

@@ -30,6 +30,7 @@
 
 namespace WebCore {
 
+class CSSCustomPropertyValue;
 class CSSParserObserver;
 class CSSSelectorList;
 class CSSValueList;
@@ -85,6 +86,7 @@ class CSSParser {
     std::optional<CSSSelectorList> parseSelector(const String&, StyleSheetContents* = nullptr);
 
     RefPtr<CSSValue> parseValueWithVariableReferences(CSSPropertyID, const CSSValue&, Style::BuilderState&);
+    RefPtr<CSSCustomPropertyValue> parseCustomPropertyValueWithVariableReferences(const CSSCustomPropertyValue&, Style::BuilderState&);
 
     WEBCORE_EXPORT static Color parseColor(const String&, const CSSParserContext&);
     // FIXME: All callers are not getting the right Settings for parsing due to lack of CSSParserContext and should switch to the parseColor function above.

Source/WebCore/style/PropertyCascade.h

@@ -62,7 +62,7 @@ class PropertyCascade {
     const Property* lastDeferredPropertyResolvingRelated(CSSPropertyID, TextDirection, WritingMode) const;
 
     bool hasCustomProperty(const AtomString&) const;
-    Property customProperty(const AtomString&) const;
+    const Property& customProperty(const AtomString&) const;
 
     Span<const CSSPropertyID> deferredPropertyIDs() const;
     const HashMap<AtomString, Property>& customProperties() const { return m_customProperties; }
@@ -158,9 +158,10 @@ inline bool PropertyCascade::hasCustomProperty(const AtomString& name) const
     return m_customProperties.contains(name);
 }
 
-inline PropertyCascade::Property PropertyCascade::customProperty(const AtomString& name) const
+inline const PropertyCascade::Property& PropertyCascade::customProperty(const AtomString& name) const
 {
-    return m_customProperties.get(name);
+    ASSERT(hasCustomProperty(name));
+    return m_customProperties.find(name)->value;
 }
 
 }