From 37040e6804ce9f62a23a32e3205306ea4d16e473 Mon Sep 17 00:00:00 2001 From: Sarah Rimron-Soutter Date: Thu, 24 Aug 2023 10:23:03 +0100 Subject: [PATCH 1/4] Throttle ckick handler --- src/js/clickable-component.js | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js index d14810d0f8..2a8cc71d7e 100644 --- a/src/js/clickable-component.js +++ b/src/js/clickable-component.js @@ -5,6 +5,7 @@ import Component from './component'; import * as Dom from './utils/dom.js'; import log from './utils/log.js'; import keycode from 'keycode'; +import { throttle } from './utils/fn'; /** * Component which is clickable or keyboard actionable, but is not a @@ -41,9 +42,11 @@ class ClickableComponent extends Component { this.controlText(this.options_.controlText); } + const throttledClick = throttle(this.handleClick.bind(this), 50); + this.handleMouseOver_ = (e) => this.handleMouseOver(e); this.handleMouseOut_ = (e) => this.handleMouseOut(e); - this.handleClick_ = (e) => this.handleClick(e); + this.handleClick_ = (e) => throttledClick(e); this.handleKeyDown_ = (e) => this.handleKeyDown(e); this.emitTapEvents(); From 0e6e778407f721f4db98ce5716d5d2500bb8bba0 Mon Sep 17 00:00:00 2001 From: Sarah Rimron-Soutter Date: Wed, 27 Sep 2023 14:41:27 +0100 Subject: [PATCH 2/4] Implement option to throttle ClickableComponent - Wait time is customisable - Default wait time is 50ms - Add unit test --- src/js/clickable-component.js | 17 ++++++- test/unit/clickable-component.test.js | 64 ++++++++++++++++++++++++++- 2 files changed, 78 insertions(+), 3 deletions(-) diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js index 2a8cc71d7e..42dce66c0f 100644 --- a/src/js/clickable-component.js +++ b/src/js/clickable-component.js @@ -33,6 +33,10 @@ class ClickableComponent extends Component { * @param {string} [options.className] * A class or space separated list of classes to add the component * + * @param {number | boolean} [options.throttle] + * A throttle will be applied to the clickHander if the number is >= 1 or the value is `true` + * A number specifies the desired wait time in ms or a default wait of 50ms will be applied + * */ constructor(player, options) { @@ -42,11 +46,20 @@ class ClickableComponent extends Component { this.controlText(this.options_.controlText); } - const throttledClick = throttle(this.handleClick.bind(this), 50); + const selectClickHandler = () => { + if (typeof this.options_.throttle === 'number' || this.options_.throttle === true) { + const wait = typeof this.options_.throttle === 'number' ? parseInt(this.options_.throttle, 10) : 50; + + return throttle(this.handleClick.bind(this), wait); + } + return this.handleClick.bind(this); + }; + + const selectedClickHander = selectClickHandler(); this.handleMouseOver_ = (e) => this.handleMouseOver(e); this.handleMouseOut_ = (e) => this.handleMouseOut(e); - this.handleClick_ = (e) => throttledClick(e); + this.handleClick_ = (e) => selectedClickHander(e); this.handleKeyDown_ = (e) => this.handleKeyDown(e); this.emitTapEvents(); diff --git a/test/unit/clickable-component.test.js b/test/unit/clickable-component.test.js index 12f5d67fb5..030bc7a6da 100644 --- a/test/unit/clickable-component.test.js +++ b/test/unit/clickable-component.test.js @@ -1,9 +1,18 @@ /* eslint-env qunit */ +import { useFakeTimers } from 'sinon'; import ClickableComponent from '../../src/js/clickable-component.js'; import TestHelpers from './test-helpers.js'; import * as Events from '../../src/js/utils/events.js'; -QUnit.module('ClickableComponent'); +QUnit.module('ClickableComponent', { + beforeEach() { + this.clock = useFakeTimers(); + }, + + afterEach() { + this.clock.restore(); + } +}); QUnit.test('should create a div with role="button"', function(assert) { assert.expect(2); @@ -154,3 +163,56 @@ QUnit.test('class and text should be settable from options', function(assert) { testClickableComponent.dispose(); player.dispose(); }); + +QUnit.test('should respect throttle option', function(assert) { + assert.expect(8); + let clicks = 0; + + class ThrottlableComponent extends ClickableComponent { + constructor(player, options) { + super(player, options); + } + handleClick() { + clicks++; + } + } + + const player = TestHelpers.makePlayer({}); + const noThrottleComponent = new ThrottlableComponent(player); + const throttledComponent = new ThrottlableComponent(player, {throttle: true}); + const customThrottledComponent1 = new ThrottlableComponent(player, {throttle: 10 }); + const customThrottledComponent2 = new ThrottlableComponent(player, {throttle: 0 }); + const noThrottleEl = noThrottleComponent.el(); + const throttledEl = throttledComponent.el(); + const customThrottledEl1 = customThrottledComponent1.el(); + const customThrottledEl2 = customThrottledComponent2.el(); + + const testThrottledClicks = (el, wait, elName) => { + clicks = 0; + // We need to wait for the durarion of the throttle wait + // parameter before proceeding to test throttled functions + this.clock.tick(wait); + Events.trigger(el, 'click'); + assert.equal(clicks, 1, `${elName}: First click is handled`); + Events.trigger(el, 'click'); + assert.equal(clicks, 1, `${elName}: Second click is ignored`); + // allow time before next click + this.clock.tick(wait); + Events.trigger(el, 'click'); + assert.equal(clicks, 2, `${elName}: third click is handled`); + }; + + // 2 instantaneous clicks on non-throttled el + Events.trigger(noThrottleEl, 'click'); + Events.trigger(noThrottleEl, 'click'); + assert.equal(clicks, 2, 'click on enabled ClickableComponent is handled twice'); + + clicks = 0; + // 0 wait is not throttled + Events.trigger(customThrottledEl2, 'click'); + Events.trigger(customThrottledEl2, 'click'); + assert.equal(clicks, 2, 'click on enabled ClickableComponent with wait of 0 is handled twice'); + + testThrottledClicks(throttledEl, 50, 'default wait'); + testThrottledClicks(customThrottledEl1, 10, '10ms wait'); +}); From f19689034b06e8f6ed9f6551532299ce5706e21e Mon Sep 17 00:00:00 2001 From: Sarah Rimron-Soutter Date: Wed, 27 Sep 2023 15:18:55 +0100 Subject: [PATCH 3/4] Ensure "toggle" controls are throttled --- src/js/control-bar/fullscreen-toggle.js | 1 + src/js/control-bar/mute-toggle.js | 1 + src/js/control-bar/picture-in-picture-toggle.js | 1 + src/js/control-bar/play-toggle.js | 1 + test/unit/controls.test.js | 3 +++ 5 files changed, 7 insertions(+) diff --git a/src/js/control-bar/fullscreen-toggle.js b/src/js/control-bar/fullscreen-toggle.js index dfcf3ae0c3..d73b23d464 100644 --- a/src/js/control-bar/fullscreen-toggle.js +++ b/src/js/control-bar/fullscreen-toggle.js @@ -22,6 +22,7 @@ class FullscreenToggle extends Button { * The key/value store of player options. */ constructor(player, options) { + options = Object.assign({}, options, {throttle: true}); super(player, options); this.setIcon('fullscreen-enter'); this.on(player, 'fullscreenchange', (e) => this.handleFullscreenChange(e)); diff --git a/src/js/control-bar/mute-toggle.js b/src/js/control-bar/mute-toggle.js index 86bf03ebf2..d563b6b142 100644 --- a/src/js/control-bar/mute-toggle.js +++ b/src/js/control-bar/mute-toggle.js @@ -24,6 +24,7 @@ class MuteToggle extends Button { * The key/value store of player options. */ constructor(player, options) { + options = Object.assign({}, options, {throttle: true}); super(player, options); // hide this control if volume support is missing diff --git a/src/js/control-bar/picture-in-picture-toggle.js b/src/js/control-bar/picture-in-picture-toggle.js index 1a81f5a74f..76f5c402dc 100644 --- a/src/js/control-bar/picture-in-picture-toggle.js +++ b/src/js/control-bar/picture-in-picture-toggle.js @@ -26,6 +26,7 @@ class PictureInPictureToggle extends Button { * @listens Player#leavepictureinpicture */ constructor(player, options) { + options = Object.assign({}, options, {throttle: true}); super(player, options); this.setIcon('picture-in-picture-enter'); diff --git a/src/js/control-bar/play-toggle.js b/src/js/control-bar/play-toggle.js index bb22ba9ee2..f4b70795cf 100644 --- a/src/js/control-bar/play-toggle.js +++ b/src/js/control-bar/play-toggle.js @@ -22,6 +22,7 @@ class PlayToggle extends Button { * The key/value store of player options. */ constructor(player, options = {}) { + options = Object.assign({}, options, {throttle: true}); super(player, options); // show or hide replay icon diff --git a/test/unit/controls.test.js b/test/unit/controls.test.js index ddad47931e..313954190a 100644 --- a/test/unit/controls.test.js +++ b/test/unit/controls.test.js @@ -19,6 +19,9 @@ import sinon from 'sinon'; QUnit.module('Controls', { beforeEach(assert) { this.clock = sinon.useFakeTimers(); + // because some click events are throttled we need to tick the clock + // forward for test click events to be handled + this.clock.tick(50); }, afterEach(assert) { this.clock.restore(); From c4aedef0bc9f5b203d4dd5bae5a90c0e4f0e3f91 Mon Sep 17 00:00:00 2001 From: Sarah Rimron-Soutter Date: Fri, 29 Sep 2023 11:27:04 +0100 Subject: [PATCH 4/4] Implement feedback - fix typos - remove `parseInt` - DRY --- src/js/clickable-component.js | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/js/clickable-component.js b/src/js/clickable-component.js index 790a1786c7..6c09621729 100644 --- a/src/js/clickable-component.js +++ b/src/js/clickable-component.js @@ -34,7 +34,7 @@ class ClickableComponent extends Component { * A class or space separated list of classes to add the component * * @param {number | boolean} [options.throttle] - * A throttle will be applied to the clickHander if the number is >= 1 or the value is `true` + * A throttle will be applied to the clickHandler if the number is >= 1 or the value is `true` * A number specifies the desired wait time in ms or a default wait of 50ms will be applied * */ @@ -46,20 +46,22 @@ class ClickableComponent extends Component { this.controlText(this.options_.controlText); } + const throttleIsNumber = typeof this.options_.throttle === 'number'; + const boundClick = this.handleClick.bind(this); const selectClickHandler = () => { - if (typeof this.options_.throttle === 'number' || this.options_.throttle === true) { - const wait = typeof this.options_.throttle === 'number' ? parseInt(this.options_.throttle, 10) : 50; + if (throttleIsNumber || this.options_.throttle === true) { + const wait = throttleIsNumber ? this.options_.throttle : 50; - return throttle(this.handleClick.bind(this), wait); + return throttle(boundClick, wait); } - return this.handleClick.bind(this); + return boundClick; }; - const selectedClickHander = selectClickHandler(); + const selectedClickHandler = selectClickHandler(); this.handleMouseOver_ = (e) => this.handleMouseOver(e); this.handleMouseOut_ = (e) => this.handleMouseOut(e); - this.handleClick_ = (e) => selectedClickHander(e); + this.handleClick_ = (e) => selectedClickHandler(e); this.handleKeyDown_ = (e) => this.handleKeyDown(e); this.emitTapEvents();