Fix menu keyboard access and ARIA labeling for screen readers (Fixes #2155 and #2769) #2897

OwenEdwards · 2015-12-08T06:20:33Z

Implement ARIA-specified keyboard access to control bar menus (as in http://www.w3.org/TR/wai-aria-practices-1.1/#menu), and ARIA markup to indicate menu role and state to screen readers.

This also fixes #1446 / #1447, which seemed to have come back (or was never actually resolved) as an issue for keyboard access of menus.

This fix depends on #2889.

… clickable.

Handle correct keyboard operation of menu, including single tab stop for menu, arrow keys within the menu, focus movement within the menu, and ARIA roles for parts of menus.

…reen readers.

gkatsev · 2015-12-08T06:22:23Z

Thanks, @OwenEdwards. I'll take a look at it ASAP.

OwenEdwards · 2015-12-09T03:48:32Z

@gkatsev hold off on pulling this - I need to do more work on the "volume menu button" since keyboard handling may conflict (arrow keys apply to the volume slider, not to moving between menu items).

gkatsev · 2015-12-09T04:12:01Z

Alright.
When we're focused inside of the menu, the arrow keys probably should just interact with the menu.
If we're just focused on the player in general, i.e., after hitting the big play button, the arrow keys should control volume/seeking.

OwenEdwards · 2015-12-09T06:11:01Z

@gkatsev that would be nice to have, but I don't see that it's implemented in V5 right now? I thought this was something that was left to videojs-hotkeys "A plugin for Video.js that enables keyboard hotkeys when the player has focus"?

I was specifically referring to the fact that a slider handles arrow keys when it has focus, but the ARIA definition of a menu defines that arrow keys move focus between menu items, so there's a conflict. By ARIA standards, the Volume Menu Button isn't really a 'menu', it's a custom widget with a button and a slider. It could even have a single keyboard focus, where arrow keys adjust volume and Space or Enter mute/unmute the player since right now its keyboard interaction is a little confused.

gkatsev · 2015-12-09T15:56:30Z

Didn't know about that plugin and yeah, most of the hotkeys should still be left to the plugin. I think some stuff we probably should have in core. Like <space> should probably toggle the playback state when the player is in focus. But f to fullscreen isn't necessary to be in core. But it's debatable.

Ah, interesting. So, the issue is how apply the ARIA spec to the volume menu button which is a "menu" in vjs but not according to ARIA?

OwenEdwards · 2015-12-11T02:13:36Z

@gkatsev right, that's the concise way of putting it! The volume menu button is a "menu" in VJS, but not a menu according to ARIA, where it's either a button and a slider, or a custom widget.

One thing which I'm still not clear about is that, for vjs menus, they open on hover due to CSS, but they also have this handling of click and lockShowing in JS. Do you know, is this to allow a CSS skin to change the onhover/onclick behavior? I haven't been able to track it down, and trying to get everything to work with keyboard-only operation without breaking existing behavior is tricky! ;-)

Let's split 'hotkeys' onto a separate GitHub issue - it ties into accessibility, but I don't want to muddy this issue too much!

gkatsev · 2015-12-11T02:23:31Z

I think the lock-showing thing may be to help with exactly this situation. So you could select the menu and it will keep it open so you could keep interacting with the things inside the menu. Or something.
I know that it used to be badly broken in some versions of vjs 4. I'm not sure whether it was ever fixed or how it works in 5.x. Also, it doesn't seem to be doing anything with the volume menu button currently.

OwenEdwards · 2015-12-15T08:14:32Z

@gkatsev @mmcc @heff can any of you explain what the vjs-lock-showing class and lockShowing method are trying to do? The more I dig, the more I think it's currently a bug.

For the Captions and Subtitles menus, they seem to do nothing because clicking on the menu button opens the menu (and calls lockShowing), but unlockShowing is called on mouseout, so the menu closes if mouse hover moves off the menu. Similarly, clicking a second time on the menu button clears vjs-lock-showing, but the menu stays open because the CSS treats vjs-lock-showing and hover identically. And vjs-lock-showing has no effect on the controls fading out during playback.

For the volume-menu-button, when it is inline, then it does seem like clicking on the mute button does sometimes lock the volume slider as visible, even when the mouse pointer moves off the mute button or slider. But this means sometimes clicking on the mute button mutes the player, and sometimes it doesn't!! I haven't been able to track down exactly when it does which one, but it means one control has two functions!!

As I try to fix keyboard accessibility of controls which are not just a button, issues with this lockShowing functionality are becoming a major roadblock, and I think they're actually usability issues becond keyboard accessibility. What do you guys think?

gkatsev · 2015-12-15T15:59:50Z

I think lock-showing is just broken or not working correctly, though, with the best intentions.
As for the inline volume menu button, the issue is that the slider was overlayed on top of the mute toggle so you sometimes were clicking the slider instead of the mute toggle. I'm fixing it here: #2913
The "lock-showing" of the volume is, I believe, a side-effect of trying to make it keyboard accessible and keeping the slider/menu open when focused. Trying to address it here: https://github.com/videojs/video.js/pull/2638/files#diff-6b8a908fd9a1289ee34105f594366da4R73.

We probably should change lock-showing to apply when clicking on the menu button and get removed when clicking on it again and not be removed on mouseout. As for the volume menu button, is fiddling around with ARIA attributes help? Should we look into actually splitting it out into two components that are siblings instead of ancestor/descedant?

heff · 2015-12-15T17:59:13Z

Re: lockShowing...if I remember right it was meant to allow for keyboard triggering of menus. So tab to the menu button, hit space bar to open the menu, tab to the first menu item and menu stays showing. I can't say if that's how it is now, but I think that would be the main use case to confirm is still working if you strip it out.

gkatsev · 2015-12-15T18:52:47Z

Like the discussion here, I think the flaw is with keyboard+mouse interaction. I.e., if you tab into the menu and then mouse over it, it'll be hidden, but should it? We probably would want to know whether we tabbed into the control and only apply the mouseout event if opened the menu with the mouse.

gkatsev · 2015-12-15T19:08:30Z

I opened a new issue that spun out of #2638 and is related to the lock-showing/keyboard/stuff discussion: #2918

OwenEdwards · 2015-12-20T01:13:54Z

@gkatsev I think the issue with keyboard+mouse interaction is a bit of a distraction - I'm thinking of control interaction in this order:

mouse only
touchscreen only (which you've been working on)
keyboard only (WCAG)
keyboard-only with screen reader (WCAG)
touchscreen with screen reader (WCAG)
voice control (e.g. Dragon NaturallySpeaking) (WCAG)
mouse+keyboard

I think mouse+keyboard is a concern, but not until we've got those other ones working!

So the questions I still feel hasn't been answered is:

1/. Do we want the Captions and Subtitles menu buttons to:

a/. Appear on hover (mouse) and on tap (touchscreen), or
b/. Appear on click (not on hover) and on tap

2/. Do we want a mechanism for mouse users to open the menu and leave it open after mouseout?

3/. Do we want the behavior in 1 and 2 to be changeable for site creators? Via CSS skin, or via Video.js parameters?

The preferred behavior of menus for keyboard-only users (including keyboard-only screen reader users) is defined by the ARIA specs, and should follow most or all of that pattern, but this does not affect the mouse and touchscreen operation of these controls.

Then we need to make a decision for the combined volume slider and mute button, in both inline and non-inline/vertical configuration. This does not have to match the menu operation, and the keyboard interaction with this combined control is definitely going to be different from keyboard interaction with the menu.

I do think we should split the volume-menu-button away from being a descendent of a menu, although it's possible that there could be some common ancestor for the two of them. In fact, I have been working on doing exactly this, and creating a component called a popup, which is part of what lead to this question about lock-showing; once we're clear on what control features we want to support, we'll can look at how much these two controls will have in common.

gkatsev · 2016-01-12T00:31:53Z

I just realized that I'm doing some of the stuff you already had in here, for example in #2994. I'll go through and de-dupe and make suggestions and what not.

gkatsev · 2016-01-12T19:05:30Z

src/js/menu/menu-item.js

+    if (this.selectable) {
+      // TODO: May need to be either menuitemcheckbox or menuitemradio,
+      //       and may need logical grouping of menu items.
+      this.el_.setAttribute('role', 'menuitemcheckbox');


locally, I've played around with the menu itself to a role of listbox and these to a role of option.
Using ChromeVox for testing it actually produced decent results since it told you how many options and whether they're selected or not.

Not sure if this is a chromevox issue or not, but if I have captions in the list, when I expand the captions menu, it'll read the label of the last caption in the list.

Going to chalk it up to a bug in chromevox.

Don't base your understanding of whether accessibility is working on ChromeVox!!! You'll hurt your credibility with accessibility experts ;-) It's convenient because it's free, but Firefox+NVDA on a PC is a much better free, standards-compliant alternative. See http://webaim.org/projects/screenreadersurvey/.

That said, I've also been looking at some options around listbox and option. Semantically, the menus (e.g. the CC menu) are complicated because they have a current state (selected item), potentially a non-actionable title, a settings menu option (which is not "selectable", but is actionable), and a set of options of which one can be picked (we had that discussion about whether more than one could be picked, if we combine CC and Subtitles into a single menu, but I think we concluded they couldn't).

I'll check out NVDA. I've been using OSX's VoiceOver the latter half of last week once I realized that ChromeVox isn't good enough.

gkatsev · 2016-01-12T21:06:39Z

Also, to answer the questions in your last comment.

I think option a is what we want. Open on hover with mouse and tap on touchscreen
I think that clicking the button should lock the menu and leave it open until it is pressed again
I don't think we need to have an option for this but we should try to use CSS and html mostly with limited js functionality for this. That way, it could possibly be configured via CSS.

Moving the volume-menu-button to be a "popup" type role. Though, I wonder whether we can do it safely while maintaining backwards compatibility.

gkatsev · 2016-01-12T21:16:23Z

Looks like ChromeVox, while nice for getting an initial idea for things, functions differently than VoiceOver and others.

gkatsev · 2016-01-20T03:06:47Z

Also, I've been thinking that we should just merge this because it's progress towards better accessibility. I have some updates to my PR (#2994) that are based on these changes that hopefully also make it slightly better.
@OwenEdwards does that sound good? Also, is there a good way to contact you for some more real-time communication?

dmlap · 2016-01-20T14:50:01Z

src/js/menu/menu-item.js

-    } else {
-      this.removeClass('vjs-selected');
-      this.el_.setAttribute('aria-selected',false);
+    if (this.selectable) {


Just a general thought, not necessary for this PR: would we have an easier time with menu-button/menu-item if we started from a <select> element?

Yes, just using a <select> should hopefully makes things easier, though, it'll make even less sense to have the caption settings menu button in it then, so, we'll have to put it elsewhere.
Also, <select> is not styleable at all.

dmlap · 2016-01-20T14:56:49Z

src/js/menu/menu.js

+    if (event.which === 37 || event.which === 40) { // Left and Down Arrows
+      event.preventDefault();
+      this.stepForward();
+    } else if (event.which === 38 || event.which === 39) { // Up and Right Arrows


If we're going to do keypress handling in a number of different places, we probably should consolidate these character codes.

Yes, yes we should.

Agreed! That was on my list to look at as park of this accessibility work; any thoughts on where it should go? utils/dom.js, perhaps? We need the ESC key from the modal dialog, too.

I've been thinking of grabbing this lib: https://github.com/timoxley/keycode
Seems to be small and tested and does exactly what we want.

The https://github.com/timoxley/keycode library seems like it would work. Potentially a little overkill (I was thinking of something as simple as the this.keys array from http://oaa-accessibility.org/example/15/), but why reinvent the wheel??

I opened an issue for this: #3022

dmlap · 2016-01-20T15:01:55Z

@gkatsev I'm cool with moving forward with this, especially if you have additional fixes in the wings. I'd be curious to hear your thoughts on any of my comments, especially the thought about <select>.

gkatsev · 2016-01-26T00:06:53Z

Superseded by #3033.

OwenEdwards added 5 commits December 4, 2015 18:51

Fix keyboard (Space or Enter) operation of buttons. Fixes videojs#2888.

6736e6e

Indicate whether menu items are selectable (on/off), rather than just…

5a3e8ab

… clickable.

Handle correct keyboard operation of menu

2fb4008

Handle correct keyboard operation of menu, including single tab stop for menu, arrow keys within the menu, focus movement within the menu, and ARIA roles for parts of menus.

Use the Menu Item ControlText to convey role/state information for sc…

9c2bedb

…reen readers.

Fix ARIA roles of the menu buttons in the control bar.

a872e9e

gkatsev added the a11y This item might affect the accessibility of the player label Jan 11, 2016

gkatsev reviewed Jan 12, 2016
View reviewed changes

gkatsev mentioned this pull request Jan 12, 2016

fix volume-menu button key handling. #2994

Closed

gkatsev mentioned this pull request Jan 18, 2016

Fix keyboard (Space or Enter) operation of buttons. Fixes #2888. #2889

Closed

dmlap reviewed Jan 20, 2016
View reviewed changes

gkatsev closed this Jan 26, 2016

OwenEdwards deleted the fix/menu-keyboard-access branch January 26, 2016 00:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix menu keyboard access and ARIA labeling for screen readers (Fixes #2155 and #2769) #2897

Fix menu keyboard access and ARIA labeling for screen readers (Fixes #2155 and #2769) #2897

OwenEdwards commented Dec 8, 2015

gkatsev commented Dec 8, 2015

OwenEdwards commented Dec 9, 2015

gkatsev commented Dec 9, 2015

OwenEdwards commented Dec 9, 2015

gkatsev commented Dec 9, 2015

OwenEdwards commented Dec 11, 2015

gkatsev commented Dec 11, 2015

OwenEdwards commented Dec 15, 2015

gkatsev commented Dec 15, 2015

heff commented Dec 15, 2015

gkatsev commented Dec 15, 2015

gkatsev commented Dec 15, 2015

OwenEdwards commented Dec 20, 2015

gkatsev commented Jan 12, 2016

gkatsev Jan 12, 2016

gkatsev Jan 12, 2016

gkatsev Jan 12, 2016

OwenEdwards Jan 20, 2016

gkatsev Jan 20, 2016

gkatsev commented Jan 12, 2016

gkatsev commented Jan 12, 2016

gkatsev commented Jan 20, 2016

dmlap Jan 20, 2016

gkatsev Jan 20, 2016

gkatsev Jan 20, 2016

dmlap Jan 20, 2016

gkatsev Jan 20, 2016

OwenEdwards Jan 21, 2016

gkatsev Jan 21, 2016

OwenEdwards Jan 22, 2016

gkatsev Jan 22, 2016

dmlap commented Jan 20, 2016

gkatsev commented Jan 26, 2016

Fix menu keyboard access and ARIA labeling for screen readers (Fixes #2155 and #2769) #2897

Fix menu keyboard access and ARIA labeling for screen readers (Fixes #2155 and #2769) #2897

Conversation

OwenEdwards commented Dec 8, 2015

gkatsev commented Dec 8, 2015

OwenEdwards commented Dec 9, 2015

gkatsev commented Dec 9, 2015

OwenEdwards commented Dec 9, 2015

gkatsev commented Dec 9, 2015

OwenEdwards commented Dec 11, 2015

gkatsev commented Dec 11, 2015

OwenEdwards commented Dec 15, 2015

gkatsev commented Dec 15, 2015

heff commented Dec 15, 2015

gkatsev commented Dec 15, 2015

gkatsev commented Dec 15, 2015

OwenEdwards commented Dec 20, 2015

gkatsev commented Jan 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gkatsev commented Jan 12, 2016

gkatsev commented Jan 12, 2016

gkatsev commented Jan 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlap commented Jan 20, 2016

gkatsev commented Jan 26, 2016