Summary: | [Pointer Events] Respect pointer capture when dispatching mouse boundary events and updating :hover | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||||||||||||||||||||||||||
Component: | UI Events | Assignee: | Antoine Quint <graouts> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | cdumez, dbates, dino, esprehn+autocc, ews-watchlist, kangil.han, rniwa, tsavell, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=197058 | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
Antoine Quint
2019-06-19 07:14:15 PDT
Created attachment 372461 [details]
Patch
Created attachment 372463 [details]
Patch
Comment on attachment 372463 [details] Patch Attachment 372463 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12521208 New failing tests: imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html pointerevents/mouse/pointer-capture.html Created attachment 372468 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 372472 [details]
Patch
Comment on attachment 372472 [details] Patch Attachment 372472 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12522352 New failing tests: pointerevents/mouse/pointer-capture.html Created attachment 372481 [details]
Archive of layout-test-results from ews104 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Created attachment 372551 [details]
Patch
Comment on attachment 372551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372551&action=review > Source/WebCore/page/EventHandler.h:417 > + bool dispatchMouseEvent(const AtomString& eventType, Node* target, bool cancelable, int clickCount, const PlatformMouseEvent&, bool setUnder, const HitTestRequest&, MouseEventWithHitTestResults&); I think you should give this a slightly different name to make it more clear. Or maybe call the other one base? Or use default parameter values? Comment on attachment 372551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372551&action=review > Source/WebCore/dom/Document.cpp:3740 > + Element* targetElement = result.targetElement(); Please use makeRefPtr. > Source/WebCore/page/EventHandler.cpp:2045 > if (swallowEvent) This function also calls updateMouseEventTargetNode for onlyUpdateScrollbars is true or when newSubframe is set up above. Don't we also need to prevent it from getting called when pointer capturing is going on? > Source/WebCore/page/EventHandler.cpp:2166 > + // If we have pointer capture enabled, it will be disabled by virtue of receiving a "pointerup" event. For :active and :hover > + // styles to be set correctly when prepareMouseEvent() is called below, we should already reset pointer capture. > + m_frame.page()->pointerCaptureController().releasePointerCapture(m_capturingMouseEventsElement.get(), mousePointerID); > + // We must also reset m_capturingMouseEventsElement to ensure boundary mouse events are dispatched on the hit-testing target. > + m_capturingMouseEventsElement = nullptr; > + // The click event target may differ from the hit-testing target, so let's not dispatch boundary mouse events as part of dispatching > + // the click event below. Can we consolidate comments into a single block? This code interleaving with comments is hard to read. > Source/WebCore/page/EventHandler.cpp:2665 > bool EventHandler::dispatchMouseEvent(const AtomString& eventType, Node* targetNode, bool /*cancelable*/, int clickCount, const PlatformMouseEvent& platformMouseEvent, bool setUnder) This function also always calls updateMouseEventTargetNode. It seems that we either need to add a condition to avoid calling updateMouseEventTargetNode when there is pointer capturing going on or assert that there is no point capturing happening. (In reply to Ryosuke Niwa from comment #10) > Comment on attachment 372551 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=372551&action=review > > > Source/WebCore/dom/Document.cpp:3740 > > + Element* targetElement = result.targetElement(); > > Please use makeRefPtr. Will fix in commit. > > Source/WebCore/page/EventHandler.cpp:2045 > > if (swallowEvent) > > This function also calls updateMouseEventTargetNode for onlyUpdateScrollbars > is true or when newSubframe is set up above. > Don't we also need to prevent it from getting called when pointer capturing > is going on? I don't think it's necessary since I believe calls to updateMouseEventTargetNode() will do the right thing based on m_capturingMouseEventsElement being set to the capturing pointer element. > > Source/WebCore/page/EventHandler.cpp:2166 > > + // If we have pointer capture enabled, it will be disabled by virtue of receiving a "pointerup" event. For :active and :hover > > + // styles to be set correctly when prepareMouseEvent() is called below, we should already reset pointer capture. > > + m_frame.page()->pointerCaptureController().releasePointerCapture(m_capturingMouseEventsElement.get(), mousePointerID); > > + // We must also reset m_capturingMouseEventsElement to ensure boundary mouse events are dispatched on the hit-testing target. > > + m_capturingMouseEventsElement = nullptr; > > + // The click event target may differ from the hit-testing target, so let's not dispatch boundary mouse events as part of dispatching > > + // the click event below. > > Can we consolidate comments into a single block? This code interleaving with > comments is hard to read. Sure thing, will fix in commit. > > Source/WebCore/page/EventHandler.cpp:2665 > > bool EventHandler::dispatchMouseEvent(const AtomString& eventType, Node* targetNode, bool /*cancelable*/, int clickCount, const PlatformMouseEvent& platformMouseEvent, bool setUnder) > > This function also always calls updateMouseEventTargetNode. > It seems that we either need to add a condition to avoid calling > updateMouseEventTargetNode > when there is pointer capturing going on or assert that there is no point > capturing happening. Like above, I don't think this is necessary since m_capturingMouseEventsElement will be set to the capturing mouse element. Calling this function should be harmless. Committed r246674: <https://trac.webkit.org/changeset/246674> Reopening to attach new patch. Created attachment 372685 [details]
Patch
Committed r246716: <https://trac.webkit.org/changeset/246716> Reopening to attach new patch. Created attachment 372695 [details]
Patch
Committed r246728: <https://trac.webkit.org/changeset/246728> Committed r246729: <https://trac.webkit.org/changeset/246729> Reopening, this caused a regression in imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_pointercapture_in_frame.html, so this wasn't fully baked. Created attachment 372919 [details]
Patch
Comment on attachment 372919 [details] Patch Attachment 372919 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12580777 New failing tests: media/audio-delete-while-slider-thumb-clicked.html Created attachment 372923 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 372919 [details] Patch Attachment 372919 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12580836 New failing tests: fast/forms/number/number-large-padding.html fast/forms/range/slider-delete-while-dragging-thumb.html fast/forms/input-appearance-spinbutton-up.html fast/forms/number/number-change-type-on-focus.html fast/forms/range/slider-transformed.html media/media-controls-drag-timeline-set-controls-property.html fast/forms/number/number-spinbutton-state.html media/audio-delete-while-slider-thumb-clicked.html fast/forms/input-step-as-double.html fast/forms/number/number-spinbutton-capturing.html fast/forms/range/slider-zoomed.html fast/forms/range/range-type-change-onchange-2.html fast/forms/number/number-spinbutton-click-in-iframe.html fast/forms/number/number-losing-renderer-on-click.html Created attachment 372924 [details]
Archive of layout-test-results from ews115 for mac-highsierra
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115 Port: mac-highsierra Platform: Mac OS X 10.13.6
Created attachment 372931 [details]
Patch
Committed r246844: <https://trac.webkit.org/changeset/246844> Reopening to attach new patch. Created attachment 372955 [details]
Patch
Committed r246849: <https://trac.webkit.org/changeset/246849> Looks like the changes in https://trac.webkit.org/changeset/246844/webkit broke 11 tests: imported/w3c/web-platform-tests/pointerevents/pointerevent_boundary_events_at_implicit_release_hoverable_pointers.html imported/w3c/web-platform-tests/pointerevents/pointerevent_capture_mouse.html imported/w3c/web-platform-tests/pointerevents/pointerevent_click_during_capture.html imported/w3c/web-platform-tests/pointerevents/pointerevent_lostpointercapture_for_disconnected_node.html imported/w3c/web-platform-tests/pointerevents/pointerevent_lostpointercapture_is_first.html imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_capture_change_hover.html imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_pointercapture_in_frame.html imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_pointercapture_inactivate_pointer.html imported/w3c/web-platform-tests/pointerevents/pointerevent_releasepointercapture_invalid_pointerid.html imported/w3c/web-platform-tests/pointerevents/pointerevent_releasepointercapture_onpointerup_mouse.html imported/w3c/web-platform-tests/pointerevents/pointerevent_setpointercapture_relatedtarget.html Results: https://build.webkit.org/results/Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r246845%20(9730)/results.html History: http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fpointerevents%2Fpointerevent_boundary_events_at_implicit_release_hoverable_pointers.html Looks like most of these are harness timeouts and some are instant failures. I think the iOS failures were addressed in https://trac.webkit.org/r246849. Going to check again. Yes, all clear on the iOS front. (In reply to Truitt Savell from comment #32) > Looks like the changes in https://trac.webkit.org/changeset/246844/webkit > > broke 11 tests: > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_boundary_events_at_implicit_release_hoverable_pointers.html > imported/w3c/web-platform-tests/pointerevents/pointerevent_capture_mouse.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_click_during_capture.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_lostpointercapture_for_disconnected_node.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_lostpointercapture_is_first.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_mouse_capture_change_hover.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_mouse_pointercapture_in_frame.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_mouse_pointercapture_inactivate_pointer.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_releasepointercapture_invalid_pointerid.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_releasepointercapture_onpointerup_mouse.html > imported/w3c/web-platform-tests/pointerevents/ > pointerevent_setpointercapture_relatedtarget.html > > Results: > https://build.webkit.org/results/ > Apple%20High%20Sierra%20Debug%20WK1%20(Tests)/r246845%20(9730)/results.html > > History: > http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform- > tests%2Fpointerevents%2Fpointerevent_boundary_events_at_implicit_release_hove > rable_pointers.html > > Looks like most of these are harness timeouts and some are instant failures. All of these failures are still occurring on Mac flakily and High Sierra Debug WK1 and Leaks constantly. Is this fixable today? Reverted r246849 for reason: 12 tests broken in r246844 require this to be rolled out. Committed r247023: <https://trac.webkit.org/changeset/247023> Reverted r246844 for reason: Broke 12 tests in imported/w3c/web-platform-tests/pointerevents/ Committed r247024: <https://trac.webkit.org/changeset/247024> Committed r247148: <https://trac.webkit.org/changeset/247148> There was a new uninitialised boolean member variable in PointerCaptureController.h that caused bad issues with --leaks. This is now addressed in the newer patch. |