Dom: Capturing event listeners are called during bubbling phase for shadow hosts

[rniwa]

We have a bug report saying that WebKit fires event listeners with capture flag set to true during bubbling phase.

Consider the following DOM:

const hostParent = document.createElement('section');
hostParent.id = 'hostParent';
hostParent.innerHTML = '<div id="host"></div>';
const host = hostParent.firstChild;
const shadowRoot = host.attachShadow({mode: 'closed'});
shadowRoot.innerHTML = '<p id="parent"><span id="target"></span></p>';
...
target.dispatchEvent(new CustomEvent('test', {detail: {}, bubbles: true, composed: true}));

Then the event listeners on target, parent, host, and hostParent are invoked in the following order on WebKit & Chrome:

• hostParent, capturing, eventPhase: CAPTURING_PHASE
• parent, capturing, eventPhase: CAPTURING_PHASE
• target, capturing, eventPhase: AT_TARGET
• target, non-capturing, eventPhase: AT_TARGET
• parent, non-capturing, eventPhase: BUBBLING_PHASE
• host, capturing, eventPhase: AT_TARGET
• host, non-capturing, eventPhase: AT_TARGET
• hostParent, non-capturing, eventPhase: BUBBLING_PHASE

As far as I can tell, the current behavior of WebKit & Chrome is in accordance with the latest DOM specification. What happens is that step 5 (event path construction) in dispatching an event ends up appending an tuple/struct (we should probably stick with either term, btw) with target set to null. In step 14, we only invoke event listeners with capture flag set if target is null, meaning that shadow hosts' event listeners with capture flag set would NOT be called. Then in step 15, we set eventPhase to AT_TARGET and invoke event listeners. Because the concept to inner invoke an event listener doesn't skip event listeners with capture flag set to true when the event phase is AT_TARGET, we end up firing capturing event listeners during bubbling.

[rniwa]

@annevk @hayatoito @smaug----

[rniwa]

I think we should update the spec to match what Gecko implemented which ends up invoking event listeners in the following order for the example:

• hostParent, capturing, eventPhase: CAPTURING_PHASE
• host, capturing, eventPhase: AT_TARGET
• parent, capturing, eventPhase: CAPTURING_PHASE
• target, capturing, eventPhase: AT_TARGET
• target, non-capturing, eventPhase: AT_TARGET
• parent, non-capturing, eventPhase: BUBBLING_PHASE
• host, non-capturing, eventPhase: AT_TARGET
• hostParent, non-capturing, eventPhase: BUBBLING_PHASE

[annevk]

I agree that what Gecko does looks a lot better and I've created #686 to align with it. @stramel, I acknowledged you as "Michael Stramel". Please let me know if you'd prefer a different name there.

I'm somewhat surprised we don't have good test coverage for this.

[stramel]

@annevk That works for me! Thanks!

@rniwa Thank you for the detailed explanation of what was happening and looking into it!

[hayatoito]

I remember there were some discussions around this area between @annevk and Dimitri (and maybe including me?), however, I don't recall it.

Let me try to find the discussions in the past. Maybe there is a reason for the current behavior.

So please don't proceed this until we could get more insights from the past discussions.

[annevk]

@hayatoito ah yes, #237 discusses it. It does seem this needs some more research.

[hayatoito]

@annevk Thanks! That's exactly what I've tried to find. Let me take a look.

[rniwa]

Okay according to https://github.com/whatwg/dom/issues/237, we want to be invoking both capturing & bubbling event listeners during pebble phase.

Presumably, this was agreed to in order to let non-capturing event listeners inside the shadow tree to do the necessary work before the event dispatched outside the shadow tree.

However, it makes no sense whatsoever that the capturing event listeners on ancestors of a shadow host are getting invoked BEFORE capturing event listeners are invoked on the target inside a shadow tree.

[hayatoito]

It looks I've changed the behavior here, which was originally requested by @azakus (Daniel Freedman).

Maybe we would like to hear more opinions or feedback from web authors before changing the behavior because the current behavior has been there for years, and the change would be a breaking change for us, especially existing web components frameworks.

@azakus, we'd appreciate if you could share your insights.

[annevk]

So the reason we went with the current model as outlined in #237 is because once you reach a target, all registered listeners get invoked in order, regardless of their capture boolean value. Due to shadow trees creating multiple targets, targets outside the shadow tree would first get their capture listeners invoked, and then their bubble listeners. So you could observe there is a shadow tree underneath if an earlier registered bubble listener is invoked before a capture listener.

