Dom: Dispatching events section may not match browsers

[cdumez]

I found this out while investigating this WPT test:

• http://w3c-test.org/dom/events/EventTarget-dispatchEvent.html (last check: "Event listeners should be called in order of addition")

This check is passing in Chrome and Firefox and failing in Safari so I investigated it. The test looks like so:

  var results = []
  var b = document.createElement("b")
  b.addEventListener("x", function() {
    results.push(1)
  }, true)
  b.addEventListener("x", function() {
    results.push(2)
  }, false)
  b.addEventListener("x", function() {
    results.push(3)
  }, true)
  b.dispatchEvent(new Event("x"))
  assert_array_equals(results, [1, 2, 3])

Chrome and Firefox get result === [1, 2, 3]. Safari gets result = [1, 3, 2].

Either I am misinterpreting the DOM spec or WebKit's implementation actually matches the specification though, which would be sad.

Notice that the second event listener has capturing=false while the other ones have capturing=true.

Per DOM Spec [1]:

• Step 14 says to call "invoke" with phase="capturing" for every struct in event path, in reverse order.
• Step 15 says to call "invoke" with phase="bubbling" for every struct in event path

"invoke" ends up calling "Inner invoke" [2]. Notice steps 3 and 4 of "inner invoke":

• If phase is "capturing" and listener’s capture is false, then continue.
• If phase is "bubbling" and listener’s capture is true, then continue.

So with my understanding of the specification, the order the event listeners were added could not possibly be maintained here when dispatching the event since some of them use capturing=true and some other use capturing=false.

I think Blink may be using the event's phase (which may be AT_TARGET) in its "inner invoke" implementation, instead of the phase passed to the "inner invoke" algorithm which is either "capturing" or "bubbling" but never at_target.

If I am misinterpreting the specification, please let me know. If not, then I would not be opposed to aligning WebKit with Blink/Gecko as long as the specification gets updated accordingly.

[1] https://dom.spec.whatwg.org/#dispatching-events (steps 13 & 14)
[2] https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke

[cdumez]

cc @Ms2ger who wrote this test.
cc @annevk

[Ms2ger]

From a quick look, your analysis seems correct, but I'll let @annevk double-check.

[dtapuska]

I recall looking at this before when @Ms2ger first added it. If I recall correctly the Event dispatched isn't a bubbling event. It seems Safari is dispatching events in the bubbling phase for non-bubbling events if I recall.

[cdumez]

