w3c / uievents

UI Events
https://w3c.github.io/uievents/
Other
145 stars 52 forks source link

Consider to make mouseenter/leave (and pointerenter/leave) uncomposed #208

Closed smaug---- closed 6 years ago

smaug---- commented 6 years ago

Webkit has a bug where mouseenter/leave firing depends on existence of event listeners in shadow DOM. https://bugs.webkit.org/show_bug.cgi?id=188561 Blink seems to have inherited that https://bugs.chromium.org/p/chromium/issues/detail?id=874082 though the code is rather different.

ea.com relies on that broken behavior https://bugzilla.mozilla.org/show_bug.cgi?id=1478959 Gecko follows the current specifications and dispatches two mouseenter events where webkit/blink only one (because there is no event listener inside shadow DOM)

I explicitly don't want the webkit/blink behavior, since it is really weird that dispatching of some event depends on whether there are listeners for it somewhere in composed event path.

But, I wonder, should we make mouseenter/leave (and pointerenter/leave) uncomposed. Currently one can detect something of the structure of shadow DOM by listening the events - just count how many events there are.

@hayatoito @rniwa @annevk @NavidZ @RByers

NavidZ commented 6 years ago

The Blink behavior does seem to be inconsistent and wierd. But regarding the ideal behavior looking at MDN documentation:

https://developer.mozilla.org/en-US/docs/Web/API/Event/composed "Propagation only occurs if the bubbles property is also true."

So if we have that property if the bubbles of an event is false it just shouldn't get passed the shadowDOM boundaries. So the host element in the root document should always get one mouseenter which is when the pointer enters its boundaries. WDYT?

annevk commented 6 years ago

@NavidZ I think that documentation is not considering the capture phase.

smaug---- commented 6 years ago

non-bubbling events do propagate back to host, so that they can be handled as AT_TARGET.

But that is not the point of this bug, but whether we want *enter/leave events inside shadow DOM to be visible to the light DOM.

hayatoito commented 6 years ago

It sounds unfortunate that WebKit/Blink has a bug, however, making mouseenter/leave uncomposed sounds a huge breaking change to me.

But that is not the point of this bug, but whether we want *enter/leave events inside shadow DOM to be visible to the light DOM.

I think enter/leave events have a related target, so the event shouldn't be visible to light dom if both event's target and relatedtarget are inside of a shadow tree. I think an event path calculation stops* at the point where re-targeted target and relatedtarget become same.

rniwa commented 6 years ago

@smaug---- : Hm... I still don't understand what the exact compat. issues with EA sites are.

Is it that the site breaks if the shadow host gets two mouseenter / mouseleave events? Or if shadow tree and shadow host get two independent mouseenter / mouseleave events?

If the issue is that the host shouldn't get two mouseenter events, that's understandable because th whole point of mouseenter is that it should be dispatched once on an element as the pointing device enters the element.

@hayatoito : I don't think this would break anything as long as we continue to dispatch mouseenter / mouseleave on the shadow host. Because mouseenter / mouseleave don't bubble and instead be dispatched on each element the pointing device enters, each element in the flat tree is independently informed of the pointing device's move. This is in contrast with other pointing device events such as mousemove, which is dispatched on the leaf node inside which the pointing device movement is detected, and the way ancestor elements detects this is by observing the event bubbling up to them.

rniwa commented 6 years ago

Assuming my understanding of the problem is correct, I think the correct solution here is to make mouseenter and mouseleave uncomposed but have the U.A. continue to dispatch those events on the shadow host as the pointing device enters and leaves its shadow tree in the flat tree.

smaug---- commented 6 years ago

@hayatoito relatedTarget is outside the shadow DOM. When mouse moves into the shadow DOM (assuming it has just one element) there should be, per current specs, two events. One for host, one for the shadow element. relatedTarget doesn't have anything to do with this.

smaug---- commented 6 years ago

The issue with ea.com is that it has basically host.open = false; host.onmouseenter = function() { this.open = !this.open; if (this.open) { openMe(); } else { closeMe(); } } So, if it gets extra mouseenters, it first opens the menu and then immediately closes it.

smaug---- commented 6 years ago

mouseenter/leave would be of course dispatched to host when mouse moves in/out shadow DOM, since those events are dispatched to all the ancestors (until same ancestor with relatedTarget is found)

smaug---- commented 6 years ago

Somewhat related testcase http://mozilla.pettay.fi/moztests/video_mouseenter.html Chrome dispatches several mouseenters. Nightly just one. Surely mouseenters inside UA shadow DOM or native anonymous content or however video controls are implemented shouldn't propagate to web content. I was told Safari also dispatches extra mouseenters. And a version of video controls in Gecko which uses shadow DOM for implementation.

Didn't have Edge around for testing.

rniwa commented 6 years ago

@smaug---- Thanks for clarification. Yeah, I think the code like the one EA deployed is probably quite common because up until now, mouseenter had been guaranteed to only fire once as the mouse enter an element. I think we should fix this by making the event not composed as you suggested.

smaug---- commented 6 years ago

@hayatoito @travisleithead would you be ok with the change?

hayatoito commented 6 years ago

I am a little confused. Can I ask clarification?

Two questions:

  1. Suppose WebKit/Blink doesn't have a bug, and EA site is rewritten so that they don't depend on WebKit/Blink's bug. Then, we don't need any action, right? Is my understanding correct?

  2. I think we only fire only one mouseenter event, per the current spec.

