Support dynamic pseudo-classes on elements with display: contents
authorantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Nov 2018 18:55:17 +0000 (18:55 +0000)
committerantti@apple.com <antti@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 12 Nov 2018 18:55:17 +0000 (18:55 +0000)
https://bugs.webkit.org/show_bug.cgi?id=181640
<rdar://problem/36605415>

Reviewed by Dean Jackson.

Source/WebCore:

The code for :hover and :active style invalidation assumes that only elements with renderer need invalidation.

This patch fixes '.display-content-element:hover span' case but not '.display-content-element:hover' case but
includes tests for both. The latter is not super useful anyway (as it only affects rendering with inherited
text properties).

Test: fast/css/display-contents-hover-active.html

* dom/Document.cpp:
(WebCore::Document::updateHoverActiveState):

    Traverse up in composed tree instead of render tree when invalidating. This has the same order as render tree
    but also includes display:content elements. This also allows removing the special display:none case.

* dom/Element.cpp:
(WebCore::Element::setActive):
(WebCore::Element::setHovered):

    Also look into display:contents style for invalidation checks.

(WebCore::Element::renderOrDisplayContentsStyle const):

    Make this helper an Element member.

* dom/Element.h:
* dom/Node.cpp:
(WebCore::Node::parentElementInComposedTree const):

    Support starting from a PseudoElement. This is consistent with ComposedTreeAncestorIterator.

* rendering/updating/RenderTreePosition.cpp:
(WebCore::RenderTreePosition::nextSiblingRenderer const):
* style/StyleTreeResolver.cpp:
(WebCore::Style::TreeResolver::resolveElement):
(WebCore::Style::TreeResolver::createAnimatedElementUpdate):
(WebCore::Style::shouldResolveElement):
(WebCore::Style::TreeResolver::resolveComposedTree):
(WebCore::Style::renderOrDisplayContentsStyle): Deleted.

    Use the Element::renderOrDisplayContentsStyle() instead.

LayoutTests:

* fast/css/display-contents-hover-active-expected.txt: Added.
* fast/css/display-contents-hover-active.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@238097 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/css/display-contents-hover-active-expected.txt [new file with mode: 0644]
LayoutTests/fast/css/display-contents-hover-active.html [new file with mode: 0644]
LayoutTests/platform/ios/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/dom/Element.cpp
Source/WebCore/dom/Element.h
Source/WebCore/dom/Node.cpp
Source/WebCore/rendering/updating/RenderTreePosition.cpp
Source/WebCore/style/StyleTreeResolver.cpp

index cf70365..e474847 100644 (file)
@@ -1,3 +1,14 @@
+2018-11-12  Antti Koivisto  <antti@apple.com>
+
+        Support dynamic pseudo-classes on elements with display: contents
+        https://bugs.webkit.org/show_bug.cgi?id=181640
+        <rdar://problem/36605415>
+
+        Reviewed by Dean Jackson.
+
+        * fast/css/display-contents-hover-active-expected.txt: Added.
+        * fast/css/display-contents-hover-active.html: Added.
+
 2018-11-12  Simon Fraser  <simon.fraser@apple.com>
 
         feFlood with alpha color doesn't work correctly
