Bug 180378 - Update composedPath to match the latest spec
Summary: Update composedPath to match the latest spec
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2017-12-04 14:10 PST by Ryosuke Niwa
Modified: 2018-09-18 00:47 PDT (History)
10 users (show)

See Also:


Attachments
WIP (10.60 KB, patch)
2018-09-08 18:59 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (16.62 KB, patch)
2018-09-12 02:21 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Added more change log description (16.99 KB, patch)
2018-09-12 02:29 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (17.53 KB, patch)
2018-09-17 20:41 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2017-12-04 14:10:33 PST
Change the implementation per https://github.com/whatwg/dom/issues/525
Comment 1 Radar WebKit Bug Importer 2018-08-01 22:41:38 PDT
<rdar://problem/42843004>
Comment 2 Ryosuke Niwa 2018-09-08 18:59:52 PDT
Created attachment 349272 [details]
WIP

This is a pretty novel approach. It avoids storing TreeScope or its parent scope.
Comment 3 Build Bot 2018-09-08 19:02:10 PDT
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.
Comment 4 Ryosuke Niwa 2018-09-12 02:21:40 PDT
Created attachment 349533 [details]
Fixes the bug
Comment 5 Build Bot 2018-09-12 02:23:29 PDT
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.
Comment 6 Ryosuke Niwa 2018-09-12 02:29:40 PDT
Created attachment 349534 [details]
Added more change log description
Comment 7 Build Bot 2018-09-12 02:31:25 PDT
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 8 Darin Adler 2018-09-14 10:27:27 PDT
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 9 Ryosuke Niwa 2018-09-17 19:47:12 PDT
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 10 Ryosuke Niwa 2018-09-17 20:09:32 PDT
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.
Comment 11 Ryosuke Niwa 2018-09-17 20:10:29 PDT
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.
Comment 12 Ryosuke Niwa 2018-09-17 20:41:57 PDT
Created attachment 349994 [details]
Patch for landing
Comment 13 Ryosuke Niwa 2018-09-17 20:42:10 PDT
Comment on attachment 349994 [details]
Patch for landing

Wait for EWS.
Comment 14 Build Bot 2018-09-17 20:43:15 PDT
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.
Comment 15 Ryosuke Niwa 2018-09-18 00:47:37 PDT
Committed r236103: <https://trac.webkit.org/changeset/236103>