-
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
composedPath() is wrong #684
Comments
FWIW, I have a new algorithm described in https://bugs.webkit.org/show_bug.cgi?id=180378. |
Not sure, I was just working off the comment which says that it doesn't implement the current algorithm. /cc @pmdartus. |
Sorry, you're right, it seems equivalent to the WebKit approach, while also keeping some of the existing structures the specification has (not requiring a change to "get the parent"). Not entirely sure what's better. |
re: Indexed DB - thanks for the heads up. PRs welcome! |
So something has bothered me a bit and I'm not sure to what extent we should try solve it now. Before shadow trees you had node trees and event trees/paths. They were roughly analogous as you would only go up and down. With shadow trees, node trees became more powerful, and some of that leaked into the event API. However, event trees/paths were still as powerful as before. The complexity of shadow trees ended up being a bunch of special cases in event trees/paths, rather than a generic mechanism that, e.g., something like Indexed DB could also use (or The approach I took in #696 moves some of the shadow tree logic into "get the parent" so it's generalized and potentially reusable by other event trees/paths, but not all of it. Is that a good direction to be heading in? (If the answer is no, I should probably do the jsdom approach, which basically used the original dispatch and "get the parent" infrastructure to enable the same thing.) |
Extending get the parent seems rather invasive. If I were you, I would have defined the depth of shadow trees as a separate concept, and simply stored that depth. My implementation in WebKit doesn't use that precise definition of the depth because computing the depth of tree happens to be an expensive operation in WebKit but one could imagine another implementation can cheaply compute the depth of (closed) shadow trees and simply store that information while building up the event path. |
It's invasive, but it would allow non-node trees to have the same capabilities as node trees. Or are you suggesting that depth would be a property of a node? That'd also be somewhat invasive probably. |
@annevk I have no concrete idea to make the situation better. Let me think somehow. |
I don't know if that's all that important. I'm suggesting that the algorithm to build up the event path could simply check if a target is a node, and if so, compute the depth of the closed shadow tree at that point. |
Okay, yeah, that's the status quo. So I guess it doesn't seem weird to you that the event API has node-specific stuff on it that cannot be used in non-node cases? |
Yeah, it's probably not that bad, and the spec would be easier to read. |
FWIW, WebKit's change landed in https://trac.webkit.org/changeset/236103. |
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
The existing algorithm exposed nodes in shadow trees that should remain hidden. (This wasn't noticed in #535.) This roughly matches the approach taken by jsdom to solve this issue and unlike #696 requires no downstream changes. Tests: shadow-dom/event-composed-path.html in wpt. Fixes #684. Closes #696.
@pmdartus discovered in jsdom/jsdom#2347 (comment) that the algorithm as-is exposes closed shadow nodes. This also happens in Firefox.
See
shadow-dom/event-composed-path.html
in wpt.It seems to solve this we'd need to record for each item in an event's path what nesting level it's at and whether it's inside a closed shadow tree?
(This was missed in #535.)
cc @whatwg/components
The text was updated successfully, but these errors were encountered: