Cascade layer styles should be lower priority than unlayered styles

https://bugs.webkit.org/show_bug.cgi?id=231342 Reviewed by Simon Fraser. LayoutTests/imported/w3c: Update from the WPT repo. * web-platform-tests/css/css-cascade/layer-basic.html: * web-platform-tests/css/css-cascade/layer-font-face-override-expected.txt: * web-platform-tests/css/css-cascade/layer-font-face-override.html: * web-platform-tests/css/css-cascade/layer-import.html: * web-platform-tests/css/css-cascade/layer-keyframes-override-expected.txt: * web-platform-tests/css/css-cascade/layer-keyframes-override.html: * web-platform-tests/css/css-cascade/layer-stylesheet-sharing.html: Source/WebCore: Update the implementation to match the resolution for w3c/csswg-drafts#6284. * style/ElementRuleCollector.cpp: (WebCore::Style::ElementRuleCollector::addMatchedRule): (WebCore::Style::compareRules): * style/ElementRuleCollector.h: * style/RuleSet.h: (WebCore::Style::RuleSet::cascadeLayerPriorityForIdentifier const): (WebCore::Style::RuleSet::cascadeLayerPriorityFor const): Return std::numeric_limits<unsigned>::max() for unlayered rules. (WebCore::Style::RuleSet::cascadeLayerOrderForIdentifier const): Deleted. (WebCore::Style::RuleSet::cascadeLayerOrderFor const): Deleted. Also update naming order->priority to better match the spec text. * style/RuleSetBuilder.cpp: (WebCore::Style::RuleSetBuilder::~RuleSetBuilder): (WebCore::Style::RuleSetBuilder::updateCascadeLayerPriorities): (WebCore::Style::RuleSetBuilder::addMutatingRulesToResolver): (WebCore::Style::RuleSetBuilder::updateCascadeLayerOrder): Deleted. * style/RuleSetBuilder.h: git-svn-id: http://svn.webkit.org/repository/webkit/trunk@283718 268f45cc-cd09-0410-ab3c-d52691b4dbfc
Igalia · Oct 7, 2021 · 4bd0284 · 4bd0284
1 parent 2f607a3
commit 4bd0284
Show file tree

Hide file tree

Showing 14 changed files with 114 additions and 67 deletions.
diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
@@ -1,3 +1,20 @@
+2021-10-07  Antti Koivisto  <antti@apple.com>
+
+        Cascade layer styles should be lower priority than unlayered styles
+        https://bugs.webkit.org/show_bug.cgi?id=231342
+
+        Reviewed by Simon Fraser.
+
+        Update from the WPT repo.
+
+        * web-platform-tests/css/css-cascade/layer-basic.html:
+        * web-platform-tests/css/css-cascade/layer-font-face-override-expected.txt:
+        * web-platform-tests/css/css-cascade/layer-font-face-override.html:
+        * web-platform-tests/css/css-cascade/layer-import.html:
+        * web-platform-tests/css/css-cascade/layer-keyframes-override-expected.txt:
+        * web-platform-tests/css/css-cascade/layer-keyframes-override.html:
+        * web-platform-tests/css/css-cascade/layer-stylesheet-sharing.html:
+
 2021-10-07  Rob Buis  <rbuis@igalia.com>
 
         Resync the-img-element WPT tests from upstream

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-basic.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-basic.html
@@ -26,19 +26,19 @@
     {
         title: 'A2 Anonymous layers',
         style: `
-            target { color: red; }
+            target { color: green; }
             @layer {
-                target { color: green; }
+                target { color: red; }
             }
         `,
     },
     {
         title: 'A3 Anonymous layers',
         style: `
             @layer {
-                target { color: green; }
+                target { color: red; }
             }
-            target { color: red; }
+            target { color: green; }
         `,
     },
     {
@@ -61,7 +61,6 @@
                     target { color: green; }
                 }
             }
