Attachment 460122 Details for Bug 241457 – Patch

[patch] Patch

bug-241457-20220609131111.patch (text/plain), 12.97 KB, created by Antti Koivisto on 2022-06-09 03:11:12 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Antti Koivisto

Created: 2022-06-09 03:11:12 PDT

Size: 12.97 KB

patch

obsolete

>From 1f6ac1c071977fe4b7833e3b9cc6ea8ccd36ef19 Mon Sep 17 00:00:00 2001
>From: Antti Koivisto <antti@apple.com>
>Date: Thu, 9 Jun 2022 12:55:15 +0300
>Subject: [PATCH] [CSS Container Queries] Correctly resolve queries when
> container size is affected by a subsequent sibling
> https://bugs.webkit.org/show_bug.cgi?id=241457
>
>Reviewed by NOBODY (OOPS!).
>
>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:
>---
> Source/WebCore/dom/Document.cpp               | 16 +++---
> Source/WebCore/dom/Element.cpp                |  4 +-
> Source/WebCore/style/StyleChange.cpp          |  6 +--
> Source/WebCore/style/StyleChange.h            |  1 +
> Source/WebCore/style/StyleTreeResolver.cpp    | 53 +++++++++++--------
> Source/WebCore/style/StyleTreeResolver.h      |  5 +-
> .../sibling-layout-dependency-expected.txt    |  8 +--
> 7 files changed, 54 insertions(+), 39 deletions(-)
>
>diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
>index ea500fd9ca83..4c55abeb9ae9 100644
>--- a/Source/WebCore/dom/Document.cpp
>+++ b/Source/WebCore/dom/Document.cpp
>@@ -2095,13 +2095,15 @@ void Document::resolveStyle(ResolveStyleType type)
>         Style::TreeResolver resolver(*this, WTFMove(m_pendingRenderTreeUpdate));
>         auto styleUpdate = resolver.resolve();
> 
>-        while (resolver.hasUnresolvedQueryContainers() && styleUpdate) {
>-            SetForScope resolvingContainerQueriesScope(m_isResolvingContainerQueries, true);
>-
>-            updateRenderTree(WTFMove(styleUpdate));
>-
>-            if (frameView.layoutContext().needsLayout())
>-                frameView.layoutContext().layout();
>+        while (resolver.hasUnresolvedQueryContainers()) {
>+            if (styleUpdate) {
>+                SetForScope resolvingContainerQueriesScope(m_isResolvingContainerQueries, true);
>+                
>+                updateRenderTree(WTFMove(styleUpdate));
>+
>+                if (frameView.layoutContext().needsLayout())
>+                    frameView.layoutContext().layout();
>+            }
> 
>             styleUpdate = resolver.resolve();
>         }
>diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
>index bf3af27ad8fe..509a89a656ba 100644
>--- a/Source/WebCore/dom/Element.cpp
>+++ b/Source/WebCore/dom/Element.cpp
>@@ -2267,8 +2267,8 @@ void Element::invalidateStyleForSubtreeInternal()
> 
> 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()
>diff --git a/Source/WebCore/style/StyleChange.cpp b/Source/WebCore/style/StyleChange.cpp
>index d768c6405740..f961a826b078 100644
>--- a/Source/WebCore/style/StyleChange.cpp
>+++ b/Source/WebCore/style/StyleChange.cpp
>@@ -57,11 +57,11 @@ Change determineChange(const RenderStyle& s1, const RenderStyle& s2)
>     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))
>diff --git a/Source/WebCore/style/StyleChange.h b/Source/WebCore/style/StyleChange.h
>index 7bbca3c8a53d..c9f2b3991a5e 100644
>--- a/Source/WebCore/style/StyleChange.h
>+++ b/Source/WebCore/style/StyleChange.h
>@@ -36,6 +36,7 @@ enum class Change : uint8_t {
>     NonInherited,
>     FastPathInherited,
>     Inherited,
>+    Descendants,
>     Renderer
> };
> 
>diff --git a/Source/WebCore/style/StyleTreeResolver.cpp b/Source/WebCore/style/StyleTreeResolver.cpp
>index 4e93b10a15de..07db0c6b1a59 100644
>--- a/Source/WebCore/style/StyleTreeResolver.cpp
>+++ b/Source/WebCore/style/StyleTreeResolver.cpp
>@@ -199,6 +199,7 @@ auto TreeResolver::computeDescendantsToResolve(Change change, Validity validity,
>     case Change::FastPathInherited:
>     case Change::Inherited:
>         return DescendantsToResolve::Children;
>+    case Change::Descendants:
>     case Change::Renderer:
>         return DescendantsToResolve::All;
>     };
>@@ -625,6 +626,8 @@ ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderSt
> 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 @@ void TreeResolver::popParent()
>     auto& parentElement = *parent().element;
> 
>     parentElement.setHasValidStyle();
>-    parentElement.clearChildNeedsStyleRecalc();
>+    // Don't clear the child flag if the are unresolved containers because we are going to resume the style resolution.
>+    if (m_unresolvedQueryContainers.isEmpty())
>+        parentElement.clearChildNeedsStyleRecalc();
> 
>     if (parent().didPushScope)
>         popScope();
>@@ -834,10 +839,21 @@ void TreeResolver::resolveComposedTree()
>         if (!style)
>             resetStyleForNonRenderedDescendants(element);
> 
>-        bool shouldIterateChildren = style && (element.childNeedsStyleRecalc() || descendantsToResolve != DescendantsToResolve::None);
>+        auto queryContainerAction = determineQueryContainerAction(element, style, previousContainerType);
>+
>+        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;
>+        }();
> 
>-        if (style && updateQueryContainer(element, *style, previousContainerType) == QueryContainerAction::Layout)
>-            shouldIterateChildren = false;
>+        // 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,39 +876,34 @@ void TreeResolver::resolveComposedTree()
>     popParentsToDepth(1);
> }
> 
>-auto TreeResolver::updateQueryContainer(Element& element, const RenderStyle& style, ContainerType previousContainerType) -> QueryContainerAction
>+auto TreeResolver::determineQueryContainerAction(Element& element, const RenderStyle* style, ContainerType previousContainerType) -> QueryContainerAction
> {
>-    if (style.containerType() != ContainerType::None)
>-        scope().selectorMatchingState.queryContainers.append(element);
>-
>-    if (m_unresolvedQueryContainers.remove(&element))
>-        return QueryContainerAction::Continue;
>+    if (!style)
>+        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)
>+    // 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;
> 
>-    if (m_update->isEmpty())
>-        return QueryContainerAction::Continue;
>+    if (m_resolvedQueryContainers.contains(&element))
>+        return QueryContainerAction::None;
> 
>-    // Bail out from TreeResolver to build a render tree and do a layout. Resolution continues after.
>     m_unresolvedQueryContainers.add(&element);
>-    return QueryContainerAction::Layout;
>+    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();
>         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);
> 
>diff --git a/Source/WebCore/style/StyleTreeResolver.h b/Source/WebCore/style/StyleTreeResolver.h
>index 42ad55da9dbd..60488092af39 100644
>--- a/Source/WebCore/style/StyleTreeResolver.h
>+++ b/Source/WebCore/style/StyleTreeResolver.h
>@@ -63,8 +63,8 @@ private:
> 
>     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(Element&, const RenderStyle*, ContainerType previousContainerType);
> 
>     enum class DescendantsToResolve : uint8_t { None, ChildrenWithExplicitInherit, Children, All };
>     std::pair<ElementUpdate, DescendantsToResolve> resolveElement(Element&, ResolutionType);
>@@ -130,6 +130,7 @@ private:
>     bool m_didSeePendingStylesheet { false };
> 
>     HashSet<RefPtr<Element>> m_unresolvedQueryContainers;
>+    HashSet<RefPtr<Element>> m_resolvedQueryContainers;
> 
>     std::unique_ptr<Update> m_update;
> };
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt
>index 886926eba332..955cef797899 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/css/css-contain/container-queries/sibling-layout-dependency-expected.txt
>@@ -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
> 
>-- 
>2.32.0 (Apple Git-132)
>

Actions: View | Formatted Diff | Diff

Attachments on bug 241457: 460120 | 460121 | 460122 | 460123