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.

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
16 changes: 12 additions & 4 deletions src/js/control-bar/volume-control/volume-bar.js
Original file line number Diff line number Diff line change
Expand Up @@ -105,11 +105,19 @@ class VolumeBar extends Slider {
* @listens Player#volumechange
*/
updateARIAAttributes(event) {
// Current value of volume bar as a percentage
const volume = Math.round((this.player_.volume() * 100)).toString();
const ariaValue = this.player_.muted() ? 0 : this.volumeAsPercentage_();

this.el_.setAttribute('aria-valuenow', volume);
this.el_.setAttribute('aria-valuetext', volume + '%');
this.el_.setAttribute('aria-valuenow', ariaValue);
Copy link
Member

Choose a reason for hiding this comment

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

Does it matter that you haven't converted ariaValue to a string by this point (where the previous code used the .toString() method on the numeric value)? According to Using the aria-valuenow attribute - Accessibility | MDN, the value should be "String representation of a number". (The aria-valuetext will become a string thanks to the + '%')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OwenEdwards Thanks for the suggestion. I don't think this does matter, though. The number gets coerced to a string when being set on the element even without + '%' there to more visibly do the coercion. Tried changing everything to strings just in case and there was no difference.

this.el_.setAttribute('aria-valuetext', ariaValue + '%');
}

/**
* Returns the current value of the player volume as a percentage
*
* @private
*/
volumeAsPercentage_() {
Copy link
Member

Choose a reason for hiding this comment

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

👍

return Math.round(this.player_.volume() * 100);
}

/**
Expand Down
46 changes: 44 additions & 2 deletions test/unit/controls.test.js
Original file line number Diff line number Diff line change
@@ -1,14 +1,23 @@
/* 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';
import TestHelpers from './test-helpers.js';
import document from 'global/document';
import Html5 from '../../src/js/tech/html5.js';

QUnit.module('Controls');
import sinon from 'sinon';

QUnit.module('Controls', {
beforeEach(assert) {
this.clock = sinon.useFakeTimers();
},
afterEach(assert) {
this.clock.restore();
}
});

QUnit.test('should hide volume control if it\'s not supported', function(assert) {
assert.expect(2);
Expand Down Expand Up @@ -138,4 +147,37 @@ 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);

this.clock.tick(1);

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);

this.clock.tick(1);

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();

// Because `MuteToggle#handleClick` is async, the `volumechange` event
Copy link
Member

Choose a reason for hiding this comment

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

It's the triggering of volumechange that is asynchronous, but otherwise, 👍 to the comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, thanks! Thoughts on update?

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good.

// doesn't end up getting fired on `player` in the test environment, so we
// run it manually.
player.trigger('volumechange');

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');
});

}