Bug 198036

Summary: The "mouseenter" and "pointerenter" events are fired from the bottom up
Product: WebKit Reporter: Antoine Quint <graouts>
Component: UI EventsAssignee: Antoine Quint <graouts>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, darin, dino, ews-watchlist, rniwa, simon.fraser, 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=195098
Attachments:
Description Flags
test
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews215 for win-future
none
Archive of layout-test-results from ews115 for mac-highsierra
none
Patch
none
Archive of layout-test-results from ews102 for mac-highsierra
none
Archive of layout-test-results from ews114 for mac-highsierra
none
Patch for landing none

Description Antoine Quint 2019-05-20 02:32:47 PDT
The "mouseenter" and "pointerenter" are fired from the bottom up, from the event target to the root element, while they are fired in the opposite order in Firefox and Chrome. See the attached test case. I could not find any wording related to this in the DOM UI Events spec.
Comment 1 Antoine Quint 2019-05-20 02:43:13 PDT
Created attachment 370248 [details]
test

Attached a test showing the discrepancy between STP and Chrome / Firefox. In STP 82, we get:

    Received pointerenter on <div id="bottom">
    Received mouseenter on <div id="bottom">
    Received pointerenter on <div id="middle">
    Received mouseenter on <div id="middle">
    Received pointerenter on <div id="top">
    Received mouseenter on <div id="top">

In Chrome and Firefox, we get:

    Received pointerenter on <div id="top">
    Received pointerenter on <div id="middle">
    Received pointerenter on <div id="bottom">
    Received mouseenter on <div id="top">
    Received mouseenter on <div id="middle">
    Received mouseenter on <div id="bottom">
Comment 2 Radar WebKit Bug Importer 2019-05-20 02:43:42 PDT
<rdar://problem/50940350>
Comment 3 Antoine Quint 2019-05-22 13:18:40 PDT
Raise an issue on the Pointer Events spec about this at https://github.com/w3c/pointerevents/issues/283.
Comment 4 Simon Fraser (smfr) 2019-05-22 20:39:27 PDT
Does this cause any known compatibility issues?
Comment 5 Antoine Quint 2019-05-23 00:20:20 PDT
No.
Comment 6 Antoine Quint 2019-05-27 08:27:22 PDT
For mouse events this is defined by https://w3c.github.io/uievents/#events-mouseevent-event-order and it would make sense for Pointer Events to follow this as well.
Comment 7 Antoine Quint 2019-05-28 04:54:58 PDT
Created attachment 370726 [details]
Patch
Comment 8 EWS Watchlist 2019-05-28 05:59:01 PDT
Comment on attachment 370726 [details]
Patch

Attachment 370726 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12307447

New failing tests:
pointerevents/mouse/enter-leave-order.html
fast/shadow-dom/mouseenter-mouseleave-on-slot-parent.html
fast/shadow-dom/mouseenter-mouseleave-across-shadow-boundary.html
fast/shadow-dom/mouseenter-mouseleave-inside-shadow-tree.html
fast/events/shadow-event-path.html
Comment 9 EWS Watchlist 2019-05-28 05:59:02 PDT
Created attachment 370728 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 10 EWS Watchlist 2019-05-28 06:13:25 PDT
Comment on attachment 370726 [details]
Patch

Attachment 370726 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12307529

