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.
Here is a native web components example: http://jsfiddle.net/4pn4x4j1/5/ Credits to Elliott of the Polymer team for this
<rdar://problem/33530455>
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.
Filed a spec issue in https://github.com/whatwg/dom/issues/685
Created attachment 349618 [details] WIP
Created attachment 349621 [details] WIP2
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
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
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).
Oh, interesting. media controls is relying on the registration order invocation. That's kind of ironic...
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
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
Created attachment 349642 [details] Implements the new behavior
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
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
I guess iOS has its own expected result, which needs rebaselinkng.
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
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
Created attachment 349718 [details] Updated iOS expected results
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?
(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.
Created attachment 349728 [details] Patch for landing
Comment on attachment 349728 [details] Patch for landing Wait for EWS
Committed r236002: <https://trac.webkit.org/changeset/236002>