Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Muting with MuteToggle sets ARIA value of VolumeBar to 0 #4099

Merged
merged 9 commits into from
Feb 21, 2017
Merged

Muting with MuteToggle sets ARIA value of VolumeBar to 0 #4099

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f1f6451
Muting with MuteToggle sets ARIA value of VolumeBar to 0
kevinlitchfield Feb 17, 2017
da8516d
Fix linting errors
kevinlitchfield Feb 17, 2017
0c5f3cc
Refactor updateARIAAttributes
kevinlitchfield Feb 17, 2017
6309a76
Use Sinon's fake timers
kevinlitchfield Feb 17, 2017
0dabd59
Fix linting errors
kevinlitchfield Feb 17, 2017
c3ebcc7
Refactor volumeAsPercentage_
kevinlitchfield Feb 17, 2017
c963d02
Trigger 'volumechange' instead of updateARIAAttributes
kevinlitchfield Feb 17, 2017
2b8f064
Add explanatory comment
kevinlitchfield Feb 17, 2017
80fb9d9
Fix comment
kevinlitchfield Feb 17, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions src/js/control-bar/volume-control/volume-bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,9 +107,15 @@ class VolumeBar extends Slider {
updateARIAAttributes(event) {
// Current value of volume bar as a percentage
const volume = Math.round((this.player_.volume() * 100)).toString();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what about changing this const to a let, and then setting it to 0, if muted is true? That way, we aren't duplicated the setting of the aria-volumenow and aria-valuetext

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, yeah, that's much better! It occurred to me that perhaps using a conditional could make updateARIAAttributes even shorter and also, by extracting the statement that returns volume as a percentage into a named method, eliminate the need for an explanatory comment; please see 0c5f3cc and let me know what you think.


this.el_.setAttribute('aria-valuenow', volume);
this.el_.setAttribute('aria-valuetext', volume + '%');
const muted = this.player_.muted();

if (muted) {
this.el_.setAttribute('aria-valuenow', 0);
this.el_.setAttribute('aria-valuetext', 0 + '%');
} else {
this.el_.setAttribute('aria-valuenow', volume);
this.el_.setAttribute('aria-valuetext', volume + '%');
}
}

/**
Expand Down
31 changes: 31 additions & 0 deletions test/unit/controls.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
/* eslint-env qunit */
import VolumeControl from '../../src/js/control-bar/volume-control/volume-control.js';
import MuteToggle from '../../src/js/control-bar/mute-toggle.js';
import VolumeBar from '../../src/js/control-bar/volume-control/volume-bar.js';
import PlaybackRateMenuButton from '../../src/js/control-bar/playback-rate-menu/playback-rate-menu-button.js';
import Slider from '../../src/js/slider/slider.js';
import FullscreenToggle from '../../src/js/control-bar/fullscreen-toggle.js';
Expand Down Expand Up @@ -138,4 +139,34 @@ if (Html5.isSupported()) {
assert.equal(player.volume(), 0.5, 'volume is set to lastVolume');
assert.equal(player.muted(), false, 'muted is set to false');
});

QUnit.test('ARIA value of VolumeBar should start at 100', function(assert) {
const player = TestHelpers.makePlayer({ techOrder: ['html5'] });
const volumeBar = new VolumeBar(player);

volumeBar.updateARIAAttributes();

assert.equal(volumeBar.el_.getAttribute('aria-valuenow'), 100, 'ARIA value of VolumeBar is 100');
});

QUnit.test('Muting with MuteToggle should set ARIA value of VolumeBar to 0', function(assert) {
const player = TestHelpers.makePlayer({ techOrder: ['html5'] });
const volumeBar = new VolumeBar(player);
const muteToggle = new MuteToggle(player);

volumeBar.updateARIAAttributes();

assert.equal(player.volume(), 1, 'Volume is 1');
assert.equal(player.muted(), false, 'Muted is false');
assert.equal(volumeBar.el_.getAttribute('aria-valuenow'), 100, 'ARIA value of VolumeBar is 100');

muteToggle.handleClick();

volumeBar.updateARIAAttributes();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this still required after the tick update?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, see comment below. The last assertion ("ARIA value is 0") fails with that line removed or replaced with another tick update.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably because it's async. One thing that might be more accurate is to trigger a volumechange on player manually, but probably not much of a difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's better. Have updated. Think there's any way around the async thing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, unless you want to change the QUnit test to be an async test, which is generally frowned upon. This should be fine, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK. I added a comment explaining the reason for theplayer.trigger('volumechange') line, let me know how that looks to you.


assert.equal(player.volume(), 1, 'Volume remains 1');
assert.equal(player.muted(), true, 'Muted is true');
assert.equal(volumeBar.el_.getAttribute('aria-valuenow'), 0, 'ARIA value of VolumeBar is 0');
});

}