Element::isFocusableWithoutResolvingFullStyle() inert checks do not u…

…pdate right away https://bugs.webkit.org/show_bug.cgi?id=239006 rdar://91485677 Reviewed by Alan Baradlay. The code for resolving computed style in non-rendered subtree fails to take invalid ancestors into account. In this case the effective inertness (a fake property) fails to inherit from the document element. * LayoutTests/TestExpectations: * LayoutTests/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt: * LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-focusing-steps-inert-expected.txt: Removed. * LayoutTests/platform/ios-wk2/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt: Removed. * LayoutTests/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt: imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable.html is now passing except on Mac WK2 for some reason. * LayoutTests/editing/style/apply-style-atomic-expected.txt: * LayoutTests/editing/style/apply-style-atomic-live-range-expected.txt: These results now correctly include <progress> inside the link. * LayoutTests/imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/remove-dialog-should-unblock-document-expected.txt: * LayoutTests/imported/w3c/web-platform-tests/svg/struct/scripted/autofocus-attribute-expected.txt: * Source/WebCore/dom/Document.cpp: (WebCore::Document::setFocusedElement): Focusability may depend on :focus-within matching as if the focus was already set. Set it tentatively before testing. This is tested in imported/w3c/web-platform-tests/css/selectors/focus-within-010.html * Source/WebCore/dom/Element.cpp: (WebCore::Element::setHasTentativeFocus): (WebCore::Element::resolveComputedStyle): This failed to check if any ancestor has invalid computed style. Due to inheritance that can affect the result. (WebCore::Element::isFocusableWithoutResolvingFullStyle const): All display:none checks are now done in resolveComputedStyle as it needs to traverse to the root anyway. (WebCore::Element::hasValidStyle const): Deleted. * Source/WebCore/dom/Element.h: Canonical link: https://commits.webkit.org/256601@main
WebKit · Nov 12, 2022 · 2b519ff · 2b519ff
1 parent 8b4aafa
commit 2b519ff
Show file tree

Hide file tree

Showing 12 changed files with 113 additions and 99 deletions.
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
@@ -949,7 +949,6 @@ imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/002.
 imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/combination_history_002.html [ Failure Pass ]
 imported/w3c/web-platform-tests/html/browsers/history/the-history-interface/combination_history_003.html [ Failure Pass ]
 imported/w3c/web-platform-tests/html/cross-origin-opener-policy/tentative/restrict-properties/iframe-popup-to-un.https.html [ Failure Pass ]
-imported/w3c/web-platform-tests/html/semantics/interactive-elements/the-dialog-element/show-modal-focusing-steps.html [ Failure Pass ]
 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/execution-timing/058.html [ Failure Pass ]
 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/json-module/repeated-imports.any.sharedworker.html [ Failure Pass ]
 imported/w3c/web-platform-tests/html/semantics/scripting-1/the-script-element/microtasks/evaluation-order-1.html [ Failure Pass ]

diff --git a/LayoutTests/editing/style/apply-style-atomic-expected.txt b/LayoutTests/editing/style/apply-style-atomic-expected.txt
@@ -2,19 +2,19 @@ Test that WebKit does not crash when we apply style to atomic elements and that
 | <a>
 |   href="a"
 |   "<#selection-anchor>1"
-| <progress>
-|   <a>
-|     style=""
-|     "2"
-|   <shadow:root>
-|     <div>
-|       pseudo="-webkit-progress-inner-element"
-|       shadow:pseudoId="-webkit-progress-inner-element"
+|   <progress>
+|     <a>
+|       style=""
+|       "2"
+|     <shadow:root>
 |       <div>
-|         pseudo="-webkit-progress-bar"
-|         shadow:pseudoId="-webkit-progress-bar"
+|         pseudo="-webkit-progress-inner-element"
+|         shadow:pseudoId="-webkit-progress-inner-element"
 |         <div>
-|           pseudo="-webkit-progress-value"
-|           style="width: -100%;"
-|           shadow:pseudoId="-webkit-progress-value"
-| <#selection-focus>
+|           pseudo="-webkit-progress-bar"
+|           shadow:pseudoId="-webkit-progress-bar"
+|           <div>
+|             pseudo="-webkit-progress-value"
+|             style="width: -100%;"
+|             shadow:pseudoId="-webkit-progress-value"
+|   <#selection-focus>
diff --git a/LayoutTests/editing/style/apply-style-atomic-live-range-expected.txt b/LayoutTests/editing/style/apply-style-atomic-live-range-expected.txt
@@ -2,19 +2,19 @@ Test that WebKit does not crash when we apply style to atomic elements and that
 | <a>
 |   href="a"
 |   "<#selection-anchor>1"
-| <progress>
-|   <a>
-|     style=""
-|     "2"
-|   <shadow:root>
-|     <div>
-|       pseudo="-webkit-progress-inner-element"
-|       shadow:pseudoId="-webkit-progress-inner-element"
+|   <progress>
+|     <a>
+|       style=""
+|       "2"
+|     <shadow:root>
 |       <div>
-|         pseudo="-webkit-progress-bar"
-|         shadow:pseudoId="-webkit-progress-bar"
+|         pseudo="-webkit-progress-inner-element"
+|         shadow:pseudoId="-webkit-progress-inner-element"
 |         <div>
-|           pseudo="-webkit-progress-value"
-|           style="width: -100%;"
-|           shadow:pseudoId="-webkit-progress-value"
-| <#selection-focus>
+|           pseudo="-webkit-progress-bar"
+|           shadow:pseudoId="-webkit-progress-bar"
+|           <div>
+|             pseudo="-webkit-progress-value"
+|             style="width: -100%;"
+|             shadow:pseudoId="-webkit-progress-value"
+|   <#selection-focus>
diff --git a/...nteractive-elements/the-dialog-element/remove-dialog-should-unblock-document-expected.txt b/...nteractive-elements/the-dialog-element/remove-dialog-should-unblock-document-expected.txt
@@ -1,4 +1,4 @@
-This is a dialog
 
-FAIL Test that removing dialog unblocks the document. assert_equals: expected false but got true
+
+PASS Test that removing dialog unblocks the document.
 
diff --git a/LayoutTests/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt
@@ -9,5 +9,5 @@ PASS Button with inert atribute is unfocusable.
 PASS All focusable elements inside inert subtree are unfocusable
 PASS Can get inert via property
 PASS Elements inside of inert subtrees return false when getting 'inert'
-FAIL Setting inert via property correctly modifies inert state assert_equals: expected true but got false
+PASS Setting inert via property correctly modifies inert state
 
diff --git a/...ests/imported/w3c/web-platform-tests/svg/struct/scripted/autofocus-attribute-expected.txt b/...ests/imported/w3c/web-platform-tests/svg/struct/scripted/autofocus-attribute-expected.txt
@@ -1,5 +1,5 @@
 
 PASS <a> should support autofocus
 PASS Renderable element with tabindex should support autofocus
-FAIL Never-rendered element with tabindex should not support autofocus promise_test: Unhandled rejection with value: object "TypeError: null is not an object (evaluating 'w.document.activeElement.tagName')"
+FAIL Never-rendered element with tabindex should not support autofocus assert_equals: expected "svg" but got "metadata"
 
diff --git a/...emantics/interactive-elements/the-dialog-element/dialog-focusing-steps-inert-expected.txt b/...emantics/interactive-elements/the-dialog-element/dialog-focusing-steps-inert-expected.txt
diff --git a/...ests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt b/...ests/html/semantics/interactive-elements/the-dialog-element/dialog-showModal-expected.txt
diff --git a/...form/mac-wk2/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt b/...form/mac-wk2/imported/w3c/web-platform-tests/inert/inert-node-is-unfocusable-expected.txt
@@ -0,0 +1,13 @@
+Outside of inert container  Inert button
+
+I'm editable
+I'm tabindexed.
+ Link
+
+PASS Button outside of inert container is focusable.
+PASS Button with inert atribute is unfocusable.
+PASS All focusable elements inside inert subtree are unfocusable
+PASS Can get inert via property
+PASS Elements inside of inert subtrees return false when getting 'inert'
+FAIL Setting inert via property correctly modifies inert state assert_equals: expected true but got false
+
diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp
@@ -4780,7 +4780,17 @@ bool Document::setFocusedElement(Element* element, const FocusOptions& options)
             return false;
     }
 
