Correct setting of target and relatedTargets post-dispatch #585

annevk · 2018-03-08T05:33:36Z

@whatwg/components this fixes the following:

Adds "touch target list" as a concept hold by event, so touch events can be retargeted. (This will require some special handling for Touch objects, but I think it's workable.)
It now resets target/relatedTarget/touch target list post-dispatch based on pre-dispatch conditions.
It makes dispatch account for non-nodes.

(The one thing remaining here is having a good look at event constructors and making sure we can make that work for the relatedTargets design. I think that requires some kind of callback type thing.)

Preview | Diff

Helps with issue whatwg/dom#562 and PR whatwg/dom#585.

hayatoito · 2018-03-08T12:45:19Z

I would like to take a look, however, I will take a vacation for a few days. Let me review in the next week.

smaug---- · 2018-03-12T13:31:25Z

dom.bs


- <li><p>If <var>target</var> is <var>relatedTarget</var> and <var>target</var> is not
- <var>event</var>'s <a for=Event>relatedTarget</a>, then return true.
+ <li><p>If <var>relatedTargets</var> <a for=list>contains</a> <var>target</var> and


I doubt this is quite right in Touch case. Need to think this case some more.
I have some such case in mind where one of the relatedTargets does contain target before retargeting and then retargeting makes also some other relatedTarget to be target.

It seems https://github.com/w3c/web-platform-tests/blob/master/touch-events/touch-retargeting.html can be extended somehow for more test coverage for touch. It's not entirely clear to me how that works given that on desktop the events are not really exposed.

smaug---- · 2018-03-12T13:34:32Z

dom.bs

-   <li><p>Otherwise, if <var>parent</var> and <var>relatedTarget</var> are identical, then set
-   <var>parent</var> to null.
+   <li><p>Otherwise, if <var>relatedTargets</var> <a for=list>contains</a> <var>parent</var>, then
+   set <var>parent</var> to null.


Gut feeling is that also this has similar problem.

Yup, I confirmed the Blink's implementation.
We need to distinguish relatedTarget and relatedTargets somehow because only relatedTarget (in the sense of before this PR) should be considered here.
"relaterTagets contains parent" causes a sort of false positive.

How should touch targets be adjusted though?

Since the feedback I got before was that whenever we have relatedTarget handling, we also need to consider touch target handling.

These steps always need some time to understand its intention exactly. :(

My intention is:

All touch targets should be adjusted in the same way as relatedTarget should be adjusted. They both should be adjusted.

However, there is one difference between relatedTarget and touch targets. Only relatedTarget should be used to decide to exit this while loop "11. While parent is non-null". This step sets parent to null, thus I think this is an attempt to exit the loop.
To decide to exit this loop or not, in other words, to decide whether an event should be stopped at a shadow root or not, only relatedTarget should be used as its judgement. Touch targets shouldn't be used there.

Since TouchEvent doesn't look to have a relatedTarget to me, TouchEvent never set parent to null here. In other words, TouchEvent is never stopped at a shadow root even if its target and one of touch targets are same.

Okay, so we effectively need to have relatedTarget as an internal slot and touchTargets as an internal slot and most of the time we do the same thing for both, but not there and not at the other place @smaug---- pointed out (which is hidden now for some reason).

I can make that change. I'd love some help with tests for touch targets btw since I don't really have a good way to run them here.

(The alternative option is that we keep one slot but branch on event interface, but that seems a little icky, but let me know if you'd prefer that.)

I would love to help to write more tests, though I'm afraid I can write tests only for synthetic TouchEvents. I am not sure how to write a test for non-synthetic TouchEvents.

(The alternative option is that we keep one slot but branch on event interface, but that seems a little icky, but let me know if you'd prefer that.)

I don't have any preference.

btw, I am wondering how these relatedTargets will be used in TouchEvent spec.
TouchEvents has 3 relatedTargets lists.

readonly attribute TouchList touches; readonly attribute TouchList targetTouches; readonly attribute TouchList changedTouches;

I was thinking that since those lists are all immutable, a flattened list backing them is fine, but that only works if retargeting is consistent regardless of which list.

I see. Your assumption is correct. Retargeting is consistent regardless of which list.

smaug---- · 2018-03-12T13:38:48Z

dom.bs

- <a for=tree>root</a> is a <a>shadow-including inclusive ancestor</a> of <var>B</var>, then return
- <var>A</var>.
+ <li><p>If <var>A</var> is a {{Window}} object, then set <var>A</var> to <var>A</var>'s
+ <a>associated <code>Document</code></a>.


Hmm, retargeting Window gives Document? Shouldn't this just return Window

Hmm yeah, I guess we just want it to behave as if you passed document.

I guess I should just remove this step altogether then?

Because window is not an instance of a Node, it falls into the line 5767 (A is not a node)?
Sounds good - but wondering what's the reasoning behind adding this line.

I added this because of #580 (comment), but I didn't really think it through.

I think removing this step is not enough.
e.g. If A is a node in a shadow tree and B is a Window, we should loop until A reaches the outer most shadow host.

The following should work, I think.

If one of the following is true

A is not a node

A’s root is not a shadow root

B is a node and A’s root is a shadow-including inclusive ancestor of B

then return A.

Set A to A’s root’s host.

Thanks, that does look good.

TakayoshiKochi · 2018-03-13T06:36:56Z

dom.bs

+
+  <ul class=brief>
+   <li><var>A</var> is not a <a>node</a>
+   <li><var>B</var> is not a <a>node</a>


Basically the change for retargeting algorithm from the original is these 2 lines above, am I understanding correct?

annevk · 2018-03-14T11:53:08Z

The PR now separates relatedTarget and touch target list and only has the special cases for relatedTarget. I still need to define an event creation callback of sorts so we can define TouchTarget / FocusEvent et al properly. I was also thinking of incorporating #402 here since we'll have to test that case anyway.

annevk · 2018-03-14T12:06:09Z

dom.bs


 <li><p>If <var>target</var> is <var>relatedTarget</var> and <var>target</var> is not
 <var>event</var>'s <a for=Event>relatedTarget</a>, then return true.
+ <!-- XXX Since we return early, we might want to unset more flags here. See #402. -->


Should we also unset target et al here if clearTargetsPostDispatch is true? It seems that would be consistent... (If that's the case it seems we should turn this into a short algorithm that we invoke here and later on.)

I have never thought about this case seriously so far. In this early return, I assume that any event listener is never called.

In this case, we don't have to worry about any leaking of a node in a shadow tree via an event object. Users don't have any change to access an event object, other than the original event dispatcher.

btw, the consistency may matter. I'm fine to reset target, relatedTarget, and touch target list here too.

Yeah, I'm mostly wondering about it from a consistency perspective. I could see you'd have some custom dispatching code and later store the event object after dispatch in a global place and maybe you don't want things to be leaked there ever. Doesn't seem like a sound setup, but other than this case we do provide the guarantee that nothing would leak.

I've now made this change.

annevk · 2018-03-15T16:23:28Z

I consider this ready apart from the event construction specification callback, which is being discussed in #414. The ongoing feedback here is much appreciated though, please keep it coming.

smaug---- · 2018-03-16T17:33:25Z

dom.bs

@@ -10070,6 +10112,7 @@ Tab Atkins,
 Takashi Sakamoto,
 Takayoshi Kochi,
 Theresa O'Connor,
+Theodore Dubois,


Is this related to this pr?

smaug---- · 2018-03-16T18:08:47Z

dom.bs

+ <li><p>Set <var>event</var>'s <a for=Event>relatedTarget</a> to <var>tuple</var>'s
+ <a for=Event/path>relatedTarget</a>.
+
+ <li><p>Set <var>event</var>'s <a for=Event>touch target list</a> to <var>tuple</var>'s


hmm, perhaps a bit confusing, but I guess should be enough for touch objects. They just need to have a pointer to the event, so that they can update their target form there (basically map their original target to retargeted target or so)

smaug----

looks ok. It is possible that touch target list stuff will need some tweaking when Touch spec is updated

hayatoito · 2018-03-20T09:20:04Z

Looks okay to me. Thank you for updating this!

annevk · 2018-03-20T13:50:19Z

So thinking about this in light of the tests at shadow-dom/event-post-dispatch.html it seems this is still not entirely correct. With the patch is proposed, you can observe whether an event started out in a shadow tree or not.

I think clearTargets should instead be based on event's path. You find the last tuple in event's path with a non-null target. And then if that tuple contains a node inside a shadow tree in one of the three target fields, you set clearTargets to true.

annevk · 2018-03-20T14:37:52Z

Hmm, and should https://whatpr.org/dom/585.html#concept-event-listener-invoke be updated to check the propagation flag later, so target et al still get updated? That would lead to problems too otherwise I think.

hayatoito · 2018-03-26T05:58:22Z

Does this need another review? Please ping me if the PR is ready.

annevk · 2018-03-26T10:51:11Z

@hayatoito I'd like input from you and @smaug---- on #585 (comment) (and the next comment) and what a solution to that might look like. The other thing we need here are more tests I suppose.

smaug---- · 2018-03-26T11:07:56Z

I think that approach sounds good. So, in case all the targets after actual dispatch are in light DOM, targets wouldn't get cleared.
And whether those targets are in light DOM is calculated before the dispatch.

hayatoito · 2018-03-27T08:52:34Z

I think clearTargets should instead be based on event's path. You find the last tuple in event's path with a non-null target. And then if that tuple contains a node inside a shadow tree in one of the three target fields, you set clearTargets to true.

I see. This sounds a reasonable change. Nice catch.

This makes it work for touch events. Tests: ... Fixes part of #561.

We cannot set them to null based on post-dispatch conditions. And we need to check each of them rather only the target. Tests: ... Fixes #562.

Fixes #580.

closes #402

fixes #602

annevk · 2018-03-27T10:47:05Z

Tentative commit message:

Shadow: fix event dispatch

This makes a number of large changes to event dispatch:

* The algorithm can no longer return early with true. The observable
  aspect is that the event object is "uninitialized" and false might 
  be returned if the event object was already canceled.
* It accounts for Touch Events in shadow trees by adding the 
  "touch target list" which is somewhat similar to relatedTarget.
* Retargeting is now solely controlled by the event dispatch 
  algorithm. The hook for other specifications is removed.
* Retargeting now properly accounts for non-node objects.
* Dispatch in general accounts for non-node objects too now.
* If targets need to be unset to prevent exposure of a shadow tree
  this is now calculated prior to invoking listeners.

Initializing relatedTarget and "touch target list" is up to the event
classes that need it. #614 will add "event constructing steps" that
enable this.

Tests: https://github.com/w3c/web-platform-tests/pull/9919
(probably still needs work given today's changes)

This closes #402, fixes #561, fixes #562, fixes #580, and fixes #602.

So more review is welcome here (for the last four commits and this commit message in particular) and more tests.

Sorry it got so big, but I'm not sure this could be elegantly made smaller.

hayatoito · 2018-03-28T06:06:37Z

I've reviewed it, and it looks okay to me. A really great work!

For whatwg/dom#585.

annevk · 2018-03-28T12:54:55Z

Bugs:

(No bug for Edge since they haven't started yet as far as I'm aware.)

Automatic update from web-platform-testsDOM: reset target/relatedTarget For whatwg/dom#585. wpt-commits: 2f758ac5c16e44915f270ceec31a84eed8f85769 wpt-pr: 9919 wpt-commits: 2f758ac5c16e44915f270ceec31a84eed8f85769 wpt-pr: 9919

Automatic update from web-platform-testsDOM: reset target/relatedTarget For whatwg/dom#585. wpt-commits: 2f758ac5c16e44915f270ceec31a84eed8f85769 wpt-pr: 9919 wpt-commits: 2f758ac5c16e44915f270ceec31a84eed8f85769 wpt-pr: 9919 UltraBlame original commit: 2d92f37b4443fac2124163799a2cb0c9f1835f95

annevk changed the title ~~Correct setting of target and relatedTargets post-dispatch~~ Correct setting of target and relatedTargets post-dispatch Mar 8, 2018

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 8, 2018

DOM: reset target/relatedTarget

c790721

Helps with issue whatwg/dom#562 and PR whatwg/dom#585.

This was referenced Mar 8, 2018

DOM: reset target/relatedTarget web-platform-tests/wpt#9919

Merged

Turn relatedTarget into relatedTargets #584

Closed

annevk added the topic: shadow Relates to shadow trees (as defined in DOM) label Mar 8, 2018

smaug---- requested changes Mar 12, 2018

View reviewed changes

This was referenced Mar 12, 2018

Suspect it removes an at-mention link tobie/pr-preview#16

Open

Reset dispatching flags when retargeting to a different target than event's #402

Closed

TakayoshiKochi reviewed Mar 13, 2018

View reviewed changes

annevk mentioned this pull request Mar 13, 2018

"create an event" doesn't work for certain UI events #414

Closed

annevk force-pushed the annevk/null-at-end-of-dispatch branch from 431c2ae to b7713de Compare March 14, 2018 11:00

annevk commented Mar 14, 2018

View reviewed changes

annevk mentioned this pull request Mar 15, 2018

Should early return for relatedTarget take cancelable flag into account #602

Closed

annevk added the topic: events label Mar 15, 2018

smaug---- reviewed Mar 16, 2018

View reviewed changes

smaug---- approved these changes Mar 16, 2018

View reviewed changes

hayatoito approved these changes Mar 20, 2018

View reviewed changes

annevk mentioned this pull request Mar 27, 2018

Add window.event as a legacy compat requirement #407

Merged

annevk added 12 commits March 27, 2018 12:28

Turn relatedTarget into relatedTargets

fcad019

This makes it work for touch events. Tests: ... Fixes part of #561.

Correct setting of target and relatedTargets post-dispatch

6755fb2

We cannot set them to null based on post-dispatch conditions. And we need to check each of them rather only the target. Tests: ... Fixes #562.

Make event dispatch care about non-nodes again

967e8de

Fixes #580.

address some feedback from smaug and hayato

6b2f164

separate relatedTarget and touch target list

ed8c98e

nit

f9d5e64

forgot to change append to an event path

ea4690c

clear an event consistently

011d0d2

closes #402

reword to avoid early return

4206f1f

fixes #602

fixup

9777fb8

address clearTargets and stop propagation issues

28946f9

remove callback comment as that's added by #614

67e64b4

annevk force-pushed the annevk/null-at-end-of-dispatch branch from d7a2136 to 67e64b4 Compare March 27, 2018 10:29

annevk added 2 commits March 27, 2018 14:53

retarget touch targets against correct object

9dfab83

don't assume it's a node

f01c585

annevk added a commit to web-platform-tests/wpt that referenced this pull request Mar 28, 2018

DOM: reset target/relatedTarget

2f758ac

For whatwg/dom#585.

annevk merged commit fc16564 into master Mar 28, 2018

annevk deleted the annevk/null-at-end-of-dispatch branch March 28, 2018 12:56

jdm mentioned this pull request Mar 28, 2018

Adjust event retargeting algorithm servo/servo#20464

Open

annevk mentioned this pull request May 7, 2018

DOM: fix relatedTarget reset test once more web-platform-tests/wpt#10862

Closed

annevk mentioned this pull request Feb 7, 2023

Update retargeting steps link w3c/touch-events#135

Merged

Correct setting of target and relatedTargets post-dispatch #585

Correct setting of target and relatedTargets post-dispatch #585

Conversation

annevk commented Mar 8, 2018 • edited by pr-preview bot Loading

hayatoito commented Mar 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Mar 14, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

annevk commented Mar 15, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smaug---- left a comment

Choose a reason for hiding this comment

hayatoito commented Mar 20, 2018

annevk commented Mar 20, 2018

annevk commented Mar 20, 2018

hayatoito commented Mar 26, 2018

annevk commented Mar 26, 2018 • edited Loading

smaug---- commented Mar 26, 2018 • edited Loading

hayatoito commented Mar 27, 2018

annevk commented Mar 27, 2018 • edited Loading

hayatoito commented Mar 28, 2018

annevk commented Mar 28, 2018

annevk commented Mar 8, 2018 •

edited by pr-preview bot

Loading

annevk commented Mar 26, 2018 •

edited

Loading

smaug---- commented Mar 26, 2018 •

edited

Loading

annevk commented Mar 27, 2018 •

edited

Loading