-
Notifications
You must be signed in to change notification settings - Fork 7.4k
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
Muting with MuteToggle
sets ARIA value of VolumeBar
to 0
#4099
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the issue you're running into is that the player isn't yet ready when you're trying to use it.
In other test files, we load in sinon's fake timers and then we tick the timer after the player is initialized to make sure that it's ready. Hopefully, doing that will mean that you won't be required to call updateARIAAttributes
manually and muteToggle will update the volume bar automatically after that.
@@ -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(); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
test/unit/controls.test.js
Outdated
|
||
muteToggle.handleClick(); | ||
|
||
volumeBar.updateARIAAttributes(); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
* | ||
* @private | ||
*/ | ||
volumeAsPercentage_() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
@gkatsev Thanks for the suggestion about fake timers! That worked in two of the places I was calling |
|
||
this.el_.setAttribute('aria-valuenow', volume); | ||
this.el_.setAttribute('aria-valuetext', volume + '%'); | ||
this.el_.setAttribute('aria-valuenow', ariaValue); |
There was a problem hiding this comment.
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 + '%'
)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Apart from some wording changes to the comment, LGTM.
Any other thoughts, @OwenEdwards?
test/unit/controls.test.js
Outdated
|
||
muteToggle.handleClick(); | ||
|
||
// Because `MuteToggle#handleClick` is async, the `volumechange` event |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Currently, the ARIA value of
VolumeBar
tracks the value ofvolume
. It should instead track the position of the slider onVolumeBar
, which tracksvolume
except when the player is muted, in which case it's 0.Addresses #4064.
Question about testing
Clicking
MuteToggle
fires thevolumechange
event on the player, which in turn runsVolumeBar#updateARIAAttributes
.However, in the testing environment—at least as I have the tests set up!—clicking
MuteToggle
doesn't firevolumechange
on the player, so I've had to manually callVolumeBar#updateARIAAttributes
where it should have been run automatically in order to get the tests to pass. Of course, this needs to be fixed before merging.Do any problems with my test configuration stick out, or is this likely a bug?