From 259ce71ee707ce93f4868ce0db8ee7405d76ab44 Mon Sep 17 00:00:00 2001 From: Liu Ruen-shen Date: Sat, 26 Jan 2019 03:26:36 +0800 Subject: [PATCH] fix: remove event handlers when menu item is removed (#5748) --- src/js/menu/menu.js | 106 ++++++++++++++++++++++++++++++++++++----- test/unit/menu.test.js | 89 ++++++++++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 13 deletions(-) diff --git a/src/js/menu/menu.js b/src/js/menu/menu.js index 389940956..f7c44e7cd 100644 --- a/src/js/menu/menu.js +++ b/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(); + const childComponent = this.addChild(component); - // don't focus menu button if item is a caption settings item - // because focus will move elsewhere - if (component.name() !== 'CaptionSettingsMenuItem') { - this.menuButton_.focus(); - } - } - })); + 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. * diff --git a/test/unit/menu.test.js b/test/unit/menu.test.js index bc72f613d..4c772eba2 100644 --- a/test/unit/menu.test.js +++ b/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(); +});