@dtapuska is right that WebKit fires the event at the "bubbling phase" (Step 14 of https://dom.spec.whatwg.org/#dispatching-events), even though the event does not bubble. However, the only reason we do this is because we're at the target. We also set the event's phase to AT_TARGET before firing.

Note that if WebKit did not do this, then we'd get result=[1, 3] in the WPT test and we'd be even more different compared to Gecko and Blink.

I guess this shows that WebKit does not even fully match the spec and does something a little different, closer to Firefox and Gecko, with a different ordering though.

[cdumez]

I think the spec would probably match Gecko / Blink if the "inner invoke" [1] relied on the event's phase, instead of the "phase" passed to the algorithm.

Could people from these engines confirm?

[1] https://dom.spec.whatwg.org/#concept-event-listener-inner-invoke (steps 3 & 4)

[cdumez]

Actually, this may not be enough, I think we may end up firing the event at the target twice, once at
step 13 and then at step 14 of https://dom.spec.whatwg.org/#dispatching-events.

It is likely Blink instead does:

1. Fire at capturing phase
2. Fire at target
3. Fire at bubbling phase if event bubbles

[cdumez]

Made a first attempt at patching WebKit to align event dispatching with Blink:
https://bugs.webkit.org/show_bug.cgi?id=196498

[cdumez]

cc @rniwa.

[dtapuska]

@cdumez Yes blink does 3 phases... take a look here: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/dom/events/event_dispatcher.cc?l=195&rcl=073e0c1f7f459cbb3b3405c41c9e4818a8d2ea24

[rniwa]

See https://github.com/whatwg/dom/issues/685. WebKit's new behavior is intended. We just need to fix WPT tests.

[rniwa]

I thought Blink also fixed this behavior but I guess not for the actual target? That seems inconsistent though. Ideally, we'd always use capturing -> bubbling order.

[rniwa]

Oh, there is a bug in the spec. Step 14 of dispatching event only runs when the event bubbles but that's the step supposed to invoke bubbling event listeners on the target so we'd have to run that even when the bubbles attribute is false.

[cdumez]

And it looks like the following WPT tests need to be updated too then:
dom/events/Event-dispatch-handlers-changed.html
dom/events/EventTarget-dispatchEvent.html (last check)

[cdumez]

And it looks like the following WPT tests need to be updated too then:
dom/events/Event-dispatch-handlers-changed.html
dom/events/EventTarget-dispatchEvent.html (last check)

Updating the last one via https://github.com/web-platform-tests/wpt/pull/16230
I have a harder time wrapping my head around the first test though. It looks like there is an extra listener getting called in WebKit?

[dtapuska]

cc @hayatoito

[hayatoito]

cc @chrishtr

[hayatoito]

In #685, it looks that some of us were not aware that the spec change will have an effect on the order of calling event listeners on the actual target, as well as shadow hosts which are not the actual target. Actually, it seems I didn't realize the impact on the actual target by the spec change there. :(

If we change the order of calling event listeners on the actual target, web compatibility concern would be bigger than we expected, I'm afraid.

[hayatoito]

I'd like to know whether Gekko's current behavior is intentional or not.

cc: @annevk

If that's unintentional one and Gekko updates the behavior, I think Blink will try to align the behavior as well, though web compatibility concern would be much bigger than I expected in #685.

[hayatoito]

Filed an issue for Blink: https://bugs.chromium.org/p/chromium/issues/detail?id=949432

[annevk]

Reading through this I think this ends up being a duplicate of #742. (Thankfully, as if you had discovered another problem I'd worry a bit more about the change made for #685.)

Changing the event listener order for targets was discussed in that issue though, e.g., https://github.com/whatwg/dom/issues/685#issuecomment-419518613 mentions it quite explicitly, but tests might have been lacking a bit?

If y'all agree that https://github.com/whatwg/dom/issues/742#issuecomment-476558030 fixes this let's close this as a duplicate.

[cdumez]

I think @annevk is right that https://github.com/whatwg/dom/issues/742 covers the spec bug.
I just landed my PR to update dom/events/EventTarget-dispatchEvent.html accordingly.

It would be good if someone more familiar with this could help figure out why Firefox/Chrome and Safari behave differently on dom/events/Event-dispatch-handlers-changed.html though. It seems related to this issue but somehow I seem to get an extra firing in WebKit so it does not seem to be a pure ordering issue.

[annevk]

Okay, so the problem there is that we copy the event listeners for a target twice, because there's now "capturing" and "bubbling" invocations of "invoke" (_phase_). This means that the event listener added during "capturing" ends up firing during "bubbling". That also means Safari is correct per the new algorithm and it also means this is still a duplicate of #742 as far as I can tell.

[annevk]

@cdumez did you have a chance to go over my analysis? Anyone else?

My position is that Safari has the correct model, we should fix the test to match Safari, and we should fix the specification per my outline in #742. I'm happy to take on that work, but it'd be good to at least have some indication it's not going to be in vain.

[cdumez]

@rniwa is more familiar with this area but I agree with @annevk 's analysis. We should fix the remaining WPT test and #742.

[annevk]

Created an update for the test, closing this as a duplicate of #742.

[josepharhar]

I'm implementing the WebKit behavior in chromium now.
As Hayato mentioned in this comment, wouldn't this be a big enough change to break websites? @rniwa did implementing this in WebKit really have no negative impact on websites?

[annevk]

I haven't heard anything from either @cdumez or @rniwa or anyone else from the WebKit team to that effect, FWIW. You have to write pretty specific listener code to determine the difference (which is also why very few existing tests were impacted by this change).

[josepharhar]

Furthermore, is the change in behavior really justified? It looks like #685, which motivated this change, is about a case which involves shadowdom. Why change the non-shadowdom behavior which has presumably been stable for a very long time?

I'm not opposed, but I just want a good reason before I potentially raise some eyebrows with this change.

[annevk]

The reason for the change is to avoid exposing shadow trees due to how event listeners are invoked. Without this change you could detect shadow trees, which is counter to the point of that feature. Another way of saying this is that we want consistent handling of listeners regardless of whether a node has an associated shadow root.

[josepharhar]

How do you detect shadow trees without this change? I used the following html to try to see the difference but I got the same result for each element.

<div id=closedparent>
  <div id=closedhost></div>
</div>

<div id=openparent>
  <div id=openhost></div>
</div>

<div id=div></div>

<script>
const closedroot = closedhost.attachShadow({mode: 'closed'});
const closedrootchild = document.createElement('div');
closedroot.appendChild(closedrootchild);

const openroot = openhost.attachShadow({mode: 'open'});
const openrootchild = document.createElement('div');
openroot.appendChild(openrootchild);

console.log('closedparent:');
test(closedparent);

console.log('closedhost:');
test(closedhost);

console.log('closedrootchild:');
test(closedrootchild);

console.log('openparent:');
test(openparent);

console.log('openhost:');
test(openhost);

console.log('openrootchild:');
test(openrootchild);

console.log('div:');
test(div);

function test(element) {
  element.addEventListener('click', () => {
    console.log('[bubble ] 1');
  }, {once: true});
  element.addEventListener('click', () => {
    console.log('[capture] 2');
  }, {capture: true, once: true});
  element.addEventListener('click', () => {
    console.log('[bubble ] 3');
  }, {once: true});
  element.addEventListener('click', () => {
    console.log('[capture] 4');
  }, {capture: true, once: true});

  element.click();
}
</script>

[rniwa]

@josepharhar Could you go over https://github.com/whatwg/dom/issues/685 first or talk to people who previously worked on web components at Google? This is nothing personal and I don't want to be a pain or rude, but I really don't have a time to explain this and other web components related issues and previously agreed-upon solutions to every new person who comes across them.

[annevk]

@josepharhar you want to dispatch the event on a node in the shadow tree and observe the listener difference on the shadow host with dispatching the event on the shadow host directly.