fix: Allow evented objects, such as components and plugins, to listen to the window object in addition to DOM objects. (videojs#5255)

misteroneill · gkatsev · commit 7fd29b4f1832 · 2018-06-20T17:05:33.000-04:00
the bug is that objects using the new-ish evented mixin cannot listen to the window object, preventing things like:

```
component.on(window, 'scroll', throttledListener);
```

This fixes that so anything that's a native EventTarget works.
diff --git a/src/js/mixins/evented.js b/src/js/mixins/evented.js
@@ -9,6 +9,17 @@ import * as Fn from '../utils/fn';
 import * as Obj from '../utils/obj';
 import EventTarget from '../event-target';
 
+/**
+ * Returns whether or not an object supports native events.
+ *
+ * @param  {Object} object
+ *         An object to test.
+ *
+ * @return {boolean}
+ *         Whether or not an object supports native events.
+ */
+const supportsNativeEvents = (object) => typeof object.addEventListener === 'function';
+
 /**
  * Returns whether or not an object has had the evented mixin applied.
  *
@@ -50,7 +61,7 @@ const isValidEventType = (type) =>
  *         The object to test.
  */
 const validateTarget = (target) => {
-  if (!target.nodeName && !isEvented(target)) {
+  if (!supportsNativeEvents(target) && !isEvented(target)) {
     throw new Error('Invalid target; must be a DOM node or evented object.');
   }
 };
@@ -154,7 +165,7 @@ const normalizeListenArgs = (self, args) => {
 const listen = (target, method, type, listener) => {
   validateTarget(target);
 
-  if (target.nodeName) {
+  if (supportsNativeEvents(target)) {
     Events[method](target, type, listener);
   } else {
     target[method](type, listener);
@@ -307,7 +318,7 @@ const EventedMixin = {
       // the same guid as the event listener in on().
       this.off('dispose', listener);
 
-      if (target.nodeName) {
+      if (supportsNativeEvents(target)) {
         Events.off(target, type, listener);
         Events.off(target, 'dispose', listener);
       } else if (isEvented(target)) {
@@ -346,7 +357,7 @@ const EventedMixin = {
  * @param  {String} [options.eventBusKey]
  *         By default, adds a `eventBusEl_` DOM element to the target object,
  *         which is used as an event bus. If the target object already has a
- *         DOM element that should be used, pass its key here.
+ *         DOM element which should be used, pass its key here.
  *
  * @return {Object}
  *         The target object.
diff --git a/test/unit/mixins/evented.test.js b/test/unit/mixins/evented.test.js
@@ -1,4 +1,6 @@
 /* eslint-env qunit */
+import document from 'global/document';
+import window from 'global/window';
 import sinon from 'sinon';
 import evented from '../../../src/js/mixins/evented';
 import * as Dom from '../../../src/js/utils/dom';
@@ -11,6 +13,20 @@ const errors = {
   target: new Error('Invalid target; must be a DOM node or evented object.')
 };
 
+// Cross-browser event creation.
+const createEvent = (type) => {
+  let event;
+
+  try {
+    event = new window.Event(type);
+  } catch (x) {
+    event = document.createEvent('Event');
+    event.initEvent(type, true, true);
+  }
+
+  return event;
+};
+
 const validateListenerCall = (call, thisValue, eventExpectation) => {
   const eventActual = call.args[0];
 
@@ -31,7 +47,13 @@ QUnit.module('mixins: evented', {
   },
 
   afterEach() {
-    Object.keys(this.targets).forEach(k => this.targets[k].trigger('dispose'));
+    Object.keys(this.targets).forEach(k => {
+      if (typeof this.targets[k].trigger === 'function') {
+        this.targets[k].trigger('dispose');
+      } else {
+        this.targets[k] = null;
+      }
+    });
   }
 });
 
@@ -61,7 +83,7 @@ QUnit.test('evented() with custom element', function(assert) {
   );
 });
 
-QUnit.test('on() and one() errors', function(assert) {
+QUnit.test('on() and one() error states', function(assert) {
   const target = this.targets.a = evented({});
 
   ['on', 'one'].forEach(method => {
@@ -74,7 +96,7 @@ QUnit.test('on() and one() errors', function(assert) {
   });
 });
 
-QUnit.test('off() errors', function(assert) {
+QUnit.test('off() error states', function(assert) {
   const target = this.targets.a = evented({});
 
   // An invalid event actually causes an invalid target error because it
@@ -86,7 +108,7 @@ QUnit.test('off() errors', function(assert) {
   assert.throws(() => target.off(evented({}), 'x', null), errors.listener, 'the expected error is thrown');
 });
 
-QUnit.test('on() can add a listener to one event type on this object', function(assert) {
+QUnit.test('on() can add a listener to one event type on itself', function(assert) {
   const a = this.targets.a = evented({});
   const spy = sinon.spy();
 
@@ -101,7 +123,7 @@ QUnit.test('on() can add a listener to one event type on this object', function(
   });
 });
 
-QUnit.test('on() can add a listener to an array of event types on this object', function(assert) {
+QUnit.test('on() can add a listener to an array of event types on itself', function(assert) {
   const a = this.targets.a = evented({});
   const spy = sinon.spy();
 
@@ -122,7 +144,7 @@ QUnit.test('on() can add a listener to an array of event types on this object',
   });
 });
 
-QUnit.test('one() can add a listener to one event type on this object', function(assert) {
+QUnit.test('one() can add a listener to one event type on itself', function(assert) {
   const a = this.targets.a = evented({});
   const spy = sinon.spy();
 
@@ -138,7 +160,7 @@ QUnit.test('one() can add a listener to one event type on this object', function
   });
 });
 
-QUnit.test('one() can add a listener to an array of event types on this object', function(assert) {
+QUnit.test('one() can add a listener to an array of event types on itself', function(assert) {
   const a = this.targets.a = evented({});
   const spy = sinon.spy();
 
@@ -161,7 +183,7 @@ QUnit.test('one() can add a listener to an array of event types on this object',
   });
 });
 
-QUnit.test('on() can add a listener to one event type on a different target object', function(assert) {
+QUnit.test('on() can add a listener to one event type on a different evented object', function(assert) {
   const a = this.targets.a = evented({});
   const b = this.targets.b = evented({});
   const spy = sinon.spy();
@@ -180,7 +202,47 @@ QUnit.test('on() can add a listener to one event type on a different target obje
   });
 });
 
-QUnit.test('on() can add a listener to an array of event types on a different target object', function(assert) {
+QUnit.test('on() can add a listener to one event type on a node object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = Dom.createEl('div');
+  const spy = sinon.spy();
+  const event = createEvent('x');
+
+  a.on(b, 'x', spy);
+  b.dispatchEvent(event);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
+QUnit.test('on() can add a listener to one event type on the window object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = window;
+  const spy = sinon.spy();
+  const event = createEvent('x');
+
+  a.on(b, 'x', spy);
+  b.dispatchEvent(event);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
+QUnit.test('on() can add a listener to an array of event types on a different evented object', function(assert) {
   const a = this.targets.a = evented({});
   const b = this.targets.b = evented({});
   const spy = sinon.spy();
@@ -206,13 +268,70 @@ QUnit.test('on() can add a listener to an array of event types on a different ta
   });
 });
 
-QUnit.test('one() can add a listener to one event type on a different target object', function(assert) {
+QUnit.test('on() can add a listener to an array of event types on a node object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = Dom.createEl('div');
+  const spy = sinon.spy();
+  const x = createEvent('x');
+  const y = createEvent('y');
+
+  a.on(b, ['x', 'y'], spy);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+  a.trigger('y');
+
+  assert.strictEqual(spy.callCount, 2, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+
+  validateListenerCall(spy.getCall(1), a, {
+    type: 'y',
+    target: b
+  });
+});
+
+QUnit.test('on() can add a listener to an array of event types on the window object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = window;
+  const spy = sinon.spy();
+  const x = createEvent('x');
+  const y = createEvent('y');
+
+  a.on(b, ['x', 'y'], spy);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+  a.trigger('y');
+
+  assert.strictEqual(spy.callCount, 2, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+
+  validateListenerCall(spy.getCall(1), a, {
+    type: 'y',
+    target: b
+  });
+});
+
+QUnit.test('one() can add a listener to one event type on a different evented object', function(assert) {
   const a = this.targets.a = evented({});
   const b = this.targets.b = evented({});
   const spy = sinon.spy();
 
   a.one(b, 'x', spy);
   b.trigger('x');
+  b.trigger('x');
 
   // Make sure we aren't magically binding a listener to "a".
   a.trigger('x');
@@ -225,9 +344,49 @@ QUnit.test('one() can add a listener to one event type on a different target obj
   });
 });
 
-// The behavior here unfortunately differs from the identical case where "a"
-// listens to itself. This is something that should be resolved...
-QUnit.test('one() can add a listener to an array of event types on a different target object', function(assert) {
+QUnit.test('one() can add a listener to one event type on a node object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = Dom.createEl('div');
+  const spy = sinon.spy();
+  const event = createEvent('x');
+
+  a.one(b, 'x', spy);
+  b.dispatchEvent(event);
+  b.dispatchEvent(event);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
+QUnit.test('one() can add a listener to one event type on the window object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = window;
+  const spy = sinon.spy();
+  const event = createEvent('x');
+
+  a.one(b, 'x', spy);
+  b.dispatchEvent(event);
+  b.dispatchEvent(event);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
+QUnit.test('one() can add a listener to an array of event types on a different evented object', function(assert) {
   const a = this.targets.a = evented({});
   const b = this.targets.b = evented({});
   const spy = sinon.spy();
@@ -250,6 +409,56 @@ QUnit.test('one() can add a listener to an array of event types on a different t
   });
 });
 
+QUnit.test('one() can add a listener to an array of event types on a node object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = Dom.createEl('div');
+  const spy = sinon.spy();
+  const x = createEvent('x');
+  const y = createEvent('y');
+
+  a.one(b, ['x', 'y'], spy);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+  a.trigger('y');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
+QUnit.test('one() can add a listener to an array of event types on the window object', function(assert) {
+  const a = this.targets.a = evented({});
+  const b = this.targets.b = window;
+  const spy = sinon.spy();
+  const x = createEvent('x');
+  const y = createEvent('y');
+
+  a.one(b, ['x', 'y'], spy);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+  b.dispatchEvent(x);
+  b.dispatchEvent(y);
+
+  // Make sure we aren't magically binding a listener to "a".
+  a.trigger('x');
+  a.trigger('y');
+
+  assert.strictEqual(spy.callCount, 1, 'the listener was called the expected number of times');
+
+  validateListenerCall(spy.getCall(0), a, {
+    type: 'x',
+    target: b
+  });
+});
+
 QUnit.test('off() with no arguments will remove all listeners from all events on this object', function(assert) {
   const a = this.targets.a = evented({});
   const spyX = sinon.spy();
