nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.44k stars 29.01k forks source link

AbortController.signal.addEventListener does not handle capture option #51703

Open krutoo opened 7 months ago

krutoo commented 7 months ago

Version

v20.11.0

Platform

Linux petrov-dm 6.5.0-15-generic #15~22.04.1-Ubuntu SMP PREEMPT_DYNAMIC Fri Jan 12 18:54:30 UTC 2 x86_64 x86_64 x86_64 GNU/Linux

Subsystem

No response

What steps will reproduce the bug?

Run code:

const controller = new AbortController();

controller.signal.addEventListener('abort', () => {
  console.log('foo');
});

controller.signal.addEventListener(
  'abort',
  () => {
    console.log('bar');
  },
  { capture: true },
);

controller.abort();

How often does it reproduce? Is there a required condition?

No response

What is the expected behavior? Why is that the expected behavior?

expected output (like in firefox):

bar
foo

What do you see instead?

actual output:

foo
bar

Additional information

In Firefox there is properly behavior but i dont know how this described in specifications

climba03003 commented 7 months ago

In chrome, it follows the order of registered event listener.

krutoo commented 7 months ago

@climba03003 yes but I thought that capture: true option should affect listener call order in the same way as in the DOM events

aduh95 commented 7 months ago

In Safari, like Firefox, the output is:

bar
foo

It'd be interesting to pin down what the spec says about this.

/cc @nodejs/web-standards

KhafraDev commented 7 months ago

When set to true, options’s capture prevents callback from being invoked when the event’s eventPhase attribute value is BUBBLING_PHASE. When false (or not present), callback will not be invoked when event’s eventPhase attribute value is CAPTURING_PHASE. Either way, callback will be invoked if event’s eventPhase attribute value is AT_TARGET.

Node doesn't implement bubbling, because there's no tree-like structure that's hierarchical (think about an HTMLElement that can have children/a parent). Even if there was, AbortController/EventTarget has no hierarchy, and should ignore capture regardless.

Furthermore, here are the steps to invoke a callback (a callback is the user-supplied function when using addEventListener): Note that an eventPhase, in node, can only be AT_TARGET or NONE

[2] For each listener in listeners, whose removed is false: [2.4] If phase is "bubbling" and listener’s capture is true, then continue. ... [2.10] Call a user object’s operation with listener’s callback, "handleEvent", « event », and event’s currentTarget attribute value. If this throws an exception, then:

There are a few important things to note here.

  1. addEventListener appends the callback to the list, not prepends
  2. the spec defines For each (step 2 above) as "performing a set of steps on each item in order," the in order is important here.

I'm not an expert at EventTarget, but it would appear as though node is correct, given the differences between browsers and servers. Maybe someone else can chime in who knows more.