Title: [295418] trunk
Revision
295418
Author
an...@apple.com
Date
2022-06-09 06:14:53 -0700 (Thu, 09 Jun 2022)

Log Message

Correctly resolve queries when container size is affected by a subsequent sibling
https://bugs.webkit.org/show_bug.cgi?id=241457

Reviewed by Alan Bujtas.

We currently only take style changes before the container into account.

This patch changes the query container resolution so that whenever we encounter a query container
we skip the subtree and restart the resolution from the main resolution loop. This way we
can optimize away unnecessary layouts while also handling backward dependencies.

* LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt:
* Source/WebCore/dom/Document.cpp:
(WebCore::Document::resolveStyle):

Loop also when there are no style changes to apply but there are unresolved container. In this case we just resume
the style resolution where we left it.

* Source/WebCore/dom/Element.cpp:
(WebCore::Element::invalidateForQueryContainerChange):

Invalidate all descendants when a query container is resized. In practice we ended up computing them all anyway.

* Source/WebCore/style/StyleChange.cpp:
(WebCore::Style::determineChange):
* Source/WebCore/style/StyleChange.h:

Add new Descendants value for when container type or name changes.

* Source/WebCore/style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::computeDescendantsToResolve):
(WebCore::Style::TreeResolver::pushParent):
(WebCore::Style::TreeResolver::popParent):
(WebCore::Style::TreeResolver::resolveComposedTree):
(WebCore::Style::TreeResolver::determineQueryContainerAction):

Use two buckets for unresolved and resolved containers. Use the resolved bucket to avoid
recomputing containers we have resolved already.

(WebCore::Style::TreeResolver::resolve):

Move the containers from the unresolved to resolved bucket in the beginning.

(WebCore::Style::TreeResolver::updateQueryContainer): Deleted.
* Source/WebCore/style/StyleTreeResolver.h:

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

Modified Paths

Diff

Modified: trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt (295417 => 295418)


--- trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt	2022-06-09 13:14:53 UTC (rev 295418)
@@ -1,7 +1,7 @@
 XXXX
 
-FAIL Sibling style mutation assert_equals: expected "10" but got "20"
-FAIL Sibling style mutation, parent is affected assert_equals: expected "10" but got "20"
-FAIL Sibling style mutation, ancestor is affected assert_equals: expected "10" but got "20"
-FAIL Sibling text mutation assert_equals: expected "10" but got "20"
+PASS Sibling style mutation
+PASS Sibling style mutation, parent is affected
+PASS Sibling style mutation, ancestor is affected
+PASS Sibling text mutation
 

Modified: trunk/Source/WebCore/dom/Document.cpp (295417 => 295418)


--- trunk/Source/WebCore/dom/Document.cpp	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/dom/Document.cpp	2022-06-09 13:14:53 UTC (rev 295418)
@@ -2095,14 +2095,16 @@
         Style::TreeResolver resolver(*this, WTFMove(m_pendingRenderTreeUpdate));
         auto styleUpdate = resolver.resolve();
 