-            target { color: red; }
         `,
     },
     {
@@ -73,7 +72,6 @@
                 }
                 target { color: red; }
             }
-            target { color: red; }
         `,
     },
     {
@@ -143,9 +141,9 @@
         title: 'B2 Named layers',
         style: `
             @layer A {
-                target { color: green; }
+                target { color: red; }
             }
-            target { color: red; }
+            target { color: green; }
         `,
     },
     {
@@ -157,7 +155,6 @@
             @layer A {
                 target { color: green; }
             }
-            target { color: red; }
         `,
     },
     {
@@ -508,11 +505,10 @@
     },
 ];
 
-for (var i = 0; i < testCases.length; ++i) {
-    var testCase = testCases[i];
-    var documentStyle = document.createElement('style');
-    documentStyle.appendChild(document.createTextNode(testCase['style']));
-    document.head.appendChild(documentStyle);
+for (let testCase of testCases) {
+    const styleElement = document.createElement('style');
+    styleElement.textContent = testCase['style'];
+    document.head.append(styleElement);
 
     test(function () {
         var targets = document.querySelectorAll('target');
@@ -521,7 +517,7 @@
                       testCase['title'] + ", target '" + target.classList[0] + "'");
     }, testCase['title']);
 
-    document.head.removeChild(documentStyle)
+    styleElement.remove();
 }
 </script>
 </body>

diff --git a/...sts/imported/w3c/web-platform-tests/css/css-cascade/layer-font-face-override-expected.txt b/...sts/imported/w3c/web-platform-tests/css/css-cascade/layer-font-face-override-expected.txt
@@ -1,6 +1,6 @@
 Test
 
-PASS @font-face layered overrides unlayered
+PASS @font-face unlayered overrides layered
 PASS @font-face override between layers
 PASS @font-face override update with appended sheet 1
 PASS @font-face override update with appended sheet 2

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-font-face-override.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-font-face-override.html
@@ -18,7 +18,7 @@
 
 const testCases = [
   {
-    title: '@font-face layered overrides unlayered',
+    title: '@font-face unlayered overrides layered',
     style: `
       #target {
         font-family: custom-font;
@@ -27,13 +27,13 @@
       @layer {
         @font-face {
           font-family: custom-font;
-          src: url('/fonts/Ahem.ttf');
+          src: url('/fonts/noto/noto-sans-v8-latin-regular.woff') format('woff');
         }
       }
 
       @font-face {
         font-family: custom-font;
-        src: url('/fonts/noto/noto-sans-v8-latin-regular.woff') format('woff');
+        src: url('/fonts/Ahem.ttf');
       }
     `
   },

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-import.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-import.html
@@ -57,24 +57,24 @@
     {
         title: 'A1 Layer rules with import',
         style: `
-            @import url(basic-red.css);
+            @import url(basic-green.css);
             @layer {
-                target { color: green; }
+                target { color: red; }
             }
         `
     },
     {
         title: 'A2 Layer rules with import',
         style: `
-            @import url(layer-green.css);
-            target { color: red; }
+            @import url(layer-red.css);
+            target { color: green; }
         `
     },
     {
         title: 'A3 Layer rules with import',
         style: `
-            @import url(layer-green.css);
-            @import url(basic-red.css);
+            @import url(basic-green.css);
+            @import url(layer-red.css);
         `
     },
     {
@@ -92,8 +92,8 @@
     {
         title: 'B1 Anonymous imports',
         style: `
-            @import url(basic-green.css) layer;
-            target { color: red; }
+            @import url(basic-red.css) layer;
+            target { color: green; }
         `
     },
     {
@@ -122,8 +122,8 @@
     {
         title: 'C1 Named imports',
         style: `
-            @import url(basic-green.css) layer(A);
-            target { color: red; }
+            @import url(basic-red.css) layer(A);
+            target { color: green; }
         `
     },
     {
@@ -260,18 +260,20 @@
         const styleText = testCase['style'].replaceAll(/url\((.+?)\)/g, (match, p1) => {
             return `url(data:text/css,${ encodeURI(imports[p1]) })`;
         });
-        styleElement.appendChild(document.createTextNode(styleText));
+        styleElement.textContent = styleText;
 
         await new Promise(resolve => {
             styleElement.onload = resolve;
-            document.head.appendChild(styleElement);
+            document.head.append(styleElement);
         });
 
-        const targets = document.querySelectorAll('target');
-        for (target of targets)
-            assert_equals(window.getComputedStyle(target).color, 'rgb(0, 128, 0)', testCase['title'] + ", target '" + target.classList[0] + "'");
-
-       document.head.removeChild(styleElement);
+        try {
+            const targets = document.querySelectorAll('target');
+            for (target of targets)
+                assert_equals(window.getComputedStyle(target).color, 'rgb(0, 128, 0)', testCase['title'] + ", target '" + target.classList[0] + "'");
+        } finally {
+            styleElement.remove();
+        }
     }, testCase['title']);
 }
 </script>

diff --git a/...sts/imported/w3c/web-platform-tests/css/css-cascade/layer-keyframes-override-expected.txt b/...sts/imported/w3c/web-platform-tests/css/css-cascade/layer-keyframes-override-expected.txt
@@ -1,5 +1,5 @@
 
-PASS @keyframes layered overrides unlayered
+PASS @keyframes unlayered overrides layered
 PASS @keyframes override between layers
 PASS @keyframes override update with appended sheet 1
 PASS @keyframes override update with appended sheet 2

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-keyframes-override.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-keyframes-override.html
@@ -23,20 +23,20 @@
 
 const testCases = [
   {
-    title: '@keyframes layered overrides unlayered',
+    title: '@keyframes unlayered overrides layered',
     style: `
       #target {
         animation: anim 1s paused;
       }
 
       @layer {
         @keyframes anim {
-          from { background-color: green; }
+          from { background-color: red; }
         }
       }
 
       @keyframes anim {
-        from { background-color: red; }
+        from { background-color: green; }
       }
     `
   },

diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-stylesheet-sharing.html b/LayoutTests/imported/w3c/web-platform-tests/css/css-cascade/layer-stylesheet-sharing.html
@@ -10,7 +10,6 @@
     display: block;
     width: 100px;
     height: 100px;
-    background-color: red;
   }
 </style>
 <link rel=stylesheet href="data:text/css,@layer{target{background-color:green}}">

diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
@@ -1,3 +1,34 @@
+2021-10-07  Antti Koivisto  <antti@apple.com>
+
+        Cascade layer styles should be lower priority than unlayered styles
+        https://bugs.webkit.org/show_bug.cgi?id=231342
+
+        Reviewed by Simon Fraser.
+
+        Update the implementation to match the resolution for https://github.com/w3c/csswg-drafts/issues/6284.
+
+        * style/ElementRuleCollector.cpp:
+        (WebCore::Style::ElementRuleCollector::addMatchedRule):
+        (WebCore::Style::compareRules):
+        * style/ElementRuleCollector.h:
+        * style/RuleSet.h:
+        (WebCore::Style::RuleSet::cascadeLayerPriorityForIdentifier const):
+        (WebCore::Style::RuleSet::cascadeLayerPriorityFor const):
+
+        Return std::numeric_limits<unsigned>::max() for unlayered rules.
+
+        (WebCore::Style::RuleSet::cascadeLayerOrderForIdentifier const): Deleted.
+        (WebCore::Style::RuleSet::cascadeLayerOrderFor const): Deleted.
+
+        Also update naming order->priority to better match the spec text.
+
+        * style/RuleSetBuilder.cpp:
+        (WebCore::Style::RuleSetBuilder::~RuleSetBuilder):
+        (WebCore::Style::RuleSetBuilder::updateCascadeLayerPriorities):
+        (WebCore::Style::RuleSetBuilder::addMutatingRulesToResolver):
+        (WebCore::Style::RuleSetBuilder::updateCascadeLayerOrder): Deleted.
+        * style/RuleSetBuilder.h:
+
 2021-10-06  Simon Fraser  <simon.fraser@apple.com>
 
         Clean up state maintenance around animated scrolls

diff --git a/Source/WebCore/style/ElementRuleCollector.cpp b/Source/WebCore/style/ElementRuleCollector.cpp
@@ -113,8 +113,8 @@ const Vector<RefPtr<const StyleRule>>& ElementRuleCollector::matchedRuleList() c
 
 inline void ElementRuleCollector::addMatchedRule(const RuleData& ruleData, unsigned specificity, const MatchRequest& matchRequest)
 {
-    auto cascadeLayerOrder = matchRequest.ruleSet ? matchRequest.ruleSet->cascadeLayerOrderFor(ruleData) : 0;
-    m_matchedRules.append({ &ruleData, specificity, matchRequest.styleScopeOrdinal, cascadeLayerOrder });
+    auto cascadeLayerPriority = matchRequest.ruleSet ? matchRequest.ruleSet->cascadeLayerPriorityFor(ruleData) : RuleSet::cascadeLayerPriorityForUnlayered;
+    m_matchedRules.append({ &ruleData, specificity, matchRequest.styleScopeOrdinal, cascadeLayerPriority });
 }
 
 void ElementRuleCollector::clearMatchedRules()
@@ -546,8 +546,8 @@ static inline bool compareRules(MatchedRule r1, MatchedRule r2)
     if (r1.styleScopeOrdinal != r2.styleScopeOrdinal)
         return r1.styleScopeOrdinal > r2.styleScopeOrdinal;
 
-    if (r1.cascadeLayerOrder != r2.cascadeLayerOrder)
-        return r1.cascadeLayerOrder < r2.cascadeLayerOrder;
+    if (r1.cascadeLayerPriority != r2.cascadeLayerPriority)
+        return r1.cascadeLayerPriority < r2.cascadeLayerPriority;
 
     if (r1.specificity != r2.specificity)
         return r1.specificity < r2.specificity;

diff --git a/Source/WebCore/style/ElementRuleCollector.h b/Source/WebCore/style/ElementRuleCollector.h
@@ -62,7 +62,7 @@ struct MatchedRule {
     const RuleData* ruleData;
     unsigned specificity;
     ScopeOrdinal styleScopeOrdinal;
-    unsigned cascadeLayerOrder;
+    unsigned cascadeLayerPriority;
 };
 
 struct MatchedProperties {

diff --git a/Source/WebCore/style/RuleSet.h b/Source/WebCore/style/RuleSet.h
@@ -103,7 +103,8 @@ class RuleSet : public RefCounted<RuleSet> {
     bool hasShadowPseudoElementRules() const { return !m_shadowPseudoElementRules.isEmpty(); }
     bool hasHostPseudoClassRulesMatchingInShadowTree() const { return m_hasHostPseudoClassRulesMatchingInShadowTree; }
 
-    unsigned cascadeLayerOrderFor(const RuleData&) const;
+    static constexpr auto cascadeLayerPriorityForUnlayered = std::numeric_limits<unsigned>::max();
+    unsigned cascadeLayerPriorityFor(const RuleData&) const;
 
 private:
     friend class RuleSetBuilder;
@@ -131,11 +132,11 @@ class RuleSet : public RefCounted<RuleSet> {
     struct CascadeLayer {
         CascadeLayerName resolvedName;
         CascadeLayerIdentifier parentIdentifier;
-        unsigned order { 0 };
+        unsigned priority { 0 };
     };
     CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) { return m_cascadeLayers[identifier - 1]; }
     const CascadeLayer& cascadeLayerForIdentifier(CascadeLayerIdentifier identifier) const { return m_cascadeLayers[identifier - 1]; }
-    unsigned cascadeLayerOrderForIdentifier(CascadeLayerIdentifier) const;
+    unsigned cascadeLayerPriorityForIdentifier(CascadeLayerIdentifier) const;
 
     struct DynamicMediaQueryRules {
         Vector<Ref<const MediaQuerySet>> mediaQuerySets;
@@ -192,19 +193,19 @@ inline const RuleSet::RuleDataVector* RuleSet::tagRules(const AtomString& key, b
     return tagRules->get(key);
 }
 
-inline unsigned RuleSet::cascadeLayerOrderForIdentifier(CascadeLayerIdentifier identifier) const
+inline unsigned RuleSet::cascadeLayerPriorityForIdentifier(CascadeLayerIdentifier identifier) const
 {
     if (!identifier)
-        return 0;
-    return cascadeLayerForIdentifier(identifier).order;
+        return cascadeLayerPriorityForUnlayered;
+    return cascadeLayerForIdentifier(identifier).priority;
 }
 
-inline unsigned RuleSet::cascadeLayerOrderFor(const RuleData& ruleData) const
+inline unsigned RuleSet::cascadeLayerPriorityFor(const RuleData& ruleData) const
 {
     if (m_cascadeLayerIdentifierForRulePosition.size() <= ruleData.position())
-        return 0;
+        return cascadeLayerPriorityForUnlayered;
     auto identifier = m_cascadeLayerIdentifierForRulePosition[ruleData.position()];
-    return cascadeLayerOrderForIdentifier(identifier);
+    return cascadeLayerPriorityForIdentifier(identifier);
 }
 
 } // namespace Style