event.composedPath() may return different results if DOM is modified between two calls #525

smaug---- · 2017-10-30T14:28:46Z

If I read the spec right, composedPath() relies on the current tree structure to figure out what to return.
So, if a listener does then something like

var cp = event.composedPath();
/*move nodes around*/
var cp2 = event.composedPath();

cp and cp2 may contain different nodes, which is surprising, since the internal path for the event hasn't changed.
In particular, relying on "shadow-including inclusive ancestor" in https://dom.spec.whatwg.org/#concept-closed-shadow-hidden in https://dom.spec.whatwg.org/#dom-event-composedpath looks wrong

smaug---- · 2017-10-30T14:32:12Z

Looks like use of 'shadow-including root' in window case is problematic too.

smaug---- · 2017-10-30T14:34:52Z

@hayatoito what does blink do here? @rniwa or webkit?
I guess I need to look at their source code.

hayatoito · 2017-10-31T04:29:45Z

Ah, my first reaction is "This is a spec bug. composedPath() shouldn't rely on the current status".

However, it looks Blink's implementation relies on the current status. :(

hayatoito · 2017-10-31T04:30:46Z

I think we should fix the spec if we can agree on that.

domenic · 2017-10-31T04:33:44Z

One thing to keep in mind if possible is to keep things "lazy", or at least optimizable. Right now if there are no capturing listeners for a non-bubbling event you can avoid computing the path. (Per spec.) It's a very nice property to have and allows us to keep events cheap so that people don't start using the XObserver pattern to replace events because it's "faster".

So, just be careful, in fixing the spec here. If possible.

hayatoito · 2017-10-31T04:36:01Z

Yup, that can be a reason we won't change the spec.

As far as I can tell, calculating composedPath() is not so cheap.

hayatoito · 2017-10-31T04:47:01Z

One more thing:
Since calculating composedPath() is not so cheap, Blink caches the result; composedPath() is calculated lazily, but only once per each tree scope. In the 2nd call of composedPath(), it just returns the cached result without relying on the current status. In this sense, Blink is not conforming spec.

smaug---- · 2017-10-31T07:50:40Z

Oh, Blink is even against the current spec, and the current spec is against, well, sanity :)

When fixing this issue one doesn't need to calculate much during event path creation. Just annotate each event path tuple in a subtree with that subtree's unique "id" and whether that subtree is open or closed.
That is basically flag + a counter. Then just use that information when creating the path lazily.
And by fixing this, composedPath() would always return the same result for the currentTarget, so caching the result would be ok per spec.

So I don't see a reason to not fix this.

hayatoito · 2017-10-31T08:01:32Z

Yup, I tend to agree that is doable without much cost. I'm fine to fix the spec. That should be considered as a spec bug, basically.