diff --git a/LayoutTests/fast/css/display-contents-hover-active-expected.txt b/LayoutTests/fast/css/display-contents-hover-active-expected.txt
new file mode 100644 (file)
index 0000000..cec8e57
--- /dev/null
@@ -0,0 +1,9 @@
+Text should turn green on hover.
+Text should turn green on hover.
+Text should turn green on mouse down.
+Text should turn green on mouse down.
+test1 FAIL
+test2 PASS
+test3 FAIL
+test4 PASS
+
diff --git a/LayoutTests/fast/css/display-contents-hover-active.html b/LayoutTests/fast/css/display-contents-hover-active.html
new file mode 100644 (file)
index 0000000..f637334
--- /dev/null
@@ -0,0 +1,66 @@
+<!DOCTYPE html>
+<style>
+.container {
+    border: 2px solid blue;
+    width: 100px;
+    height: 100px;
+}
+.contents {
+    display:contents;
+}
+#innercontainer {
+    display:content;
+}
+#test1:hover { color: green }
+#test2:hover span { color: green  }
+#test3:active { color: green }
+#test4:active span { color: green  }
+</style>
+<div class=container>
+<div id=test1 class=contents>
+Text should turn green on hover.
+</div>
+</div>
+<div class=container>
+<div id=test2 class=contents>
+<span>
+Text should turn green on hover.
+</spans>
+</div>
+</div>
+<div class=container>
+<div id=test3 class=contents>
+Text should turn green on mouse down.
+</div>
+</div>
+<div class=container>
+<div id=test4 class=contents>
+<span>
+Text should turn green on mouse down.
+</spans>
+</div>
+</div>
+<div id=log></div>
+
+<script>
+function test(testId, activate) {
+    const testElement = document.getElementById(testId);
+    eventSender.mouseMoveTo(testElement.parentNode.offsetLeft + 5, testElement.parentNode.offsetTop + 5);
+    if (activate)
+        eventSender.mouseDown();
+    const verifyElement = testElement.firstElementChild ? testElement.firstElementChild : testElement;
+    const pass = getComputedStyle(verifyElement, null).color == 'rgb(0, 128, 0)';
+    log.innerHTML += `${testId} ${pass ? "PASS" : "FAIL"}<br>`;
+    if (activate)
+        eventSender.mouseUp();
+}
+
+if (window.testRunner) {
+    testRunner.dumpAsText();
+    test('test1');
+    test('test2');
+    test('test3', true);
+    test('test4', true);
+}
+</script>
+
index 0a7d9ca..3d47e04 100644 (file)
@@ -1008,6 +1008,7 @@ editing/pasteboard/get-data-text-plain-drop.html [ Skip ]
 editing/pasteboard/text-selection.html [ Skip ]
 fast/css/affected-by-hover-after-style-change.html [ Skip ]
 fast/css/ancestor-of-hovered-element-removed.html [ Skip ]
+fast/css/display-contents-hover-active.html [ Skip ]
 fast/css-generated-content/hover-inline.html [ Skip ]
 fast/css/hover-active-drag.html [ Skip ]
 fast/css/hover-affects-ancestor.html [ Skip ]
index 79965af..e6211ac 100644 (file)
@@ -1,3 +1,52 @@
+2018-11-12  Antti Koivisto  <antti@apple.com>
+
+        Support dynamic pseudo-classes on elements with display: contents
+        https://bugs.webkit.org/show_bug.cgi?id=181640
+        <rdar://problem/36605415>
+
+        Reviewed by Dean Jackson.
+
+        The code for :hover and :active style invalidation assumes that only elements with renderer need invalidation.
+
+        This patch fixes '.display-content-element:hover span' case but not '.display-content-element:hover' case but
+        includes tests for both. The latter is not super useful anyway (as it only affects rendering with inherited
+        text properties).
+
+        Test: fast/css/display-contents-hover-active.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::updateHoverActiveState):
+
+            Traverse up in composed tree instead of render tree when invalidating. This has the same order as render tree
+            but also includes display:content elements. This also allows removing the special display:none case.
+
+        * dom/Element.cpp:
+        (WebCore::Element::setActive):
+        (WebCore::Element::setHovered):
+
+            Also look into display:contents style for invalidation checks.
+
+        (WebCore::Element::renderOrDisplayContentsStyle const):
+
+            Make this helper an Element member.
+
+        * dom/Element.h:
+        * dom/Node.cpp:
+        (WebCore::Node::parentElementInComposedTree const):
+
+            Support starting from a PseudoElement. This is consistent with ComposedTreeAncestorIterator.
+
+        * rendering/updating/RenderTreePosition.cpp:
+        (WebCore::RenderTreePosition::nextSiblingRenderer const):
+        * style/StyleTreeResolver.cpp:
+        (WebCore::Style::TreeResolver::resolveElement):
+        (WebCore::Style::TreeResolver::createAnimatedElementUpdate):
+        (WebCore::Style::shouldResolveElement):
+        (WebCore::Style::TreeResolver::resolveComposedTree):
+        (WebCore::Style::renderOrDisplayContentsStyle): Deleted.
+
+            Use the Element::renderOrDisplayContentsStyle() instead.
+
 2018-11-12  Antoine Quint  <graouts@apple.com>
 
         [Web Animations] Turn Web Animations experimental
