Closed Bug 486899 Opened 15 years ago Closed 15 years ago

Keyboard Accessibility on video element (also audio)

Categories

(Toolkit :: Video/Audio Controls, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1

People

(Reporter: BijuMailList, Assigned: Dolske)

References

(Blocks 1 open bug)

Details

(Keywords: access, fixed1.9.1, user-doc-complete)

Attachments

(3 files, 6 obsolete files)

I dont find a way using keyboard only to jump in to get video controls

Steps 
1. goto http://www.double.co.nz/video_test/test5.html
2. press tab multiple times till you get focus on video controls.

Result:-
You never reach video controls

Expected:-
A way to access HTML5 VIDEO's using keyboard.

PS: once we use mouse to make control appear thru mouse over. We can modify use current time, do play/pause etc. thru keyboard
Flags: wanted1.9.1?
Keywords: access
fwiw, when the mouse is over the video element tabbing does indeed work, I think the controls need to made visible for :focus events, which would enable the tabbing purely through keyboard navigation.
Flags: blocking1.9.1?
Blocks: TrackAVUI
Flags: wanted1.9.1?
Flags: wanted1.9.1+
Flags: blocking1.9.1?
There are several issues with keyboard accessibility for the videocontrols:

1. When the controls aren't visible, they are skipped in the tab order.
2. There should be visible focus rings around focused elements
3. The scale doesn't seem to react to key presses (seems to be a permission problem)
4. The upcoming volumn control will need to be included in some way
(In reply to comment #2)
> 1. When the controls aren't visible, they are skipped in the tab order.

Yeah, this will be also a problem at two cases one when we automatically hide while paying. And other when control is disabled by author.
One way to solve this is when user is tabbing have a tab stop for <video> tag. Then when user press Down Arrow, make controls visible and allow tabbing. And when user finally tabs out from last control item on the <video> tag, change visibility of controls to what it was originally.
If user dont press Down Arrow, even if control is visible he will continue tabbing to other items on the page rather than tabbing thru each item in VIDEO.
also we may want to add more bits for pref accessibility.tabfocus
accessibility.tabfocus = 8  //tab on video elements
accessibility.tabfocus = 16  //tab on video controls
(In reply to comment #3)
> (In reply to comment #2)
> > 1. When the controls aren't visible, they are skipped in the tab order.
> 
> Yeah, this will be also a problem at two cases one when we automatically hide
> while paying. And other when control is disabled by author.
> One way to solve this is when user is tabbing have a tab stop for <video> tag.

Yes.

> Then when user press Down Arrow, make controls visible and allow tabbing.

What about displaying them when the <video> gets focus?
(And hiding them when focus is not on the <video> or in the controls)

I'm bumping this to major... it will be a bad scene if we can't fix this.
Severity: normal → major
(In reply to comment #5)
> > Then when user press Down Arrow, make controls visible and allow tabbing.
> 
> What about displaying them when the <video> gets focus?
> (And hiding them when focus is not on the <video> or in the controls)

Only issue is, if user was not planning stop in VIDEO tag he has to tab 4 more tab stops to come out of VIDEO tag now. 
This will increase in future if we add more controls for VIDEO element.
ie, why I proposed a down arrow key to go inside VIDEO controls set.
(In reply to comment #6)
> (In reply to comment #5)
> > > Then when user press Down Arrow, make controls visible and allow tabbing.
> > 
> > What about displaying them when the <video> gets focus?
> > (And hiding them when focus is not on the <video> or in the controls)
> 
> Only issue is, if user was not planning stop in VIDEO tag he has to tab 4 more
> tab stops to come out of VIDEO tag now. 
> This will increase in future if we add more controls for VIDEO element.
> ie, why I proposed a down arrow key to go inside VIDEO controls set.

Yeah I can see some use cases where this would be annoying. I can live with the arrow down.
Justin, what's the current status of 1,2 and 3 of comment #2? (I know you are busy)
I haven't had a chance to look at this, I assume since it's not already working that it's again an issue with the controls being native anonymous content, no idea what's involved with fixing that.
Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9.2a1pre) Gecko/20090513 Minefield/3.6a1pre

Play/puase buttons as well as the mute/unmute buttons correctly handle keyboard events. They get a focus ring when tabbed and are toggled when I hit <space>.

The time scale partially works, it responds to <left-arrow> and <right-arrow> (a bit wonky, but responds) as well as to <page-up> and <page-down>.

The volume slider however is problematic, it does not appear when the volume button receives keyboard focus. It is basically the same problem as the controls themselves, when hidden they do not receive keyboard focus.

So, I'd say 1 and 4 from comment 2 are still pretty much broken...
(In reply to comment #10)
> So, I'd say 1 and 4 from comment 2 are still pretty much broken...

Okay so we want < video > to be in the tab index, and down arrow to bring up the controls right? Can we pull Neal Deakin out of vacation? :)
(In reply to comment #10)

> The volume slider however is problematic, it does not appear when the volume
> button receives keyboard focus. It is basically the same problem as the
> controls themselves, when hidden they do not receive keyboard focus.

I would say we should show volume slider when mute button gets a focus. Mute button act like menu button?
(In reply to comment #11)
> (In reply to comment #10)
> > So, I'd say 1 and 4 from comment 2 are still pretty much broken...
> 
> Okay so we want < video > to be in the tab index, and down arrow to bring up
> the controls right? Can we pull Neal Deakin out of vacation? :)

Oh, and one more thing, the slider doesn't get a focus ring, so you can't really tell that it is indeed focused.
(In reply to comment #12)
> (In reply to comment #10)
> 
> > The volume slider however is problematic, it does not appear when the volume
> > button receives keyboard focus. It is basically the same problem as the
> > controls themselves, when hidden they do not receive keyboard focus.
> 
> I would say we should show volume slider when mute button gets a focus. Mute
> button act like menu button?

Sorry, I meant to say we shouldn't show volume slider on mute button focus and
should mute button act like menu button?
Each media element should be a single focus target where space pauses, left/right seek by a fixed number of seconds, and up/down change volume.  This would be consistent with standalone media players and YouTube.

This bug should be a blocker: a major new feature is inaccessible to keyboard-only users.  The inaccessibility is especially appalling given <audio>'s potential for improving the Web for blind users.  (Also, I find it annoying that there's no easy way to re-watch a small part of a long video, which would be solved by left/right keys seeking by 5 seconds.)
Flags: blocking1.9.1?
(In reply to comment #15)
> Each media element should be a single focus target where space pauses,
> left/right seek by a fixed number of seconds, and up/down change volume.  This
> would be consistent with standalone media players and YouTube.

Yep and QuickTime I think.

> 
> This bug should be a blocker

Agreed (See end of comment #5). We should agree on the keyboard strategy ASAP so that work isn't delayed.
Keywords: sec508
Now I feel Jesse's idea in comment #15 is better than mine at Comment #3 
Only thing to add 
* ctrl + arrowkey - should jump 1 minutes
* page up/dn - should jump 10 minutes

I have also one for context menu bug 493348
(In reply to comment #16)
> (In reply to comment #15)
> > This bug should be a blocker
> 
> Agreed (See end of comment #5). We should agree on the keyboard strategy ASAP
> so that work isn't delayed.

OK. We have about 3 days to design, implement, review and land the fix.
Flags: blocking1.9.1? → blocking1.9.1+
(In reply to comment #18)
Thanks Roc! Justin mentioned on #developers last night that he was working on this bug.

(In reply to comment #15)
Jesse you are right, this is a common keyboard interaction and we should have it.

Justin, I think this boils down to making sure:
1. media elements are tab navigable (you can tab *to* < video > and < audio >).
2. Jesse's keyboard Ux is available at that point.

Bonus points:
3. Biju's arrow down invokes the controls.
4. With controls invoked, the controls at tab navigable.
5. With controls invoked, and focus on a control, Jesse's keyboard Ux is disabled.
Attached patch Patch v.1 (WIP) (obsolete) — Splinter Review
So, this is a fairly simple approach, but has a few potential issues. Basically, it's keeping the focus on the <video> itself, instead of the individual control elements. Basically what Jesse's saying in comment 15, I think that makes for the best experience -- otherwise keys to different things depending on which control element happens to be focused.

The two things I'm not entirely sure about:

1) Should the video's tabindex be removed when the default controls are not being used?

2) Will accessibility stuff have problems now that the XUL control elements do not get focus?

For #1, I can't really do that from the JS side of things, perhaps nsHTMLMediaElement can handle this itself (via ::IsHTMLFocusable()? Not sure). OTOH, I can see wanting to have the tabindex of media elements behave consistently, and a page author can explicitly set tabindex="-1" to get rid of it if not desired. If we don't clear tabindex, nonstandard videocontrols may start relying on that to be the default.
Assignee: nobody → dolske
(It might also be possible to put the tabindex into the binding content, so that when |controls| is not set the existing CSS in html.css hides us and the tabindex, yet when controls are enabled-but-faded-out the tabindex is still alive. I didn't experiment with this idea.)
(In reply to comment #20)

> 2) Will accessibility stuff have problems now that the XUL control elements do
> not get focus?

As far as I know yes. They should be focusable otherwise AT won't deal with them. Let's check this with Marco.
This more or less obsoletes/breaks all the work we did in bug 483573. As long as I just try to toggle Play/Pause and Mute/Unmute from NVDA's virtual buffer, I'm fine. However, as soon as I try to set focus to *any* of the sliders or progress meters, focus goes into nowhere land, and from that point on, the video controls disappear from NVDA's virtual buffer.
So... Where does that leave us? My proposed patch seems like the right way to go for keyboard accessibility in general. It sucks to toss out recent work, but if that's what's needed to get to a better state of affairs, then so be it. :( Can the A11Y stuff adapt to this?

We talked in email about the possibility of having that code interact more with the HTML5 audio/video API instead of the XUL controls, maybe that's a route forward here?
(In reply to comment #24)

> We talked in email about the possibility of having that code interact more with
> the HTML5 audio/video API instead of the XUL controls, maybe that's a route
> forward here?

I believe this is something that requires changes on screen readers side. So at least it's long term.
(In reply to comment #23)
> This more or less obsoletes/breaks all the work we did in bug 483573.

Marco, how does it obsolete/break this work? Do you mean if media controls aren't focusable then we shouldn't create accessibles and etc to expose them to AT?
We can no longer interact with the sliders using NVDA's Focus mode. The buttons still work, but trying to focus the sliders will cause the whole grouping that contains the video controls to disappear from our a11y tree. From that point on the buttons are no longer there, the sliders have gone, and there is no information left to deal with except for the rest of the page.

The only solution I see is that, if trying to focus the individual sliders, focus should be redirected to the grouping parent (as if we had just tabbed to it). But explaining that to blind users will be hard, since they do not know that left and right will skip through the file, up and down will change volume (which doesn't work with this patch, by the way) etc. So the user experience will be unpleasant I am afraid.
Thanks for your attention on this bug Justin.

(In reply to comment #20)
> The two things I'm not entirely sure about:
> 
> 1) Should the video's tabindex be removed when the default controls are not
> being used?

I don't think so, but I might not be thinking of the problematic use cases.

> 2) Will accessibility stuff have problems now that the XUL control elements do
> not get focus?

Yep problems. Also, it is just good design to allow sighted keyboard users access to things they can see. There are sighted users who can use the keyboard (or an AT that synthesizes keyboard events) but not a pointing device like the mouse.

(In reply to comment #24)
> So... Where does that leave us? My proposed patch seems like the right way to
> go for keyboard accessibility in general.

I think keeping this work to allow keyboard based video control while the video element is focused is useful. Can we add this:

1. Down arrow brings up the control HUD and enables tab based navigation (tabindex="0") for the controls.
2. As for closing the HUD, perhaps tabbing away from the last control, or perhaps also pressing up arrow?
(In reply to comment #28)
> 2. As for closing the HUD, perhaps tabbing away from the last control, or
> perhaps also pressing up arrow?

Actually [ESC] is an obvious choice (after coffee).
(In reply to comment #27)
> We can no longer interact with the sliders using NVDA's Focus mode. The buttons
> still work, but trying to focus the sliders will cause the whole grouping that
> contains the video controls to disappear from our a11y tree.

Hmm, I wonder if finishing my patch will fix that. I was suppressing focus on the play/mute buttons, but didn't look into exactly what parts of the scrubber needed fixed (since it's composed of multiple elements). Let me update the patch, maybe this isn't going to be a problem.


(In reply to comment #28)

> Can we add this:
> 
> 1. Down arrow brings up the control HUD and enables tab based navigation
> (tabindex="0") for the controls.

I don't think this is good keyboard UI. Consider the steps required decrease the volume of a video, pause it, seek to a new spot, and then resume playing:

With the controls I'm proposing, the user would:

1) Tab to video element
2) <down arrow> (volume)
3) <space> (pause)
4) <right arrow> (seek)
5) <space> (play)

With focusing each thing separately:

1) Tab to video element
2) <down arrow> (HUD, presumably focuses play button)
3) <tab> (focus slider)
4) <tab> (focus mute)
5) *mumble* (something to activate volume slider)
6) <down arrow>
7) *mumble* (something to deactivate volume slider)
8) <shift-tab> (focus slider)
9) <shift-tab> (focus play)
10) <space> (pause)
11) <tab> (focus slider)
12) <right arrow> (seek)
13) <shift-tab> (focus play)
14) <space> (play)
15) <up-arrow> (deactivate HUD)

Far more steps, much slower when using different commands.
(In reply to comment #30)

> Hmm, I wonder if finishing my patch will fix that. I was suppressing focus on
> the play/mute buttons, but didn't look into exactly what parts of the scrubber
> needed fixed (since it's composed of multiple elements). Let me update the
> patch, maybe this isn't going to be a problem.

Hmm, so, me "/* XXX need to do this for the scrubber, too */" comment in videocontrols.css might be a lie. The scrubber/thumb doesn't seem to steal focus when clicked, so it's working fine for me. [The buttons, on the other hand, did need this. Else clicking the mute button and pressing space would trigger the mute button again, not toggle play/pause.]

So... Marco, could you look into what's failing in NVDA or the A11Y bits? Since this patch isn't changing how the slider's focus works (other than making the <video> itself focusable), it seems like this is probably an existing problem.
Suggested keyboard shortcuts (ctrl=cmd on OSX)

 up         = increase volume
 down       = decrease volume
 ctrl-up    = unmute
 ctrl-down  = mute
 left       = back 15s
 right      = ahead 15s
 ctrl-left  = back 25% of length
 ctrl-right = ahead 25% of length
 home       = skip to start
 end        = skip to end
 space      = pause/play toggle
In case it isn't clear from my other comments, I'm on board with direct keyboard manipulation of video! :)

I'm just greedy. KB control of the HUD would be a nice bonus.
Attached patch Patch v.2 (obsolete) — Splinter Review
Cleaned up version of first patch, with key changes discussed with beltzner.

NeilAway, I guess you're next up since Enn is, err, away. Bounce to mconnor if not?
Attachment #377888 - Attachment is obsolete: true
Attachment #378477 - Flags: review?(neil)
(I'd also like Marco's feedback to comment 31 before landing this, but we should get the review kicked off now.)
1) 

(In reply to comment #31)
> (In reply to comment #30)
> 
> > Hmm, I wonder if finishing my patch will fix that. I was suppressing focus on
> > the play/mute buttons, but didn't look into exactly what parts of the scrubber
> > needed fixed (since it's composed of multiple elements). Let me update the
> > patch, maybe this isn't going to be a problem.
> 
> So... Marco, could you look into what's failing in NVDA or the A11Y bits? Since
> this patch isn't changing how the slider's focus works (other than making the
> <video> itself focusable), it seems like this is probably an existing problem.

I think it makes sense to get rid a focus from xul slider of your scrubber.

2) Marco, so all controls are available for screen reader user (excepting progress scrubber), right?

3) Btw, why do you make all controls unfocusable, why can't your approaches from comment #30 live together? Truly I personally like to tab through group of controls to communicate with them.
(In reply to comment #36)

> 3) Btw, why do you make all controls unfocusable, why can't your approaches
> from comment #30 live together?

I don't think it makes sense to add two different ways to use the controls, unless there's an extremely compelling reason to do so. It's more code to write, more code to test, and (inevitably) more bugs to fix.
(In reply to comment #37)
> (In reply to comment #36)
> 
> > 3) Btw, why do you make all controls unfocusable, why can't your approaches
> > from comment #30 live together?
> 
> I don't think it makes sense to add two different ways to use the controls,
> unless there's an extremely compelling reason to do so. It's more code to
> write, more code to test, and (inevitably) more bugs to fix.

At least you need less bugs to fix when custom controls are used :) Have you considered the case events you listen are processed by custom controls already?

David, what do you think about all this?
(In reply to comment #38)
 
> At least you need less bugs to fix when custom controls are used :) Have you
> considered the case events you listen are processed by custom controls already?

The patch here does nothing if it's keypress handler is called when the video's |controls| attribute is not set [see the top of keyHandler()]. We can't do anything when our controls are disabled... For all we know, the page is using custom funky controls that user left-arrow to decrease volume and down-arrow to seek backwards. If we did our own actions, the result would be rather odd to the user.
Ok, Justin. It's ok then. However I have a feeling we'll do a wrong thing making this controls unfocusable. I sent letter to Aaron to grap his opinion on this issue.
The real problem with not allowing a user to navigate to individual controls is discoverability. Only real geeks like us will ever know what the shortcuts are just by reading the docs. Basically, no one. Space bar is pretty standard for pause/play but that's only 1 control, and not everyone knows it. If you interview users 1 year after you release you'll find that 1% of the users know 3 of the shortcuts.

Without discoverability, the feature is really just being put in to say that it's in and satisfy the a11y folks, but not to be actually used by regular people. 

I see several choices if you want it to be discoverable.
1. Make every item tabbable. This may be considered tedious if there are a lot of controls.
2. Add items to a menu somewhere. This can be in the menu bar, a context menu or some kind of drop down menu that is tabbed to. The menu items can list the keyboard shortcuts, although usually context menus don't show keyboard shortcuts, for whatever reason. But, at least if it's in a menu it's reachable.
3. Use underlined mnemonics, at least for the most important or least obvious shortcuts, such as louder and softer.
4. Allow toolbar-style navigation where the entire focus group is in the tab order once. You tab to the previously active control, and then arrow key moves between them. Unfortunately this conflicts with slider arrow key use.
5. Reasonable combination of above approaches, perhaps the most obvious shortcuts like space/left/right don't need mention.
6. Just allow tabbing

Question: will the pause/play/stop/prev/next etc. buttons built into keyboards for OS support work? Those are pretty discoverable.
Let's make sure whatever lands here gets automated tests (even via follow up bug).
OK, here are some observations:
1. Without Justin's patch, I can tab to the Play/Pause button, the slider to skip through the file, and the Mute/Unmute button.
2. With Justin's patch, I can tab to the grouping, *and* the skip through file slider.
3. In both cases, when landing on the Skip Through File slider, I get a focus redirect to the label which we marked up as ARIA role "presentation". Again, this is with and without Justin's patch.
4. When trying to go into Focus mode on any of the sliders with NVDA, focus gets lost somewhere. It is neither directed to the slider nor the grouping, but somewhere seemingly random on the page. This is worse with Justin's patch, since this causes even the Play/Pause and Mute/Unmute buttons to be erased from NVDA's virtual buffer, and no way to get them back. Without the patch, this does not happen, the buttons stay reachable.
Comment on attachment 378477 [details] [diff] [review]
Patch v.2

>+.playButton, .muteButton {
>+    -moz-user-focus: none;
>+}

Marco, what happens if you apply the patch with the above style change reverted?
(In reply to comment #41)
> The real problem with not allowing a user to navigate to individual controls is
> discoverability.

For discoverability (uaag 4.1.5) of direct keyboard commands we could adopt something like context sensitive help in the form of a keyboard help HUD: bug 492557. This could be connected via aria-describedby for AT pickup.

> 3. Use underlined mnemonics, at least for the most important or least obvious
> shortcuts, such as louder and softer.

Yeah, on Windows pressing and holding down [alt] allows the user to scan for underlined letters to find and learn shortcuts. Perhaps holding down alt or ? on the [video] could bring up the HUD.
Comment on attachment 378477 [details] [diff] [review]
Patch v.2

>+.playButton, .muteButton {
>+    -moz-user-focus: none;
>+}
Hardly seems worth making them buttons any more ;-)

>+                    // Ignore keys when content might be providing it's own.
its

>+                    var commandModifier = isMac && event.metaKey ||
>+                                          !isMac && event.ctrlKey;
isMac ? : (but if using <handler> below then can use modifiers="accel any")

>+                    if (!key && event.charCode == KeyEvent.DOM_VK_SPACE)
>+                        key = event.charCode;
If the charCode really is 32 (which as a char code is not necessarily equal to DOM_VK_SPACE) then key should be zero anyway.

>+                    try {
>+                      switch (key) {
>+                        case KeyEvent.DOM_VK_SPACE:
>+                            this.togglePause();
Interesting mix of indents.

>+                        case KeyEvent.DOM_VK_LEFT:
>+                            oldval = this.video.currentTime;
>+                            if (commandModifier)
>+                                newval = oldval - (isNaN(this.video.duration) ?
>+                                                   this.maxCurrentTimeSeen : this.video.duration) / 10;
(this.video.duration || this.maxCurrentTimeSeen) / 10

>+                            this.video.currentTime = newval;
What if newval < 0 ?

>+                        default:
>+                            preventDefault = false;
Perhaps early return here instead?

>+                    // Make the <video> element keyboard accessible.
>+                    this.video.setAttribute("tabindex", 0);
>+                    this.video.addEventListener("keypress", function (e) { self.keyHandler(e) }, false);
You could use <content tabindex="0"> and <handler event="keypress">
Or you could implement tabbability in the C++ as it is for e.g. <input>
Or you could check before clobbering any tabindex attribute on the <video>
(but then do you check for controls or do you assume pages set tabindex="-1"?)
(In reply to comment #44)
> (From update of attachment 378477 [details] [diff] [review])
> >+.playButton, .muteButton {
> >+    -moz-user-focus: none;
> >+}
> Marco, what happens if you apply the patch with the above style change
> reverted?

A few things:
1. The buttons are focusable again, but the problem on the slider(s) is still there.
2. The change from Play to Pause and Mute to Unmute is once again announced by NVDA. Without this reverted, bug 491443 is basically broken.
(In reply to comment #41)
> The real problem with not allowing a user to navigate to individual controls is
> discoverability. Only real geeks like us will ever know what the shortcuts are
> just by reading the docs.

Perhaps, but I don't think it's very good to have a discoverable feature that's too tedious to use.

I'm not sure discoverability is a huge problem in this case, though. It's not a <foopy> tag with some bizarre Control-F15-Shift-bracket accelerator. It's a media element, with keyboard shortcuts similar to other media player apps. Which, to be fair, hardly have a standard set of keyboard commands, but arrows+space+modifier seem relatively discoverable in that context.

> Without discoverability, the feature is really just being put in to say that
> it's in and satisfy the a11y folks, but not to be actually used by regular
> people.

Disagree. If I was just trying to whitewash over A11Y concerns, I wouldn't be arguing against what was proposed and would just go with the flow! :) I want a good set of keyboard commands, and am trying to figure out how to balance what makes sense to me with for A11Y.

> 2. Add items to a menu somewhere.

We do already have a context menu for media elements, with play/pause and mute/unmute commands.

> Question: will the pause/play/stop/prev/next etc. buttons built into keyboards
> for OS support work? Those are pretty discoverable.

I didn't see any such keycodes in nsIDOMKeyEvent.idl, so I'm guessing we'd still need to add support for them. That would be a good followup enhancement, even though it doesn't help with the immediate problem at hand (for those without such keyboards).
I'm finding it really hard to understand the state of this bug, and that's pretty distressing as I think we're all trying to do the right thing.

As I understand it, our collective goal is to ensure that the <video> and <audio> controls are accessible. There are two dimensions to this accessibility. First is that they can be reached by a HID other than the mouse (this bug). Second is that once the controls are reached, they have accessible names that can be announced by software (bug 489549 and bug 490287).

As I further understand it, the proposed fix for the lack of keyboard accessibility on this bug will break the existing fixes for accessible names.

Question 1: is that right?

If that's the case, I would hope that whatever fixes we made for accessible names could just be applied to this patch, carrying over the entity names for the slider and the play/pause button, and leaving all the other shortcuts behind. No, they won't be discoverable, but we can fix that later. Let's not let the perfect beat up the good here.

If such a clean carryover isn't easy, then I would submit that keyboard accessibility should trump the accessible names, or we should just hardcode english accessible names as part of this patch.

Davidb, MarcoZ: do you agree with this strategy? We need a clear decision NOW.

MarcoZ: Also, I don't understand comment 43; I can't tab into any video controls at all.
(In reply to comment #45)
> (In reply to comment #41)
> > The real problem with not allowing a user to navigate to individual controls is
> > discoverability.
> 
> For discoverability (uaag 4.1.5) of direct keyboard commands we could adopt
> something like context sensitive help in the form of a keyboard help HUD: bug
> 492557. This could be connected via aria-describedby for AT pickup.

David, aria-describedby won't help most keyboard users as most keyboard users don't have an AT.
(In reply to comment #48)
> (In reply to comment #41)
> Perhaps, but I don't think it's very good to have a discoverable feature that's
> too tedious to use.
I'm not suggesting it be tedious.

> I'm not sure discoverability is a huge problem in this case, though. It's not 
> a <foopy> tag with some bizarre Control-F15-Shift-bracket accelerator. It's a
> media element, with keyboard shortcuts similar to other media player apps.
> Which, to be fair, hardly have a standard set of keyboard commands, but
> arrows+space+modifier seem relatively discoverable in that context.
I haven't made a specific recommendation -- just opening up some possibilities for discussion as I think we want to optimize for discoverability as much as possible.

> > Without discoverability, the feature is really just being put in to say that
> > it's in and satisfy the a11y folks, but not to be actually used by regular
> > people.
> 
> Disagree. If I was just trying to whitewash over A11Y concerns, I wouldn't be
> arguing against what was proposed and would just go with the flow! :) I want a
> good set of keyboard commands, and am trying to figure out how to balance what
> makes sense to me with for A11Y.
I'm of course not saying you are trying to whitewash a11y concerns :) Sorry if it sounded that way. Just saying, that from my experience, most people whole could benefit will not know what the keyboard commands are unless you specifically work to make them discoverable or find a defacto standard to follow. That's not necessarily convenient for us, but that's how it is. I appreciate that you need to balance stuff.

> > 2. Add items to a menu somewhere.
> 
> We do already have a context menu for media elements, with play/pause and
> mute/unmute commands.
That's great. Does that work with Shift+F10 (Win + Lin) and Ctrl+Space (on Mac?)

> > Question: will the pause/play/stop/prev/next etc. buttons built into keyboards
> > for OS support work? Those are pretty discoverable.
> 
> I didn't see any such keycodes in nsIDOMKeyEvent.idl, so I'm guessing we'd
> still need to add support for them. That would be a good followup enhancement,
> even though it doesn't help with the immediate problem at hand (for those
> without such keyboards).
Yeah, it would be a shame not to support those keys! They are totally discoverable after all.
(In reply to comment #48)
> > Question: will the pause/play/stop/prev/next etc. buttons built into keyboards
> > for OS support work? Those are pretty discoverable.
> 
> I didn't see any such keycodes in nsIDOMKeyEvent.idl, so I'm guessing we'd
> still need to add support for them.

Those are special, like Back/Forward keys, which are supported but not as keycodes for KeyEvents, as far as I know.
This also means that media keys should work regardless of the "controls" attribute, as content can't handle them.
> Supporting the media keys would be easy.
That's a big help right there.

It probably helps only partially for volume and mute though. Those tend to be tied to the system volume, and I doube know that we want to override that behavior. Or?
Mute, I think, is expected to be system-wide. But volume up/down should be up to the application to decide. Media players don't handle that consistently, fwiw.
(In reply to comment #50)
> (In reply to comment #45)
> > (In reply to comment #41)
> > > The real problem with not allowing a user to navigate to individual controls is
> > > discoverability.
> > 
> > For discoverability (uaag 4.1.5) of direct keyboard commands we could adopt
> > something like context sensitive help in the form of a keyboard help HUD: bug
> > 492557. This could be connected via aria-describedby for AT pickup.
> 
> David, aria-describedby won't help most keyboard users as most keyboard users
> don't have an AT.

The HUD wouldn't require an AT and is intended for sighted users. I'm thinking since it would contain keyboarding info, might as well hook it up so that ATs can pick it up.
(In reply to comment #49)
> I'm finding it really hard to understand the state of this bug, and that's
> pretty distressing as I think we're all trying to do the right thing.
> 

Yeah, let's try to get consensus, do the fix, and move on. 

> As I understand it, our collective goal is to ensure that the <video> and
> <audio> controls are accessible.

Yes.

There are 3 keyboard user options that I've distilled from the comments and patches:

A. tab to video/audio and use shortcuts (comment #32)
B. tab to video, invoke controls, tab among them, interact using space, arrows etc.
C. A + B

I think our best option is C. (see related comment #28)

What do others think? (Time is ticking)
(In reply to comment #49)
> As I further understand it, the proposed fix for the lack of keyboard
> accessibility on this bug will break the existing fixes for accessible names.
> 
> Question 1: is that right?

The answer, I'm reliably informed by dbolter, is "yes and no".

The patch (for those like me who aren't fully following along)

 - puts the <video> and <audio> element in the tab order
 - does not allow the user to tab through the various controls
 - once focused, allows keyboard shortcut controls

This is sub-ideal as the shortcuts aren't discoverable and the individual control elements aren't announced by screen readers nor can they be tabbed to.

It is, however, *much more ideal* than not being able to tab over to the elements at all, which is where we are right now as I understand things.

It could be made more ideal if:

 - we made sure that screen readers announced that you'd tabbed into a <video> control
 - we made sure that hitting F1 when in a <video> control went to a SUMO page that described the keyboard shorcuts

^ This is the direction we want for this bug if we have any hope of seeing <video> and <audio> keyboard accessible in 3.5. Can someone add the F1 support and see if we can make it in before freeze?

(Follow-up bugs for making each element keyboard-tabbable should be filed, as well as follow-ups for supporting keyboard media keys whenever there's a <video> or <audio> element anywhere on the page (I don't think it should require focus for the keys to activate.)
Whiteboard: [product direction in comment 57]
I like C. Btw, does A include keyboard shortcut to show controls or are they intendent to be shown on video/audio focus?

Btw, focus frame of focused buttons is poor visible.
Whiteboard: [product direction in comment 57] → [product direction in comment 59]
(In reply to comment #59)
> (Follow-up bugs for making each element keyboard-tabbable should be filed, as
> well as follow-ups for supporting keyboard media keys whenever there's a
> <video> or <audio> element anywhere on the page (I don't think it should
> require focus for the keys to activate.)

Filed as bug 494175.
(In reply to comment #59)
>  - we made sure that hitting F1 when in a <video> control went to a SUMO page
> that described the keyboard shorcuts
> 
> ^ This is the direction we want for this bug if we have any hope of seeing
> <video> and <audio> keyboard accessible in 3.5. Can someone add the F1 support
> and see if we can make it in before freeze?

Something like http://support.mozilla.com/keyboard/media ?

I guess just en-US locale for now?

Justin, I guess just add something like this:
case KeyEvent.DOM_VK_F1:
    window.open('http://support.mozilla.com/keyboard/media','Keyboard Shortcuts','height=200,width=300')
    break;

I'll ping #sumo now.
Chris Ilias suggests adding a media section to http://support.mozilla.com/en-US/kb/Keyboard+shortcuts

Chris, can you make that happen?
Blocks: 494192
Yup. I've filed bug 494192 for adding a Media section to the keyboard shortcuts article, and marked it as dependent on this bug. 

For the URL (in Firefox), it generally looks like http://support.mozilla.com/1/%PRODUCT%/%VERSION%/%PLATFORM%/%LOCALE%/<help-topic> . For example, the private browsing URL looks like:
http://support.mozilla.com/1/%APP%/%VERSION%/%OS%/%LOCALE%/private-browsing

Then we update the htaccess file to point to the correct article, which in the end would look something like:
http://support.mozilla.com/%LOCALE%/kb/Keyboard+shortcuts?style_mode=inproduct#media

So we need a name for the help-topic.
media-player?
audio-video-player?
player?
playa'?
(In reply to comment #51)

> > We do already have a context menu for media elements
> That's great. Does that work with Shift+F10 (Win + Lin) and Ctrl+Space (on
> Mac?)

Yes, they're just additional items in the normal context menu.

(In reply to comment #46)

> >+                    if (!key && event.charCode == KeyEvent.DOM_VK_SPACE)
> >+                        key = event.charCode;
> If the charCode really is 32 (which as a char code is not necessarily equal to
> DOM_VK_SPACE) then key should be zero anyway.

I don't really understand this comment. But I've changed the code to be:

          var key = event.keyCode;
          if (!key && event.charCode == ' ');
              key = KeyEvent.DOM_VK_SPACE;

Which is maybe a better fit for the bizarre way keypress events set exactly one of these properties (depending on if the key was a printable character or not).

> You could use <content tabindex="0"> and <handler event="keypress">
> Or you could implement tabbability in the C++ as it is for e.g. <input>
> Or you could check before clobbering any tabindex attribute on the <video>
> (but then do you check for controls or do you assume pages set tabindex="-1"?)

Still looking at this and adding the F1/Help thing, updated patch coming shortly.
For the help topic, I was thinking something like media-shortcuts , but beltzner's suggestions had me thinking...If there's going to be a help-topic link for when someone uses F1 while focus is on the media player, it might be better to create a separate article about the media player. Not only listing shortcuts, but also explaining open video, that it is not a plugin, etc.

But that's a discussion that needs to take place somewhere else. I like beltzner's first suggestion of media-player.
(In reply to comment #27)
> We can no longer interact with the sliders using NVDA's Focus mode. The buttons
> still work, but trying to focus the sliders will cause the whole grouping that
> contains the video controls to disappear from our a11y tree. From that point on
> the buttons are no longer there, the sliders have gone, and there is no
> information left to deal with except for the rest of the page.
> 
> The only solution I see is that, if trying to focus the individual sliders,
> focus should be redirected to the grouping parent (as if we had just tabbed to
> it). But explaining that to blind users will be hard, since they do not know
> that left and right will skip through the file, up and down will change volume
> (which doesn't work with this patch, by the way) etc. So the user experience
> will be unpleasant I am afraid.

Marco, we have some clear direction here, see comment #59 and Justin is proceeding with the shortcut-only approach. Given this, we should make sure we don't expose AT inoperable controls.

Alexander, given the time crunch, and the focus problem Marco describes, do you agree we should not expose inoperable controls? You did the heavy lifting here, what would it take to un-expose them for 1.9.1 only?
Attached patch Patch v.3 (obsolete) — Splinter Review
> You could use <content tabindex="0"> and <handler event="keypress">

Couldn't get this to work -- the controls never took focus (or were even in taborder), and thus the keypress handler wasn't ever invoked. I'm assuming this is likely due to native anonymous content biting us again...

So, attached is an updated version of patch v.2 with previous review comments. Same basic approach. I rewrote keyHandler() to make it easier to be strict about not allowing extra modifiers (eg, don't allow alt-space to pause the video).

Also, I've nixed the keyboard shortcut linking to a SUMO page (discussed this with beltzner, after implementing it, *sigh*), for a few reasons:

  * Realistically, no is ever going to focus a media element and decide to
    press F1 to learn about keyboard shortcuts. They'll Google it.
  * Opening a window gets blocked by the popup blocker, so opening a help
    link would have to replace the current page. That sucks.
  * Uncertainty about building up the localized SUMO URL. The locale isn't
    available through the usual means (since we're unprivledged code), dunno if
    navigator.language is suitable to use or not.

We should still list the commands in some SUMO page, of course.


One more revision coming up, though, as I just found that the control key modifier doesn't work on Linux (and probably Windows too?) *sigh*
Attachment #378477 - Attachment is obsolete: true
Attachment #378477 - Flags: review?(neil)
There's ctrlKey. I don't think controlKey exists on any platform.
Comment on attachment 378973 [details] [diff] [review]
Patch v.3

>+                    if (event.metaKey)
>+                        command += (isMac ? "command-" : "meta-");
>+                    if (event.controlKey)
>+                        command += (isMac ? "control-" : "command-");

#ifdef XP_MACOSX
  if (aEvent.metaKey)
#else
  if (aEvent.ctrlKey)
#endif
    command += "accel-";

?
Attached patch Patch v.4 (obsolete) — Splinter Review
Bah, just a stupid typo that was easy to miss on OSX.
Attachment #378973 - Attachment is obsolete: true
Attachment #378982 - Flags: review?(mconnor)
(In reply to comment #68)

> Alexander, given the time crunch, and the focus problem Marco describes, do you
> agree we should not expose inoperable controls? You did the heavy lifting here,
> what would it take to un-expose them for 1.9.1 only?

If Marco happy with this and it doesn't break AT users experience. 

I need some technical details: what exactly does mean inoperable and not expose?
(In reply to comment #73)
> (In reply to comment #68)
> > Alexander, given the time crunch, and the focus problem Marco describes, do you
> > agree we should not expose inoperable controls? You did the heavy lifting here,
> > what would it take to un-expose them for 1.9.1 only?
> If Marco happy with this and it doesn't break AT users experience. 

Basically, the most important controls to reach from NVDA's virtual buffer are the buttons to start and pause the video, the mute/unmute the video, and to be able to skip through the video. However, only the buttons work reliably right now. If I try to focus one of the sliders, without Justin's patch, focus gets redirected to the label we marked up as "presentation", and with Justin's patch v.2, it was lost and thrown somewhere up to the location bar.

> I need some technical details: what exactly does mean inoperable and not
> expose?

For inoperable, see above. For "not expose", I think it would be best to not expose the sliders at all to a11y right now, just the buttons. And perhaps the label that says "xx:yy of zz:aa elapsed" as what it is, a label.

This, for  a basic first start, should be enough. And again, this should only go into 1.9.1. For 1.9.2, we should keep what we have and improve upon that.
(In reply to comment #74)

> For "not expose", I think it would be best to not
> expose the sliders at all to a11y right now, just the buttons. And perhaps the
> label that says "xx:yy of zz:aa elapsed" as what it is, a label.

Unfortunately I don't see a way how to do this simply and quickly if role="presentaiton" on slider won't help. And of course neither scale nor underlying slider must be not focusable.
Attached patch Patch v.5 (obsolete) — Splinter Review
After looking closer at keyboard commands for some other media apps (see https://wiki.mozilla.org/MediaKeys), decided to keep what we have so far. Looked at adding support for the F7/F8/F9 keys (volume adjustment), but were wary of (1) conflicting with F7 carat browsing in some annoying way and (2) conflicting with other system F-key usage. So, we'll punt on this for 1.9.1, we can revisit later.
Attachment #378982 - Attachment is obsolete: true
Attachment #379051 - Flags: review?(mconnor)
Attachment #378982 - Flags: review?(mconnor)
Attachment #379051 - Flags: review?(mconnor) → review+
Comment on attachment 379051 [details] [diff] [review]
Patch v.5

I think most of these can actually be done as a followup, they don't alter behaviour, they just avoid burning cycles.

>diff --git a/toolkit/content/widgets/videocontrols.xml b/toolkit/content/widgets/videocontrols.xml

>+                keyHandler : function(event) {

>+                    var keystroke = "";
>+                    if (event.altKey)
>+                        keystroke += "alt-";
>+                    if (event.shiftKey)
>+                        keystroke += "shift-";
>+#ifdef XP_MACOSX
>+                    if (event.metaKey)
>+                        keystroke += "accel-";
>+                    if (event.ctrlKey)
>+                        keystroke += "control-";
>+#else
>+                    if (event.metaKey)
>+                        keystroke += "meta-";
>+                    if (event.ctrlKey)
>+                        keystroke += "accel-";
>+#endif

So, since if you have anything other than accel- we're not going to find anything below, so we should bail.  Something like this, and then dao's suggestion, would seem a little cleaner/nicer.

  if (event.altKey || event.shiftKey ||
#ifdef XP_MACOSX
      event.ctrlKey)
#else
      event.metaKey)
#endif
    return;

>+                    if (String.fromCharCode(event.charCode) == ' ')
>+                        keystroke += "space";

Move this into the switch inside of a default block in the previous switch statement, so we only do this if we haven't already matched on something, and return if this fails?

>+                    try {
>+                        switch (keystroke) {

>+                            case "downArrow": /* Volume decrease */

I'm not at all sure this volume behaviour is right, there's some strangeness with volume down unmuting if the slider is visibly at the bottom,  but it's fine for now.
Pushed http://hg.mozilla.org/mozilla-central/rev/d37e68656412

Pushed to 191: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/8387e5800784

Will deal will followups from comment 77 after some zzzzs, and other issues when we can.
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite?
Keywords: fixed1.9.1
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(In reply to comment #69)
> > You could use <content tabindex="0"> and <handler event="keypress">
> Couldn't get this to work -- the controls never took focus (or were even in
> taborder), and thus the keypress handler wasn't ever invoked. I'm assuming this
> is likely due to native anonymous content biting us again...
Sorry, it just occured to me that maybe the issue is that the videocontrols element responds to -moz-user-focus rather than a tabindex attribute.
Backed out because of across-the-board a11y test failure, e.g. http://tinderbox.mozilla.org/showlog.cgi?log=Firefox3.5/1242975494.1242979635.1802.gz
Status: RESOLVED → REOPENED
Keywords: fixed1.9.1
Resolution: FIXED → ---
Just woke up... catching up... what's happening? We either disable the tests for 1.9.1 or?
(In reply to comment #81)
> Just woke up... catching up... what's happening? We either disable the tests
> for 1.9.1 or?

Disabling the tests is not a good idea, IMO. They clearly show that this patch breaks the namechange event firing, meaning NVDA and other ATs don't get notified of the change from "Play" to "Pause", for example.
This morning, I did some research. Since Firefox suffers from bug 78414 (the keyboard nav in and out of plugins bug), I used IE and tried various sites with all different kinds of mostly Flash-based players.

The results were mostly that, either the controls were all tabbable (albait not always labelled for accessibility), or keyboard navigation was not possible at all inside the plugin. If the controls were tabbable, one could activate them using SPACE. If they were NOT tabbable, SPACE or so would usually not do anything, either. Only the screen reader's mouse emulation would sometimes, through hit or miss, be able to do anything useful and get playback started.

I therefore propose, for the cleanest and most discoverable keyboard navigation possible, to go with the suggestion to make each interactive element inside the video element container tabbable. At a minimum, these would be the Play/Pause button, the volume slider, the position slider, and the Mute/Unmute button.

This would also be consistent with the rest of the page which usually consists of tabbable elements anyway.
(In reply to comment #83)
> I therefore propose, for the cleanest and most discoverable keyboard navigation
> possible, to go with the suggestion to make each interactive element inside the
> video element container tabbable. At a minimum, these would be the Play/Pause
> button, the volume slider, the position slider, and the Mute/Unmute button.

Marco, please see comment 59. That's not the approach we're going to take with this patch, but in comment 61 David Bolter agreed to file this approach as bug 494175.
This patch let's a11y tests pass.
Attachment #379051 - Attachment is obsolete: true
Comment on attachment 379140 [details] [diff] [review]
justin's latest patch with failing tests commented out

wrong patch
Attachment #379140 - Attachment is obsolete: true
Attachment #379140 - Flags: review?(dolske)
No longer blocks: 492956
Depends on: 492956
To be clear, davidb disabled the tests which were testing focus on the video
controls, which was intentionally turned off in favour of focusing the element
as a whole and using keyboard-shortcuts in this bug. He's also added a
reference to bug 494175 which aims to, later, restore the ability to focus the
video controls individually.

MarcoZ and davidb talked on IRC and agreed comment 82 isn't quite correct in
its interpretation of the test failure logs.

Should be a pretty simple carry-forward review, but would like other eyes on
it. Maybe Gavin?
Attachment #379141 - Flags: review?(dolske) → review?(gavin.sharp)
I tried Justing patch without making buttons unfocusable and it looks fine with me. I think that approach should work and for you and for us, i.e. keyboard guys can control video and if controls are shown they can tab them, so it's good for a11y. 

But when video control is focused then there is no dashed border around it. It's a problem.
(In reply to comment #90)
> Created an attachment (id=379153) [details]
> latests david's version without tests disabling and unfocusabling buttons

I though Marco tried this? (see comment #44, and comment #47)
David, I believe it's ok with the patch from bug 494346
I think we should
1. Draw dashed border around focused video element
2. Show controls on focus
3. Remove line if (!this.video.hasAttribute("controls")) from patch because it doens't work (when @controls is presented then keyboard shortcuts still work)
Attachment #379153 - Flags: review+
Comment on attachment 379153 [details] [diff] [review]
latests david's version without tests disabling and unfocusabling buttons

This is fine.  Can it be ship time nao?
Whiteboard: [product direction in comment 59]
checked in, http://hg.mozilla.org/mozilla-central/rev/b93c27231300
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
(In reply to comment #89)
> I tried Justing patch without making buttons unfocusable and it looks fine with
> me.

Unfortunately, this is broken. The reason my patch made the buttons unfocusable is to prevent double actions or unexpected actions when they become focused, because the keypress handler is still getting those event too.

So, with your patch: start with a paused video, click the Play button (to focus it and start playback) and then press space. The space will trigger the keypress handler (pausing the video) and the button (playing it again), and thus ends up doing nothing. Similarly for the mute button -- focus it, press space, and you'll both toggle mute and play/pause the video.

Bug 494346 makes this worse, in that the scrubber now seems to be taking focus (it wasn't before, so I wasn't explicitly disabling it). After focusing the scrubber, up/down arrow trigger seeks in addition to changing the volume.

Discussed with beltzner and davidb on irc -- plan is to back out the last patch and bug 494346. It's too late to hold up 3.5.0 for this, but we'll should fix it all with a vengeance for 3.5.1.
Thanks Justin, "with a vengeance" is right :)

191 backed out via bsmedberg:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0e0202a65953
Keywords: fixed1.9.1
So, a local backout of 494346 didn't fix the scrubber focusing. Either some other patch did that, or my original testing was incomplete. Probably the latter, as it looks like it's only noticeable while playing. [Arrowkey increments on the XUL <scale> only move it by 1ms, due to bug 473103, so maybe it ends up being ignored while paused.]

So, the change to videocontrols.css should be as follows to suppress the slider focus:

.playButton, .muteButton,
.scrubber .scale-slider, .volumeControl .scale-slider {
    -moz-user-focus: none;
}

This might have actually been causing some of Marco's headaches, as it could explain why the scrubber was eating focus in a weird was.
backed out on trunk per comment #97 issues
http://hg.mozilla.org/mozilla-central/rev/4e851a98735e
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Regarding this last patch, the disabling of the entire test_elm_media.html is because the tests within are either failing (focus), or not useful (name changes), when the controls are not focusable. We'll use it instead on bug 494175.
Pushed http://hg.mozilla.org/mozilla-central/rev/324d492445e3

This is attachment 379260 [details] [diff] [review] (comment 101), which in turn is:

  * My original Patch v.5 (suppresses button focus)
  * CSS change from comment 99 (suppress slider focus too)
  * Disabling of A11Y media test

Since the A11Y test was depending on being able to focus the buttons and slider, davidb decided it was better to just disable the whole thing for now.
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Resolution: --- → FIXED
(In reply to comment #97)
> (In reply to comment #89)
> > I tried Justing patch without making buttons unfocusable and it looks fine with
> > me.
> 
> Unfortunately, this is broken. The reason my patch made the buttons unfocusable
> is to prevent double actions or unexpected actions when they become focused,
> because the keypress handler is still getting those event too.

You just need to listen original target I think.

> Bug 494346 makes this worse, in that the scrubber now seems to be taking focus
> (it wasn't before, so I wasn't explicitly disabling it). After focusing the
> scrubber, up/down arrow trigger seeks in addition to changing the volume.
> 

It's pure a11y fix it doens't affect on your patch if you don't use screen readers.
(In reply to comment #99)
> So, the change to videocontrols.css should be as follows to suppress the slider
> focus:
> 
> .playButton, .muteButton,
> .scrubber .scale-slider, .volumeControl .scale-slider {
>     -moz-user-focus: none;
> }

That's what I said in comment #36.
Blocks: 492415
Component: Video/Audio → Video/Audio Controls
Product: Core → Toolkit
QA Contact: video.audio → video.audio
Version: Trunk → unspecified
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: