Fix the structure of elements in menus to comply with ARIA requirements #4034
Conversation
…e not children of actionable elements, as required by ARIA.
There was a problem hiding this comment.
This is awesome.
One thing I noticed when testing, if I'm just using the keyboard, tabbing into the menu and opening it, and then selecting an item works great. However, after selecting an item, when the captions menu closes, it'll require pressing space twice to open it up again.
src/js/menu/menu-button.js
Outdated
| this.update(); | ||
|
|
||
| this.enabled_ = true; | ||
|
|
||
| this.el_.setAttribute('aria-haspopup', 'true'); | ||
| this.el_.setAttribute('role', 'menuitem'); | ||
| this.menuButton_.on('tap', Fn.bind(this, this.handleClick)); |
There was a problem hiding this comment.
I think you can change this to:
this.on(this.menuButton_, 'tap', this.handleClick);
src/js/menu/menu-button.js
Outdated
| * | ||
| * @method handleFocus | ||
| */ | ||
| handleFocus() { |
There was a problem hiding this comment.
not that we need to right now, but I think we need a keyhandler-mixin.
|
I think these two commits fix all of the issues @gkatsev pointed out. |
OwenEdwards
dismissed
gkatsev’s stale review
Asked @gkatsev to review again, based on additional commits to address his points
| * @return {string} | ||
| * The constructed wrapper DOM `className` | ||
| */ | ||
| buildWrapperCSSClass() { |
There was a problem hiding this comment.
I just realized that the change to this buildWrapperCSSClass means that the elements lose out on having something like vjs-captions-button applied to the main wrapper element.
I think we want to return something like
return `${buttonClass} ${super.buildCSSClass()} ${this.buildCSSClass()}`;instead, since the components are generally already set up with buildCSSClass to return the correct thing.
I'll open an issue for this.
In #4034, we changed the way that menu buttons work slightly by introduction a wrapper element with a separate wrapper css builder. However, this broke, at least the playback-rate menu. This PR adds a buildWrapperCSSClass to each of our menu buttons.
Description
The previous (v5.x) menu structure violated an ARIA requirement, that actionable elements (e.g. the menu button) not have child elements which are also actionable (e.g. the menu items). This PR changes that structure to make "MenuButton" actually a
<div>containing a<button>and a menu. The v5.x structure, and the use ofrole="group"on the Control Bar, were a workaround which made the menus work with most screen readers, but the structure might fail in future screen readers.This change is very similar to the change to the combined volume from v5 -> v6.
Specific Changes proposed
Restructure the HTML elements in a MenuButton & Menu; change the Javascript to handle this. Also, remove unnecessary
aria-labelattribute on the MenuButton, since the actual button used to open the menu is now correctly labeled by thecontrolText.Requirements Checklist