Hovering over a slotted Text node clears hover state
authorrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 12 Nov 2016 00:48:46 +0000 (00:48 +0000)
committerrniwa@webkit.org <rniwa@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sat, 12 Nov 2016 00:48:46 +0000 (00:48 +0000)
https://bugs.webkit.org/show_bug.cgi?id=164002
<rdar://problem/29040471>

Reviewed by Simon Fraser.

Source/WebCore:

The bug was caused by HitTestResult::innerElement returning the parent element of a Text node without
taking the shadow root or slots into account. For hit testing, we always want to use the "flat tree"
or "composed tree" (imprecisely but close enough in this case).

Fixed the bug by making HitTestResult::innerElement use parentNodeInComposedTree. Also renamed it to
HitTestResult::targetElement to be consistent with HitTestResult::targetNode.

Tests: fast/shadow-dom/activate-over-slotted-content.html
       fast/shadow-dom/hover-over-slotted-content.html

* dom/Document.cpp:
(WebCore::Document::prepareMouseEvent):
* html/MediaElementSession.cpp:
(WebCore::isMainContentForPurposesOfAutoplay):
* page/EventHandler.cpp:
(WebCore::EventHandler::eventMayStartDrag):
(WebCore::EventHandler::hitTestResultAtPoint):
(WebCore::EventHandler::handleWheelEvent):
(WebCore::EventHandler::sendContextMenuEventForKey):
(WebCore::EventHandler::hoverTimerFired):
(WebCore::EventHandler::handleDrag):
(WebCore::EventHandler::handleTouchEvent):
* rendering/HitTestResult.cpp:
(WebCore::HitTestResult::targetElement): Renamed from innerElement.
Now finds the parent element in the composed tree.
* rendering/HitTestResult.h:
(WebCore::HitTestResult::innerNode):

Source/WebKit/mac:

* WebView/WebImmediateActionController.mm:
(-[WebImmediateActionController performHitTestAtPoint:]):

Source/WebKit2:

* WebProcess/WebPage/WebPage.cpp:
(WebKit::WebPage::determinePrimarySnapshottedPlugIn):
* WebProcess/WebPage/mac/WebPageMac.mm:
(WebKit::WebPage::performImmediateActionHitTestAtLocation):

LayoutTests:

Added two reference tests for activating and hovering over a Text node.
The text node should activate :hover and :activate rules in the shadow tree respectively.

* fast/shadow-dom/activate-over-slotted-content-expected.html: Added.
* fast/shadow-dom/activate-over-slotted-content.html: Added.
* fast/shadow-dom/hover-over-slotted-content-expected.html: Added.
* fast/shadow-dom/hover-over-slotted-content.html: Added.
* platform/ios-simulator/TestExpectations: Skip the newly added tests since iOS doesn't
support :hover or :activate via mouse down.

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

17 files changed:
LayoutTests/ChangeLog
LayoutTests/fast/shadow-dom/activate-over-slotted-content-expected.html [new file with mode: 0644]
LayoutTests/fast/shadow-dom/activate-over-slotted-content.html [new file with mode: 0644]
LayoutTests/fast/shadow-dom/hover-over-slotted-content-expected.html [new file with mode: 0644]
LayoutTests/fast/shadow-dom/hover-over-slotted-content.html [new file with mode: 0644]
LayoutTests/platform/ios-simulator/TestExpectations
Source/WebCore/ChangeLog
Source/WebCore/dom/Document.cpp
Source/WebCore/html/MediaElementSession.cpp
Source/WebCore/page/EventHandler.cpp
Source/WebCore/rendering/HitTestResult.cpp
Source/WebCore/rendering/HitTestResult.h
Source/WebKit/mac/ChangeLog
Source/WebKit/mac/WebView/WebImmediateActionController.mm
Source/WebKit2/ChangeLog
Source/WebKit2/WebProcess/WebPage/WebPage.cpp
Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm

