CI: https://ci.nodejs.org/job/node-test-pull-request/34043/

Have we tested them with WPT?

@joyeecheung the problem is the WPT for EventTarget are using Nodes and not EventTargets so they ned to be ported manually - we've done a few of these and manually ported others (e.g. #33621 and a few others).

@joyeecheung I did however, go test-by-test over the WPT, Deno's EventTarget tests, JSDom's and userland tests and our event target is mostly correct (our AbortSignal still needs #35931 but that's relatively minor and I wouldn't block solely on that since it's relatively minor)

I don't feel qualified to make a judgment about the Node.js project's graduation guidance. Web platform tests seem like the right criteria in general, and I'm glad to hear that despite them not being easily runnable, @benjamingr has been keeping up to date on running them.

I noticed when skimming #35931 that your event handler implementation doesn't implement the full complexities of the spec: https://html.spec.whatwg.org/#event-handler-idl-attributes . In particular the activate/deactivate semantics. (Thankfully you can ignore all the ridiculousness in https://html.spec.whatwg.org/#getting-the-current-value-of-the-event-handler since you don't have HTML content attributes, i.e. "internal raw uncompiled handlers".) The note and example below https://html.spec.whatwg.org/#event-handler-event-type discuss the consequences of the spec's complexity, which I think you would be missing with the current implementation.

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

Thanks Domenic!

First to clarify a point:

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

AFAIK workers are not Web Workers and our MessagePort is not a WHATWG message port (yet! Anna is working towards some level of compatibility - @addaleax feel free to correct me here). This is only about graduating EventTarget and AbortController.

I noticed when skimming #35931 that your event handler implementation doesn't implement the full complexities of the spec: https://html.spec.whatwg.org/#event-handler-idl-attributes . In particular the activate/deactivate semantics.

We only recently moved it from a simple onabort property on the object to use the same event handler order so that the order is correct.

Any help in understanding parts we might have missed would be appreciated.

(Our only EventTarget at the moment is AbortController which to the best of my understanding doesn't have activation behaviour. Eventually our MessagePort will be spec compliant, hopefully)

since you don't have HTML content attributes, i.e. "internal raw uncompiled handlers".

Apologies for the possibly dumb question - but is that for example the onclick in <div onclick="someFunction()"> ?

(If so, I believe (hope)indeed we don't have those)

Also onmessage on MessagePort is special and has its own side effects, which I don't believe are implemented based on skimming that PR.

AFAIK workers are not Web Workers and our MessagePort is not a WHATWG message port (yet! Anna is working towards some level of compatibility - @addaleax feel free to correct me here). This is only about graduating EventTarget and AbortController.

Any incompatibility with the HTML spec would be considered a bug here, and MessagePorts are considered a stable API at this point. If the “own side effects” that are being referred to here are about starting the flow of messages when the listener is added, then yes, we implement that.

@addaleax that's great, I thought we were making progress towards compatibility but I didn't think Node actually implements the WHATWG MessagePort spec for message ports. I recall reviewing a bunch of stuff that helps with that but I didn't make the connection.

Props and awesome! 🎉 🙏

We only recently moved it from a simple onabort property on the object to use the same event handler order so that the order is correct. Any help in understanding parts we might have missed would be appreciated.

Well, it might the case that you've just implemented a very concise version of the algorithms at https://html.spec.whatwg.org/#event-handler-idl-attributes (including the linked "deactivate an event handler" and "activate an event handler"). That is, looking at the code it doesn't seem to match up to the spec steps very much, but maybe it has the same observable consequences. I guess the test would be the following:

const { port1: mp } = new MessageChannel();
mp.addEventListener("message", () => console.log("1"));
mp.onmessage = () => console.log("not called");
mp.addEventListener("message", () => console.log("3"));
mp.onmessage = () => console.log("2");
mp.addEventListener("message", () => console.log("4"));

mp.dispatchEvent(new Event("message"));

which should log 1 / 2 / 3 / 4.

Apologies for the possibly dumb question - but is that for example the onclick in <div onclick="someFunction()"> ?

Yes, exactly. Those are a nighmare of complexity :).

which should log 1 / 2 / 3 / 4.

It does log 1 / 3 / 2 / 4, so yes, I assume that’s still a bug here. But this isn’t specific to onmessage, right, it’s a problem for all EventTarget?

It does log 1 / 3 / 2 / 4, so yes, I assume that’s still a bug here. But this isn’t specific to onmessage, right, it’s a problem for all EventTarget?

That sounds like a bug, I think as it's a timing bug it's a big enough to block graduation and I think I just misunderstood the spec at that point.

@addaleax would you mind if I pushed the fix for it + a test to #35931 instead of opening a new PR since it already touches that code ?

It's a pretty simple fix.

(If you prefer a new PR - happy to do that)

Also, I had no idea it worked that way :O

@benjamingr Perfectly fine with me :)

Ok, I'll push a fix tomorrow morning and add a test. I have misunderstood the spec and didn't realize our test (that just checks the order between event handlers and event listeners without overriding the handler) was sufficient.

Edit: fix pushed

@nodejs/tsc Seems like this is a big enough deal that more TSC eyes would be good.

@jasnell is this still waiting for anything atm?

Landed in 513764b