-        while (resolver.hasUnresolvedQueryContainers() && styleUpdate) {
-            SetForScope resolvingContainerQueriesScope(m_isResolvingContainerQueries, true);
+        while (resolver.hasUnresolvedQueryContainers()) {
+            if (styleUpdate) {
+                SetForScope resolvingContainerQueriesScope(m_isResolvingContainerQueries, true);
+                
+                updateRenderTree(WTFMove(styleUpdate));
 
-            updateRenderTree(WTFMove(styleUpdate));
+                if (frameView.layoutContext().needsLayout())
+                    frameView.layoutContext().layout();
+            }
 
-            if (frameView.layoutContext().needsLayout())
-                frameView.layoutContext().layout();
-
             styleUpdate = resolver.resolve();
         }
 

Modified: trunk/Source/WebCore/dom/Element.cpp (295417 => 295418)


--- trunk/Source/WebCore/dom/Element.cpp	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/dom/Element.cpp	2022-06-09 13:14:53 UTC (rev 295418)
@@ -2267,8 +2267,8 @@
 
 void Element::invalidateForQueryContainerChange()
 {
-    // FIXME: This doesn't really need to recompute the element style.
-    Node::invalidateStyle(Style::Validity::ElementInvalid);
+    // FIXME: Ideally we would just recompute things that are actually affected by containers queries within the subtree.
+    Node::invalidateStyle(Style::Validity::SubtreeInvalid);
 }
 
 void Element::invalidateEventListenerRegions()

Modified: trunk/Source/WebCore/style/StyleChange.cpp (295417 => 295418)


--- trunk/Source/WebCore/style/StyleChange.cpp	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/style/StyleChange.cpp	2022-06-09 13:14:53 UTC (rev 295418)
@@ -57,11 +57,11 @@
     if (s1.hasTextCombine() != s2.hasTextCombine())
         return Change::Renderer;
 
-    if (!s1.descendantAffectingNonInheritedPropertiesEqual(s2))
-        return Change::Inherited;
-
     // Query container changes affect descendant style.
     if (s1.containerType() != s2.containerType() || s1.containerNames() != s2.containerNames())
+        return Change::Descendants;
+
+    if (!s1.descendantAffectingNonInheritedPropertiesEqual(s2))
         return Change::Inherited;
 
     if (!s1.nonFastPathInheritedEqual(s2))

Modified: trunk/Source/WebCore/style/StyleChange.h (295417 => 295418)


--- trunk/Source/WebCore/style/StyleChange.h	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/style/StyleChange.h	2022-06-09 13:14:53 UTC (rev 295418)
@@ -36,6 +36,7 @@
     NonInherited,
     FastPathInherited,
     Inherited,
+    Descendants,
     Renderer
 };
 

Modified: trunk/Source/WebCore/style/StyleTreeResolver.cpp (295417 => 295418)


--- trunk/Source/WebCore/style/StyleTreeResolver.cpp	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/style/StyleTreeResolver.cpp	2022-06-09 13:14:53 UTC (rev 295418)
@@ -199,6 +199,7 @@
     case Change::FastPathInherited:
     case Change::Inherited:
         return DescendantsToResolve::Children;
+    case Change::Descendants:
     case Change::Renderer:
         return DescendantsToResolve::All;
     };
@@ -625,6 +626,8 @@
 void TreeResolver::pushParent(Element& element, const RenderStyle& style, Change change, DescendantsToResolve descendantsToResolve)
 {
     scope().selectorMatchingState.selectorFilter.pushParent(&element);
+    if (style.containerType() != ContainerType::None)
+        scope().selectorMatchingState.queryContainers.append(element);
 
     Parent parent(element, style, change, descendantsToResolve);
 
@@ -645,7 +648,9 @@
     auto& parentElement = *parent().element;
 
     parentElement.setHasValidStyle();
-    parentElement.clearChildNeedsStyleRecalc();
+    // Don't clear the child flags if there are unresolved containers because we are going to resume the style resolution.
+    if (!hasUnresolvedQueryContainers())
+        parentElement.clearChildNeedsStyleRecalc();
 
     if (parent().didPushScope)
         popScope();
@@ -834,11 +839,22 @@
         if (!style)
             resetStyleForNonRenderedDescendants(element);
 
-        bool shouldIterateChildren = style && (element.childNeedsStyleRecalc() || descendantsToResolve != DescendantsToResolve::None);
+        auto queryContainerAction = determineQueryContainerAction(element, style, previousContainerType);
 
-        if (style && updateQueryContainer(element, *style, previousContainerType) == QueryContainerAction::Layout)
-            shouldIterateChildren = false;
+        bool shouldIterateChildren = [&] {
+            // display::none, no need to resolve descendants.
+            if (!style)
+                return false;
+            // Style resolution will be resumed after the container has been resolved.
+            if (queryContainerAction == QueryContainerAction::Resolve)
+                return false;
+            return element.childNeedsStyleRecalc() || descendantsToResolve != DescendantsToResolve::None;
+        }();
 
+        // Ensure we respect DescendantsToResolve::All after resuming the style resolution.
+        if (queryContainerAction == QueryContainerAction::Resolve && descendantsToResolve == DescendantsToResolve::All)
+            element.invalidateStyleForSubtreeInternal();
+
         if (!m_didSeePendingStylesheet)
             m_didSeePendingStylesheet = hasLoadingStylesheet(m_document.styleScope(), element, !shouldIterateChildren);
 
@@ -860,29 +876,28 @@
     popParentsToDepth(1);
 }
 
