Closed
Bug 1325594
Opened 8 years ago
Closed 8 years ago
Pressing the space key do nothing when the focus is on the Play/Pause button
Categories
(Toolkit :: Video/Audio Controls, defect)
Tracking
()
VERIFIED
FIXED
mozilla53
Tracking | Status | |
---|---|---|
firefox52 | --- | unaffected |
firefox53 | + | verified |
People
(Reporter: hyacoub, 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.
Comment 1•8 years ago
|
||
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
Updated•8 years ago
|
status-firefox52:
--- → unaffected
tracking-firefox53:
--- → ?
Assignee | ||
Comment 2•8 years ago
|
||
(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 4•8 years ago
|
||
I think this is fixable by just setting the HTML tabindex attribute to -1 for all of the controls.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 7•8 years ago
|
||
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
Updated•8 years ago
|
Attachment #8823568 -
Flags: review?(gijskruitbosch+bugs) → review?(mdeboer)
Comment 8•8 years ago
|
||
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•8 years ago
|
||
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)
Updated•8 years ago
|
Attachment #8823568 -
Flags: review?(mdeboer)
Comment 10•8 years ago
|
||
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 hidden (mozreview-request) |
Assignee | ||
Comment 12•8 years ago
|
||
> 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 13•8 years ago
|
||
mozreview-review |
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+
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 14•8 years ago
|
||
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)
Assignee | ||
Comment 15•8 years ago
|
||
(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)
Comment 16•8 years ago
|
||
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
Comment 17•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/4ad7e63e3cc0
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Reporter | ||
Comment 18•8 years ago
|
||
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
You need to log in
before you can comment on or make changes to this bug.
Description
•