Bug 198999

Summary: [Pointer Events] Respect pointer capture when dispatching mouse boundary events and updating :hover
Product: WebKit Reporter: Antoine Quint <graouts>
Component: UI EventsAssignee: 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 Flags
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Patch
none
Archive of layout-test-results from ews104 for mac-highsierra-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
Patch dino: review+

Description Antoine Quint 2019-06-19 07:14:15 PDT
[Pointer Events] Respect pointer capture when dispatching mouse boundary events and updating :hover
Comment 1 Antoine Quint 2019-06-19 07:31:45 PDT
Created attachment 372461 [details]
Patch
Comment 2 Antoine Quint 2019-06-19 08:19:29 PDT
Created attachment 372463 [details]
Patch
Comment 3 EWS Watchlist 2019-06-19 09:39:29 PDT
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
Comment 4 EWS Watchlist 2019-06-19 09:39:30 PDT
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
Comment 5 Antoine Quint 2019-06-19 10:22:18 PDT
Created attachment 372472 [details]
Patch
Comment 6 EWS Watchlist 2019-06-19 11:18:40 PDT
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
Comment 7 EWS Watchlist 2019-06-19 11:18:42 PDT
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
Comment 8 Antoine Quint 2019-06-20 07:08:27 PDT
Created attachment 372551 [details]
Patch
Comment 9 Dean Jackson 2019-06-20 11:39:15 PDT
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 10 Ryosuke Niwa 2019-06-20 15:10:30 PDT
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.
Comment 11 Antoine Quint 2019-06-21 01:45:10 PDT
(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.
Comment 12 Antoine Quint 2019-06-21 01:47:35 PDT
Committed r246674: <https://trac.webkit.org/changeset/246674>
Comment 13 Radar WebKit Bug Importer 2019-06-21 01:49:16 PDT
<rdar://problem/51979477>
Comment 14 Antoine Quint 2019-06-22 13:07:39 PDT
Reopening to attach new patch.
Comment 15 Antoine Quint 2019-06-22 13:07:41 PDT
Created attachment 372685 [details]
Patch
Comment 16 Antoine Quint 2019-06-22 13:09:58 PDT
Committed r246716: <https://trac.webkit.org/changeset/246716>
Comment 17 Antoine Quint 2019-06-23 07:32:54 PDT
Reopening to attach new patch.
Comment 18 Antoine Quint 2019-06-23 07:32:56 PDT
Created attachment 372695 [details]
Patch
Comment 19 Antoine Quint 2019-06-24 03:01:08 PDT
Committed r246728: <https://trac.webkit.org/changeset/246728>
Comment 20 Antoine Quint 2019-06-24 03:45:42 PDT
Committed r246729: <https://trac.webkit.org/changeset/246729>
Comment 21 Antoine Quint 2019-06-24 03:47:10 PDT
Reopening, this caused a regression in imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_pointercapture_in_frame.html, so this wasn't fully baked.
Comment 22 Antoine Quint 2019-06-26 06:02:40 PDT
Created attachment 372919 [details]
Patch
Comment 23 EWS Watchlist 2019-06-26 07:28:52 PDT
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
Comment 24 EWS Watchlist 2019-06-26 07:28:54 PDT
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 25 EWS Watchlist 2019-06-26 08:16:03 PDT
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
Comment 26 EWS Watchlist 2019-06-26 08:16:05 PDT
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
Comment 27 Antoine Quint 2019-06-26 09:43:46 PDT
Created attachment 372931 [details]
Patch
Comment 28 Antoine Quint 2019-06-26 11:36:53 PDT
Committed r246844: <https://trac.webkit.org/changeset/246844>
Comment 29 Antoine Quint 2019-06-26 15:08:45 PDT
Reopening to attach new patch.
Comment 30 Antoine Quint 2019-06-26 15:08:47 PDT
Created attachment 372955 [details]
Patch
Comment 31 Antoine Quint 2019-06-26 15:11:40 PDT
Committed r246849: <https://trac.webkit.org/changeset/246849>
Comment 32 Truitt Savell 2019-06-26 16:48:49 PDT
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.
Comment 33 Antoine Quint 2019-06-26 21:43:58 PDT
I think the iOS failures were addressed in https://trac.webkit.org/r246849. Going to check again.
Comment 34 Antoine Quint 2019-06-26 23:19:26 PDT
Yes, all clear on the iOS front.
Comment 35 Truitt Savell 2019-06-28 07:55:42 PDT
(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?
Comment 36 Truitt Savell 2019-07-01 14:23:49 PDT
Reverted r246849 for reason:

12 tests broken in r246844 require this to be rolled out.

Committed r247023: <https://trac.webkit.org/changeset/247023>
Comment 37 Truitt Savell 2019-07-01 14:26:01 PDT
Reverted r246844 for reason:

Broke 12 tests in imported/w3c/web-platform-tests/pointerevents/

Committed r247024: <https://trac.webkit.org/changeset/247024>
Comment 38 Antoine Quint 2019-07-05 03:25:42 PDT
Committed r247148: <https://trac.webkit.org/changeset/247148>
Comment 39 Antoine Quint 2019-07-05 03:26:41 PDT
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.