events: improve argument handling, start passive

Co-authored-by: Benjamin Gruenbaum <[email protected]> PR-URL: #34015 Reviewed-By: Denys Otrishko <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]>
nodejs · Jun 27, 2020 · c34f474 · c34f474
1 parent ea1a2d7
commit c34f474
Show file tree

Hide file tree

Showing 4 changed files with 319 additions and 157 deletions.
diff --git a/lib/internal/event_target.js b/lib/internal/event_target.js
@@ -203,15 +203,31 @@ 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
+      // No error code for this since it is a Warning
+      // eslint-disable-next-line no-restricted-syntax
+      const w = new Error(`addEventListener called with ${listener}` +
+                          ' which has no effect.');
+      w.name = 'AddEventListenerArgumentTypeWarning';
+      w.target = this;
+      w.type = type;
+      process.emitWarning(w);
+      return;
+    }
+    type = String(type);
+
     let root = this[kEvents].get(type);
 
     if (root === undefined) {
@@ -242,9 +258,15 @@ class EventTarget {
   }
 
   removeEventListener(type, listener, options = {}) {
-    validateListener(listener);
+    if (!shouldAddListener(listener))
+      return;
+
     type = String(type);
-    const { capture } = validateEventListenerOptions(options);
+    // TODO(@jasnell): If it's determined this cannot be backported
+    // to 12.x, then this can be simplified to:
+    //   const capture = Boolean(options?.capture);
+    const capture = options != null && options.capture === true;
+
     const root = this[kEvents].get(type);
     if (root === undefined || root.next === undefined)
       return;
@@ -426,13 +448,17 @@ 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)
+    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,69 @@
+// Flags: --expose-internals
+'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);
+}