A
├──/shadow-host
│   └──/shadow-root
│       ├── B
│       └── C
└── D
  1. mouse moved from D to C: => mouseenter fired on [C, shadow-root, shadow-host, A]
  2. Then, mouse moved from C to B: => mouseenter fired on [B, shadow-root] (the event is fired only inside of shadow tree)
  3. Then, mouse moved from B to C: => mouseenter fired on [C, shadow-root] (the event is fired only inside of shadow tree)

A receives only one mouseenter, doesn't it?

smaug---- commented 6 years ago

Remember, there are several mouseenter events per single mousemove if the subtree height > 1.

In (1), there is one event on C, separate event on shadow-host and separate event on A. (in fact, it is not defined, whether to dispatch separate event also on shadow root.)

(2) is not interesting for this issue, nor (3)

A isn't the target ea.com uses, but shadow-host. And in (1) it gets several events. The one dispatched to C, which, although is capture only, is handled also at AT_TARGET phase on shadow-host, meaning also listeners on bubble phase are called. But there is separate event dispatched to shadow-host itself.

hayatoito commented 6 years ago

Remember, there are several mouseenter events per single mousemove if the subtree height > 1.

Ah, thank you for pointing that out. Please ignore my previous questions. I wrongly thought that mouseenter had a similar behavior to mouseover, regarding event dispatching. This explained well.

My previous questions are about the behavior of mouseover. That didn't apply to mouseenter definitely.

I am now surprised that mouseenter is dispatched on all ancestors! What a ridiculous... :(

Then, it sounds reasonable to make mouseenter non-composed events. Compatibility-risk may come if there is an mousenter event listener for capturing-phase?

smaug---- commented 6 years ago

yes, but even then, do the listeners expect events both from light dom and shadow dom? I'm not too worried about the compatibility, given that ea.com (and other ea-related web sites) exists.

(and yes, enter/leave events are rather bad - or, they have nice behavior from API user point of view when using bubbling listeners only, but because of multiple events, performance can be rather bad. This is why mouseenter/leave usage in Firefox browser UI warns about bad performance.)

garykac commented 6 years ago

Re: mouseover and mouseenter, I created an event viewer to show the events being fired over nested divs to make it easier to see what's going on. https://w3c.github.io/uievents/tools/mouse-event-viewer.html

(I've considered adding shadow-dom support to the test page, but haven't gotten around to it yet.)

travisleithead commented 6 years ago

non-composed seems a much better default than composed for mouseenter/mouseleave, even though it breaks the consistency noted in the MDN article (i.e., all UA-defined events are composed). Seems like the semantics of this event require it--and it fixes the site :)

travisleithead commented 6 years ago

@garykac yes please add ShadowDOM support to that page !

hayatoito commented 6 years ago

@garykac the event viewer is great! I finally understand how mouseenter works exactly.

Let me double-check what is being proposed in this issue here.

Given the following tree (of trees):

A
├── B
│   └── C
│       └──/shadow-root-1
│           └── D
│               └── E
│                   └──/shadow-root-2
│                       └── F
│                           └── G
└── H

C and E are shadow hosts.

Case 1

mouse moves from H to G

The current spec:

Composed mouseenter events are fired on [shadow-root-2, F, G], in this order, if my understanding is correct. Note that it's a composed event, so E, for example, receives also a mouseenter at TARGET phase, for each event dispatching.

New proposal

Non-composed mouseenter events are fired on [B, C, shadow-root1, D, E, shadow-root-2, F, G], in this order. (Or any other ideas? If we fire non-composed mouseenter events only on [shadow-root-2, F, G], E doesn't receive any mouseenter event in any phase, I am assuming E should receive the event too)

Case 2

mouse moves from F to G

The current spec:

Composed mousenter event is fired on only [G]

New proposal

Non-composed mouseenter event is fired on [G]

annevk commented 6 years ago

Aside: @travisleithead it's only UA-dispatched UI events that are composed. E.g., form submission events are not.

smaug---- commented 6 years ago

Per new proposal events would fire on exactly the same targets as before. Event wouldn't just be composed. So the events inside shadow DOM wouldn't propagate to host. Because the event is dispatched to E, it would still get an event.

(It is a separate issue whether enter/leave should be dispatched on ShadowRoot explicitly. I guess yes.)

hayatoito commented 6 years ago

Thanks. I confirmed that Blink fires mouseenter events on all shadow-including descendants, using https://jsfiddle.net/eu1pn7Lx/21/.

I also found that it is difficult to tell how mouseenter event's relatedtarget is set there. :( Thus, it could happen that a shadow host can receive more than one mouseenter at TARGET phase if an event is a composed event.

Given that UA already fires mouseenter events on all shadow-including descendants, reacting mouse moves, I support to make enter/leave event non-composed. There is no reason to make it composed.

hayatoito commented 6 years ago

I've filed a bug for Blink: https://bugs.chromium.org/p/chromium/issues/detail?id=876994

rniwa commented 5 years ago

WebKit changed this behavior in https://trac.webkit.org/changeset/235865.

garykac commented 5 years ago

Following up on a previous comment: I finally got around to creating a version of the Mouse Event Viewer with shadow DOM elements: https://w3c.github.io/uievents/tools/mouse-event-viewer-shadow.html