Non-bubbling event should run non-capture listeners #750
Conversation
This fixes a regression from #686. Tests: various tests were failing because of this. web-platform-tests/wpt#16307 aligns the remaining failing test with the new model. Fixes #742.
| @@ -1405,15 +1405,21 @@ for discussion). | |||
| </ol> | |||
|
|
|||
| <li> | |||
| <p>If <var>event</var>'s {{Event/bubbles}} attribute is true, then <a for=list>for each</a> | |||
| <var>struct</var> in <var>event</var>'s <a for=Event>path</a>: | |||
| <p><a for=list>For each</a> <var>struct</var> in <var>event</var>'s <a for=Event>path</a>: | |||
|
|
|||
| <ol> | |||
| <li><p>If <var>struct</var>'s <a for=Event/path>target</a> is non-null, then set | |||
There was a problem hiding this comment.
I actually don't get this check. How can target ever be null?
Don't we need to check that target is equal to targetOverride instead?
In general, steps 5.9.7-9 seem to be in the need of a bit more clarification as to what they're trying to do.
There was a problem hiding this comment.
An event's path consists of a list of structs. target is only non-null for the actual target and any shadow tree hosts that the actual target might be "nested" in.
There was a problem hiding this comment.
I understand that's what this sentence assumes but I don't see how such a condition is met given what's in https://dom.spec.whatwg.org/#concept-event-dispatch. It passes parent as the target of the struct in most cases and each get the parent relies on parent not being null. So I don't see how that's possible...
There was a problem hiding this comment.
Yeah, but that argument is what becomes "item" on the struct, not "target". See https://dom.spec.whatwg.org/#concept-event-path-append. I agree this is somewhat confusing and I'd appreciate if someone could come up with better variable names as I'm still somewhat stuck.
There was a problem hiding this comment.
Oh, I see. That is indeed very confusing. Maybe we can use different variable names or something? Having item, target, & targetOverride is confusing enough.
|
Okay, with that clarification, the change looks good. That's basically what I implemented in WebKit. |
|
I had forgotten that I already got some suggestions in #645. I'll see about fixing that after this. |
|
We can probably merge this fix as is though. We make editorial improvements in #645. |
|
FWIW, I added a test in https://trac.webkit.org/browser/webkit/trunk/LayoutTests/fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shadow-trees.html?rev=236002. Feel free to create a PR to merge it into WPT. I can make one too if you'd like. |
|
@rniwa if you could turn that into a PR that'd be great, thanks! |
This fixes a regression from #686.
Tests: various tests were failing because of this. web-platform-tests/wpt#16307 aligns the remaining failing test with the new model.
Fixes #742.