Changeset 278346 in webkit

Timestamp:

Jun 2, 2021, 2:41:43 AM (5 years ago)

Author:

Antti Koivisto

Message:

REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style
​https://bugs.webkit.org/show_bug.cgi?id=226369
<rdar://problem/78684562>

Reviewed by Sam Weinig.

Source/WebCore:

Style resolvers are stateful in respect to media queries. We would only invalidate in the first shadow tree
because that evaluation flipped the state and the second evaluation would see nothing changing in media query
state.

• style/StyleScope.cpp:

(WebCore::Style::Scope::collectResolverScopes):

Add a helper to collect all scopes associated with a resolver.

(WebCore::Style::Scope::evaluateMediaQueries):

Only evaluate each resolver once, then invalidate all associated scopes if needed.

• style/StyleScope.h:

LayoutTests:

Expand the existing test to have multiple shadow trees sharing style.

• fast/shadow-dom/media-query-in-shadow-style-expected.html:
• fast/shadow-dom/media-query-in-shadow-style.html:
• fast/shadow-dom/resources/media-query-in-shadow-style-frame.html:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/fast/shadow-dom/media-query-in-shadow-style-expected.html (modified) (1 diff)
• LayoutTests/fast/shadow-dom/media-query-in-shadow-style.html (modified) (1 diff)
• LayoutTests/fast/shadow-dom/resources/media-query-in-shadow-style-frame.html (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/style/StyleScope.cpp (modified) (1 diff)
• Source/WebCore/style/StyleScope.h (modified) (3 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2021-06-02  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style
+        https://bugs.webkit.org/show_bug.cgi?id=226369
+        <rdar://problem/78684562>
+
+        Reviewed by Sam Weinig.
+
+        Expand the existing test to have multiple shadow trees sharing style.
+
+        * fast/shadow-dom/media-query-in-shadow-style-expected.html:
+        * fast/shadow-dom/media-query-in-shadow-style.html:
+        * fast/shadow-dom/resources/media-query-in-shadow-style-frame.html:
+
 2021-06-02  Youenn Fablet  <youenn@apple.com>
 

trunk/LayoutTests/fast/shadow-dom/media-query-in-shadow-style-expected.html

@@ -1 @@
-<iframe src="data:text/html,<div style='color:red'>Text</div>" width=150 height=50></iframe><br>
-<iframe src="data:text/html,<div style='color:green;background-color: lightgrey'>Text</div>" width=300 height=50></iframe><br>
-<iframe src="data:text/html,<div style='color:red'>Text</div>" width=150 height=50></iframe><br>
-<iframe src="data:text/html,<div style='color:green;background-color: lightgrey'>Text</div>" width=300 height=50></iframe><br>
+<iframe src="data:text/html,<div style='color:red'>First</div><div style='color:red'>Second</div>" width=150 height=100></iframe><br>
+<iframe src="data:text/html,<div style='color:green;background-color: lightgrey'>First</div><div style='color:green;background-color: lightgrey'>Second</div>" width=300 height=100></iframe><br>
+<iframe src="data:text/html,<div style='color:red'>First</div><div style='color:red'>Second</div>" width=150 height=100></iframe><br>
+<iframe src="data:text/html,<div style='color:green;background-color: lightgrey'>First</div><div style='color:green;background-color: lightgrey'>Second</div>" width=300 height=100></iframe><br>

trunk/LayoutTests/fast/shadow-dom/media-query-in-shadow-style.html

@@ -1 @@
-<iframe src=resources/media-query-in-shadow-style-frame.html width=150 height=50></iframe><br>
-<iframe src=resources/media-query-in-shadow-style-frame.html width=300 height=50></iframe><br>
-<iframe src=resources/media-query-in-shadow-style-frame.html width=300 height=50 onload='resize(event.target, 150)'></iframe><br>
-<iframe src=resources/media-query-in-shadow-style-frame.html width=150 height=50 onload='resize(event.target, 300)'></iframe><br>
+<iframe src=resources/media-query-in-shadow-style-frame.html width=150 height=100></iframe><br>
+<iframe src=resources/media-query-in-shadow-style-frame.html width=300 height=100></iframe><br>
+<iframe src=resources/media-query-in-shadow-style-frame.html width=300 height=100 onload='resize(event.target, 150)'></iframe><br>
+<iframe src=resources/media-query-in-shadow-style-frame.html width=150 height=100 onload='resize(event.target, 300)'></iframe><br>
 <script>
 if (window.testRunner)

trunk/LayoutTests/fast/shadow-dom/resources/media-query-in-shadow-style-frame.html

@@ -1 @@
-<div id=test><span>Text</span></div>
+<div class=test><span>First</span></div>
+<div class=test><span>Second</span></div>
 <script>
-const shadow = test.attachShadow({mode: 'open'});
-shadow.innerHTML = `
-    <style>
-    @media (min-width:200px) {
-        div { color: green }
-        :host { background-color: lightgrey }
-    }
-    @media (max-width:200px) {
-        ::slotted(*) { color: red }
-    }
-    </style>
-    <div><slot></slot></div>
-`;
+for (test of document.querySelectorAll('.test')) {
+    const shadow = test.attachShadow({mode: 'open'});
+    shadow.innerHTML = `
+        <style>
+        @media (min-width:200px) {
+            div { color: green }
+            :host { background-color: lightgrey }
+        }
+        @media (max-width:200px) {
+            ::slotted(*) { color: red }
+        }
+        </style>
+        <div><slot></slot></div>
+    `;
+}
 </script>

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2021-06-02  Antti Koivisto  <antti@apple.com>
+
+        REGRESSION(r276882): Style not invalidated correctly for media queries in shadow trees that share style
+        https://bugs.webkit.org/show_bug.cgi?id=226369
+        <rdar://problem/78684562>
+
+        Reviewed by Sam Weinig.
+
+        Style resolvers are stateful in respect to media queries. We would only invalidate in the first shadow tree
+        because that evaluation flipped the state and the second evaluation would see nothing changing in media query
+        state.
+
+        * style/StyleScope.cpp:
+        (WebCore::Style::Scope::collectResolverScopes):
+
+        Add a helper to collect all scopes associated with a resolver.
+
+        (WebCore::Style::Scope::evaluateMediaQueries):
+
+        Only evaluate each resolver once, then invalidate all associated scopes if needed.
+
+        * style/StyleScope.h:
+
 2021-05-28  Antoine Quint  <graouts@webkit.org>
 

trunk/Source/WebCore/style/StyleScope.cpp

@@ -653 +653 @@
 }
 
+auto Scope::collectResolverScopes() -> ResolverScopes
+{
+    ASSERT(!m_shadowRoot);
+
+    if (!resolverIfExists())
+        return { };
+
+    ResolverScopes resolverScopes;
+
+    resolverScopes.add(makeRef(*resolverIfExists()), Vector<WeakPtr<Scope>> { makeWeakPtr(*this) });
+
+    for (auto* shadowRoot : m_document.inDocumentShadowRoots()) {
+        auto& scope = shadowRoot->styleScope();
+        auto* resolver = scope.resolverIfExists();
+        if (!resolver)
+            continue;
+        resolverScopes.add(makeRef(*resolver), Vector<WeakPtr<Scope>> { }).iterator->value.append(makeWeakPtr(scope));
+    }
+    return resolverScopes;
+}
+
 template <typename TestFunction>
 void Scope::evaluateMediaQueries(TestFunction&& testFunction)
 {
-    auto* resolver = resolverIfExists();
-    if (!resolver)
-        return;
-
-    auto evaluationChanges = testFunction(*resolver);
-    if (evaluationChanges) {
-        switch (evaluationChanges->type) {
-        case DynamicMediaQueryEvaluationChanges::Type::InvalidateStyle: {
-            Invalidator invalidator(evaluationChanges->invalidationRuleSets);
-            invalidator.invalidateStyle(*this);
-            break;
+    bool hadChanges = false;
+
+    auto resolverScopes = collectResolverScopes();
+    for (auto& [resolver, scopes] : resolverScopes) {
+        auto evaluationChanges = testFunction(resolver.get());
+        if (!evaluationChanges)
+            continue;
+        hadChanges = true;
+
+        for (auto& scope : scopes) {
+            switch (evaluationChanges->type) {
+            case DynamicMediaQueryEvaluationChanges::Type::InvalidateStyle: {
+                Invalidator invalidator(evaluationChanges->invalidationRuleSets);
+                invalidator.invalidateStyle(*scope);
+                break;
+            }
+            case DynamicMediaQueryEvaluationChanges::Type::ResetStyle:
+                scope->scheduleUpdate(UpdateType::ContentsOrInterpretation);
+                break;
+            }
         }
-        case DynamicMediaQueryEvaluationChanges::Type::ResetStyle:
-            scheduleUpdate(UpdateType::ContentsOrInterpretation);
-            break;
-        }
-
+    }
+
+    if (hadChanges)
         InspectorInstrumentation::mediaQueryResultChanged(m_document);
-    }
-
-    if (!m_shadowRoot) {
-        for (auto* descendantShadowRoot : m_document.inDocumentShadowRoots())
-            descendantShadowRoot->styleScope().evaluateMediaQueries(testFunction);
-    }
 }
 

trunk/Source/WebCore/style/StyleScope.h

@@ -36 +36 @@
 #include <wtf/RefPtr.h>
 #include <wtf/Vector.h>
+#include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
@@ -55 +56 @@
 class Resolver;
 
-class Scope {
+class Scope : public CanMakeWeakPtr<Scope> {
     WTF_MAKE_FAST_ALLOCATED;
 public:
@@ -134 +135 @@
     void scheduleUpdate(UpdateType);
 
+    using ResolverScopes = HashMap<Ref<Resolver>, Vector<WeakPtr<Scope>>>;
+    ResolverScopes collectResolverScopes();
     template <typename TestFunction> void evaluateMediaQueries(TestFunction&&);