It would make some sense to me if we changed the model and always invoked capture listeners before bubble listeners, but I don't know how compatible that would be.

[annevk]

I.e., change the order in which these are invoked:

t = document.createElement("hi");
t.addEventListener("test", e => w("bubble"), false);
t.addEventListener("test", e => w("capture"), true);
t.dispatchEvent(new Event("test"));

Given that all browsers agree that might be somewhat unrealistic.

[annevk]

And if we don't want to change how that simple scenario works and we want to avoid leaking the existence of shadow trees, we're stuck with the model described in OP, however weird that may seem. (It's weird enough to warrant an explanation in the DOM Standard, at least.)

[hayatoito]

I agree that new one is more natural. It looks I had the same opinion before.

I guess the only remaining concern here is how compatible that would be. We need to evaluate it somehow. I am now asking feedback for Polymer team.

From the implementation perspective, I don't see any difficulty to change the Blink's behavior.

[rniwa]

I agree this is a slight Web compatibility concern here given WebKit & Blink have been shipping like this for years but I think it's worth an attempt given how bad the current behavior is.

FWIW, if there is a rough consensus that the new behavior Anne specified in https://github.com/whatwg/dom/pull/686 is reasonable, then I can implement it in WebKit. People can play with it in the next STP, and we may find out any web compatibility issues that may come out.

[smaug----]

FWIW, Gecko has used the model where capturing listeners get call before bubble listeners on host
(where host can be for example input element) at least for 12 years.

[rniwa]

FWIW, Gecko has used the model where capturing listeners get call before bubble listeners on host
(where host can be for example input element) at least for 12 years.

As far as I can tell, this oddly ordered invocation of capturing event listener isn't observable when there is no author-defined shadow tree. So I think the compat risk with content which uses attachShadow is real since WebKit and Blink always shipped this odd event dispatching behavior.

[smaug----]

input element and such have native anonymous content in Gecko, so effectively behaving as host.
(it wasn't called host before shadow DOM, but that doesn't matter. XBL bound element is also host.)

[annevk]

That, coupled with the fact that I haven't seen any bug reports about that behavior, makes me somewhat hopeful we can actually ship capture-listeners-and-then-later-on-bubble-listeners for targets. 😊

[rniwa]

input element and such have native anonymous content in Gecko, so effectively behaving as host.
(it wasn't called host before shadow DOM, but that doesn't matter. XBL bound element is also host.)

Right, but if you can't see what's happening inside the shadow tree, the proposed new behavior & what WebKit & Blink implement are identical. Otherwise, we would have caught this difference much earlier.

[rniwa]

Okay, I finally understand what @annevk and @smaug---- are talking about. Yes, the order of invocations of event listeners on a given element would change from the registration order to capturing event listeners then bubbling event listeners.

But I'm a lot less concerned about that (especially now that you state Gecko has been doing this for years, and scripts don't typically register both capturing & bubbling event listeners on a single element) than the fact order of event dispatching on different elements would change. It's a lot more likely that some event listeners on different elements are relying on event listeners on another element getting events before/after them by stopping propagation, etc...

[hayatoito]

If we all agree on the new behavior, and the spec is updated, I'll try to align the Blink's behavior to the new one on Chrome Canary.

Given it would be really difficult for us to predict what would be broken with this change, it would be worth trying this change on Chrome Canary as well as WebKit, and seeing what would be broken in wild.
If someone knows any better way to evaluate the impact of this change, please let us know that.

I'll add chromestatus entry for this change so that web developers can be noticed with this change.

It's a lot more likely that some event listeners on different elements are relying on event listeners on another element getting events before/after them by stopping propagation, etc...

Yes, that could be more likely.

[annevk]

