1325594 - Pressing the space key do nothing when the focus is on the Play/Pause button

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect)

Description

[Hani Yacoub, Desktop QA]

[Affected versions]: 
Nightly 53.0a1

[Affected platforms]:
All platforms: Windows 10 x 64, Ubuntu 16.04, Mac OS X 10.11

[Steps to reproduce]:
1. Launch Nightly 53.0a1 with a new profile and then open this link https://people-mozilla.org/~ralin/vtt/.
2. Press Tab to navigate throw the media control icons.
3. When the focus is on the Play/Pause button, press space key.

[Expected result]:
The video should play or pause depend on the previous behaviour.

[Actual result]:
Pressing the space key do nothing.

Comment 1

[:Gijs (he/him)]

I admit to not having followed bug 1271765 in full all the time, but was there an explicit decision to make these controls focusable? They weren't in the old video controls, and IMHO we shouldn't change that to avoid confusion (or if we do, we should style them better (the focus state is almost impossible to see, certainly on OS X) and make sure they are accessible :-) ).

Comment 2

[Ray Lin[:ralin]]

(In reply to :Gijs (gone until 3 jan) from comment #1)
> I admit to not having followed bug 1271765 in full all the time, but was
> there an explicit decision to make these controls focusable? They weren't in
> the old video controls, and IMHO we shouldn't change that to avoid confusion
> (or if we do, we should style them better (the focus state is almost
> impossible to see, certainly on OS X) and make sure they are accessible :-)
> ).

There's no plan to change the focusability in bug 1271765, which should simply a styling work. To make the behavior align with original <video>, I guess we might need a css fix to avoid controls being focused since we didn't address this issue while converting xul to html.

Comment 3

[Marcia Knous [:marcia]]

Tracking 53+ for the reasons in Comment 1.

Comment 4

[:Gijs (he/him)]

I think this is fixable by just setting the HTML tabindex attribute to -1 for all of the controls.

Comment 6

[Ray Lin[:ralin]]

Attachment: Bug 1325594 - Prevent controls being tabbed inside video control.

Review commit: https://reviewboard.mozilla.org/r/102106/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/102106/

Comment 7

[Ray Lin[:ralin]]

Hi Gijs,

I got a quick fix to this bug per your comment, could you help me to review it? Thanks!!

Comment 8

[:Gijs (he/him)]

I'm passing this on to Mike because my queue is quite full, and because I made the suggestion you've used, so it's probably good if someone else reviews it rather than me rubberstamping what I had suggested already. :-)

Comment 9

[Mike de Boer [:mikedeboer]]

Ray, whilst comparing Fx 51 controls to the ones in Nightly (old vs. new), I see the following:

OLD:
* The video element itself can be tabbed to, but the next tab hit moves the focus to the Closed Captions button (no focus state, but hitting space toggles the menu).
* Another tab hit moves focus to the fullscreen button.
* The third tab hit moves focus out of the video element.

NEW:
* The video element itself can be tabbed to and the next tab hit moves the focus to the play button. Somehow, pressing spacebar makes the video play & stop quickly.
* The next tab hit moves the focus to the throbber and I can use the arrow keys to navigate the movie (if I'm fast enough before the controls hide).
* The next tab hit moves the focus to the mute button, etc, etc.

I'd recommend to add the tabindex=-1 property to all the controls *except* the CC and fulscreen buttons, which'll match the old sceneario.
What do you think?

Comment 10

[:Gijs (he/him)]

I also realized that the CSS that made these unfocusable in the old version is still lying around:

https://dxr.mozilla.org/mozilla-central/source/toolkit/content/widgets/videocontrols.css#13-18

if we decide to use tabindex we should remove it, or we can update it so the selectors work again (assuming that's a problem right now).



Note that this CSS was added before the controls had a fullscreen and CC button. ( https://hg.mozilla.org/mozilla-central/file/324d492445e3/toolkit/content/widgets/videocontrols.xml ). In bug 486899 (which is that rev) the inner controls were made non-focusable to avoid issues with them - but all the functionality of those controls (play/pause/volume/mute) is keyboard-accessible anyway. This is documented: https://support.mozilla.org/en-US/kb/keyboard-shortcuts-perform-firefox-tasks-quickly?bl=n&redirectlocale=en-US&redirectslug=Keyboard+shortcuts#w_media-shortcuts .

We don't have real keyboard equivalents for the CC and full screen buttons on video specifically. Mike is right that we should therefore probably omit those and fix them in a separate bug. Without reading all of 486899 right now, I still suspect we will want to do the same thing with them, but we should loop in some a11y folks and check.

Comment 11

[Ray Lin[:ralin]]

Comment on attachment 8823568 [details]
Bug 1325594 - Prevent controls being tabbed inside video control.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/102106/diff/1-2/

Comment 12

[Ray Lin[:ralin]]

> I'd recommend to add the tabindex=-1 property to all the controls *except*
> the CC and fulscreen buttons, which'll match the old sceneario.
> What do you think?

It makes sense. Sorry for I just inferred that all controls should be updated at once... Patch updated, could you help to review it again? Thanks

-----

Hi Gijs,

> if we decide to use tabindex we should remove it, or we can update it so the
> selectors work again (assuming that's a problem right now).
I'd keep them both for now as mobile video control still need these css. 

Thanks for giving the context about accessibility. I agree that we should omit it until we come up with a better way to make cc & fullscreen btn accessible (in a separate bug).

Comment 13

[Mike de Boer [:mikedeboer]]

Comment on attachment 8823568 [details]
Bug 1325594 - Prevent controls being tabbed inside video control.

https://reviewboard.mozilla.org/r/102106/#review102860

I like it! Thanks, Ray!

Comment 14

[Mike de Boer [:mikedeboer]]

Ray, can you make sure that Gijs' remarks about a solid a11y story for the video controls is captured in (a) bug(s)?

Thanks!

Comment 15

[Ray Lin[:ralin]]

(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Ray, can you make sure that Gijs' remarks about a solid a11y story for the
> video controls is captured in (a) bug(s)?
> 
> Thanks!

np, I'll file a bug for it :-)

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4ad7e63e3cc0
Prevent controls being tabbed inside video control. r=mikedeboer

Comment 17

[Iris Hsiao [:ihsiao]]

https://hg.mozilla.org/mozilla-central/rev/4ad7e63e3cc0

Comment 18

[Hani Yacoub, Desktop QA]

Tested this issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11 on Firefox Nightly 53.0a1 and I confirm that the new player match the functionality with the old one.

Comment 19

[Mike de Boer [:mikedeboer]]

Thank you!