From 4d71a995dfd39e9b5041c12f07587019220abceb Mon Sep 17 00:00:00 2001 From: "rniwa@webkit.org" Date: Sat, 12 Nov 2016 00:48:46 +0000 Subject: [PATCH] Hovering over a slotted Text node clears hover state https://bugs.webkit.org/show_bug.cgi?id=164002 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 --- LayoutTests/ChangeLog | 18 +++++++++ .../activate-over-slotted-content-expected.html | 7 ++++ .../shadow-dom/activate-over-slotted-content.html | 45 ++++++++++++++++++++++ .../hover-over-slotted-content-expected.html | 7 ++++ .../shadow-dom/hover-over-slotted-content.html | 42 ++++++++++++++++++++ .../platform/ios-simulator/TestExpectations | 2 + Source/WebCore/ChangeLog | 36 +++++++++++++++++ Source/WebCore/dom/Document.cpp | 2 +- Source/WebCore/html/MediaElementSession.cpp | 2 +- Source/WebCore/page/EventHandler.cpp | 26 ++++++------- Source/WebCore/rendering/HitTestResult.cpp | 13 +++---- Source/WebCore/rendering/HitTestResult.h | 2 +- Source/WebKit/mac/ChangeLog | 11 ++++++ .../mac/WebView/WebImmediateActionController.mm | 2 +- Source/WebKit2/ChangeLog | 13 +++++++ Source/WebKit2/WebProcess/WebPage/WebPage.cpp | 2 +- .../WebKit2/WebProcess/WebPage/mac/WebPageMac.mm | 2 +- 17 files changed, 206 insertions(+), 26 deletions(-) create mode 100644 LayoutTests/fast/shadow-dom/activate-over-slotted-content-expected.html create mode 100644 LayoutTests/fast/shadow-dom/activate-over-slotted-content.html create mode 100644 LayoutTests/fast/shadow-dom/hover-over-slotted-content-expected.html create mode 100644 LayoutTests/fast/shadow-dom/hover-over-slotted-content.html diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog index 8b4dd25..2cf4ff1 100644 --- a/LayoutTests/ChangeLog +++ b/LayoutTests/ChangeLog @@ -1,3 +1,21 @@ +2016-11-11 Ryosuke Niwa + + Hovering over a slotted Text node clears hover state + https://bugs.webkit.org/show_bug.cgi?id=164002 + + + 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 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 index 0000000..e704d24 --- /dev/null +++ b/LayoutTests/fast/shadow-dom/activate-over-slotted-content-expected.html @@ -0,0 +1,7 @@ + + + +

Test passes if you see a single 100px by 100px green box below.

+
+ + 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 index 0000000..12a52d8 --- /dev/null +++ b/LayoutTests/fast/shadow-dom/activate-over-slotted-content.html @@ -0,0 +1,45 @@ + + + + + + +

Test passes if you see a single 100px by 100px green box below.

+Mouse down over this text + + + + 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 index 0000000..e704d24 --- /dev/null +++ b/LayoutTests/fast/shadow-dom/hover-over-slotted-content-expected.html @@ -0,0 +1,7 @@ + + + +

Test passes if you see a single 100px by 100px green box below.

+
+ + 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 index 0000000..23da2cf --- /dev/null +++ b/LayoutTests/fast/shadow-dom/hover-over-slotted-content.html @@ -0,0 +1,42 @@ + + + + + + +

Test passes if you see a single 100px by 100px green box below.

