Improve CommandEvent and ToggleEvent source retargeting #11345
Conversation
This change prevents CommandEvent.source from leaking nodes across shadow boundaries. More context here: whatwg#11255 (comment)
There was a problem hiding this comment.
I think this looks good, it would certainly solve the issue. I'm a little curious if this can be simplified to "if target is null or currentTarget is null, return null, otherwise retarget source against currentTarget". I'm not sure of the performance cost of re-targeting twice.
I think there are cases where currentTarget is null but target is not null after event dispatch, so we should continue considering target in addition to currentTarget. See the "clearTargets" variable here and how it only removes the target from the event in some situations (this prevents leaking nodes): https://dom.spec.whatwg.org/#concept-event-dispatch |
|
I just pushed a change to move the retargeting logic into a separate algorithm so it can also be called for the ToggleEvent.source getter |
josepharhar
changed the title
| data-x="dom-Event-currentTarget">currentTarget</code>.</p></li> | ||
|
|
||
| <li><p>If <var>retargetAgainst</var> is null, then set <var>retargetAgainst</var> to | ||
| <var>event</var>'s <span data-x="concept-event-target">target</span>.</p></li> |
There was a problem hiding this comment.
I don't understand this setup. We want events which have been dispatched in light DOM to have their .source retargeted? What is the use case for that. Events dispatched in shadow DOM would get just null.
There was a problem hiding this comment.
We want events which have been dispatched in light DOM to have their .source retargeted?
If the event has been dispatched in light DOM, then retargeting won't change the resulting source element that we return; we are effectively not retargeting events that are dispatched in light DOM.
Events dispatched in shadow DOM would get just null.
If the event was dispatched in shadow DOM, then source will become null after event dispatch is done, just like target and relatedTarget.
There was a problem hiding this comment.
If the event has been dispatched in light dom and source is in shadow DOM, retargeting will happen.
But what are we trying to achieve with this rather unusual setup? Why do we need to fall back to 'target'?
There was a problem hiding this comment.
If the event has been dispatched in light dom and source is in shadow DOM, retargeting will happen.
Ah yes, that is true. In this case, I'm assuming that you still don't want source to be leaked somewhere since it is in a shadow root, so we should do retargeting.
But what are we trying to achieve with this rather unusual setup? Why do we need to fall back to 'target'?
Target is better than currentTarget because after event dispatch target is likely still set to something we can retarget against which won't leak any nodes, whereas currentTarget just becomes null.
Now that I think about it, maybe we can just always retarget against target instead of currentTarget
There was a problem hiding this comment.
I removed the usage of currentTarget and now it just uses target. It shouldn't change the behavior.
There was a problem hiding this comment.
Is that also what happens for relatedTarget? Ideally source works the same way as relatedTarget.
This change prevents CommandEvent.source from leaking nodes across shadow boundaries. More context here:
#11255 (comment)
Event.currentTarget and Event.target already have retargeting capabilities built into them to prevent leaking nodes across shadow boundaries. By retargeting against those, we can make sure that Event.source does not leak across shadow boundaries.
(See WHATWG Working Mode: Changes for more details.)
/interaction.html ( diff )