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

Change menu items to ARIA role of "menuitemradio" when only one item can be selected #5171

Merged

Conversation

OwenEdwards
Copy link
Member

Description

Change menu items to ARIA role of "menuitemradio" when only one item can be selected can be selected (i.e. in the Captions, Subtitles, Descriptions, Chapters, Audio Tracks, and Rate menus). Fixes #5136.

Specific Changes proposed

Add an option of multiSelectable on MenuItem component; if true, the item is marked as a checkbox (role="menuitemcheckbox"), otherwise the item is marked as a radio button (role="menuitemradio"). All existing menu items, except the Captions Settings Menu, will be marked as radio buttons (the Settings Menu item has role="menuitem"), as will any components which extend the existing components and don't set 'multiSelectable'.

Requirements Checklist

  • Feature implemented / Bug fixed
  • If necessary, more likely in a feature request than a bug fix
    • Change has been verified in an actual browser (Chome, Firefox, IE)
    • Unit Tests updated or fixed
  • Reviewed by Two Core Contributors

…can be selected (i.e. in the Captions, Subtitles, Descriptions, Chapters, Audio Tracks and Rate menus). Fixes videojs#5136.
@OwenEdwards OwenEdwards requested a review from gkatsev May 15, 2018 03:35
@prozak915
Copy link

What's now??

@OwenEdwards
Copy link
Member Author

Also fixes #3663

@OwenEdwards OwenEdwards added a11y This item might affect the accessibility of the player needs: LGTM Needs one or more additional approvals labels May 16, 2018
@gkatsev
Copy link
Member

gkatsev commented May 23, 2018

Is the decision regarding Chapters is that it's fine for them to be selectable so no need to keep that issue open?

@gkatsev gkatsev added confirmed and removed needs: LGTM Needs one or more additional approvals labels May 23, 2018
@OwenEdwards
Copy link
Member Author

@gkatsev yes, that's what we decided. It's fine for Chapters to be selectable, but #5182 needs to be implemented/checked so that the menu doesn't have out-of-date information as the current time progresses.

@gkatsev
Copy link
Member

gkatsev commented May 23, 2018

Ah, nice. I thought it already did that? Maybe it went missing at some point.

@OwenEdwards
Copy link
Member Author

I haven't verified it with any SRs, so the Issue is more of a note to check it, and perhaps add some hidden text which clarifies that it's a little more complicated than just radio buttons. We also need to verify that it doesn't announce via an SR when the menu isn't focused/opened, like I noted for other menus in #5172.

@OwenEdwards OwenEdwards merged commit f3d7ac2 into videojs:master May 23, 2018
@OwenEdwards OwenEdwards deleted the fix/aria-roles-of-menu-items branch May 23, 2018 19:47
@OwenEdwards OwenEdwards restored the fix/aria-roles-of-menu-items branch May 24, 2018 16:41
@OwenEdwards OwenEdwards deleted the fix/aria-roles-of-menu-items branch May 24, 2018 16:42
gkatsev pushed a commit that referenced this pull request May 24, 2018
…upport

Change menu items to ARIA role of "menuitemradio" when only one item can be selected (i.e. in the Captions, Subtitles, Descriptions, Chapters, Audio Tracks and Rate menus).

This is the 6.x version of #5171 (f3d7ac2)

Fixes #5136.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a11y This item might affect the accessibility of the player confirmed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

text track menus use role of menuitemcheckbox, menuitemradio more appropriate
3 participants