Bug 174288 - Capturing event listeners are called during bubbling phase for shadow hosts
Summary: Capturing event listeners are called during bubbling phase for shadow hosts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: Safari 10
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2017-07-07 19:44 PDT by Michael Stramel
Modified: 2020-12-17 09:32 PST (History)
10 users (show)

See Also:


Attachments
WIP (13.37 KB, patch)
2018-09-12 20:31 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
WIP2 (13.62 KB, patch)
2018-09-12 20:49 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-sierra (3.10 MB, application/zip)
2018-09-12 22:54 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.43 MB, application/zip)
2018-09-13 00:56 PDT, EWS Watchlist
no flags Details
Implements the new behavior (38.21 KB, patch)
2018-09-13 01:08 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews125 for ios-simulator-wk2 (2.59 MB, application/zip)
2018-09-13 05:03 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews106 for mac-sierra-wk2 (3.49 MB, application/zip)
2018-09-13 16:42 PDT, EWS Watchlist
no flags Details
Updated iOS expected results (38.87 KB, patch)
2018-09-13 17:38 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Patch for landing (38.17 KB, patch)
2018-09-13 19:39 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 Michael Stramel 2017-07-07 19:44:22 PDT
Steps to reproduce the problem:
Here are 3 JsFiddles that show the confusion...

Standard DOM: http://jsfiddle.net/stramel/joqqf2qz/2/  (Fires as expected)
Shadow DOM (Nested): http://jsfiddle.net/stramel/4pn4x4j1/ (Fires reverse expected)
Shadow DOM (Slotted): http://jsfiddle.net/stramel/03fum0gc/ (Fires as expected)

What is the expected behavior?
Parent Capture listener should fire before the child capture listener

What went wrong?
Child fires before the parent when using nested elements and Shadow dom

Did this work before? N/A 

Does this work in other browsers? Yes
 Using the `useCapture` event listener on a child and parent in conjunction with Shadow DOM causes the child to fire before the parent.
Comment 1 Michael Stramel 2017-07-07 20:05:09 PDT
Here is a native web components example: http://jsfiddle.net/4pn4x4j1/5/

Credits to Elliott of the Polymer team for this
Comment 2 Radar WebKit Bug Importer 2017-07-25 20:27:07 PDT
<rdar://problem/33530455>
Comment 3 Ryosuke Niwa 2018-09-05 21:37:57 PDT
As far as I can tell, the current behavior of WebKit is in accordance with the spec. It is indeed counter-intuitive. What Gecko implements is probably what we want but it's not what the spec says.
Comment 4 Ryosuke Niwa 2018-09-05 21:57:44 PDT
Filed a spec issue in https://github.com/whatwg/dom/issues/685
Comment 5 Ryosuke Niwa 2018-09-12 20:31:44 PDT
Created attachment 349618 [details]
WIP
Comment 6 Ryosuke Niwa 2018-09-12 20:49:22 PDT
Created attachment 349621 [details]
WIP2
Comment 7 EWS Watchlist 2018-09-12 22:54:30 PDT
Comment on attachment 349621 [details]
WIP2

Attachment 349621 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/9199358

New failing tests:
imported/w3c/web-platform-tests/dom/events/Event-dispatch-handlers-changed.html
imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent.html
media/media-load-event.html
media/modern-media-controls/airplay-support/airplay-support.html
Comment 8 EWS Watchlist 2018-09-12 22:54:32 PDT
Created attachment 349631 [details]
Archive of layout-test-results from ews112 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-sierra  Platform: Mac OS X 10.12.6
Comment 9 Ryosuke Niwa 2018-09-13 00:01:51 PDT
This patch seems to affect media controls because now capturing event listeners on the media elements will be invoked before capturing event listeners inside a shadow root is invoked (capturing event listeners on the ancestors of media elements would have been invoked before capturing/at-target event listeners in the shadow trees so any bug this patch will introduce was likely a bug before this patch as well).
Comment 10 Ryosuke Niwa 2018-09-13 00:11:10 PDT
Oh, interesting. media controls is relying on the registration order invocation. That's kind of ironic...
Comment 11 EWS Watchlist 2018-09-13 00:56:14 PDT
Comment on attachment 349621 [details]
WIP2

Attachment 349621 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9200138

New failing tests:
imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent.html
media/media-load-event.html
imported/w3c/web-platform-tests/dom/events/Event-dispatch-handlers-changed.html
Comment 12 EWS Watchlist 2018-09-13 00:56:16 PDT
Created attachment 349640 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 13 Ryosuke Niwa 2018-09-13 01:08:17 PDT
Created attachment 349642 [details]
Implements the new behavior
Comment 14 EWS Watchlist 2018-09-13 05:03:28 PDT
Comment on attachment 349642 [details]
Implements the new behavior

Attachment 349642 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9201622

New failing tests:
imported/w3c/web-platform-tests/dom/events/EventTarget-dispatchEvent.html
Comment 15 EWS Watchlist 2018-09-13 05:03:30 PDT
Created attachment 349654 [details]
Archive of layout-test-results from ews125 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews125  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 16 Ryosuke Niwa 2018-09-13 14:21:18 PDT
I guess iOS has its own expected result, which needs rebaselinkng.
Comment 17 EWS Watchlist 2018-09-13 16:42:51 PDT
Comment on attachment 349642 [details]
Implements the new behavior

Attachment 349642 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9207261

New failing tests:
accessibility/smart-invert-reference.html
Comment 18 EWS Watchlist 2018-09-13 16:42:53 PDT
Created attachment 349713 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6
Comment 19 Ryosuke Niwa 2018-09-13 17:38:10 PDT
Created attachment 349718 [details]
Updated iOS expected results
Comment 20 Darin Adler 2018-09-13 18:41:38 PDT
Comment on attachment 349718 [details]
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-shadow-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?
Comment 21 Ryosuke Niwa 2018-09-13 19:37:07 PDT
(In reply to Darin Adler from comment #20)
> Comment on attachment 349718 [details]
> 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.

If we're going in that route, we could just check just add eventPhaseForBindings which returns AT_TARGET when target() == currentTarget().
But I think this approach would more closely match the spec.

> > LayoutTests/fast/shadow-dom/capturing-and-bubbling-event-listeners-across-shadow-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.

When this assertion fails, it would spit out which property had a mismatching entry, and its values (it won't just be "[object Array]" but the serialized array values). Unfortunately, testharness.js doesn't really have a way of outputting multiple assertions.

> > LayoutTests/imported/w3c/ChangeLog:8
> > +        * web-platform-tests/dom/events/EventTarget-dispatchEvent-expected.txt:
> 
> This doesn’t list all the files.

Oh weird, the line for Event-dispatch-handlers-changed-expected.txt is somehow in LayoutTests/ChangeLog. Moved it here.

> > 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.

Yeah, the spec change is awaiting an implementation change, which is this patch.

> > 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?

Yes, the test case looks like this:
  var results = []
  var b = document.createElement("b")
  b.addEventListener("x", this.step_func(function() {
    results.push(1)
  }), true)
  b.addEventListener("x", this.step_func(function() {
    results.push(2)
  }), false)
  b.addEventListener("x", this.step_func(function() {
    results.push(3)
  }), true)
  b.dispatchEvent(new Event("x"))
  assert_array_equals(results, [1, 2, 3])
Because we're not dispatching capturing event listeners first, 1, 3 would be called before 2.
Comment 22 Ryosuke Niwa 2018-09-13 19:39:16 PDT
Created attachment 349728 [details]
Patch for landing
Comment 23 Ryosuke Niwa 2018-09-13 19:40:57 PDT
Comment on attachment 349728 [details]
Patch for landing

Wait for EWS
Comment 24 Ryosuke Niwa 2018-09-13 22:56:21 PDT
Committed r236002: <https://trac.webkit.org/changeset/236002>