New failing tests:
fast/shadow-dom/mouseenter-mouseleave-across-shadow-boundary.html
fast/shadow-dom/mouseenter-mouseleave-inside-shadow-tree.html
fast/shadow-dom/mouseenter-mouseleave-on-slot-parent.html
Comment 11 EWS Watchlist 2019-05-28 06:13:27 PDT
Created attachment 370730 [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 12 EWS Watchlist 2019-05-28 06:21:45 PDT
Comment on attachment 370726 [details]
Patch

Attachment 370726 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/12307545

New failing tests:
fast/shadow-dom/mouseenter-mouseleave-across-shadow-boundary.html
fast/shadow-dom/mouseenter-mouseleave-inside-shadow-tree.html
fast/shadow-dom/mouseenter-mouseleave-on-slot-parent.html
Comment 13 EWS Watchlist 2019-05-28 06:21:48 PDT
Created attachment 370731 [details]
Archive of layout-test-results from ews215 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews215  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit
Comment 14 EWS Watchlist 2019-05-28 06:42:58 PDT
Comment on attachment 370726 [details]
Patch

Attachment 370726 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12307552

New failing tests:
pointerevents/mouse/enter-leave-order.html
fast/events/shadow-event-path.html
fast/shadow-dom/mouseenter-mouseleave-across-shadow-boundary.html
fast/shadow-dom/mouseenter-mouseleave-inside-shadow-tree.html
fast/shadow-dom/mouseenter-mouseleave-on-slot-parent.html
Comment 15 EWS Watchlist 2019-05-28 06:43:00 PDT
Created attachment 370733 [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 16 Antoine Quint 2019-05-28 07:04:43 PDT
Created attachment 370735 [details]
Patch
Comment 17 Antoine Quint 2019-05-28 07:06:12 PDT
I really don't understand the WK1 failure for the new test. It seems there are no mouse events received at all for it and I cannot figure out why that is.
Comment 18 EWS Watchlist 2019-05-28 08:09:07 PDT
Comment on attachment 370735 [details]
Patch

Attachment 370735 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/12308181

New failing tests:
pointerevents/mouse/enter-leave-order.html
Comment 19 EWS Watchlist 2019-05-28 08:09:09 PDT
Created attachment 370740 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 20 EWS Watchlist 2019-05-28 08:57:16 PDT
Comment on attachment 370735 [details]
Patch

Attachment 370735 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/12308251

New failing tests:
pointerevents/mouse/enter-leave-order.html
Comment 21 EWS Watchlist 2019-05-28 08:57:18 PDT
Created attachment 370747 [details]
Archive of layout-test-results from ews114 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114  Port: mac-highsierra  Platform: Mac OS X 10.13.6
Comment 22 Darin Adler 2019-05-28 09:00:37 PDT
Comment on attachment 370735 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=370735&action=review

Looks like the test isn’t passing on EWS yet.

> Source/WebCore/page/PointerCaptureController.cpp:194
> +            Vector<Ref<Element>, 32> enterChain;

Putting the elements into a vector before dispatching events has another effect besides reversing the order. It also means that DOM mutation inside the event handler has no effect on which elements receive the dispatched events. So the change is observable.

Therefore, I think we should consider using a vector in both cases. This would also make the code less repetitive.

We should also consider making test cases that demonstrate how mutating the DOM to change the parent chain affects or does not affect which elements get an event dispatched. This is the kind of thing that can lead to incompatibilities between web browsers when their implementations differ. It’s probably not all that unusual for an event handler to end up removing the node that received the event from its parent, for example.

> Source/WebCore/page/PointerCaptureController.cpp:195
> +            for (Element* element = &downcast<Element>(target); element; element = element->parentElementInComposedTree()) {

Not new to this patch, but why not use targetElement instead of writing &downcast<Element>(target) again?

> Source/WebCore/page/PointerCaptureController.cpp:203
> +            for (Element* element = &downcast<Element>(target); element; element = element->parentElementInComposedTree()) {

Seems like this loop needs to use RefPtr<Element>. Otherwise what guarantees that the code we dispatch to won’t deallocate the element? Same idea about targetElement as above.
Comment 23 Ryosuke Niwa 2019-05-28 14:07:13 PDT
Note that dispatching an event (https://dom.spec.whatwg.org/#dispatching-events) already "caches" a list of event targets to fire events on so creating a Vector here is likely more correct.
Comment 24 Antoine Quint 2019-06-04 06:52:09 PDT
Created attachment 371275 [details]
Patch for landing
Comment 25 Antoine Quint 2019-06-04 06:55:08 PDT
(In reply to Darin Adler from comment #22)
> Comment on attachment 370735 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=370735&action=review
> 
> Looks like the test isn’t passing on EWS yet.

There's an issue on WK1 that I haven't investigated (yet) and that affects other tests, see webkit.org/b/195098.

> > Source/WebCore/page/PointerCaptureController.cpp:194
> > +            Vector<Ref<Element>, 32> enterChain;
> 
> Putting the elements into a vector before dispatching events has another
> effect besides reversing the order. It also means that DOM mutation inside
> the event handler has no effect on which elements receive the dispatched
> events. So the change is observable.
> 
> Therefore, I think we should consider using a vector in both cases. This
> would also make the code less repetitive.

I've made that change in the patch for landing. Thanks!

> We should also consider making test cases that demonstrate how mutating the
> DOM to change the parent chain affects or does not affect which elements get
> an event dispatched. This is the kind of thing that can lead to
> incompatibilities between web browsers when their implementations differ.
> It’s probably not all that unusual for an event handler to end up removing
> the node that received the event from its parent, for example.

I've written a test about this and noticed discrepancies between implementations. I'm working on filing relevant GitHub issues on Pointer Events and UI Events and will update this bug with references once it's done.

> > Source/WebCore/page/PointerCaptureController.cpp:195
> > +            for (Element* element = &downcast<Element>(target); element; element = element->parentElementInComposedTree()) {
> 
> Not new to this patch, but why not use targetElement instead of writing
> &downcast<Element>(target) again?

No good reason, this was an oversight and is fixed in the patch for landing.

> > Source/WebCore/page/PointerCaptureController.cpp:203
> > +            for (Element* element = &downcast<Element>(target); element; element = element->parentElementInComposedTree()) {
> 
> Seems like this loop needs to use RefPtr<Element>. Otherwise what guarantees
> that the code we dispatch to won’t deallocate the element? Same idea about
> targetElement as above.

I got rid of this loop altogether and now dispatch the event based on the Vector<Ref<Element>> defined above.
Comment 26 WebKit Commit Bot 2019-06-04 07:34:17 PDT
Comment on attachment 371275 [details]
Patch for landing

Clearing flags on attachment: 371275

Committed r246061: <https://trac.webkit.org/changeset/246061>
Comment 27 WebKit Commit Bot 2019-06-04 07:34:19 PDT
All reviewed patches have been landed.  Closing bug.
Comment 28 Antoine Quint 2019-06-04 07:36:31 PDT
Raised https://github.com/w3c/pointerevents/issues/285.