Bug 198036 – The "mouseenter" and "pointerenter" events are fired from the bottom up

• Status: RESOLVED
• Resolution: FIXED
• Component: UI Events

[Antoine Quint]

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.

[Antoine Quint]

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">

[Radar WebKit Bug Importer]

<rdar://problem/50940350>

[Antoine Quint]

Raise an issue on the Pointer Events spec about this at https://github.com/w3c/pointerevents/issues/283.

[Simon Fraser (smfr)]

Does this cause any known compatibility issues?

[Antoine Quint]

No.

[Antoine Quint]

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.

[Antoine Quint]

Created attachment 370726 [details]
Patch

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Antoine Quint]

Created attachment 370735 [details]
Patch

[Antoine Quint]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Darin Adler]

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.

[Ryosuke Niwa]

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.

[Antoine Quint]

Created attachment 371275 [details]
Patch for landing

[Antoine Quint]

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

[WebKit Commit Bot]

Comment on attachment 371275 [details]
Patch for landing

Clearing flags on attachment: 371275

Committed r246061: <https://trac.webkit.org/changeset/246061>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Antoine Quint]

Raised https://github.com/w3c/pointerevents/issues/285.