events: improve `EventListener` validation

This fixes validating `EventListener` given to `add/removeEventListener`
to improve the Web API compatibility.

According to the WPT test failure with the current validation,
`addEventListener` should not require `handleEvent` to be defined on
an `EventListener`".  IIUC, the same can be applied to
`removeEventListener` also.

Signed-off-by: Daeyeon Jeong daeyeon.dev@gmail.com

PR-URL: https://github.com/nodejs/node/pull/43491
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

lib/internal/event_target.js

@@ -4,7 +4,6 @@ const {
   ArrayFrom,
   Boolean,
   Error,
-  FunctionPrototypeBind,
   FunctionPrototypeCall,
   NumberIsInteger,
   ObjectAssign,
@@ -381,7 +380,10 @@ class Listener {
       this.callback = listener;
       this.listener = listener;
     } else {
-      this.callback = FunctionPrototypeBind(listener.handleEvent, listener);
+      this.callback = async (...args) => {
+        if (listener.handleEvent)
+          await ReflectApply(listener.handleEvent, listener, args);
+      };
       this.listener = listener;
     }
   }
@@ -463,7 +465,7 @@ class EventTarget {
     if (arguments.length < 2)
       throw new ERR_MISSING_ARGS('type', 'listener');
 
-    // We validateOptions before the shouldAddListeners check because the spec
+    // We validateOptions before the validateListener check because the spec
     // requires us to hit getters.
     const {
       once,
@@ -474,7 +476,7 @@ class EventTarget {
       weak,
     } = validateEventListenerOptions(options);
 
-    if (!shouldAddListener(listener)) {
+    if (!validateEventListener(listener)) {
       // The DOM silently allows passing undefined as a second argument
       // No error code for this since it is a Warning
       // eslint-disable-next-line no-restricted-syntax
@@ -547,7 +549,7 @@ class EventTarget {
   removeEventListener(type, listener, options = kEmptyObject) {
     if (!isEventTarget(this))
       throw new ERR_INVALID_THIS('EventTarget');
-    if (!shouldAddListener(listener))
+    if (!validateEventListener(listener))
       return;
 
     type = String(type);
@@ -856,7 +858,7 @@ ObjectDefineProperties(NodeEventTarget.prototype, {
 
 // EventTarget API
 
-function shouldAddListener(listener) {
+function validateEventListener(listener) {
   if (typeof listener === 'function' ||
       typeof listener?.handleEvent === 'function') {
     return true;
@@ -865,6 +867,11 @@ function shouldAddListener(listener) {
   if (listener == null)
     return false;
 
+  if (typeof listener === 'object') {
+    // Require `handleEvent` lazily.
+    return true;
+  }
+
   throw new ERR_INVALID_ARG_TYPE('listener', 'EventListener', listener);
 }
 

test/parallel/test-eventtarget.js

@@ -232,6 +232,30 @@ let asyncTest = Promise.resolve();
   eventTarget.dispatchEvent(event);
 }
 
+{
+  const target = new EventTarget();
+  const listener = {};
+  // AddEventListener should not require handleEvent to be
+  // defined on an EventListener.
+  target.addEventListener('foo', listener);
+  listener.handleEvent = common.mustCall(function(event) {
+    strictEqual(event.type, 'foo');
+    strictEqual(this, listener);
+  });
+  target.dispatchEvent(new Event('foo'));
+}
+
+{
+  const target = new EventTarget();
+  const listener = {};
+  // do not throw
+  target.removeEventListener('foo', listener);
+  target.addEventListener('foo', listener);
+  target.removeEventListener('foo', listener);
+  listener.handleEvent = common.mustNotCall();
+  target.dispatchEvent(new Event('foo'));
+}
+
 {
   const uncaughtException = common.mustCall((err, origin) => {
     strictEqual(err.message, 'boom');
@@ -308,7 +332,6 @@ let asyncTest = Promise.resolve();
   [
     'foo',
     1,
-    {},  // No handleEvent function
     false,
   ].forEach((i) => throws(() => target.addEventListener('foo', i), err(i)));
 }

test/parallel/test-whatwg-events-eventtarget-this-of-listener.js

@@ -0,0 +1,119 @@
+'use strict';
+
+require('../common');
+const { test, assert_equals, assert_unreached } =
+  require('../common/wpt').harness;
+
+// Manually ported from: https://github.com/web-platform-tests/wpt/blob/6cef1d2087d6a07d7cc6cee8cf207eec92e27c5f/dom/events/EventTarget-this-of-listener.html
+
+// Mock document
+const document = {
+  createElement: () => new EventTarget(),
+  createTextNode: () => new EventTarget(),
+  createDocumentFragment: () => new EventTarget(),
+  createComment: () => new EventTarget(),
+  createProcessingInstruction: () => new EventTarget(),
+};
+
+test(() => {
+  const nodes = [
+    document.createElement('p'),
+    document.createTextNode('some text'),
+    document.createDocumentFragment(),
+    document.createComment('a comment'),
+    document.createProcessingInstruction('target', 'data'),
+  ];
+
+  let callCount = 0;
+  for (const node of nodes) {
+    node.addEventListener('someevent', function() {
+      ++callCount;
+      assert_equals(this, node);
+    });
+
+    node.dispatchEvent(new Event('someevent'));
+  }
+
+  assert_equals(callCount, nodes.length);
+}, 'the this value inside the event listener callback should be the node');
+
+test(() => {
+  const nodes = [
+    document.createElement('p'),
+    document.createTextNode('some text'),
+    document.createDocumentFragment(),
+    document.createComment('a comment'),
+    document.createProcessingInstruction('target', 'data'),
+  ];
+
+  let callCount = 0;
+  for (const node of nodes) {
+    const handler = {};
+
+    node.addEventListener('someevent', handler);
+    handler.handleEvent = function() {
+      ++callCount;
+      assert_equals(this, handler);
+    };
+
+    node.dispatchEvent(new Event('someevent'));
+  }
+
+  assert_equals(callCount, nodes.length);
+}, 'addEventListener should not require handleEvent to be defined on object listeners');
+
+test(() => {
+  const nodes = [
+    document.createElement('p'),
+    document.createTextNode('some text'),
+    document.createDocumentFragment(),
+    document.createComment('a comment'),
+    document.createProcessingInstruction('target', 'data'),
+  ];
+
+  let callCount = 0;
+  for (const node of nodes) {
+    function handler() {
+      ++callCount;
+      assert_equals(this, node);
+    }
+
+    handler.handleEvent = () => {
+      assert_unreached('should not call the handleEvent method on a function');
+    };
+
+    node.addEventListener('someevent', handler);
+
+    node.dispatchEvent(new Event('someevent'));
+  }
+
+  assert_equals(callCount, nodes.length);
+}, 'handleEvent properties added to a function before addEventListener are not reached');
+
+test(() => {
+  const nodes = [
+    document.createElement('p'),
+    document.createTextNode('some text'),
+    document.createDocumentFragment(),
+    document.createComment('a comment'),
+    document.createProcessingInstruction('target', 'data'),
+  ];
+
+  let callCount = 0;
+  for (const node of nodes) {
+    function handler() {
+      ++callCount;
+      assert_equals(this, node);
+    }
+
+    node.addEventListener('someevent', handler);
+
+    handler.handleEvent = () => {
+      assert_unreached('should not call the handleEvent method on a function');
+    };
+
+    node.dispatchEvent(new Event('someevent'));
+  }
+
+  assert_equals(callCount, nodes.length);
+}, 'handleEvent properties added to a function after addEventListener are not reached');