index 8b4dd25..2cf4ff1 100644 (file)
@@ -1,3 +1,21 @@
+2016-11-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Hovering over a slotted Text node clears hover state
+        https://bugs.webkit.org/show_bug.cgi?id=164002
+        <rdar://problem/29040471>
+
+        Reviewed by Simon Fraser.
+
+        Added two reference tests for activating and hovering over a Text node.
+        The text node should activate :hover and :activate rules in the shadow tree respectively.
+
+        * fast/shadow-dom/activate-over-slotted-content-expected.html: Added.
+        * fast/shadow-dom/activate-over-slotted-content.html: Added.
+        * fast/shadow-dom/hover-over-slotted-content-expected.html: Added.
+        * fast/shadow-dom/hover-over-slotted-content.html: Added.
+        * platform/ios-simulator/TestExpectations: Skip the newly added tests since iOS doesn't
+        support :hover or :activate via mouse down.
+
 2016-11-11  Brent Fulgham  <bfulgham@apple.com>
 
         Neutered ArrayBuffers are not properly serialized
diff --git a/LayoutTests/fast/shadow-dom/activate-over-slotted-content-expected.html b/LayoutTests/fast/shadow-dom/activate-over-slotted-content-expected.html
new file mode 100644 (file)
index 0000000..e704d24
--- /dev/null
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<body>
+    <p>Test passes if you see a single 100px by 100px green box below.</p>
+    <div style="width: 100px; height: 100px; background: green;"></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/shadow-dom/activate-over-slotted-content.html b/LayoutTests/fast/shadow-dom/activate-over-slotted-content.html
new file mode 100644 (file)
index 0000000..12a52d8
--- /dev/null
@@ -0,0 +1,45 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+my-host {
+    display: block;
+    width: 100px;
+    height: 100px;
+    background: purple;
+}
+</style>
+</head>
+<body>
+<p>Test passes if you see a single 100px by 100px green box below.</p>
+<my-host>Mouse down over this text</my-host>
+
+<script>
+const host = document.querySelector('my-host');
+host.attachShadow({mode: 'closed'}).innerHTML = `
+<a><span><slot></slot></span></a>
+<style>
+a {
+    display: block;
+    width: 80px;
+    height: 80px;
+    padding: 10px;
+    background: red;
+}
+a:active {
+    background: green;
+    color: green;
+}
+span {
+    background: green;
+}
+</style>`;
+
+if (window.eventSender) {
+    eventSender.mouseMoveTo(host.offsetLeft + 15, host.offsetTop + 15);
+    eventSender.mouseDown();
+}
+
+</script>
+</body>
+</html>
diff --git a/LayoutTests/fast/shadow-dom/hover-over-slotted-content-expected.html b/LayoutTests/fast/shadow-dom/hover-over-slotted-content-expected.html
new file mode 100644 (file)
index 0000000..e704d24
--- /dev/null
@@ -0,0 +1,7 @@
+<!DOCTYPE html>
+<html>
+<body>
+    <p>Test passes if you see a single 100px by 100px green box below.</p>
+    <div style="width: 100px; height: 100px; background: green;"></div>
+</body>
+</html>
diff --git a/LayoutTests/fast/shadow-dom/hover-over-slotted-content.html b/LayoutTests/fast/shadow-dom/hover-over-slotted-content.html
new file mode 100644 (file)
index 0000000..23da2cf
--- /dev/null
@@ -0,0 +1,42 @@
+<!DOCTYPE html>
+<html>
+<head>
+<style>
+my-host {
+    display: block;
+    width: 100px;
+    height: 100px;
+    background: purple;
+}
+</style>
+</head>
+<body>
+<p>Test passes if you see a single 100px by 100px green box below.</p>
+<my-host>Hover over this text</my-host>
+
+<script>
+const host = document.querySelector('my-host');
+host.attachShadow({mode: 'closed'}).innerHTML = `
+<div id="container"><span><slot></slot></span></div>
+<style>
+#container {
+    width: 80px;
+    height: 80px;
+    padding: 10px;
+    background: red;
+}
+#container:hover {
+    background: green;
+    color: green;
+}
+span {
+    background: green;
+}
+</style>`;
+
+if (window.eventSender)
+    eventSender.mouseMoveTo(host.offsetLeft + 15, host.offsetTop + 15);
+
+</script>
+</body>
+</html>
index c3adedb..b86dfe9 100644 (file)
@@ -341,6 +341,8 @@ fast/text/all-small-caps-whitespace.html [ Skip ]
 webkit.org/b/155233 fast/events/max-tabindex-focus.html [ Skip ]
 fast/shadow-dom/shadow-host-removal-crash.html [ Skip ]
 fast/shadow-dom/input-element-in-shadow.html [ Skip ]
