Closed Bug 1325594 Opened 5 years ago Closed 5 years ago

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

Categories

(Toolkit :: Video/Audio Controls, defect)

53 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla53
Tracking Status
firefox52 --- unaffected
firefox53 + verified

People

(Reporter: hani.yacoub, Assigned: ralin)

References

Details

(Keywords: access, regression)

Attachments

(1 file)

[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.
Blocks: 1271765
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 :-) ).
Flags: needinfo?(jaws)
Keywords: access, regression
(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.
Tracking 53+ for the reasons in Comment 1.
I think this is fixable by just setting the HTML tabindex attribute to -1 for all of the controls.
Duplicate of this bug: 1327289
Hi Gijs,

I got a quick fix to this bug per your comment, could you help me to review it? Thanks!!
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Attachment #8823568 - Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
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. :-)
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?
Flags: needinfo?(jaws) → needinfo?(ralin)
Attachment #8823568 - Flags: review?(mdeboer)
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.
> 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).
Flags: needinfo?(ralin)
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!
Attachment #8823568 - Flags: review?(mdeboer) → review+
Keywords: checkin-needed
Ray, can you make sure that Gijs' remarks about a solid a11y story for the video controls is captured in (a) bug(s)?

Thanks!
Flags: needinfo?(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 :-)
Flags: needinfo?(ralin)
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ad7e63e3cc0
Prevent controls being tabbed inside video control. r=mikedeboer
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/4ad7e63e3cc0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
See Also: → 1329555
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.
Status: RESOLVED → VERIFIED
Thank you!
Depends on: 1327891
You need to log in before you can comment on or make changes to this bug.