-auto TreeResolver::updateQueryContainer(Element& element, const RenderStyle& style, ContainerType previousContainerType) -> QueryContainerAction
+auto TreeResolver::determineQueryContainerAction(const Element& element, const RenderStyle* style, ContainerType previousContainerType) -> QueryContainerAction
 {
-    if (style.containerType() != ContainerType::None)
-        scope().selectorMatchingState.queryContainers.append(element);
+    if (!style)
+        return QueryContainerAction::None;
 
-    if (m_unresolvedQueryContainers.remove(&element))
-        return QueryContainerAction::Continue;
+    // FIXME: Render tree needs to be updated before proceeding to children also if we have a former query container
+    // because container unit resolution for descendants relies on it being up-to-date.
+    if (style->containerType() == ContainerType::None && previousContainerType == ContainerType::None)
+        return QueryContainerAction::None;
 
-    // Render tree needs to be updated before proceeding to children also if we have a former query container
-    // because container query resolution for descendants relies on it being up-to-date.
-    if (style.containerType() == ContainerType::None && previousContainerType == ContainerType::None)
+    if (m_resolvedQueryContainers.contains(element))
         return QueryContainerAction::None;
 
-    if (m_update->isEmpty())
-        return QueryContainerAction::Continue;
-
-    // Bail out from TreeResolver to build a render tree and do a layout. Resolution continues after.
-    m_unresolvedQueryContainers.add(&element);
-    return QueryContainerAction::Layout;
+    m_unresolvedQueryContainers.append(element);
+    return QueryContainerAction::Resolve;
 }
 
 std::unique_ptr<Update> TreeResolver::resolve()
 {
+    m_resolvedQueryContainers.add(m_unresolvedQueryContainers.begin(), m_unresolvedQueryContainers.end());
+    m_unresolvedQueryContainers.clear();
+
     Element* documentElement = m_document.documentElement();
     if (!documentElement) {
         m_document.styleScope().resolver();
@@ -889,10 +904,6 @@
         return nullptr;
     }
 
-    // FIXME: Just need to restore the ancestor marking.
-    for (auto& queryContainer : m_unresolvedQueryContainers)
-        queryContainer->invalidateStyleForSubtreeInternal();
-
     if (!documentElement->childNeedsStyleRecalc() && !documentElement->needsStyleRecalc())
         return WTFMove(m_update);
 

Modified: trunk/Source/WebCore/style/StyleTreeResolver.h (295417 => 295418)


--- trunk/Source/WebCore/style/StyleTreeResolver.h	2022-06-09 12:05:35 UTC (rev 295417)
+++ trunk/Source/WebCore/style/StyleTreeResolver.h	2022-06-09 13:14:53 UTC (rev 295418)
@@ -63,8 +63,8 @@
 
     void resolveComposedTree();
 
-    enum class QueryContainerAction : uint8_t { None, Continue, Layout };
-    QueryContainerAction updateQueryContainer(Element&, const RenderStyle&, ContainerType previousContainerType);
+    enum class QueryContainerAction : uint8_t { None, Resolve };
+    QueryContainerAction determineQueryContainerAction(const Element&, const RenderStyle*, ContainerType previousContainerType);
 
     enum class DescendantsToResolve : uint8_t { None, ChildrenWithExplicitInherit, Children, All };
     std::pair<ElementUpdate, DescendantsToResolve> resolveElement(Element&, ResolutionType);
@@ -129,7 +129,8 @@
     Vector<Parent, 32> m_parentStack;
     bool m_didSeePendingStylesheet { false };
 
-    HashSet<RefPtr<Element>> m_unresolvedQueryContainers;
+    Vector<Ref<const Element>> m_unresolvedQueryContainers;
+    HashSet<Ref<const Element>> m_resolvedQueryContainers;
 
     std::unique_ptr<Update> m_update;
 };
_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to