Skip to content
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.

Sign up for GitHub

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

Jump to bottom

composedPath() is wrong #684

Closed
annevk opened this issue Aug 28, 2018 · 16 comments
Closed

composedPath() is wrong #684

annevk opened this issue Aug 28, 2018 · 16 comments
Labels
topic: events topic: shadow Relates to shadow trees (as defined in DOM)

Comments

@annevk
Copy link
Member

annevk commented Aug 28, 2018

@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
@annevk annevk added topic: shadow Relates to shadow trees (as defined in DOM) topic: events labels Aug 28, 2018
@rniwa
Copy link
Collaborator

rniwa commented Sep 12, 2018

FWIW, I have a new algorithm described in https://bugs.webkit.org/show_bug.cgi?id=180378.

@domenic
Copy link
Member

domenic commented Sep 12, 2018

It may be worth comparing to https://github.com/jsdom/jsdom/blob/aa8a1a624e277e3a37597352f95f73938ca4e38d/lib/jsdom/living/events/Event-impl.js#L83

annevk added a commit that referenced this issue Sep 13, 2018
@annevk
Fix composedPath() once more
d2fa21b 
The existing algorithm exposed nodes in shadow trees that should remain hidden. (This wasn't noticed in #535.)

This changes "get the parent" to return a tuple and therefore requires downstream changes in at least Indexed DB.

Tests: ...

Fixes #684.
@annevk annevk mentioned this issue Sep 13, 2018
Closed
@annevk
Copy link
Member Author

annevk commented Sep 13, 2018

I created a fix for this in #696, but note that this also requires changes to Indexed DB. I can make those if we can agree on the approach.

@domenic that seems to be basically the existing algorithm, which has the bug, no?

cc @inexorabletash

@domenic
Copy link
Member

domenic commented Sep 13, 2018

Not sure, I was just working off the comment which says that it doesn't implement the current algorithm. /cc @pmdartus.

@annevk
Copy link
Member Author

annevk commented Sep 13, 2018

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.

@pmdartus
Copy link
Member

You are right @annevk, the algorithms are sensibly the same.

As far as I understand @rniwa's algorithm the shadow depth is computed in advance and added to the path, while in the jsdom implementation shadow depth is computed in the getComposedPath.

@inexorabletash
Copy link
Member

re: Indexed DB - thanks for the heads up. PRs welcome!

@annevk
Copy link
Member Author

annevk commented Sep 14, 2018

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 new EventTarget() once it exposes "get the parent").

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.)

@rniwa
Copy link
Collaborator

rniwa commented Sep 14, 2018

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.

@annevk
Copy link
Member Author

annevk commented Sep 14, 2018

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.

@hayatoito
Copy link
Member

@annevk
Yeah, it is a little difficult for me to review the event dispatch steps and composed path steps in the standard. One of the reasons to me is Blink uses the different algorithm (but it should have the equivalent result), so I can't be confident with the correctness of each steps in the standard. :(

I have no concrete idea to make the situation better. Let me think somehow.

@annevk annevk mentioned this issue Sep 14, 2018
Open
@rniwa
Copy link
Collaborator

rniwa commented Sep 14, 2018

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.

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.

@annevk
Copy link
Member Author

annevk commented Sep 14, 2018

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?

@rniwa
Copy link
Collaborator

rniwa commented Sep 18, 2018

Yeah, it's probably not that bad, and the spec would be easier to read.

@rniwa
Copy link
Collaborator

rniwa commented Sep 18, 2018

FWIW, WebKit's change landed in https://trac.webkit.org/changeset/236103.

kisg pushed a commit to paul99/webkit-mips that referenced this issue Sep 18, 2018
Update composedPath to match the latest spec
d3d8678 
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 Sep 18, 2018
@annevk
Fix composedPath() once more
60b4d4a 
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: ...

Fixes #684.
@annevk annevk mentioned this issue Sep 18, 2018
Merged
@annevk
Copy link
Member Author

annevk commented Sep 18, 2018

I created an alternative PR that effectively copies @pmdartus's approach in jsdom: #699 (with minor tweaks). That would not affect "get the parent" or Indexed DB and keeps dispatch rather special.

annevk added a commit that referenced this issue Nov 27, 2018
@annevk
Fix composedPath() once more
23ec642 
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
topic: events topic: shadow Relates to shadow trees (as defined in DOM)
Milestone
No milestone
Development

No branches or pull requests
6 participants
@hayatoito @rniwa @domenic @inexorabletash @annevk @pmdartus