Set event.target to null when dispatching finishes at ShadowRoot #513

TakayoshiKochi · 2017-09-15T04:57:10Z

Resolves #511.

Preview | Diff

TakayoshiKochi · 2017-09-15T04:58:05Z

@annevk @hayatoito PTAL

TakayoshiKochi · 2017-09-15T05:19:02Z

This should be accompanied with a corresponding layout test - will be created as a separate PR for WPT repository.

hayatoito · 2017-09-15T05:45:35Z

dom.bs

 <li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/NONE}}.

+ <li><p>Set <var>event</var>'s {{Event/target}} attribute to null if {{Event/target}}'s root is a
+ <a for=/>shadow root</a> and <var>event</var>'s <a>composed flag</a> is unset.


and event's composed flag is unset.

We should remove this condition.
We need to clear event.target even if composed flag is set because target and relatedTarget can be identical in the middle and event path can be trimmed for that case.

Good catch. So we can determine if the event is stopped at shadow root just by looking at whether target's root is a shadow root.

hayatoito · 2017-09-15T05:49:18Z

This should be accompanied with a corresponding layout test - will be created as a separate PR for WPT repository.

Yeah, I am happy to review PR for WPT too.

annevk

LGTM, I'll land this when we have the WPT PR. It would also be good to file browser bugs against Chrome and WebKit. Neither Edge nor Firefox has an implementation at the moment so that seems less needed.

Resolves whatwg#511.

TakayoshiKochi · 2017-10-05T07:08:33Z

@hayatoito @annevk PTAL for relatedTarget addition.

Also, the corresponding WPT pull request is at
web-platform-tests/wpt#7481
(via https://chromium-review.googlesource.com/c/chromium/src/+/681894)

annevk · 2017-10-05T07:16:41Z

@TakayoshiKochi I don't quite know how relatedTarget works in all cases. Can we end up with a situation where target is null and relatedTarget is not? Or vice versa? Should we perhaps set both to null if either condition is true?

annevk · 2017-10-05T07:17:01Z

Also, is currentTarget affected?

annevk · 2017-10-05T07:17:31Z

dom.bs


 <li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/NONE}}.

+ <li><p>Set <var>event</var>'s {{Event/target}} attribute to null if {{Event/target}}'s root is a


We should link "root" here and below as well.

TakayoshiKochi · 2017-10-05T07:54:54Z

@TakayoshiKochi I don't quite know how relatedTarget works in all cases. Can we end up with a situation where target is null and relatedTarget is not? Or vice versa? Should we perhaps set both to null if either condition is true?

With the status quo, if an event bubbling ends up at a shadow root, its relatedTarget can be either in outside shadow tree or some shadow tree. If the former, target will become null but relatedTarget remain in my current spec patch - which may be weird and does not sound useful, even though it does not leak shadow tree.

On the other hand, once event.target is outside of any shadow tree, its corresponding relatedTarget should be also in the tree as a result of retargeting.

So I'm in favor of your idea, to set both to null if either condition is true.

Also, is currentTarget affected?

currentTarget is already covered in step 16 (in the original spec) so that it will be set to null.

If either condition is met, set target and relatedTarget to null.

annevk · 2017-10-05T08:00:03Z

Given what you're saying there, we only have to check target, right? Since relatedTarget is either equal or exposes less.

TakayoshiKochi · 2017-10-05T08:06:34Z

Given what you're saying there, we only have to check target, right? Since relatedTarget is either equal or exposes less.

Absolutely. Thanks for pointing out.

annevk · 2017-10-05T08:08:01Z

Thanks, my last change request is to put the condition first. "If X, then set Y to Z." is generally how we write these steps.

Given there's tests I think this is ready to merge then. Thanks a lot!

annevk · 2017-10-05T08:13:00Z

dom.bs


 <li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/NONE}}.

+ <li><p>If {{Event/target}}'s <a for=tree>root</a> is a <a for=/>shadow root</a>, set


Thanks, done.

Will add a test case there for relatedTarget is in document tree while target is in shadow tree and event stops at shadow root to make sure both are set to null.

hayatoito · 2017-10-06T03:27:26Z

LGTM

This corresponds to the DOM spec change whatwg/dom#513 not to leak elements inside shadow tree after event dispatch is complete. Bug: 771580, 762829 Change-Id: Ib77b4475c2c5bf174a6bcf3dc9f4dff583fd7d2a

This corresponds to the DOM spec change whatwg/dom#513 not to leak elements inside shadow tree after event dispatch is complete. Bug: 771580, 762829 Change-Id: Ib77b4475c2c5bf174a6bcf3dc9f4dff583fd7d2a Reviewed-on: https://chromium-review.googlesource.com/681894 Commit-Queue: Eriko Kurimoto <[email protected]> Reviewed-by: Takayoshi Kochi <[email protected]> Cr-Commit-Position: refs/heads/master@{#507013}

This corresponds to the DOM spec change whatwg/dom#513 not to leak elements inside shadow tree after event dispatch is complete. Bug: 771580, 762829 Change-Id: Ib77b4475c2c5bf174a6bcf3dc9f4dff583fd7d2a Reviewed-on: https://chromium-review.googlesource.com/681894 Commit-Queue: Eriko Kurimoto <[email protected]> Reviewed-by: Takayoshi Kochi <[email protected]> Cr-Original-Commit-Position: refs/heads/master@{#507013} Cr-Mirrored-From: https://chromium.googlesource.com/chromium/src Cr-Mirrored-Commit: 9c1d91c2ce388ffd5bfb0c005cd1c9f7016c7e3a

hayatoito reviewed Sep 15, 2017

View reviewed changes

annevk approved these changes Sep 15, 2017

View reviewed changes

TakayoshiKochi mentioned this pull request Oct 5, 2017

Event.target should return null after dispatch is complete, when stopped at ShadowRoot #511

Closed

TakayoshiKochi added 3 commits October 5, 2017 15:46

Set event.target to null when dispatching finishes at ShadowRoot

cb73a2e

Resolves whatwg#511.

Remove event's composed flag is unset condition.

fa26341

Take care of Event.relatedTarget so as not to leak Shadow tree internal.

f71afca

TakayoshiKochi force-pushed the squashing_target branch from c25dc11 to f71afca Compare October 5, 2017 06:57

annevk reviewed Oct 5, 2017

View reviewed changes

Unify the conditions and set both target and relatedTarget to null

166fadf

If either condition is met, set target and relatedTarget to null.

Simplify the condition.

7acb776

Put if-clause first.

0667f08

annevk reviewed Oct 5, 2017

View reviewed changes

Wording fix (then set*)

22d1fb6

annevk merged commit 9ada239 into whatwg:master Oct 5, 2017

TakayoshiKochi deleted the squashing_target branch October 5, 2017 08:27

chromium-wpt-export-bot mentioned this pull request Oct 6, 2017

Add test for the behavior after EventDispatch web-platform-tests/wpt#7481

Merged

annevk mentioned this pull request Mar 8, 2018

Should "If target's root is a shadow root, then set event’s target attribute and event’s relatedTarget to null. " be based on the path before dispatch #562

Closed


		<li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/NONE}}.

		<li><p>Set <var>event</var>'s {{Event/target}} attribute to null if {{Event/target}}'s root is a


		<li><p>Set <var>event</var>'s {{Event/eventPhase}} attribute to {{Event/NONE}}.

		<li><p>If {{Event/target}}'s <a for=tree>root</a> is a <a for=/>shadow root</a>, set

Set event.target to null when dispatching finishes at ShadowRoot #513

Set event.target to null when dispatching finishes at ShadowRoot #513

Uh oh!

Conversation

TakayoshiKochi commented Sep 15, 2017 • edited by pr-preview bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

TakayoshiKochi commented Sep 15, 2017

Uh oh!

TakayoshiKochi commented Sep 15, 2017

Uh oh!

hayatoito Sep 15, 2017

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi Sep 15, 2017

Choose a reason for hiding this comment

Uh oh!

hayatoito commented Sep 15, 2017

Uh oh!

annevk left a comment

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Oct 5, 2017

Uh oh!

annevk commented Oct 5, 2017

Uh oh!

annevk commented Oct 5, 2017

Uh oh!

annevk Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi commented Oct 5, 2017

Uh oh!

annevk commented Oct 5, 2017

Uh oh!

TakayoshiKochi commented Oct 5, 2017

Uh oh!

annevk commented Oct 5, 2017

Uh oh!

annevk Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

TakayoshiKochi Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

hayatoito commented Oct 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

TakayoshiKochi commented Sep 15, 2017 •

edited by pr-preview bot

Loading