Bug 198999 – [Pointer Events] Respect pointer capture when dispatching mouse boundary events and updating :hover

• Component: UI Events
• Status: RESOLVED FIXED

[Antoine Quint]

[Pointer Events] Respect pointer capture when dispatching mouse boundary events and updating :hover

[Antoine Quint]

Created attachment 372461 [details]
Patch

[Antoine Quint]

Created attachment 372463 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

Created attachment 372472 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

Created attachment 372551 [details]
Patch

[Dean Jackson]

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?

[Ryosuke Niwa]

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.

[Antoine Quint]

(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.

[Antoine Quint]

Committed r246674: <https://trac.webkit.org/changeset/246674>

[Radar WebKit Bug Importer]

<rdar://problem/51979477>

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 372685 [details]
Patch

[Antoine Quint]

Committed r246716: <https://trac.webkit.org/changeset/246716>

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 372695 [details]
Patch

[Antoine Quint]

Committed r246728: <https://trac.webkit.org/changeset/246728>

[Antoine Quint]

Committed r246729: <https://trac.webkit.org/changeset/246729>

[Antoine Quint]

Reopening, this caused a regression in imported/w3c/web-platform-tests/pointerevents/pointerevent_mouse_pointercapture_in_frame.html, so this wasn't fully baked.

[Antoine Quint]

Created attachment 372919 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

Created attachment 372931 [details]
Patch

[Antoine Quint]

Committed r246844: <https://trac.webkit.org/changeset/246844>

[Antoine Quint]

Reopening to attach new patch.

[Antoine Quint]

Created attachment 372955 [details]
Patch

[Antoine Quint]

Committed r246849: <https://trac.webkit.org/changeset/246849>

[Truitt Savell]

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.

[Antoine Quint]

I think the iOS failures were addressed in https://trac.webkit.org/r246849. Going to check again.

[Antoine Quint]

Yes, all clear on the iOS front.

[Truitt Savell]

(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?

[Truitt Savell]

Reverted r246849 for reason:

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

Committed r247023: <https://trac.webkit.org/changeset/247023>

[Truitt Savell]

Reverted r246844 for reason:

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

Committed r247024: <https://trac.webkit.org/changeset/247024>

[Antoine Quint]

Committed r247148: <https://trac.webkit.org/changeset/247148>

[Antoine Quint]

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.