Bug 174288

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>