fix: remove event handlers when menu item is removed (#5748)

Author: liuruenshen

src/js/menu/menu.js

@@ -36,6 +36,60 @@ class Menu extends Component {
     this.focusedChild_ = -1;
 
     this.on('keydown', this.handleKeyPress);
+
+    // All the menu item instances share the same blur handler provided by the menu container.
+    this.boundHandleBlur_ = Fn.bind(this, this.handleBlur);
+    this.boundHandleTapClick_ = Fn.bind(this, this.handleTapClick);
+  }
+
+  /**
+   * Add event listeners to the {@link MenuItem}.
+   *
+   * @param {Object} component
+   *        The instance of the `MenuItem` to add listeners to.
+   *
+   */
+  addEventListenerForItem(component) {
+    if (!(component instanceof Component)) {
+      return;
+    }
+
+    component.on('blur', this.boundHandleBlur_);
+    component.on(['tap', 'click'], this.boundHandleTapClick_);
+  }
+
+  /**
+   * Remove event listeners from the {@link MenuItem}.
+   *
+   * @param {Object} component
+   *        The instance of the `MenuItem` to remove listeners.
+   *
+   */
+  removeEventListenerForItem(component) {
+    if (!(component instanceof Component)) {
+      return;
+    }
+
+    component.off('blur', this.boundHandleBlur_);
+    component.off(['tap', 'click'], this.boundHandleTapClick_);
+  }
+
+  /**
+   * This method will be called indirectly when the component has been added
+   * before the component adds to the new menu instance by `addItem`.
+   * In this case, the original menu instance will remove the component
+   * by calling `removeChild`.
+   *
+   * @param {Object} component
+   *        The instance of the `MenuItem`
+   */
+  removeChild(component) {
+    if (typeof component === 'string') {
+      component = this.getChild(component);
+    }
+
+    this.removeEventListenerForItem(component);
+    super.removeChild(component);
   }
 
   /**
@@ -46,20 +100,11 @@ class Menu extends Component {
    *
    */
   addItem(component) {
-    this.addChild(component);
-    component.on('blur', Fn.bind(this, this.handleBlur));
-    component.on(['tap', 'click'], Fn.bind(this, function(event) {
-      // Unpress the associated MenuButton, and move focus back to it
-      if (this.menuButton_) {
-        this.menuButton_.unpressButton();
-
-        // don't focus menu button if item is a caption settings item
-        // because focus will move elsewhere
-        if (component.name() !== 'CaptionSettingsMenuItem') {
-          this.menuButton_.focus();
-        }
-      }
-    }));
+    const childComponent = this.addChild(component);
+
+    if (childComponent) {
+      this.addEventListenerForItem(childComponent);
+    }
   }
 
   /**
@@ -96,6 +141,8 @@ class Menu extends Component {
 
   dispose() {
     this.contentEl_ = null;
+    this.boundHandleBlur_ = null;
+    this.boundHandleTapClick_ = null;
 
     super.dispose();
   }
@@ -123,6 +170,39 @@ class Menu extends Component {
     }
   }
 
+  /**
+   * Called when a `MenuItem` gets clicked or tapped.
+   *
+   * @param {EventTarget~Event} event
+   *        The `click` or `tap` event that caused this function to be called.
+   *
+   * @listens click,tap
+   */
+  handleTapClick(event) {
+    // Unpress the associated MenuButton, and move focus back to it
+    if (this.menuButton_) {
+      this.menuButton_.unpressButton();
+
+      const childComponents = this.children();
+
+      if (!Array.isArray(childComponents)) {
+        return;
+      }
+
+      const foundComponent = childComponents.filter(component => component.el() === event.target)[0];
+
+      if (!foundComponent) {
+        return;
+      }
+
+      // don't focus menu button if item is a caption settings item
+      // because focus will move elsewhere
+      if (foundComponent.name() !== 'CaptionSettingsMenuItem') {
+        this.menuButton_.focus();
+      }
+    }
+  }
+
   /**
    * Handle a `keydown` event on this menu. This listener is added in the constructor.
    *

test/unit/menu.test.js

@@ -1,8 +1,12 @@
 /* eslint-env qunit */
+import * as DomData from '../../src/js/utils/dom-data';
 import MenuButton from '../../src/js/menu/menu-button.js';
+import Menu from '../../src/js/menu/menu.js';
+import CaptionSettingsMenuItem from '../../src/js/control-bar/text-track-controls/caption-settings-menu-item';
 import MenuItem from '../../src/js/menu/menu-item.js';
 import TestHelpers from './test-helpers.js';
 import * as Events from '../../src/js/utils/events.js';
+import sinon from 'sinon';
 
 QUnit.module('MenuButton');
 
@@ -106,3 +110,88 @@ QUnit.test('should keep all the added menu items', function(assert) {
   assert.ok(menuButton.el().contains(menuItem1.el()), 'the menu button contains the DOM element of `menuItem1` after second update');
   assert.ok(menuButton.el().contains(menuItem2.el()), 'the menu button contains the DOM element of `menuItem2` after second update');
 });
+
+QUnit.test('should remove old event listeners when the menu item adds to the new menu', function(assert) {
+  const player = TestHelpers.makePlayer();
+  const menuButton = new MenuButton(player, {});
+  const oldMenu = new Menu(player, { menuButton });
+  const newMenu = new Menu(player, { menuButton });
+
+  oldMenu.addItem('MenuItem');
+
+  const menuItem = oldMenu.children()[0];
+
+  assert.ok(menuItem instanceof MenuItem, '`menuItem` should be the instanceof of `MenuItem`');
+
+  /**
+   * A reusable collection of assertions.
+   */
+  function validateMenuEventListeners(watchedMenu) {
+    const eventData = DomData.getData(menuItem.eventBusEl_);
+    // `MenuButton`.`unpressButton` will be called when triggering click event on the menu item.
+    const unpressButtonSpy = sinon.spy(menuButton, 'unpressButton');
+    // `MenuButton`.`focus` will be called when triggering click event on the menu item.
+    const focusSpy = sinon.spy(menuButton, 'focus');
+
+    // `Menu`.`children` will be called when triggering blur event on the menu item.
+    const menuChildrenSpy = sinon.spy(watchedMenu, 'children');
+
+    // The number of blur listeners is two because `ClickableComponent`
+    // adds the blur event listener during the construction and
+    // `MenuItem` inherits from `ClickableComponent`.
+    assert.strictEqual(eventData.handlers.blur.length, 2, 'the number of blur listeners is two');
+    // Same reason mentioned above.
+    assert.strictEqual(eventData.handlers.click.length, 2, 'the number of click listeners is two');
+
+    const blurListenerAddedByMenu = eventData.handlers.blur[1];
+    const clickListenerAddedByMenu = eventData.handlers.click[1];
+
+    assert.strictEqual(
+      typeof blurListenerAddedByMenu.calledOnce,
+      'undefined',
+      'previous blur listener wrapped in the spy should be removed'
+    );
+
+    assert.strictEqual(
+      typeof clickListenerAddedByMenu.calledOnce,
+      'undefined',
+      'previous click listener wrapped in the spy should be removed'
+    );
+
+    const blurListenerSpy = eventData.handlers.blur[1] = sinon.spy(blurListenerAddedByMenu);
+    const clickListenerSpy = eventData.handlers.click[1] = sinon.spy(clickListenerAddedByMenu);
+
+    TestHelpers.triggerDomEvent(menuItem.el(), 'blur');
+
+    assert.ok(blurListenerSpy.calledOnce, 'blur event listener should be called');
+    assert.strictEqual(blurListenerSpy.getCall(0).args[0].target, menuItem.el(), 'event target should be the `menuItem`');
+    assert.ok(menuChildrenSpy.calledOnce, '`watchedMenu`.`children` has been called');
+
+    TestHelpers.triggerDomEvent(menuItem.el(), 'click');
+
+    assert.ok(clickListenerSpy.calledOnce, 'click event listener should be called');
+    assert.strictEqual(clickListenerSpy.getCall(0).args[0].target, menuItem.el(), 'event target should be the `menuItem`');
+    assert.ok(unpressButtonSpy.calledOnce, '`menuButton`.`unpressButtion` has been called');
+    assert.ok(focusSpy.calledOnce, '`menuButton`.`focus` has been called');
+
+    unpressButtonSpy.restore();
+    focusSpy.restore();
+    menuChildrenSpy.restore();
+  }
+
+  validateMenuEventListeners(oldMenu);
+
+  newMenu.addItem(menuItem);
+  validateMenuEventListeners(newMenu);
+
+  const focusSpy = sinon.spy(menuButton, 'focus');
+  const captionMenuItem = new CaptionSettingsMenuItem(player, {
+    kind: 'subtitles'
+  });
+
+  newMenu.addItem(captionMenuItem);
+  TestHelpers.triggerDomEvent(captionMenuItem.el(), 'click');
+  assert.ok(!focusSpy.called, '`menuButton`.`focus` should never be called');
+
+  focusSpy.restore();
+});