index f2546b0..ce97b7e 100644 (file)
@@ -7208,21 +7208,9 @@ void Document::updateHoverActiveState(const HitTestRequest& request, Element* in
     Vector<RefPtr<Element>, 32> elementsToAddToChain;
 
     if (oldHoverObj != newHoverObj) {
-        // If the old hovered element is not nil but it's renderer is, it was probably detached as part of the :hover style
-        // (for instance by setting display:none in the :hover pseudo-class). In this case, the old hovered element (and its ancestors)
-        // must be updated, to ensure it's normal style is re-applied.
-        if (oldHoveredElement && !oldHoverObj) {
-            for (Element* element = oldHoveredElement.get(); element; element = element->parentElementInComposedTree()) {
-                if (!mustBeInActiveChain || element->inActiveChain())
-                    elementsToRemoveFromChain.append(element);
-            }
-        }
-
-        // The old hover path only needs to be cleared up to (and not including) the common ancestor;
-        for (RenderElement* curr = oldHoverObj; curr && curr != ancestor; curr = curr->hoverAncestor()) {
-            Element* element = curr->element();
-            if (!element)
-                continue;
+        for (auto* element = oldHoveredElement.get(); element; element = element->parentElementInComposedTree()) {
+            if (ancestor && ancestor->element() == element)
+                break;
             if (!mustBeInActiveChain || element->inActiveChain())
                 elementsToRemoveFromChain.append(element);
         }
@@ -7233,11 +7221,7 @@ void Document::updateHoverActiveState(const HitTestRequest& request, Element* in
         }
     }
 
-    // Now set the hover state for our new object up to the root.
-    for (RenderElement* curr = newHoverObj; curr; curr = curr->hoverAncestor()) {
-        Element* element = curr->element();
-        if (!element)
-            continue;
+    for (auto* element = newHoveredElement; element; element = element->parentElementInComposedTree()) {
         if (!mustBeInActiveChain || element->inActiveChain())
             elementsToAddToChain.append(element);
     }
index a4bb89f..88207e5 100644 (file)
@@ -573,7 +573,7 @@ void Element::setActive(bool flag, bool pause)
 
     document().userActionElements().setActive(*this, flag);
 
-    const RenderStyle* renderStyle = this->renderStyle();
+    auto* renderStyle = renderOrDisplayContentsStyle();
     bool reactsToPress = (renderStyle && renderStyle->affectedByActive()) || styleAffectedByActive();
     if (reactsToPress)
         invalidateStyleForSubtree();
@@ -634,22 +634,23 @@ void Element::setHovered(bool flag)
 
     document().userActionElements().setHovered(*this, flag);
 
+    auto* style = renderOrDisplayContentsStyle();
+    if (style && (style->affectedByHover() || childrenAffectedByHover()))
+        invalidateStyleForSubtree();
+
     if (!renderer()) {
         // When setting hover to false, the style needs to be recalc'd even when
         // there's no renderer (imagine setting display:none in the :hover class,
         // if a nil renderer would prevent this element from recalculating its
         // style, it would never go back to its normal style and remain
         // stuck in its hovered style).
-        if (!flag)
+        if (!flag && !style)
             invalidateStyleForSubtree();
 
         return;
     }
 
-    if (renderer()->style().affectedByHover() || childrenAffectedByHover())
-        invalidateStyleForSubtree();
-
-    if (renderer()->style().hasAppearance())
+    if (style->hasAppearance())
         renderer()->theme().stateChanged(*renderer(), ControlStates::HoverState);
 }
 
@@ -2908,6 +2909,20 @@ const RenderStyle* Element::existingComputedStyle() const
     return renderStyle();
 }
 
+const RenderStyle* Element::renderOrDisplayContentsStyle() const
+{
+    if (auto* style = renderStyle())
+        return style;
+
+    if (!hasRareData())
+        return nullptr;
+    auto* style = elementRareData()->computedStyle();
+    if (style && style->display() == DisplayType::Contents)
+        return style;
+
+    return nullptr;
+}
+
 const RenderStyle& Element::resolveComputedStyle()
 {
     ASSERT(isConnected());
index 87b44ee..bc6eba8 100644 (file)
@@ -527,6 +527,7 @@ public:
     LayoutRect absoluteEventHandlerBounds(bool& includesFixedPositionElements) override;
 
     const RenderStyle* existingComputedStyle() const;
+    const RenderStyle* renderOrDisplayContentsStyle() const;
 
     void setBeforePseudoElement(Ref<PseudoElement>&&);
     void setAfterPseudoElement(Ref<PseudoElement>&&);
index aa7c65f..c06c126 100644 (file)
@@ -1224,6 +1224,8 @@ Element* Node::parentElementInComposedTree() const
 {
     if (auto* slot = assignedSlot())
         return slot;
+    if (is<PseudoElement>(*this))
+        return downcast<PseudoElement>(*this).hostElement();
     if (auto* parent = parentNode()) {
         if (is<ShadowRoot>(*parent))
             return downcast<ShadowRoot>(*parent).host();
index cace700..541d76c 100644 (file)
@@ -71,7 +71,7 @@ RenderObject* RenderTreePosition::nextSiblingRenderer(const Node& node) const
     Vector<Element*, 30> elementStack;
 
     // In the common case ancestor == parentElement immediately and this just pushes parentElement into stack.
-    auto* ancestor = is<PseudoElement>(node) ? downcast<PseudoElement>(node).hostElement() : node.parentElementInComposedTree();
+    auto* ancestor = node.parentElementInComposedTree();
     while (true) {
         elementStack.append(ancestor);
         if (ancestor == parentElement)
index 29c2be0..c8af097 100644 (file)
@@ -162,15 +162,6 @@ static bool affectsRenderedSubtree(Element& element, const RenderStyle& newStyle
     return false;
 }
 
-static const RenderStyle* renderOrDisplayContentsStyle(const Element& element)
-{
-    if (auto* renderStyle = element.renderStyle())
-        return renderStyle;
-    if (element.hasDisplayContents())
-        return element.existingComputedStyle();
-    return nullptr;
-}
-
 static DescendantsToResolve computeDescendantsToResolve(Change change, Validity validity, DescendantsToResolve parentDescendantsToResolve)
 {
     if (parentDescendantsToResolve == DescendantsToResolve::All)
@@ -203,7 +194,7 @@ ElementUpdates TreeResolver::resolveElement(Element& element)
     if (!affectsRenderedSubtree(element, *newStyle))
         return { };
 
-    auto* existingStyle = renderOrDisplayContentsStyle(element);
+    auto* existingStyle = element.renderOrDisplayContentsStyle();
 
     if (m_didSeePendingStylesheet && (!existingStyle || existingStyle->isNotFinal())) {
         newStyle->setIsNotFinal();
@@ -284,7 +275,7 @@ const RenderStyle* TreeResolver::parentBoxStyle() const
 
 ElementUpdate TreeResolver::createAnimatedElementUpdate(std::unique_ptr<RenderStyle> newStyle, Element& element, Change parentChange)
 {
-    auto* oldStyle = renderOrDisplayContentsStyle(element);
+    auto* oldStyle = element.renderOrDisplayContentsStyle();
 
     bool shouldRecompositeLayer = false;
 
@@ -397,7 +388,7 @@ static bool shouldResolveElement(const Element& element, DescendantsToResolve pa
     case DescendantsToResolve::All:
         return true;
     case DescendantsToResolve::ChildrenWithExplicitInherit:
-        auto* existingStyle = renderOrDisplayContentsStyle(element);
+        auto* existingStyle = element.renderOrDisplayContentsStyle();
         return existingStyle && existingStyle->hasExplicitlyInheritedProperties();
     };
     ASSERT_NOT_REACHED();
@@ -482,7 +473,7 @@ void TreeResolver::resolveComposedTree()
             continue;
         }
 
-        auto* style = renderOrDisplayContentsStyle(element);
+        auto* style = element.renderOrDisplayContentsStyle();
         auto change = NoChange;
         auto descendantsToResolve = DescendantsToResolve::None;