-    if (newFocusedElement && newFocusedElement->isFocusable()) {
+    auto isNewElementFocusable = [&] {
+        if (!newFocusedElement)
+            return false;
+        // Resolving isFocusable() may require matching :focus-within as if the focus was already on the new element.
+        newFocusedElement->setHasTentativeFocus(true);
+        bool isFocusable = newFocusedElement->isFocusable();
+        newFocusedElement->setHasTentativeFocus(false);
+        return isFocusable;
+    }();
+
+    if (isNewElementFocusable) {
         if (&newFocusedElement->document() != this) {
             // Bluring oldFocusedElement may have moved newFocusedElement across documents.
             return false;

diff --git a/Source/WebCore/dom/Element.cpp b/Source/WebCore/dom/Element.cpp
@@ -866,6 +866,15 @@ void Element::setHasFocusWithin(bool value)
     }
 }
 
+void Element::setHasTentativeFocus(bool value)
+{
+    // Tentative focus is used when trying to set the focus on a new element.
+    for (auto& ancestor : composedTreeAncestors(*this)) {
+        ASSERT(ancestor.hasFocusWithin() != value);
+        document().userActionElements().setHasFocusWithin(ancestor, value);
+    }
+}
+
 void Element::setHovered(bool value, Style::InvalidationScope invalidationScope, HitTestRequest)
 {
     if (value == hovered())
@@ -3698,25 +3707,58 @@ const RenderStyle* Element::renderOrDisplayContentsStyle(PseudoId pseudoId) cons
 const RenderStyle* Element::resolveComputedStyle(ResolveComputedStyleMode mode)
 {
     ASSERT(isConnected());
-    ASSERT(!existingComputedStyle() || hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag));
 
-    Deque<RefPtr<Element>, 32> elementsRequiringComputedStyle({ this });
-    const RenderStyle* computedStyle = nullptr;
+    bool isInDisplayNoneTree = false;
 
-    // Collect ancestors until we find one that has style.
-    for (auto& ancestor : composedTreeAncestors(*this)) {
-        if (auto* existingStyle = ancestor.existingComputedStyle()) {
-            computedStyle = existingStyle;
-            break;
+    // Traverse the ancestor chain to find the rootmost element that has invalid computed style.
+    auto* rootmostInvalidElement = [&]() -> const Element* {
+        if (document().hasPendingFullStyleRebuild())
+            return document().documentElement();
+
+        if (!document().documentElement() || document().documentElement()->hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag))
+            return document().documentElement();
+
+        const Element* rootmost = nullptr;
+
+        for (auto* element = this; element; element = element->parentElementInComposedTree()) {
+            if (element->hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag)) {
+                rootmost = element;
+                continue;
+            }
+            auto* existing = element->existingComputedStyle();
+            if (!existing) {
+                rootmost = element;
+                continue;
+            }
+            if (mode == ResolveComputedStyleMode::RenderedOnly && existing->display() == DisplayType::None) {
+                isInDisplayNoneTree = true;
+                return nullptr;
+            }
         }
-        elementsRequiringComputedStyle.prepend(&ancestor);
-    }
+        return rootmost;
+    }();
+
+    if (isInDisplayNoneTree)
+        return nullptr;
+
+    if (!rootmostInvalidElement)
+        return existingComputedStyle();
+
+    auto* ancestorWithValidStyle = rootmostInvalidElement->parentElementInComposedTree();
+
+    Vector<RefPtr<Element>, 32> elementsRequiringComputedStyle;
+    for (auto* toResolve = this; toResolve != ancestorWithValidStyle; toResolve = toResolve->parentElementInComposedTree())
+        elementsRequiringComputedStyle.append(toResolve);
+
+    auto* computedStyle = ancestorWithValidStyle ? ancestorWithValidStyle->existingComputedStyle() : nullptr;
 
     // On iOS request delegates called during styleForElement may result in re-entering WebKit and killing the style resolver.
     Style::PostResolutionCallbackDisabler disabler(document(), Style::PostResolutionCallbackDisabler::DrainCallbacks::No);
 
     // Resolve and cache styles starting from the most distant ancestor.
-    for (auto& element : elementsRequiringComputedStyle) {
+    // FIXME: This is not as efficient as it could be. For example if an ancestor has a non-inherited style change but
+    // the styles are otherwise clean we would not need to re-resolve descendants.
+    for (auto& element : makeReversedRange(elementsRequiringComputedStyle)) {
         bool hadDisplayContents = element->hasDisplayContents();
         auto style = document().styleForElementIgnoringPendingStylesheets(*element, computedStyle);
         computedStyle = style.get();
@@ -3733,21 +3775,6 @@ const RenderStyle* Element::resolveComputedStyle(ResolveComputedStyleMode mode)
     return computedStyle;
 }
 
-bool Element::hasValidStyle() const
-{
-    if (!document().needsStyleRecalc())
-        return true;
-
-    if (document().hasPendingFullStyleRebuild())
-        return false;
-
-    for (auto& element : lineageOfType<Element>(*this)) {
-        if (element.styleValidity() != Style::Validity::Valid)
-            return false;
-    }
-    return true;
-}
-
 bool Element::isFocusableWithoutResolvingFullStyle() const
 {
     auto isFocusableStyle = [](const RenderStyle* style) {
@@ -3758,26 +3785,12 @@ bool Element::isFocusableWithoutResolvingFullStyle() const
             && !style->effectiveInert();
     };
 
-    if (renderStyle() || hasValidStyle())
+    if (renderStyle())
         return isFocusableStyle(renderStyle());
 
-    auto computedStyleForElement = [](Element& element) -> const RenderStyle* {
-        auto* style = element.hasNodeFlag(NodeFlag::IsComputedStyleInvalidFlag) ? nullptr : element.existingComputedStyle();
-        return style ? style : element.resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
-    };
-
     // Compute style in yet unstyled subtree.
-    auto* style = computedStyleForElement(const_cast<Element&>(*this));
-    if (!isFocusableStyle(style))
-        return false;
-
-    for (auto& element : composedTreeAncestors(const_cast<Element&>(*this))) {
-        auto* style = computedStyleForElement(element);
-        if (!style || style->display() == DisplayType::None)
-            return false;
-    }
-
-    return true;
+    auto* style = const_cast<Element&>(*this).resolveComputedStyle(ResolveComputedStyleMode::RenderedOnly);
+    return isFocusableStyle(style);
 }
 
 const RenderStyle& Element::resolvePseudoElementStyle(PseudoId pseudoElementSpecifier)

diff --git a/Source/WebCore/dom/Element.h b/Source/WebCore/dom/Element.h
@@ -373,6 +373,7 @@ class Element : public ContainerNode {
     void setBeingDragged(bool);
     void setHasFocusVisible(bool);
     void setHasFocusWithin(bool);
+    void setHasTentativeFocus(bool);
 
     std::optional<int> tabIndexSetExplicitly() const;
     bool shouldBeIgnoredInSequentialFocusNavigation() const { return defaultTabIndex() < 0 && !supportsFocus(); }
@@ -401,7 +402,6 @@ class Element : public ContainerNode {
 
     bool needsStyleInvalidation() const;
 
-    bool hasValidStyle() const;
     bool isFocusableWithoutResolvingFullStyle() const;
 
     // Methods for indicating the style is affected by dynamic updates (e.g., children changing, our position changing in our sibling list, etc.)