-
Notifications
You must be signed in to change notification settings - Fork 295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
event.composedPath() may return different results if DOM is modified between two calls #525
Comments
Looks like use of 'shadow-including root' in window case is problematic too. |
@hayatoito what does blink do here? @rniwa or webkit? |
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. :( |
I think we should fix the spec if we can agree on that. |
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. |
Yup, that can be a reason we won't change the spec. As far as I can tell, calculating composedPath() is not so cheap. |
One more thing: |
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. So I don't see a reason to not fix this. |
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. :( |
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. |
@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). |
How is that O(depth)? |
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. |
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).
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 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.
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. |
@domenic |
I see. That is disappointing, and means people will be forever creating XObservers and avoiding events just to get performance benefits. |
Rough summary from the IRC debate, @rniwa:
It seems @domenic would prefer a version where event's path can be disregarded. |
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. |
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.) |
@domenic Yup, let me investigate further. I'll answer it back when I get more insights. What I know as of now:
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. |
The current model leads to also very surprising results if one removes a node from closed shadow tree during event dispatch. |
@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. I'm having hard time to see why we'd keep the current model |
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 ;) |
@smaug---- and I tried to sketch out a different model here, review appreciated!
|
I tried to apply the given algorithm to the following DOM trees:
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:
It looks event.composedPath() would return reverse([A, sr1, B]), instead of returning reverse([A, sr1, B, C]). Is my understanding correct? |
It should return A, sr1, B, C. |
Yeah, I guess that's the intention.
Is this condition correct? It checks that targetOverride is non-null, but targetOverride is null for the slot in the path in this case. |
ahaa, that sounds like a bug when my original algorithm was converted. |
Yeah, that makes sense. Thank you! One more thing I have noticed:
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:
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?
|
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. :( |
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. |
Oh, -1 shouldn't happen if slot's closed root is already in the composed path. |
@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? |
ä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? |
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
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
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
The text was updated successfully, but these errors were encountered: