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

[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 web-platform-tests/wpt#13030.

[rniwa]

Reviewed.