+fast/shadow-dom/activate-over-slotted-content.html [ Skip ]
+fast/shadow-dom/hover-over-slotted-content.html [ Skip ]
 fast/events/keyboardevent-key.html [ Skip ]
 fast/events/keyboardevent-code.html [ Skip ]
 
index 421da65..42c65bf 100644 (file)
@@ -1,3 +1,39 @@
+2016-11-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Hovering over a slotted Text node clears hover state
+        https://bugs.webkit.org/show_bug.cgi?id=164002
+        <rdar://problem/29040471>
+
+        Reviewed by Simon Fraser.
+
+        The bug was caused by HitTestResult::innerElement returning the parent element of a Text node without
+        taking the shadow root or slots into account. For hit testing, we always want to use the "flat tree"
+        or "composed tree" (imprecisely but close enough in this case).
+
+        Fixed the bug by making HitTestResult::innerElement use parentNodeInComposedTree. Also renamed it to
+        HitTestResult::targetElement to be consistent with HitTestResult::targetNode.
+
+        Tests: fast/shadow-dom/activate-over-slotted-content.html
+               fast/shadow-dom/hover-over-slotted-content.html
+
+        * dom/Document.cpp:
+        (WebCore::Document::prepareMouseEvent):
+        * html/MediaElementSession.cpp:
+        (WebCore::isMainContentForPurposesOfAutoplay):
+        * page/EventHandler.cpp:
+        (WebCore::EventHandler::eventMayStartDrag):
+        (WebCore::EventHandler::hitTestResultAtPoint):
+        (WebCore::EventHandler::handleWheelEvent):
+        (WebCore::EventHandler::sendContextMenuEventForKey):
+        (WebCore::EventHandler::hoverTimerFired):
+        (WebCore::EventHandler::handleDrag):
+        (WebCore::EventHandler::handleTouchEvent):
+        * rendering/HitTestResult.cpp:
+        (WebCore::HitTestResult::targetElement): Renamed from innerElement.
+        Now finds the parent element in the composed tree.
+        * rendering/HitTestResult.h:
+        (WebCore::HitTestResult::innerNode):
+
 2016-11-11  Brent Fulgham  <bfulgham@apple.com>
 
         Unreviewed build fix after r208628
index d714172..151fb67 100644 (file)
@@ -3277,7 +3277,7 @@ MouseEventWithHitTestResults Document::prepareMouseEvent(const HitTestRequest& r
     renderView()->hitTest(request, result);
 
     if (!request.readOnly())
-        updateHoverActiveState(request, result.innerElement());
+        updateHoverActiveState(request, result.targetElement());
 
     return MouseEventWithHitTestResults(event, result);
 }
index 9671b68..5313190 100644 (file)
@@ -676,7 +676,7 @@ static bool isMainContentForPurposesOfAutoplay(const HTMLMediaElement& element)
     // Elements which are obscured by other elements cannot be main content.
     mainRenderView.hitTest(request, result);
     result.setToNonUserAgentShadowAncestor();
-    Element* hitElement = result.innerElement();
+    Element* hitElement = result.targetElement();
     if (hitElement != &element)
         return false;
 
index f5c002a..5ae8e04 100644 (file)
@@ -892,7 +892,8 @@ bool EventHandler::eventMayStartDrag(const PlatformMouseEvent& event) const
     HitTestResult result(view->windowToContents(event.position()));
     renderView->hitTest(request, result);
     DragState state;
-    return result.innerElement() && page->dragController().draggableElement(&m_frame, result.innerElement(), result.roundedPointInInnerNodeFrame(), state);
+    Element* targetElement = result.targetElement();
+    return targetElement && page->dragController().draggableElement(&m_frame, targetElement, result.roundedPointInInnerNodeFrame(), state);
 }
 
 void EventHandler::updateSelectionForMouseDrag()
