From a0d5c96687751af325b557124eb52fe5a9a6cde6 Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 10:15:44 -0500 Subject: [PATCH 1/8] Improve mute button usability --- src/js/control-bar/mute-toggle.js | 10 +++- .../control-bar/volume-control/volume-bar.js | 36 ++++++++++++- src/js/player.js | 50 +++++++++++++++++++ test/unit/controls.test.js | 41 +++++++++++++++ 4 files changed, 135 insertions(+), 2 deletions(-) diff --git a/src/js/control-bar/mute-toggle.js b/src/js/control-bar/mute-toggle.js index c7d211d777..1279cd0bf3 100644 --- a/src/js/control-bar/mute-toggle.js +++ b/src/js/control-bar/mute-toggle.js @@ -53,7 +53,15 @@ class MuteToggle extends Button { * @listens click */ handleClick(event) { - this.player_.muted(this.player_.muted() ? false : true); + const vol = this.player_.volume(); + const lastVolume = this.player_.lastVolume(); + + if (vol === 0) { + this.player_.volume(lastVolume); + this.player_.muted(false); + } else { + this.player_.muted(this.player_.muted() ? false : true); + } } /** diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index 45d96484de..db153fe906 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -25,7 +25,8 @@ class VolumeBar extends Slider { */ constructor(player, options) { super(player, options); - + this.on('slideractive', this.updateVolumeBeforeDrag); + this.on('sliderinactive', this.updateLastVolume); this.on(player, 'volumechange', this.updateARIAAttributes); player.ready(() => this.updateARIAAttributes); } @@ -112,6 +113,39 @@ class VolumeBar extends Slider { this.el_.setAttribute('aria-valuetext', volume + '%'); } + /** + * Store value of volume in `volumeBeforeDrag` when user begins dragging the + * VolumeBar (so long as volume is above zero). This is used to restore + * volume to its starting level after dragging volume to zero and clicking + * MuteToggle. + * + * @listens slideractive + */ + updateVolumeBeforeDrag() { + const vol = this.player_.volume(); + + if (vol > 0) { + this.player_.volumeBeforeDrag(vol); + } + } + + /** + * After user finishes dragging the VolumeBar, if volume is zero, set + * `lastVolume` (the value used in setting the volume after clicking + * MuteToggle when volume is zero) to `volumeBeforeDrag`, so that volume will + * be restored to its starting level before the drag. + * + * @listens sliderinactive + */ + updateLastVolume() { + const vol = this.player_.volume(); + const volumeBeforeDrag = this.player_.volumeBeforeDrag(); + + if (vol === 0) { + this.player_.lastVolume(volumeBeforeDrag); + } + } + } /** diff --git a/src/js/player.js b/src/js/player.js index 458ec998fb..4a957872c9 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -335,6 +335,10 @@ class Player extends Component { // Set controls this.controls_ = !!options.controls; + // Set default values for lastVolume and volumeBeforeDrag + this.lastVolume_ = 1; + this.volumeBeforeDrag_ = 1; + // Original tag settings stored in options // now remove immediately so native controls don't flash. // May be turned back on by HTML5 tech if nativeControlsForTouch is true @@ -1812,6 +1816,10 @@ class Player extends Component { this.cache_.volume = vol; this.techCall_('setVolume', vol); + if (vol > 0) { + this.lastVolume(vol); + } + return; } @@ -1872,6 +1880,48 @@ class Player extends Component { return this.techGet_('defaultMuted') || false; } + /** + * Get the last volume, or set it + * + * @param {number} [percentAsDecimal] + * The new last volume as a decimal percent: + * - 0 is muted/0%/off + * - 1.0 is 100%/full + * - 0.5 is half volume or 50% + * + * @return {Player|number} + * a reference to the calling player when setting and the + * current volume as a percent when getting + */ + lastVolume(percentAsDecimal) { + if (percentAsDecimal !== undefined) { + this.lastVolume_ = percentAsDecimal; + return this; + } + return this.lastVolume_; + } + + /** + * Get the last volume before dragging the VolumeBar, or set it + * + * @param {number} [percentAsDecimal] + * The new last volume as a decimal percent: + * - 0 is muted/0%/off + * - 1.0 is 100%/full + * - 0.5 is half volume or 50% + * + * @return {Player|number} + * a reference to the calling player when setting and the + * current volume as a percent when getting + */ + volumeBeforeDrag(percentAsDecimal) { + if (percentAsDecimal !== undefined) { + this.volumeBeforeDrag_ = percentAsDecimal; + return this; + } + return this.volumeBeforeDrag_; + } + /** * Check if current tech can support native fullscreen * (e.g. with built in controls like iOS, so not our flash swf) diff --git a/test/unit/controls.test.js b/test/unit/controls.test.js index 6bcc2be318..b10b7799f5 100644 --- a/test/unit/controls.test.js +++ b/test/unit/controls.test.js @@ -92,3 +92,44 @@ QUnit.test('Fullscreen control text should be correct when fullscreenchange is t QUnit.equal(fullscreentoggle.controlText(), 'Fullscreen', 'Control Text is correct while switching back to normal mode'); player.dispose(); }); + +QUnit.test('Clicking MuteToggle when volume is above 0 should toggle muted property and not change volume', function(assert) { + const player = TestHelpers.makePlayer({ techOrder: ['html5'] }); + const muteToggle = new MuteToggle(player); + + assert.equal(player.volume(), 1, 'volume is above 0'); + assert.equal(player.muted(), false, 'player is muted'); + + muteToggle.handleClick(); + + assert.equal(player.volume(), 1, 'volume is same'); + assert.equal(player.muted(), true, 'player is not muted'); +}); + +QUnit.test('Clicking MuteToggle when volume is 0 and muted is false should set volume to lastVolume and keep muted false', function(assert) { + const player = TestHelpers.makePlayer({ techOrder: ['html5'] }); + const muteToggle = new MuteToggle(player); + + player.volume(0); + assert.equal(player.lastVolume(), 1, 'lastVolume is set'); + assert.equal(player.muted(), false, 'player is muted'); + + muteToggle.handleClick(); + + assert.equal(player.volume(), 1, 'volume is set to lastVolume'); + assert.equal(player.muted(), false, 'muted remains false'); +}); + +QUnit.test('Clicking MuteToggle when volume is 0 and muted is true should set volume to lastVolume and sets muted to false', function(assert) { + const player = TestHelpers.makePlayer({ techOrder: ['html5'] }); + const muteToggle = new MuteToggle(player); + + player.volume(0); + player.muted(true); + player.lastVolume(0.5); + + muteToggle.handleClick(); + + assert.equal(player.volume(), 0.5, 'volume is set to lastVolume'); + assert.equal(player.muted(), false, 'muted is set to false'); +}); From b2212a12803b341c7d01ed70a5c0f36f7cce17be Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 10:27:26 -0500 Subject: [PATCH 2/8] Fix text in test --- test/unit/controls.test.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/unit/controls.test.js b/test/unit/controls.test.js index b10b7799f5..fdff35bb3d 100644 --- a/test/unit/controls.test.js +++ b/test/unit/controls.test.js @@ -98,12 +98,12 @@ QUnit.test('Clicking MuteToggle when volume is above 0 should toggle muted prope const muteToggle = new MuteToggle(player); assert.equal(player.volume(), 1, 'volume is above 0'); - assert.equal(player.muted(), false, 'player is muted'); + assert.equal(player.muted(), false, 'player is not muted'); muteToggle.handleClick(); assert.equal(player.volume(), 1, 'volume is same'); - assert.equal(player.muted(), true, 'player is not muted'); + assert.equal(player.muted(), true, 'player is muted'); }); QUnit.test('Clicking MuteToggle when volume is 0 and muted is false should set volume to lastVolume and keep muted false', function(assert) { From 0095835a6ea398d30b7bae563cd1f71200e4077c Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 11:18:14 -0500 Subject: [PATCH 3/8] Simplify code to set lastVolume --- .../control-bar/volume-control/volume-bar.js | 38 ++++++------------- src/js/player.js | 21 ---------- 2 files changed, 11 insertions(+), 48 deletions(-) diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index db153fe906..7183300835 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -25,8 +25,7 @@ class VolumeBar extends Slider { */ constructor(player, options) { super(player, options); - this.on('slideractive', this.updateVolumeBeforeDrag); - this.on('sliderinactive', this.updateLastVolume); + this.on('slideractive', this.updateLastVolume); this.on(player, 'volumechange', this.updateARIAAttributes); player.ready(() => this.updateARIAAttributes); } @@ -114,36 +113,21 @@ class VolumeBar extends Slider { } /** - * Store value of volume in `volumeBeforeDrag` when user begins dragging the - * VolumeBar (so long as volume is above zero). This is used to restore - * volume to its starting level after dragging volume to zero and clicking - * MuteToggle. + * When user starts dragging the VolumeBar, store the volume and listen for + * the end of the drag. When the drag ends, if the volume was set to zero, + * set lastVolume to the stored volume. * * @listens slideractive */ - updateVolumeBeforeDrag() { - const vol = this.player_.volume(); - - if (vol > 0) { - this.player_.volumeBeforeDrag(vol); - } - } - - /** - * After user finishes dragging the VolumeBar, if volume is zero, set - * `lastVolume` (the value used in setting the volume after clicking - * MuteToggle when volume is zero) to `volumeBeforeDrag`, so that volume will - * be restored to its starting level before the drag. - * - * @listens sliderinactive - */ updateLastVolume() { - const vol = this.player_.volume(); - const volumeBeforeDrag = this.player_.volumeBeforeDrag(); + const volumeBeforeDrag = this.player_.volume(); - if (vol === 0) { - this.player_.lastVolume(volumeBeforeDrag); - } + this.on('sliderinactive', function setLastVolume() { + if (this.player_.volume() === 0) { + this.player_.lastVolume(volumeBeforeDrag); + } + this.off('sliderinactive', setLastVolume); + }); } } diff --git a/src/js/player.js b/src/js/player.js index 4a957872c9..51989be220 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -1901,27 +1901,6 @@ class Player extends Component { return this.lastVolume_; } - /** - * Get the last volume before dragging the VolumeBar, or set it - * - * @param {number} [percentAsDecimal] - * The new last volume as a decimal percent: - * - 0 is muted/0%/off - * - 1.0 is 100%/full - * - 0.5 is half volume or 50% - * - * @return {Player|number} - * a reference to the calling player when setting and the - * current volume as a percent when getting - */ - volumeBeforeDrag(percentAsDecimal) { - if (percentAsDecimal !== undefined) { - this.volumeBeforeDrag_ = percentAsDecimal; - return this; - } - return this.volumeBeforeDrag_; - } - /** * Check if current tech can support native fullscreen * (e.g. with built in controls like iOS, so not our flash swf) From ad8dd5d4368e572c2fa9c754f5c50e3357d0eaea Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 11:18:47 -0500 Subject: [PATCH 4/8] lastVolume can never be set to 0 After the changes to how lastVolume is set, dragging the VolumeBar to 0 and then clicking on the leftmost part of the volumebar would result in lastVolume being set to 0. This prevents that from happening. --- src/js/player.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/js/player.js b/src/js/player.js index 51989be220..9fb37b8d5c 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -1894,7 +1894,7 @@ class Player extends Component { * current volume as a percent when getting */ lastVolume(percentAsDecimal) { - if (percentAsDecimal !== undefined) { + if (percentAsDecimal !== undefined && percentAsDecimal !== 0) { this.lastVolume_ = percentAsDecimal; return this; } From 3bb6419916b25e650454000e3529c6a305f99074 Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 15:51:09 -0500 Subject: [PATCH 5/8] Update naming --- src/js/control-bar/mute-toggle.js | 2 +- .../control-bar/volume-control/volume-bar.js | 2 +- src/js/player.js | 20 +++++++++---------- test/unit/controls.test.js | 4 ++-- 4 files changed, 13 insertions(+), 15 deletions(-) diff --git a/src/js/control-bar/mute-toggle.js b/src/js/control-bar/mute-toggle.js index 1279cd0bf3..157bb443d3 100644 --- a/src/js/control-bar/mute-toggle.js +++ b/src/js/control-bar/mute-toggle.js @@ -54,7 +54,7 @@ class MuteToggle extends Button { */ handleClick(event) { const vol = this.player_.volume(); - const lastVolume = this.player_.lastVolume(); + const lastVolume = this.player_.lastVolume_(); if (vol === 0) { this.player_.volume(lastVolume); diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index 7183300835..c489043498 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -124,7 +124,7 @@ class VolumeBar extends Slider { this.on('sliderinactive', function setLastVolume() { if (this.player_.volume() === 0) { - this.player_.lastVolume(volumeBeforeDrag); + this.player_.lastVolume_(volumeBeforeDrag); } this.off('sliderinactive', setLastVolume); }); diff --git a/src/js/player.js b/src/js/player.js index 9fb37b8d5c..057a353f17 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -335,9 +335,8 @@ class Player extends Component { // Set controls this.controls_ = !!options.controls; - // Set default values for lastVolume and volumeBeforeDrag - this.lastVolume_ = 1; - this.volumeBeforeDrag_ = 1; + // Set default values for lastVolume + this.cache_.lastVolume = 1; // Original tag settings stored in options // now remove immediately so native controls don't flash. @@ -1817,7 +1816,7 @@ class Player extends Component { this.techCall_('setVolume', vol); if (vol > 0) { - this.lastVolume(vol); + this.lastVolume_(vol); } return; @@ -1889,16 +1888,15 @@ class Player extends Component { * - 1.0 is 100%/full * - 0.5 is half volume or 50% * - * @return {Player|number} - * a reference to the calling player when setting and the - * current volume as a percent when getting + * @return {number} + * the current value of lastVolume as a percent when getting */ - lastVolume(percentAsDecimal) { + lastVolume_(percentAsDecimal) { if (percentAsDecimal !== undefined && percentAsDecimal !== 0) { - this.lastVolume_ = percentAsDecimal; - return this; + this.cache_.lastVolume = percentAsDecimal; + return; } - return this.lastVolume_; + return this.cache_.lastVolume; } /** diff --git a/test/unit/controls.test.js b/test/unit/controls.test.js index fdff35bb3d..0b77715e38 100644 --- a/test/unit/controls.test.js +++ b/test/unit/controls.test.js @@ -111,7 +111,7 @@ QUnit.test('Clicking MuteToggle when volume is 0 and muted is false should set v const muteToggle = new MuteToggle(player); player.volume(0); - assert.equal(player.lastVolume(), 1, 'lastVolume is set'); + assert.equal(player.lastVolume_(), 1, 'lastVolume is set'); assert.equal(player.muted(), false, 'player is muted'); muteToggle.handleClick(); @@ -126,7 +126,7 @@ QUnit.test('Clicking MuteToggle when volume is 0 and muted is true should set vo player.volume(0); player.muted(true); - player.lastVolume(0.5); + player.lastVolume_(0.5); muteToggle.handleClick(); From 500e692afff368e9f00d6774142f3a47a0df98f7 Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 22:35:05 -0500 Subject: [PATCH 6/8] Make updateLastVolume private --- src/js/control-bar/volume-control/volume-bar.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index c489043498..75cca60506 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -25,7 +25,7 @@ class VolumeBar extends Slider { */ constructor(player, options) { super(player, options); - this.on('slideractive', this.updateLastVolume); + this.on('slideractive', this.updateLastVolume_); this.on(player, 'volumechange', this.updateARIAAttributes); player.ready(() => this.updateARIAAttributes); } @@ -119,7 +119,7 @@ class VolumeBar extends Slider { * * @listens slideractive */ - updateLastVolume() { + updateLastVolume_() { const volumeBeforeDrag = this.player_.volume(); this.on('sliderinactive', function setLastVolume() { From c530565e7178861830cee75224988c7a3b427ed5 Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 22:43:36 -0500 Subject: [PATCH 7/8] Add `@private` jsdoc directives --- src/js/control-bar/volume-control/volume-bar.js | 1 + src/js/player.js | 2 ++ 2 files changed, 3 insertions(+) diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index 75cca60506..b31606eb99 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -118,6 +118,7 @@ class VolumeBar extends Slider { * set lastVolume to the stored volume. * * @listens slideractive + * @private */ updateLastVolume_() { const volumeBeforeDrag = this.player_.volume(); diff --git a/src/js/player.js b/src/js/player.js index 057a353f17..8d96d84259 100644 --- a/src/js/player.js +++ b/src/js/player.js @@ -1890,6 +1890,8 @@ class Player extends Component { * * @return {number} * the current value of lastVolume as a percent when getting + * + * @private */ lastVolume_(percentAsDecimal) { if (percentAsDecimal !== undefined && percentAsDecimal !== 0) { From ffac358bf745a1f13b2dbd13b5182b75ad27973c Mon Sep 17 00:00:00 2001 From: Kevin Litchfield Date: Fri, 27 Jan 2017 22:43:52 -0500 Subject: [PATCH 8/8] Use `one` instead of `on`/`off` --- src/js/control-bar/volume-control/volume-bar.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/js/control-bar/volume-control/volume-bar.js b/src/js/control-bar/volume-control/volume-bar.js index b31606eb99..9dfd977155 100644 --- a/src/js/control-bar/volume-control/volume-bar.js +++ b/src/js/control-bar/volume-control/volume-bar.js @@ -123,11 +123,10 @@ class VolumeBar extends Slider { updateLastVolume_() { const volumeBeforeDrag = this.player_.volume(); - this.on('sliderinactive', function setLastVolume() { + this.one('sliderinactive', () => { if (this.player_.volume() === 0) { this.player_.lastVolume_(volumeBeforeDrag); } - this.off('sliderinactive', setLastVolume); }); }