+Hover over this text + + + + diff --git a/LayoutTests/platform/ios-simulator/TestExpectations b/LayoutTests/platform/ios-simulator/TestExpectations index c3adedb..b86dfe9 100644 --- a/LayoutTests/platform/ios-simulator/TestExpectations +++ b/LayoutTests/platform/ios-simulator/TestExpectations @@ -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 ] diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog index 421da65..42c65bf 100644 --- a/Source/WebCore/ChangeLog +++ b/Source/WebCore/ChangeLog @@ -1,3 +1,39 @@ +2016-11-11 Ryosuke Niwa + + Hovering over a slotted Text node clears hover state + https://bugs.webkit.org/show_bug.cgi?id=164002 + + + 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 Unreviewed build fix after r208628 diff --git a/Source/WebCore/dom/Document.cpp b/Source/WebCore/dom/Document.cpp index d714172..151fb67 100644 --- a/Source/WebCore/dom/Document.cpp +++ b/Source/WebCore/dom/Document.cpp @@ -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); } diff --git a/Source/WebCore/html/MediaElementSession.cpp b/Source/WebCore/html/MediaElementSession.cpp index 9671b68..5313190 100644 --- a/Source/WebCore/html/MediaElementSession.cpp +++ b/Source/WebCore/html/MediaElementSession.cpp @@ -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; diff --git a/Source/WebCore/page/EventHandler.cpp b/Source/WebCore/page/EventHandler.cpp index f5c002a..5ae8e04 100644 --- a/Source/WebCore/page/EventHandler.cpp +++ b/Source/WebCore/page/EventHandler.cpp @@ -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 = result.innerElement(); + RefPtr element = result.targetElement(); RefPtr scrollableContainer; WeakPtr 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) diff --git a/Source/WebCore/rendering/HitTestResult.cpp b/Source/WebCore/rendering/HitTestResult.cpp index bbc68ab..5ef6381 100644 --- a/Source/WebCore/rendering/HitTestResult.cpp +++ b/Source/WebCore/rendering/HitTestResult.cpp @@ -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(*node)) - return downcast(node); - return node->parentElement(); + for (Node* node = m_innerNode.get(); node; node = node->parentInComposedTree()) { + if (is(*node)) + return downcast(node); + } + return nullptr; } Element* HitTestResult::innerNonSharedElement() const diff --git a/Source/WebCore/rendering/HitTestResult.h b/Source/WebCore/rendering/HitTestResult.h index b0376f5..4a9bee1 100644 --- a/Source/WebCore/rendering/HitTestResult.h +++ b/Source/WebCore/rendering/HitTestResult.h @@ -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 dictationAlternatives() const; Node* targetNode() const; + WEBCORE_EXPORT Element* targetElement() const; private: NodeSet& mutableRectBasedTestResult(); // See above. diff --git a/Source/WebKit/mac/ChangeLog b/Source/WebKit/mac/ChangeLog index eebde42..6885eb7 100644 --- a/Source/WebKit/mac/ChangeLog +++ b/Source/WebKit/mac/ChangeLog @@ -1,3 +1,14 @@ +2016-11-11 Ryosuke Niwa + + Hovering over a slotted Text node clears hover state + https://bugs.webkit.org/show_bug.cgi?id=164002 + + + Reviewed by Simon Fraser. + + * WebView/WebImmediateActionController.mm: + (-[WebImmediateActionController performHitTestAtPoint:]): + 2016-11-11 Wenson Hsieh [WK2] autocorrect and autocapitalize attributes do not work in contenteditable elements diff --git a/Source/WebKit/mac/WebView/WebImmediateActionController.mm b/Source/WebKit/mac/WebView/WebImmediateActionController.mm index ad98356..1f4a50d 100644 --- a/Source/WebKit/mac/WebView/WebImmediateActionController.mm +++ b/Source/WebKit/mac/WebView/WebImmediateActionController.mm @@ -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(); } diff --git a/Source/WebKit2/ChangeLog b/Source/WebKit2/ChangeLog index 395e0e3..4ebbe0a 100644 --- a/Source/WebKit2/ChangeLog +++ b/Source/WebKit2/ChangeLog @@ -1,3 +1,16 @@ +2016-11-11 Ryosuke Niwa + + Hovering over a slotted Text node clears hover state + https://bugs.webkit.org/show_bug.cgi?id=164002 + + + Reviewed by Simon Fraser. + + * WebProcess/WebPage/WebPage.cpp: + (WebKit::WebPage::determinePrimarySnapshottedPlugIn): + * WebProcess/WebPage/mac/WebPageMac.mm: + (WebKit::WebPage::performImmediateActionHitTestAtLocation): + 2016-11-11 Wenson Hsieh [WK2] autocorrect and autocapitalize attributes do not work in contenteditable elements diff --git a/Source/WebKit2/WebProcess/WebPage/WebPage.cpp b/Source/WebKit2/WebProcess/WebPage/WebPage.cpp index e4bd911..6f03a28 100644 --- a/Source/WebKit2/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit2/WebProcess/WebPage/WebPage.cpp @@ -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; diff --git a/Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm b/Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm index ca87e63..a2a0bf4 100644 --- a/Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm +++ b/Source/WebKit2/WebProcess/WebPage/mac/WebPageMac.mm @@ -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) -- 1.8.3.1