From d1498f49681a086b4e776edd4c1b549795ac3bf2 Mon Sep 17 00:00:00 2001 From: Jacob Trimble Date: Fri, 9 Aug 2019 14:01:00 -0700 Subject: [PATCH] Don't add Control event listeners multiple times. Because configure() is called multiple times, we used to add some of the event handlers multiple times. For fullscreen, this caused us to try to toggle fullscreen multiple times. This changes to only add the main event listeners one time and the DOM-specific ones have been moved to be added in the configure() call. This also moves the fullscreen toggle to the try so we can propagate any errors from it. Lastly, this adds an "error" listener to the tests to fail the test. Closes #2054 Issue #2089 Change-Id: Ic78417ce52cae1c53133b5a4217004bb91ebcc4f --- test/test/util/ui_utils.js | 20 ++++++++++ test/ui/ui_unit.js | 21 +++++++++++ ui/controls.js | 76 +++++++++++++++++++------------------- 3 files changed, 79 insertions(+), 38 deletions(-) diff --git a/test/test/util/ui_utils.js b/test/test/util/ui_utils.js index febd34c27..d0d9536f1 100644 --- a/test/test/util/ui_utils.js +++ b/test/test/util/ui_utils.js @@ -31,6 +31,7 @@ shaka.test.UiUtils = class { // Create UI config = config || {}; const ui = new shaka.ui.Overlay(player, videoContainer, video); + ui.getControls().addEventListener('error', (/** * */ e) => fail(e.detail)); ui.configure(config); return ui; } @@ -121,4 +122,23 @@ shaka.test.UiUtils = class { await less.refresh(/* reload */ true, /* modifyVars*/ false, /* clearFileCache */ false); } + + /** + * Simulates a native event (e.g. 'click') on the given element. + * + * @param {EventTarget} target + * @param {string} name + */ + static simulateEvent(target, name) { + const type = { + 'click': 'MouseEvent', + 'dblclick': 'MouseEvent', + }[name] || 'CustomEvent'; + + // Note we can't use the MouseEvent constructor since it isn't supported on + // IE11. + const event = document.createEvent(type); + event.initEvent(name, true, true); + target.dispatchEvent(event); + } }; diff --git a/test/ui/ui_unit.js b/test/ui/ui_unit.js index ac6b30063..80a20edd2 100644 --- a/test/ui/ui_unit.js +++ b/test/ui/ui_unit.js @@ -188,6 +188,27 @@ describe('UI', () => { videoContainer.appendChild(video); }); + it('goes into fullscreen on double click', async () => { + const config = { + controlPanelElements: [ + 'overflow_menu', + ], + overflowMenuButtons: [ + 'quality', + ], + }; + const ui = UiUtils.createUIThroughAPI(videoContainer, video, config); + const controls = ui.getControls(); + + const spy = spyOn(controls, 'toggleFullScreen'); + + const controlsContainer = + videoContainer.querySelector('.shaka-controls-container'); + UiUtils.simulateEvent(controlsContainer, 'dblclick'); + await Util.shortDelay(); + expect(spy).toHaveBeenCalledTimes(1); + }); + describe('all the controls', () => { /** @type {!HTMLElement} */ let controlsContainer; diff --git a/ui/controls.js b/ui/controls.js index 0d178c412..5583fc64a 100644 --- a/ui/controls.js +++ b/ui/controls.js @@ -148,6 +148,7 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { // Configure and create the layout of the controls this.configure(this.config_); + this.addEventListeners_(); /** * The pressed keys set is used to record which keys are currently pressed @@ -368,7 +369,27 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { // Init the play state this.onPlayStateChange_(); - this.addEventListeners_(); + // Elements that should not propagate clicks (controls panel, menus) + const noPropagationElements = this.videoContainer_.getElementsByClassName( + 'shaka-no-propagation'); + for (const element of noPropagationElements) { + const cb = (event) => event.stopPropagation(); + this.eventManager_.listen(element, 'click', cb); + this.eventManager_.listen(element, 'dblclick', cb); + } + + // Keep showing controls if one of those elements is hovered + const showControlsElements = this.videoContainer_.getElementsByClassName( + 'shaka-show-controls-on-mouse-over'); + for (const element of showControlsElements) { + this.eventManager_.listen(element, 'mouseover', () => { + this.overrideCssShowControls_ = true; + }); + + this.eventManager_.listen(element, 'mouseleave', () => { + this.overrideCssShowControls_ = false; + }); + } } /** @@ -574,12 +595,12 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { if (document.pictureInPictureElement) { await document.exitPictureInPicture(); } + await this.videoContainer_.requestFullscreen(); } catch (error) { this.dispatchEvent(new shaka.util.FakeEvent('error', { detail: error, })); } - await this.videoContainer_.requestFullscreen(); } } @@ -687,6 +708,14 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { 'shaka-show-controls-on-mouse-over'); this.bottomControls_.appendChild(this.controlsButtonPanel_); + // Overflow menus are supposed to hide once you click elsewhere + // on the video element. The code in onContainerClick_ ensures that. + // However, clicks on controls panel don't propagate to the container, + // so we have to explicitly hide the menus onclick here. + this.eventManager_.listen(this.controlsButtonPanel_, 'click', () => { + this.hideSettingsMenus(); + }); + // Create the elements specified by controlPanelElements for (const name of this.config_.controlPanelElements) { if (shaka.ui.ControlsPanel.elementNamesToFactories_.get(name)) { @@ -700,7 +729,13 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { } } - /** @private */ + /** + * Adds static event listeners. This should only add event listeners to + * things that don't change (e.g. Player). Dynamic elements (e.g. controls) + * should have their event listeners added when they are created. + * + * @private + */ addEventListeners_() { this.eventManager_.listen(this.player_, 'buffering', () => { this.onBufferingStateChange_(); @@ -715,11 +750,6 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { this.eventManager_.listen( this.controlsContainer_, 'dblclick', () => this.toggleFullScreen()); - // If double-clicking on the bottom bar, don't allow the click to propagate - // to toggle fullscreen above. - this.eventManager_.listen( - this.bottomControls_, 'dblclick', (e) => e.stopPropagation()); - this.eventManager_.listen(this.video_, 'play', () => { this.onPlayStateChange_(); }); @@ -734,28 +764,6 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { this.onPlayStateChange_(); }); - // Elements that should not propagate clicks (controls panel, menus) - const noPropagationElements = this.videoContainer_.getElementsByClassName( - 'shaka-no-propagation'); - for (const element of noPropagationElements) { - this.eventManager_.listen(element, 'click', (event) => { - event.stopPropagation(); - }); - } - - // Keep showing controls if one of those elements is hovered - const showControlsElements = this.videoContainer_.getElementsByClassName( - 'shaka-show-controls-on-mouse-over'); - for (const element of showControlsElements) { - this.eventManager_.listen(element, 'mouseover', () => { - this.overrideCssShowControls_ = true; - }); - - this.eventManager_.listen(element, 'mouseleave', () => { - this.overrideCssShowControls_ = false; - }); - } - this.eventManager_.listen(this.videoContainer_, 'mousemove', (e) => { this.onMouseMove_(e); }); @@ -772,14 +780,6 @@ shaka.ui.Controls = class extends shaka.util.FakeEventTarget { this.onMouseLeave_(); }); - // Overflow menus are supposed to hide once you click elsewhere - // on the video element. The code in onContainerClick_ ensures that. - // However, clicks on controls panel don't propagate to the container, - // so we have to explicitly hide the menus onclick here. - this.eventManager_.listen(this.controlsButtonPanel_, 'click', () => { - this.hideSettingsMenus(); - }); - this.eventManager_.listen(this.castProxy_, 'caststatuschanged', () => { this.onCastStatusChange_(); });