[webkit-reviews] review granted: [Bug 180378] Update composedPath to match the latest spec : [Attachment 349534] Added more change log description

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 180378: Update composedPath to match the latest spec
https://bugs.webkit.org/show_bug.cgi?id=180378

Attachment 349534: Added more change log description

https://bugs.webkit.org/attachment.cgi?id=349534&action=review




--- Comment #8 from Darin Adler <darin at apple.com> ---
Comment on attachment 349534
  --> https://bugs.webkit.org/attachment.cgi?id=349534
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-afte
r-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?