@@ -1159,7 +1160,7 @@ HitTestResult EventHandler::hitTestResultAtPoint(const LayoutPoint& point, HitTe
     HitTestRequest request(hitType | HitTestRequest::AllowChildFrameContent);
     renderView->hitTest(request, result);
     if (!request.readOnly())
-        m_frame.document()->updateHoverActiveState(request, result.innerElement());
+        m_frame.document()->updateHoverActiveState(request, result.targetElement());
 
     if (request.disallowsUserAgentShadowContent())
         result.setToNonUserAgentShadowAncestor();
@@ -2689,7 +2690,7 @@ bool EventHandler::handleWheelEvent(const PlatformWheelEvent& event)
     HitTestResult result(view->windowToContents(event.position()));
     renderView->hitTest(request, result);
 
-    RefPtr<Element> element = result.innerElement();
+    RefPtr<Element> element = result.targetElement();
     RefPtr<ContainerNode> scrollableContainer;
     WeakPtr<ScrollableArea> scrollableArea;
     bool isOverWidget = result.isOverWidget();
@@ -2875,7 +2876,7 @@ bool EventHandler::sendContextMenuEventForKey()
     // Use the focused node as the target for hover and active.
     HitTestResult result(position);
     result.setInnerNode(targetNode);
-    doc->updateHoverActiveState(HitTestRequest::Active | HitTestRequest::DisallowUserAgentShadowContent, result.innerElement());
+    doc->updateHoverActiveState(HitTestRequest::Active | HitTestRequest::DisallowUserAgentShadowContent, result.targetElement());
 
     // The contextmenu event is a mouse event even when invoked using the keyboard.
     // This is required for web compatibility.
@@ -2997,7 +2998,7 @@ void EventHandler::hoverTimerFired()
             HitTestRequest request(HitTestRequest::Move | HitTestRequest::DisallowUserAgentShadowContent);
             HitTestResult result(view->windowToContents(m_lastKnownMousePosition));
             renderView->hitTest(request, result);
-            m_frame.document()->updateHoverActiveState(request, result.innerElement());
+            m_frame.document()->updateHoverActiveState(request, result.targetElement());
         }
     }
 }
@@ -3442,7 +3443,7 @@ bool EventHandler::handleDrag(const MouseEventWithHitTestResults& event, CheckDr
         HitTestResult result(m_mouseDownPos);
         m_frame.contentRenderer()->hitTest(request, result);
         if (m_frame.page())
-            dragState().source = m_frame.page()->dragController().draggableElement(&m_frame, result.innerElement(), m_mouseDownPos, dragState());
+            dragState().source = m_frame.page()->dragController().draggableElement(&m_frame, result.targetElement(), m_mouseDownPos, dragState());
         
         if (!dragState().source)
             m_mouseDownMayStartDrag = false; // no element is draggable
@@ -3915,14 +3916,13 @@ bool EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
             } else
                 continue;
 
-            // FIXME: This code should use Element* instead of Node*.
-            Node* node = result.innerElement();
-            ASSERT(node);
+            Element* element = result.targetElement();
+            ASSERT(element);
 
-            if (node && InspectorInstrumentation::handleTouchEvent(m_frame, *node))
+            if (element && InspectorInstrumentation::handleTouchEvent(m_frame, *element))
                 return true;
 
-            Document& doc = node->document();
+            Document& doc = element->document();
             // Record the originating touch document even if it does not have a touch listener.
             if (freshTouchEvents) {
                 m_originatingTouchPointDocument = &doc;
@@ -3930,8 +3930,8 @@ bool EventHandler::handleTouchEvent(const PlatformTouchEvent& event)
             }
             if (!doc.hasTouchEventHandlers())
                 continue;
-            m_originatingTouchPointTargets.set(touchPointTargetKey, node);
-            touchTarget = node;
+            m_originatingTouchPointTargets.set(touchPointTargetKey, element);
+            touchTarget = element;
         } else if (pointState == PlatformTouchPoint::TouchReleased || pointState == PlatformTouchPoint::TouchCancelled) {
             // No need to perform a hit-test since we only need to unset :hover and :active states.
             if (!shouldGesturesTriggerActive() && allTouchReleased)
index bbc68ab..5ef6381 100644 (file)
@@ -764,14 +764,13 @@ Node* HitTestResult::targetNode() const
     return node;
 }
 
