Bug 180378

<rdar://problem/42843004>

Created attachment 349272 [details] WIP This is a pretty novel approach. It avoids storing TreeScope or its parent scope.

Attachment 349272 [details] did not pass style-queue: ERROR: Source/WebCore/dom/EventContext.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] ERROR: Source/WebCore/dom/EventPath.cpp:265: More than one command on the same line [whitespace/newline] [4] Total errors found: 2 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 349533 [details] Fixes the bug

Attachment 349533 [details] did not pass style-queue: ERROR: Source/WebCore/dom/EventContext.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 349534 [details] Added more change log description

Attachment 349534 [details] did not pass style-queue: ERROR: Source/WebCore/dom/EventContext.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 349534 [details] Added more change log description View in context: https://bugs.webkit.org/attachment.cgi?id=349534&action=review > Source/WebCore/ChangeLog:13 > + 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 https://github.com/whatwg/dom/issues/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. At least some of what’s described in the change log here seems like it should be a comment in the code. Even something like "keeping track of depths allows us to dispatch the events to the right nodes even when ...". > Source/WebCore/ChangeLog:41 > + ancestors) and downwards (i.e. descendents) from the context object and decrease the *allowed depth* Spelling error: descendants > Source/WebCore/ChangeLog:47 > + 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. Do we have a test case that covers this? > Source/WebCore/dom/EventPath.cpp:114 > + using MakeEventContext = std::unique_ptr<EventContext> (*)(Node&, EventTarget*, EventTarget*, unsigned closedShadowDepth); Maybe this should be int instead of unsigned? > LayoutTests/imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt:3 > -FAIL Event.composedPath() should return the same result even if DOM is mutated (1/2) assert_array_equals: lengths differ, expected 3 got 2 > -FAIL Event.composedPath() should return the same result even if DOM is mutated (2/2) assert_array_equals: lengths differ, expected 5 got 2 > +PASS Event.composedPath() should return the same result even if DOM is mutated (1/2) > +PASS Event.composedPath() should return the same result even if DOM is mutated (2/2) Is this really enough test coverage for the rather-subtle algorithm? Do we have tests covering all edge cases we can think of?

Comment on attachment 349534 [details] Added more change log description View in context: https://bugs.webkit.org/attachment.cgi?id=349534&action=review >> Source/WebCore/ChangeLog:41 >> + ancestors) and downwards (i.e. descendents) from the context object and decrease the *allowed depth* > > Spelling error: descendants Fixed. >> Source/WebCore/ChangeLog:47 >> + and it gets out of its shadow host. > > Do we have a test case that covers this? Yes, I had a bug where I had assumed it to be positive, and a bunch of tests started failing. >> Source/WebCore/dom/EventPath.cpp:114 >> + using MakeEventContext = std::unique_ptr<EventContext> (*)(Node&, EventTarget*, EventTarget*, unsigned closedShadowDepth); > > Maybe this should be int instead of unsigned? Oops, right. Fixed. >> LayoutTests/imported/w3c/web-platform-tests/shadow-dom/event-composed-path-after-dom-mutation-expected.txt:3 >> +PASS Event.composedPath() should return the same result even if DOM is mutated (2/2) > > Is this really enough test coverage for the rather-subtle algorithm? Do we have tests covering all edge cases we can think of? Yes, there are more tests for composed path in complicated shadow trees. e.g. imported/w3c/web-platform-tests/shadow-dom/Extensions-to-Event-Interface.html imported/w3c/web-platform-tests/shadow-dom/event-composed-path-with-related-target.html imported/w3c/web-platform-tests/shadow-dom/event-composed-path.html These tests are specifically testing when the tree is mutated during event dispatching. In theory, we could add more tests but I've artificially introduced bugs into my code, and some test always seems to have my bug so we seem to have a pretty decent test coverage as is.

Comment on attachment 349534 [details] Added more change log description View in context: https://bugs.webkit.org/attachment.cgi?id=349534&action=review >> Source/WebCore/ChangeLog:13 >> + the removed node since it's no longer in the closed shadow tree. > > At least some of what’s described in the change log here seems like it should be a comment in the code. > > Even something like "keeping track of depths allows us to dispatch the events to the right nodes even when ...". Sure, added a comment next to closedShadowDepth as follows: int closedShadowDepth = 0; // Depths are used to decided which nodes are excluded in event.composedPath when the tree is mutated during event dispatching. // They could be negative for nodes outside the shadow tree of the target node. And then to EventPath::computePathUnclosedToTarget, I added the following comments: // Any node whose depth computed in EventPath::buildPath is greater than the context object is excluded. // Because we can exit out of a closed shadow tree and re-enter another closed shadow tree via a slot, // we decrease the *allowed depth* whenever we moved to a "shallower" (closer-to-document) tree.

Thanks for the review! Anne is incorporating this algorithm into the spec in https://github.com/whatwg/dom/issues/684 so perhaps this code becomes more self-evidently correct once that happens.

Created attachment 349994 [details] Patch for landing

Comment on attachment 349994 [details] Patch for landing Wait for EWS.

Attachment 349994 [details] did not pass style-queue: ERROR: Source/WebCore/dom/EventContext.cpp:40: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 1 in 6 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r236103: <https://trac.webkit.org/changeset/236103>