[webkit-reviews] review granted: [Bug 174288] Capturing event listeners are called during bubbling phase for shadow hosts : [Attachment 349718] Updated iOS expected results

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Ryosuke Niwa <rniwa at webkit.org>'s
request for review:
Bug 174288: Capturing event listeners are called during bubbling phase for
shadow hosts
https://bugs.webkit.org/show_bug.cgi?id=174288

Attachment 349718: Updated iOS expected results

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




--- Comment #20 from Darin Adler <darin at apple.com> ---
Comment on attachment 349718
  --> https://bugs.webkit.org/attachment.cgi?id=349718
Updated iOS expected results

View in context: https://bugs.webkit.org/attachment.cgi?id=349718&action=review

> Source/WebCore/ChangeLog:50
> +	   To implement this behavior, this patch introduces
EventTarget::EventInvokePhase indicating whether we're
> +	   in the capturing phase or bubbling phase to
EventTarget::fireEventListeners. We can't use Event's eventPhase
> +	   enum because that's set to Event::Phase::AT_TARGET when we're at a
shadow host.

Are you sure we want to pass this as an argument? It seems we could put this
inside the Event without making Event bigger; we’d just need two sub-flavors of
AT_TARGET, which would require only a fraction of a bit more (1 bit in practice
since phase takes 2 bits now). We’d make some change to setEventPhase so we
could have two different "at target" states, but make sure that eventPhase
still returned Event::Phase::AT_TARGET for both flavors. Then have a separate
getter to indicate bubbling vs. capturing. I like that this goes with the flow
of the idea of storing the phase inside the event and it means fewer code
changes.

>
LayoutTests/fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shado
w-trees.html:47
> +    assert_object_equals(logs, [
> +	   ['capturing', Event.CAPTURING_PHASE, 'target', 'test1',
composedPath],
> +	   ['capturing', Event.CAPTURING_PHASE, 'target', 'parent',
composedPath],
> +	   ['capturing', Event.AT_TARGET, 'target', 'target', composedPath],
> +	   ['bubbling', Event.AT_TARGET, 'target', 'target', composedPath],
> +	   ['bubbling', Event.BUBBLING_PHASE, 'target', 'parent',
composedPath],
> +	   ['bubbling', Event.BUBBLING_PHASE, 'target', 'test1', composedPath],
> +    ]);

Would be nice if more of the test results were visible rather than just a PASS.
A single assertion for this entire array doesn’t seem great from a
"understanding the test output" point of view.

> LayoutTests/imported/w3c/ChangeLog:8
> +	   *
web-platform-tests/dom/events/EventTarget-dispatchEvent-expected.txt:

This doesn’t list all the files.

>
LayoutTests/imported/w3c/web-platform-tests/dom/events/Event-dispatch-handlers-
changed-expected.txt:2
> -PASS  Dispatch additional events inside an event listener  
> +FAIL  Dispatch additional events inside an event listener 
assert_array_equals: actual_targets lengths differ, expected 16 got 17

I guess we are waiting for an updated version of the tests with appropriate
expected results in the tests. It’s a bit annoying to be checking things in in
this newly failing state but knowing that it’s a progression.

>
LayoutTests/platform/ios/imported/w3c/web-platform-tests/dom/events/EventTarget
-dispatchEvent-expected.txt:28
> -PASS Event listeners should be called in order of addition 
> +PASS Event listeners should be +FAIL Event listeners should be called in
order of addition assert_array_equals: property 1, expected 2 but got 3

This looks strange; is this really the expected result?