events: improve argument handling, start passive

nodejs · benjamingr · May 29, 2020 · May 29, 2020 · May 29, 2020 · May 29, 2020
commit 056bffb528b846ac964e8a592bf6b62175fb693e
diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -15,7 +15,7 @@ const {
     ERR_INVALID_ARG_TYPE,
     ERR_EVENT_RECURSION,
     ERR_OUT_OF_RANGE,
-    ERR_MISSING_ARGS
+    ERR_MISSING_ARGS,
   }
 } = require('internal/errors');
 
@@ -180,15 +180,23 @@ class EventTarget {
   [kRemoveListener](size, type, listener, capture) {}
 
   addEventListener(type, listener, options = {}) {
-    validateListener(listener);
-    type = String(type);
-
+    if (arguments.length < 2) {
+      throw new ERR_MISSING_ARGS('type', 'listener');
+    }
+    // We validateOptions before the shouldAddListeners check because the spec
+    // requires us to hit getters.
     const {
       once,
       capture,
       passive
     } = validateEventListenerOptions(options);
 
+    if (!shouldAddListener(listener)) {
+      // The DOM silently allows passing undefined as a second argument
+      return;
+    }
+    type = String(type);
+
     let root = this[kEvents].get(type);
 
     if (root === undefined) {
@@ -219,9 +227,11 @@ class EventTarget {
   }
 
   removeEventListener(type, listener, options = {}) {
-    validateListener(listener);
+    if (!shouldAddListener(listener)) {
+      return;
+    }
     type = String(type);
-    const { capture } = validateEventListenerOptions(options);
+    const capture = !!(options?.capture);
     const root = this[kEvents].get(type);
     if (root === undefined || root.next === undefined)
       return;
@@ -402,12 +412,15 @@ Object.defineProperties(NodeEventTarget.prototype, {
 
 // EventTarget API
 
-function validateListener(listener) {
+function shouldAddListener(listener) {
   if (typeof listener === 'function' ||
       (listener != null &&
        typeof listener === 'object' &&
        typeof listener.handleEvent === 'function')) {
-    return;
+    return true;
+  }
+  if (listener === null || listener === undefined) {
+    return false;
   }
   throw new ERR_INVALID_ARG_TYPE('listener', 'EventListener', listener);
 }

diff --git a/test/parallel/test-eventtarget-whatwg-passive.js b/test/parallel/test-eventtarget-whatwg-passive.js
@@ -0,0 +1,71 @@
+'use strict';
+
+const common = require('../common');
+
+const {
+  Event,
+  EventTarget,
+} = require('internal/event_target');
+
+const {
+  fail,
+  ok,
+  strictEqual
+} = require('assert');
+
+// Manually ported from WPT AddEventListenerOptions-passive.html
+{
+  const document = new EventTarget();
+  let supportsPassive = false;
+  const query_options = {
+    get passive() {
+      supportsPassive = true;
+      return false;
+    },
+    get dummy() {
+      fail('dummy value getter invoked');
+      return false;
+    }
+  };
+
+  document.addEventListener('test_event', null, query_options);
+  ok(supportsPassive);
+
+  supportsPassive = false;
+  document.removeEventListener('test_event', null, query_options);
+  strictEqual(supportsPassive, false);
+}
+{
+  function testPassiveValue(optionsValue, expectedDefaultPrevented) {
+    const document = new EventTarget();
+    let defaultPrevented;
+    function handler(e) {
+      if (e.defaultPrevented) {
+        fail('Event prematurely marked defaultPrevented');
+      }
+      e.preventDefault();
+      defaultPrevented = e.defaultPrevented;
+    }
+    document.addEventListener('test', handler, optionsValue);
+    // TODO the WHATWG test is more extensive here and tests dispatching on
+    // document.body, if we ever support getParent we should amend this
+    const ev = new Event('test', { bubbles: true, cancelable: true });
+    const uncanceled = document.dispatchEvent(ev);
+
+    strictEqual(defaultPrevented, expectedDefaultPrevented);
+    strictEqual(uncanceled, !expectedDefaultPrevented);
+
+    document.removeEventListener('test', handler, optionsValue);
+  }
+  testPassiveValue(undefined, true);
+  testPassiveValue({}, true);
+  testPassiveValue({ passive: false }, true);
+
+  common.skip('TODO: passive listeners is still broken');
+  testPassiveValue({ passive: 1 }, false);
+  testPassiveValue({ passive: true }, false);
+  testPassiveValue({ passive: 0 }, true);
+
+
+
+}
diff --git a/test/parallel/test-eventtarget.js b/test/parallel/test-eventtarget.js
@@ -115,6 +115,18 @@ ok(EventTarget);
   const fn = common.mustCall((event) => strictEqual(event, ev));
   eventTarget.addEventListener('foo', fn, { once: true });
   eventTarget.dispatchEvent(ev);
+  const eventTarget = new EventTarget();
+  // single argument throws
+  throws(() => eventTarget.addEventListener('foo'), TypeError);
+  // Null events - does not throw
+  eventTarget.addEventListener('foo', null);
+  eventTarget.removeEventListener('foo', null);
+  eventTarget.addEventListener('foo', undefined);
+  eventTarget.removeEventListener('foo', undefined);
+  // strings, booleans
+  throws(() => eventTarget.addEventListener('foo', 'hello'), TypeError);
+  throws(() => eventTarget.addEventListener('foo', false), TypeError);
+  throws(() => eventTarget.addEventListener('foo', Symbol()), TypeError);
 }
 {
   const eventTarget = new NodeEventTarget();
@@ -309,8 +321,6 @@ ok(EventTarget);
     'foo',
     1,
     {},  // No handleEvent function
-    false,
-    undefined
   ].forEach((i) => {
     throws(() => target.addEventListener('foo', i), {
       code: 'ERR_INVALID_ARG_TYPE'
@@ -321,8 +331,6 @@ ok(EventTarget);
     'foo',
     1,
     {},  // No handleEvent function
-    false,
-    undefined
   ].forEach((i) => {
     throws(() => target.removeEventListener('foo', i), {
       code: 'ERR_INVALID_ARG_TYPE'