-Element* HitTestResult::innerElement() const
+Element* HitTestResult::targetElement() const
 {
-    Node* node = m_innerNode.get();
-    if (!node)
-        return nullptr;
-    if (is<Element>(*node))
-        return downcast<Element>(node);
-    return node->parentElement();
+    for (Node* node = m_innerNode.get(); node; node = node->parentInComposedTree()) {
+        if (is<Element>(*node))
+            return downcast<Element>(node);
+    }
+    return nullptr;
 }
 
 Element* HitTestResult::innerNonSharedElement() const
index b0376f5..4a9bee1 100644 (file)
@@ -59,7 +59,6 @@ public:
     WEBCORE_EXPORT HitTestResult& operator=(const HitTestResult&);
 
     Node* innerNode() const { return m_innerNode.get(); }
-    WEBCORE_EXPORT Element* innerElement() const;
     Node* innerNonSharedNode() const { return m_innerNonSharedNode.get(); }
     WEBCORE_EXPORT Element* innerNonSharedElement() const;
     Element* URLElement() const { return m_innerURLElement.get(); }
@@ -151,6 +150,7 @@ public:
     Vector<String> dictationAlternatives() const;
 
     Node* targetNode() const;
+    WEBCORE_EXPORT Element* targetElement() const;
 
 private:
     NodeSet& mutableRectBasedTestResult(); // See above.
index eebde42..6885eb7 100644 (file)
@@ -1,3 +1,14 @@
+2016-11-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Hovering over a slotted Text node clears hover state
+        https://bugs.webkit.org/show_bug.cgi?id=164002
+        <rdar://problem/29040471>
+
+        Reviewed by Simon Fraser.
+
+        * WebView/WebImmediateActionController.mm:
+        (-[WebImmediateActionController performHitTestAtPoint:]):
+
 2016-11-11  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [WK2] autocorrect and autocapitalize attributes do not work in contenteditable elements
index ad98356..1f4a50d 100644 (file)
@@ -150,7 +150,7 @@ using namespace WebCore;
     _hitTestResult = coreFrame->eventHandler().hitTestResultAtPoint(IntPoint(viewPoint));
     coreFrame->eventHandler().setImmediateActionStage(ImmediateActionStage::PerformedHitTest);
 
-    if (Element* element = _hitTestResult.innerElement())
+    if (Element* element = _hitTestResult.targetElement())
         _contentPreventsDefault = element->dispatchMouseForceWillBegin();
 }
 
index 395e0e3..4ebbe0a 100644 (file)
@@ -1,3 +1,16 @@
+2016-11-11  Ryosuke Niwa  <rniwa@webkit.org>
+
+        Hovering over a slotted Text node clears hover state
+        https://bugs.webkit.org/show_bug.cgi?id=164002
+        <rdar://problem/29040471>
+
+        Reviewed by Simon Fraser.
+
+        * WebProcess/WebPage/WebPage.cpp:
+        (WebKit::WebPage::determinePrimarySnapshottedPlugIn):
+        * WebProcess/WebPage/mac/WebPageMac.mm:
+        (WebKit::WebPage::performImmediateActionHitTestAtLocation):
+
 2016-11-11  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [WK2] autocorrect and autocapitalize attributes do not work in contenteditable elements
index e4bd911..6f03a28 100644 (file)
@@ -5281,7 +5281,7 @@ void WebPage::determinePrimarySnapshottedPlugIn()
             HitTestResult hitTestResult(plugInRectRelativeToTopDocument.center());
             mainRenderView.hitTest(request, hitTestResult);
 
-            Element* element = hitTestResult.innerElement();
+            Element* element = hitTestResult.targetElement();
             if (!element)
                 continue;
 
index ca87e63..a2a0bf4 100644 (file)
@@ -1001,7 +1001,7 @@ void WebPage::performImmediateActionHitTestAtLocation(WebCore::FloatPoint locati
     HitTestResult hitTestResult = mainFrame.eventHandler().hitTestResultAtPoint(locationInContentCoordinates);
 
     bool immediateActionHitTestPreventsDefault = false;
-    Element* element = hitTestResult.innerElement();
+    Element* element = hitTestResult.targetElement();
 
     mainFrame.eventHandler().setImmediateActionStage(ImmediateActionStage::PerformedHitTest);
     if (element)