Improve mute button usability #3942

kevinlitchfield · 2017-01-13T18:42:56Z

Clicking MuteToggle when volume is 0 now has more user-friendly behavior. Previously, clicking MuteToggle when volume was 0 had no user-visible effect (as discussed in #3909, which this PR addresses).

There are three possible scenarios I'm aware of for setting the volume to 0 and then dealing with a click on MuteToggle that need to be taken into account:

User drags VolumeBar to 0 with mouse or finger. In this case, volume should be restored to the level it was at when the user started dragging VolumeBar.
User steps volume to 0 with keyboard. In this case, clicking MuteToggle should restore the volume to its last value before it reached 0.
volume is set directly to 0 with external JS, by a plugin, etc. In this case, it should be up to the programmer to control what level the volume should be set to when the user clicks MuteToggle, but it should never remain at 0.

How this solution works:

Clicking MuteToggle when volume is zero sets volume to the value of a new property, lastVolume.
lastVolume is updated to match volume when Player#volume sets a new value for volume, unless the new value of volume is 0.
When a user starts to drag VolumeBar, volume is saved into another new property, volumeBeforeDrag. When the user finishes dragging VolumeBar, if volume is 0, lastVolume is updated to match volumeBeforeDrag.

Note: This should not be merged until #3918 is fixed. As explained in that bug report:

If clicking MuteToggle when volume is zero sets volume to something greater than zero, then users will be able to accidentally trigger that behavior by mouseuping over MuteToggle after dragging VolumeBar to zero.

brandonocasey · 2017-01-20T19:18:48Z

Overall this is great! Thanks for the PR!

I think that there may be a way to make this a bit simpler:

Remove volumeBeforeDrag and listening for sliderinactive in the VolumeBar constructor
Player#lastVolume is updated by VolumeBar:
1. VolumeBar listens to slideractive on itself (as it does in the current implementation)
2. The slideractive listener records the starting volume and starts listening for one sliderinactive event on itself
3. The sliderinactive listener:
  - Sets lastVolume to the current volume unless it is zero or the volume from the slideractive listener if its not zero
  - Does nothing if the volume was changed from 0 to 0

I also think that we should probably make lastVolume on the player a private function prefixed with an _ at the end of it. What does everyone else think? @videojs/contributors

Also note that #3918 should be fixed in the current master. Can you rebase your PR on the current master when you get a chance?

gkatsev · 2017-01-27T19:59:54Z

src/js/player.js

+  lastVolume(percentAsDecimal) {
+    if (percentAsDecimal !== undefined) {
+      this.lastVolume_ = percentAsDecimal;
+      return this;


This should just return when used as setter.

gkatsev · 2017-01-27T20:01:12Z

src/js/player.js

+    *         a reference to the calling player when setting and the
+    *         current volume as a percent when getting
+    */
+  volumeBeforeDrag(percentAsDecimal) {


this should also be marked as private

gkatsev · 2017-01-27T20:02:01Z

src/js/control-bar/mute-toggle.js

@@ -65,7 +65,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();


is it possible for the lastVolume to end up being 0? Is it worth handling? At that point, I guess would be to set the volume to 0.5?

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.

brandonocasey · 2017-01-27T22:40:19Z

src/js/control-bar/volume-control/volume-bar.js

+   *
+   * @listens slideractive
+   */
+  updateLastVolume() {


we probably want this function to be private (only used internally) just like Player#lastVolume_()

also, @private jsdoc directive

brandonocasey · 2017-01-27T22:41:53Z

src/js/control-bar/volume-control/volume-bar.js

+  updateLastVolume() {
+    const volumeBeforeDrag = this.player_.volume();
+
+    this.on('sliderinactive', function setLastVolume() {


You can actually use this.one('sliderinactive' here that way you don't have to use this.off below. I would also say that you can just use an arrow function here rather than a named one. ex:

this.one('sliderinactive', () => { // current function goes here });

gkatsev · 2017-01-27T23:44:43Z

src/js/player.js

+   * @return {number}
+   *         the current value of lastVolume as a percent when getting
+   */
+  lastVolume_(percentAsDecimal) {


this should have an @private jsdoc directive.

gkatsev · 2017-01-27T23:45:02Z

src/js/control-bar/volume-control/volume-bar.js

+   *
+   * @listens slideractive
+   */
+  updateLastVolume() {


also, @private jsdoc directive

kevinlitchfield · 2017-01-28T03:54:40Z

@brandonocasey @gkatsev Thanks so much for the feedback! Have addressed.

gkatsev · 2017-01-31T20:29:20Z

@kevinlitchfield thanks so much for your work!

gkatsev moved this from Ready for Review to In Progress in 6.0 Jan 27, 2017

gkatsev reviewed Jan 27, 2017

View reviewed changes

kevinlitchfield added 4 commits January 27, 2017 15:08

Improve mute button usability

a0d5c96

Fix text in test

b2212a1

Simplify code to set lastVolume

0095835

lastVolume can never be set to 0

ad8dd5d

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.

kevinlitchfield force-pushed the improve-mute-button-usability branch from 651907f to ad8dd5d Compare January 27, 2017 20:13

Update naming

3bb6419

brandonocasey requested changes Jan 27, 2017

View reviewed changes

gkatsev reviewed Jan 27, 2017

View reviewed changes

kevinlitchfield added 3 commits January 27, 2017 22:35

Make updateLastVolume private

500e692

Add @private jsdoc directives

c530565

Use one instead of on/off

ffac358

gkatsev approved these changes Jan 31, 2017

View reviewed changes

gkatsev merged commit cb42fcf into videojs:master Jan 31, 2017

gkatsev moved this from In Progress to Done in 6.0 Jan 31, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve mute button usability #3942

Improve mute button usability #3942

kevinlitchfield commented Jan 13, 2017

brandonocasey commented Jan 20, 2017 •

edited

gkatsev Jan 27, 2017

gkatsev Jan 27, 2017

gkatsev Jan 27, 2017

brandonocasey Jan 27, 2017

gkatsev Jan 27, 2017

brandonocasey Jan 27, 2017

gkatsev Jan 27, 2017

gkatsev Jan 27, 2017

kevinlitchfield commented Jan 28, 2017

gkatsev commented Jan 31, 2017

Improve mute button usability #3942

Improve mute button usability #3942

Conversation

kevinlitchfield commented Jan 13, 2017

brandonocasey commented Jan 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kevinlitchfield commented Jan 28, 2017

gkatsev commented Jan 31, 2017

brandonocasey commented Jan 20, 2017 •

edited