It is not intentional that Blink is not confirming the current spec. It looks I forgot a possibility of DOM mutation between calls. :(

rniwa · 2017-10-31T08:47:43Z

Initially I thought this would be fine but there's a challenge here. Just remembering to which tree each node belongs is not enough because a shadow host could move from one tree to another during an event dispatching. It would mean that we have to compute the relationships between each shadow tree & document tree before the events get dispatched. This is going to be quite expensive since you'd have to remember to which shadow/document tree each node belongs as well as what the shadow tree's parent tree was. That's not the kind of a thing we'd like to implement in WebKit. As such I'm against making this spec change.

domenic · 2017-10-31T08:51:54Z

@smaug---- your algorithm is still O(depth) of the tree, right? That's not great; making event firing always O(n) even for non-bubbling events with no capture listeners seems like a really bad slowdown to me (given how few pages use capture listeners and how many events are non-bubbling).

smaug---- · 2017-10-31T08:59:05Z

How is that O(depth)?

hayatoito · 2017-10-31T09:01:51Z

O(depth) is okay. We have to spend O(depth) cost at any case in event dispatching.

For most kinds of events, we have to traverse a tree up anyway because we can't tell there is no event listener for capture/bubble phase at each node beforehand.

Blink has some optimization for some kinds of events so that we can skip event path calculation entirely. But this case is very limited, AFAIK.

I'll spend some time later to know how feasible this spec change is for Blink, from performance's perspective.

domenic · 2017-10-31T09:07:18Z

How is that O(depth)?

I'd assumed "Just annotate each event path tuple in a subtree" requires iterating over each node in the path, and thus computing the path, which is O(depth).

We have to spend O(depth) cost at any case in event dispatching.

I don't think we do. We can easily tell when there is no capture event listener for a given event name in the document beforehand, just by keeping track. Which means that for non-bubbling events of that type we can just totally avoid event path computation and only dispatch on the single node.

From what I understand talking to @ikilpatrick Blink does not currently implement this optimization because of Blink's non-standard event.path which is computed eagerly. But we could implement it, and thus have O(1) event dispatching in the somewhat-common case I am talking about. We should not prevent ourselves or other browsers from achieving this speed-up, if at all possible.

If it would be helpful I can implement this O(1) dispatching in jsdom so you can see an example of how the implementation would look.

Blink has some optimization for some kind of events so that we can skip event path calculation entirely. But this case is limited.

Maybe the case is currently limited in Blink, but I think it could be quite common. (All non-bubbling events when there are no composed listeners for that event type on the document.) Regardless of how limited or common it is, it would be sad to mandate in the spec that we can never skip the event path calculation.

annevk · 2017-10-31T09:11:10Z

@domenic event.composedPath() is not lazily the way you describe now either. I don't think anyone would want it to be fully lazy.

domenic · 2017-10-31T09:13:43Z

I see. That is disappointing, and means people will be forever creating XObservers and avoiding events just to get performance benefits.

annevk · 2017-10-31T09:21:21Z

Rough summary from the IRC debate, @rniwa:

Prefers keeping the status quo.
Preserving encapsulation at composedPath() invocation time is important.
Keeping dispatch as simple as possible is important for performance.

@smaug----:

Dislikes the status quo.
Would prefer storing additional bits during dispatch to make composedPath() faster.
Only concerned about preserving encapsulation at time of dispatch. Non-closed nodes in the event's path that become closed were already revealed.

It seems @domenic would prefer a version where event's path can be disregarded.

domenic · 2017-10-31T09:35:15Z

I am happy to defer to implementers here. I had thought they would all be excited about O(1) event dispatch, but perhaps the fact that nobody implemented that so far (and that nobody got sad when we added composedPath(), making it impossible) means that it is not important. For example maybe they've not seen event dispatch being in the critical path for real-world performance.

I think it will have somewhat bad ecosystem effects with regard to XObserver proliferation, but that's already a lost cause, I guess.

smaug---- · 2017-10-31T09:37:00Z

My approach wouldn't change O(x) of event dispatch.

(FWIW, I've spent quite some time optimizing event dispatch code in Gecko and I don't want to make it slower.)

hayatoito · 2017-10-31T09:39:10Z

@domenic Yup, let me investigate further. I'll answer it back when I get more insights.

What I know as of now:

event.path and event.composedPath() are almost same in Blink. They are backed by the same helper function. So having event.path doesn't matter here.
Blink has a kind of optimization for some events. See
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.h?q=Document.h&sq=package:chromium&dr=CSs&l=823

As far as I know, none of them are for O(1) dispatch where there is only one listener on the target. All of them are for skipping event dispatch entirely.

smaug---- · 2017-10-31T17:44:12Z

The current model leads to also very surprising results if one removes a node from closed shadow tree during event dispatch.
Say, there is node A in light dom, B somewhere underneath it in a closed shadow dom, and the C in a (non-closed) shadow dom under B.
Capturing event listener on A won't get any shadow nodes in composedPath(), since C is hidden because it is underneath closed B. Now, if B is removed from its parent, it's root isn't anymore closed shadow root, so also everything in C becomes visible, and now if A has a listener in bubble phase too, the composedPath() becomes very different to what it was during capture phase.
I'd definitely prefer keep returning the same composed path all the time per currentTarget, and the value of the path would be solely based on the initial path.

smaug---- · 2017-11-01T12:15:34Z

@rniwa so that ^ above ends up revealing used-to-be-closed nodes (and still should be hidden, IMO) to the event listeners in the current model, and I think that is way more likely case than moving nodes from open shadow DOM to closed one.
And as the current model leads to changing which nodes are in the composedPath, it also weirdly changes where the event.target which the listener sees is in the composed path, since event.target is set based on the path.

I'm having hard time to see why we'd keep the current model

rniwa · 2017-11-01T22:13:51Z

Okay. The fact a node that gets removed from a closed shadow tree gets exposed in the composed path is a more serious issue. Given that example, I agree we should change the spec. We'll come up with some optimizations for WebKit so we can stay ahead of the game in the event dispatching time ;)

annevk · 2017-11-06T15:16:40Z

@smaug---- and I tried to sketch out a different model here, review appreciated!

When appending to path assume the arguments in the tuple are always named (target, targetOverride, relatedTarget), in step 5, 9.2.2, and 9.4.2 of https://dom.spec.whatwg.org/#concept-event-dispatch. Assume that invokes an algorithm that runs the following steps before appending the tuple to the path:

If target is a shadow root whose mode is "closed", then set root-of-closed-tree to true in the tuple.

If target is a slot, target's root is a shadow root whose mode is "closed", targetOverride is non-null, targetOverride is assigned, and targetOverride's assigned slot is target, then set slot-in-closed-tree to true in the tuple.

Then composedPath() is defined as follows:

let reversedComposedPath be an empty list

let hiddenSubtreeLevel be 0

let hasSeenCurrentTarget be false

reverse iterate path (from Window to the target of the event):

if tuple's item is currentTarget, set hasSeenCurrentTarget to true

else if hasSeenCurrentTarget is true and tuple's item has root-of-closed-tree flag, increase hiddenSubtreeLevel by 1.

if hiddenSubtreeLevel is 0, append tuple's item to reversedComposedPath.

if tuple's item has slot-in-closed-tree, decrease hiddenSubtreeLevel by 1.

return reverse(reversedComposedPath)

hayatoito · 2017-11-08T08:47:32Z

I tried to apply the given algorithm to the following DOM trees:

A
└──/shadowRoot1 (open)
    └── B
        ├──/shadowRoot2 (closed)
        │   └── D
        │       └── slot
        └── C

Then, let's assume that an event is fired on C, and we call event.composedPath() in an event listener registered on A.

My understanding is:

parent	target	(target,	targetOverride,	root-of-closed-tree	slot-in-closed-tree	reversedComposedPath	hiddenSubtreeLevel	hasSeenCurrentTarget
	C	C	C			A,sr1,B (We didn't add C here)	1	true
slot		slot	null		We didn't set true, here. Is that correct?	A,sr1,B	1 (Not decreased)	true
D		D	null			A,sr1,B	1	true
sr2		sr2	null	true		A,sr1,B	1 (increased)	true
B		B	null			A,sr1,B	0	true
sr1		sr1	null			A,sr1	0	true
A	A	A	A			A	0	set to true

It looks event.composedPath() would return reverse([A, sr1, B]), instead of returning reverse([A, sr1, B, C]). Is my understanding correct?

smaug---- · 2017-11-08T10:21:00Z

It should return A, sr1, B, C.
(when C is handled, the level should be back to 0, since the slot in the path was in closed tree and
'if tuple's item has slot-in-closed-tree, decrease hiddenSubtreeLevel by 1.' would be run.)

hayatoito · 2017-11-08T10:33:54Z

It should return A, sr1, B, C.

Yeah, I guess that's the intention.

If target is a slot, target's root is a shadow root whose mode is "closed", targetOverride is non-null, targetOverride is assigned, and targetOverride's assigned slot is target, then set slot-in-closed-tree to true in the tuple.

Is this condition correct? It checks that targetOverride is non-null, but targetOverride is null for the slot in the path in this case.

smaug---- · 2017-11-08T10:38:52Z

ahaa, that sounds like a bug when my original algorithm was converted.
https://public.etherpad-mozilla.org/p/composedPath has
" If target is assigned and target's assigned slot's root is a shadow root whose mode is "closed", then add a slot-in-closed-tree flag to the tuple when adding that slot to the path."

hayatoito · 2017-11-09T03:42:10Z

Yeah, that makes sense. Thank you!

One more thing I have noticed:

if tuple's item has slot-in-closed-tree, decrease hiddenSubtreeLevel by 1.

I think this could set hiddenSubtreeLevel to a negative value in some cases. I guess that is unintentional.

For example, suppose an event is fired on C, and the currentTarget is D, in the following DOM trees:

A
└──/shadowRoot1 (open)
    └── B
        ├──/shadowRoot2 (closed)
        │   └── D
        │       └── slot
        └── C

item	root-of-closed-tree	slot-in-closed-tree	hasSeenCurrentTarget	hiddenSubtreeLevel	reversedComposedPath
C			true	-1	A,sr1,B,sr2,D,slot
slot		true	true	0 -> -1	A,sr1,B,sr2,D,slot
D			true	0	A,sr1,B,sr2,D
sr2	true		false	0	A,sr1,B,sr2
B			false	0	A,sr1,B
sr1			false	0	A,sr1
A			false	0	A

composedPath() should be [A,sr1,B,sr2,D,slot,C] in this case, but C is not added because hiddenSubtreeLevel became -1 in the slot.

Can we fix this as follows?

if tuple's item has slot-in-closed-tree and hiddenSubtreLevel is positive, decrease hiddenSubtreeLevel by 1.

hayatoito · 2017-11-09T04:08:41Z

BTW, I have just tried to update Blink's implementation to align with new spec, however, I've found that Blink already supports it. Blink calculates composedPath() based on the structure just before an event is fired. DOM mutations in an event listener doesn't have any affect on event.composedPath().

Actually, I implemented it in the past, considering also "closed-ness". But it looks I forgot it. :(

Fixes #525.

annevk · 2017-11-09T10:59:56Z

I don't think it matters for now, but the main downside of not trying to encapsulate all this into "get the parent" somehow is that the encapsulation remains tied to node trees and is not something others can adopt for their event dispatching needs. This is mitigated by 1) "get the parent" not being exposed in API form and 2) "get the parent" only being used for IDB outside of node trees and in a fairly limited way.

smaug---- · 2017-11-09T14:56:25Z

Oh, -1 shouldn't happen if slot's closed root is already in the composed path.
How to express that...
While creating the composed path, add a flag to the closed root whether it is hidden or not and then when dealing with slot, do -1 only if its root is hidden

annevk · 2017-11-09T15:52:35Z

@smaug---- can't the root have changed at that point? That is, how do you obtain the root from the slot?

Also, if that is indeed different from the algorithm @hayatoito suggests what is the test case we need to add to catch the difference?

smaug---- · 2017-11-09T16:08:33Z

äh, yes, root may have changed. But ok, not doing -1 when we're at level 0 should work, I think.

Did @hayatoito suggest some other algorithm?

annevk · 2017-11-09T16:12:22Z

No, that's what he suggested (and what's in my PR).

Tests: web-platform-tests/wpt#8385. Fixes #525. (This also changes some formatting to align with Bikeshed changes.)

https://bugs.webkit.org/show_bug.cgi?id=180378 <rdar://problem/42843004> Reviewed by Darin Adler. LayoutTests/imported/w3c: Rebaselined the test now that all test cases pass. * web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt: Source/WebCore: This patch makes the check for whether a given node in the event path be included in composedPath pre-determined at the time of the event dispatching per whatwg/dom#525. This was a fix for the issue that if an event listener in a closed shadow tree removes a node in the same tree in the event path, then composedPath called on its shadow host, for example, will include the removed node since it's no longer in the closed shadow tree. Naively, implementing this behavior would require remembering the original document or shadow root of each node in the event path as well as its parent shadow root, or worse which node is disclosed to every other node at the time of computing the event path. This patch takes a more novel and efficient approach to implement the new behavior by adding a single integer indicating the number of closed-mode shadow root ancestors of each node in the event path. In computePathUnclosedToTarget, any node whose *depth* is greater than the context object is excluded. Consider the following example: div ------- ShadowRoot (closed) +- span +- slot If an event is dispatched on span, then the event path would be [span, slot, ShadowRoot, div]. Then the values of integers assigned to each node would be: [0, 1, 1, 0] respectively. When composedPath is called on span or div, slot and ShadowRoot are excluded because they have a greater *depth* value. Unfortunately, this simplistic solution doesn't work when there are multiple shadow roots of the same depth through which an event is dispatched as in: section -- ShadowRoot (closed, SR2) | +- slot (s2) +div ------ ShadowRoot (closed, SR1) +- span +- slot (s1) If an event is dispatched on span, the event path would be [span, s1, SR1, div, s2, SR2, section]. The values of integers assigned are: [0, 1, 1, 0, 1, 1, 0] respectively. When composedPath is called on SR1, the simplistic approach would include s2 and SR2, which would be wrong. To account for this case, in computePathUnclosedToTarget, we traverse the event path upwards (i.e. ancestors) and downwards (i.e. descendants) from the context object and decrease the *allowed depth* of shadow trees when we traverse out of a shadow tree in either direction. When traversing upwards, therefore, moving out of a shadow root to its host would would decrease the allowed depth. When traversing dowards, moving from a slot element to its assigned node would decrease the allowed depth. Note that the depths can be negative when a composed event is dispatched inside a closed shadow tree, and it gets out of its shadow host. Unfortunately, the latest DOM specification has a bug and doesn't match the behavior of Chrome. This patch proposes a new algorithm which can be adopted in whatwg/dom#684. Test: imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation.html * dom/EventContext.cpp: (WebCore::EventContext::EventContext): (WebCore::MouseOrFocusEventContext::MouseOrFocusEventContext): (WebCore::TouchEventContext::TouchEventContext): * dom/EventContext.h: (WebCore::EventContext::closedShadowDepth const): Added. * dom/EventPath.cpp: (WebCore::WindowEventContext::WindowEventContext): (WebCore::EventPath::buildPath): Compute the closed shadow tree's depths for each node in the path. (WebCore::computePathUnclosedToTarget const): Implemented the aforementioned algorithm. (WebCore::EventPath::EventPath): git-svn-id: http://svn.webkit.org/repository/webkit/trunk@236103 268f45cc-cd09-0410-ab3c-d52691b4dbfc

annevk added a commit that referenced this issue Nov 9, 2017

Compute everything needed for composedPath() during dispatch

4b422dc

Fixes #525.

annevk mentioned this issue Nov 9, 2017

Compute everything needed for composedPath() during dispatch #535

Merged

annevk closed this as completed in #535 Nov 21, 2017

annevk added a commit that referenced this issue Nov 21, 2017

Compute everything needed for composedPath() during dispatch

56ca18c

Tests: web-platform-tests/wpt#8385. Fixes #525. (This also changes some formatting to align with Bikeshed changes.)

smaug---- mentioned this issue Jan 10, 2018

'Retargeting steps' expose closed Shadow DOM to the outside world w3c/touch-events#92

Open

event.composedPath() may return different results if DOM is modified between two calls #525

event.composedPath() may return different results if DOM is modified between two calls #525

Comments

smaug---- commented Oct 30, 2017 • edited by nox Loading

smaug---- commented Oct 30, 2017

smaug---- commented Oct 30, 2017

hayatoito commented Oct 31, 2017

hayatoito commented Oct 31, 2017

domenic commented Oct 31, 2017

hayatoito commented Oct 31, 2017

hayatoito commented Oct 31, 2017 • edited Loading

smaug---- commented Oct 31, 2017

hayatoito commented Oct 31, 2017 • edited Loading

rniwa commented Oct 31, 2017

domenic commented Oct 31, 2017

smaug---- commented Oct 31, 2017

hayatoito commented Oct 31, 2017 • edited Loading

domenic commented Oct 31, 2017

annevk commented Oct 31, 2017

domenic commented Oct 31, 2017

annevk commented Oct 31, 2017

domenic commented Oct 31, 2017

smaug---- commented Oct 31, 2017 • edited Loading

hayatoito commented Oct 31, 2017 • edited Loading

smaug---- commented Oct 31, 2017

smaug---- commented Nov 1, 2017

rniwa commented Nov 1, 2017

annevk commented Nov 6, 2017

hayatoito commented Nov 8, 2017 • edited Loading

smaug---- commented Nov 8, 2017

hayatoito commented Nov 8, 2017

smaug---- commented Nov 8, 2017

hayatoito commented Nov 9, 2017 • edited Loading

hayatoito commented Nov 9, 2017 • edited Loading

annevk commented Nov 9, 2017

smaug---- commented Nov 9, 2017

annevk commented Nov 9, 2017

smaug---- commented Nov 9, 2017

annevk commented Nov 9, 2017

smaug---- commented Oct 30, 2017 •

edited by nox

Loading

hayatoito commented Oct 31, 2017 •

edited

Loading

hayatoito commented Oct 31, 2017 •

edited

Loading

hayatoito commented Oct 31, 2017 •

edited

Loading

smaug---- commented Oct 31, 2017 •

edited

Loading

hayatoito commented Oct 31, 2017 •

edited

Loading

hayatoito commented Nov 8, 2017 •

edited

Loading

hayatoito commented Nov 9, 2017 •

edited

Loading

hayatoito commented Nov 9, 2017 •

edited

Loading