Bug 180378 – Update composedPath to match the latest spec

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Ryosuke Niwa]

Change the implementation per https://github.com/whatwg/dom/issues/525

[Radar WebKit Bug Importer]

<rdar://problem/42843004>

[Ryosuke Niwa]

Created attachment 349272 [details]
WIP

This is a pretty novel approach. It avoids storing TreeScope or its parent scope.

[EWS Watchlist]

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.

[Ryosuke Niwa]

Created attachment 349533 [details]
Fixes the bug

[EWS Watchlist]

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.

[Ryosuke Niwa]

Created attachment 349534 [details]
Added more change log description

[EWS Watchlist]

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.

[Darin Adler]

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?

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

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.

[Ryosuke Niwa]

Created attachment 349994 [details]
Patch for landing

[Ryosuke Niwa]

Comment on attachment 349994 [details]
Patch for landing

Wait for EWS.

[EWS Watchlist]

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.

[Ryosuke Niwa]

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