I'd prefer to not update the specification until we've tried it at least via Safari Technology Preview or Chrome Canary. It doesn't seem worth making the change if we have to revert it months later. I do think we have reached agreement on this new model and by keeping the capture listeners split from the bubble listeners there's also no way to observe shadow trees (which is why we ended up with the weirder model the last time around; I guess we didn't consider changing the normal dispatch behavior enough).

[hayatoito]

Okay, I think that's fine. Usually, Blink needs the (updated) spec to ship any web facing changes, however, this discussion can be used as justification to ship this change, I think.

However, at least, we might want WPT so that we can be sure our implementations are interoperable.
Can we have WPT without updating the spec?

[annevk]

I see, if we need to update tests, we should probably change the standard as well (they're best kept in sync), and then revert or change it to something else if we hit a roadblock.

@smaug---- @rniwa @dstorey any concerns?

[domenic]

You could also do a spec PR + .tentative.html tests.

[travisleithead]

@dstorey and I first wrote down what we expected to happen in this scenario, then consulted @rniwa's observations. Turns out we expected the new proposed behavior (of invoking capture listeners first, then bubble), and were surprised to find it speced differently. We cross-checked with Edge and IE to see if either had ever fired events at target in the proposed manner, but that never seems to have been the case (we have always fired at target listeners in registration order).

To clarify, we believe that non-shadow dom use cases would be potentially impacted by this change (if we change so that e.g., capture listeners and bubble listeners are maintained in separate lists for at target nodes and invoked in registration order within those lists, starting with capture listeners).

Since the proposed change makes more rational sense to us, we are strongly in support of a spec and behavior change here if it can be shown to be web compatible.

[hayatoito]

You could also do a spec PR + .tentative.html tests.

This sounds good to me.

[rniwa]

It looks like dom/events/Event-dispatch-handlers-changed.html is the only WPT test imported into WebKit which got affected by this change.

[rniwa]

Gecko also seems to always invoke event listeners on the target in the registration order. For simplify, I'm going to implement what's being proposed in https://github.com/whatwg/dom/pull/686 but we may need to re-visit this if it happens to be not Web compatible.

[hayatoito]

dom/events/Event-dispatch-handlers-changed.html

I guess this test is not good enough. Does someone have a plan to add wpt? I'm okay to add a very basic test. Should we wait for https://github.com/whatwg/dom/pull/686? It looks the discussion is on-going, though.

[annevk]

It would be good if @smaug---- could confirm that NONE is fine, but apart from that I think the change is agreed upon, pending web compatibility. If you could write a basic that'd be great.

[rniwa]

Given Blink & WebKit uses event phase of NONE at the time of running the legacy-pre-activation behavior, we don't really need to wait for that particular discussions to conclude.

If anything, that's a completely orthogonal behavior change we may or may not want to make. Quite honestly, whether it's NONE or CAPTURING_PHASE probably doesn't matter in practice; checking the state of a completely different event's eventPhase in the middle of change event on input is oddly specific thing to do. This is probably why we've getting away with inconsistent behavior between Gecko and WebKit/Blink in the first place.

[smaug----]

yeah, that NONE case is separate issue.

[annevk]

I created some basic tests for both issues based on @rniwa's code:

• https://github.com/web-platform-tests/wpt/pull/13030
• https://github.com/web-platform-tests/wpt/pull/13031

Both can be expanded to assert more bits. Feel free to land these as .tentative or otherwise I'll land them once we're more sure this is going to stick. Review of both is needed though so if someone is willing to do that, please.

[rniwa]

WebKit change has been landed in https://trac.webkit.org/changeset/236002.

[hayatoito]

Blink landed the change with a small basic wpt:

• https://chromium-review.googlesource.com/c/chromium/src/+/1212255
• https://chromium-review.googlesource.com/c/chromium/src/+/1229741

[annevk]

Wow, great. What would be a reasonable time to allow for some Safari Technology Preview / Chrome Canary fallout, while also not waiting all the way until release, before landing this? October 15 perhaps?

[hayatoito]

• Chrome Canary will get this change in a few days.
• Chrome M71 will get this change, roughly in the following schedule:
  
  
  
  • Chrome M71 beta will be out around the end of Oct
  
  
  • Chrome M71 Stable will be released around earlier in Dec
  
  

[annevk]

Filed bugs against the remaining browsers given how likely this is to happen:

• https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/19035285/
• https://bugzilla.mozilla.org/show_bug.cgi?id=1492446

[rniwa]

FWIW, we haven't gotten any bugs reports from this change.

[rniwa]

We've "shipped" this behavior in iOS 12.2 & macOS 10.14.4 betas, and I haven't heard of any issues yet. Chrome stable is also at M72. It seems like we can just merge this spec change now? The fact we have two browser engines successfully adopted this new behavior seems to indicate that the new behavior is quite Web compatible.

[hayatoito]

+1 for that.

[annevk]

I'll make it so, there's one wpt PR I pinged you both on that still needs review though.

[rniwa]

@annevk which PR needs reviewing??

[annevk]

@rniwa https://github.com/web-platform-tests/wpt/pull/13030.

[rniwa]

Reviewed.