Closed Bug 1333008 Opened 7 years ago Closed 7 years ago

Pressing the space key do nothing after clicking Play/Pause button from the media control panel

Categories

(Toolkit :: Video/Audio Controls, defect)

53 Branch
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla54
Tracking Status
firefox53 + fixed
firefox54 --- verified

People

(Reporter: hyacoub, Assigned: ralin)

References

Details

Attachments

(1 file)

[Affected versions]: 
Nightly 53.0a1

[Affected platforms]:
All platforms: Windows 10 x 64, Ubuntu 16.04

[Steps to reproduce]:
1. Launch Nightly 53.0a1 with a new profile and then open this link https://people-mozilla.org/~ralin/vtt/.
2. Click on the play button from the control panel.
3. Press space key.

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

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

[Note]
It's not reproducible on Mac OS X 10.11
Blocks: 1271765
I suspect that on Windows & Ubuntu, the <video> element isn't focused after clicking(on play button). Sorry that I don't get a Windows environment on hand right now, I'll check on it tomorrow. 

Thanks.
Flags: needinfo?(ralin)
This was mentioned by SV as partially blocking "Video Play Visual Refresh" feature. Tracked for 53+
Video actually play->pause->play or pause->play->pause when pressing space in a flash. 

After a quick check, I got the result: 

Mac OS : only <video> is focused. Press space, then play/pause depends on current state [0]
Windows: <video>, <videocontrols>, <button>(play button), are all focused. Similar to Mac OS, but pressing space trigger 'click' on play button as well. On the one hand, <video> handler play the video, on the other hand, <button> pause the video right away. (vice versa)
Ubuntu : haven't verified yet. Should be same as Windows


Two thoughts:
1. disable the focusability of all controls (except CC and Fullsceen Button, see Bug 1329555). Apparently, it make more sense as that is how videocontrols works in Mac OS.
2. prevent `keypress` event from default behavior. 


[0] https://dxr.mozilla.org/mozilla-central/rev/3cedab21a7e65e6a1c4c2294ecfb5502575a46e3/toolkit/content/widgets/videocontrols.xml#1320
Hi Jared,

I initially thought we could use `-moz-user-focus: ignore` to avoid focusing on the controls, and after a while trying, I found another bug 379939 addressing `-moz-user-focus` issue on HTML elements.

Not sure is it a good way to call "event.target.blur()" right away while controls being focused or "preventDefault" to "keypress"?
Could you give me some suggestions? Thanks.
Flags: needinfo?(ralin)
Flags: needinfo?(jaws)
I believe the reason you can't reproduce on OSX is because by default OSX has Full Keyboard Access for All Controls disabled (see https://www.macobserver.com/tmo/article/mac_os_x_the_power_of_full_keyboard_access for how to enable it).

I don't think we should be removing keyboard focus from these buttons. I would prefer instead if we made it easier to use the keyboard with video controls, though that is probably more work for another bug.

The "space" key triggers whichever button has focus. It also is being used to play/pause the video. In videocontrols.xml's keyHandler function, inside of the switch case for "space", we should check if the event.originalTarget's localName is "button" and if the button is not disabled. If the originalTarget is an enabled button, then we should break out of the switch statement and not call togglePause().
Flags: needinfo?(jaws)
Assignee: nobody → ralin
Status: NEW → ASSIGNED
Comment on attachment 8838024 [details]
Bug 1333008 - Don't toggle play or pause when button is focused.

https://reviewboard.mozilla.org/r/113026/#review114532

r=me with the below issue fixed.

::: toolkit/content/widgets/videocontrols.xml:1358
(Diff revision 1)
>          try {
>            switch (keystroke) {
>              case "space": /* Play */
> +              let target = event.originalTarget;
> +              if (target.localName === "button" && !target.disabled) {
> +                return;

This should be `break;` instead of `return;`. Note that at the bottom of this function we call `event.preventDefault();` to prevent page scrolling.
Attachment #8838024 - Flags: review?(jaws) → review+
Also, it would be good to include a test here.
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Also, it would be good to include a test here.

Thank you Jared :)

issues are fixed
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/75c3c966cf29
Don't toggle play or pause when button is focused. r=jaws
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/75c3c966cf29
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8838024 [details]
Bug 1333008 - Don't toggle play or pause when button is focused.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1271765
[User impact if declined]: cannot toggle video play/pause by press space on play button
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: only add a specified condition for early return 
[String changes made/needed]: none

Thanks :)
Attachment #8838024 - Flags: approval-mozilla-aurora?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Build ID: 20170221030205
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:54.0) Gecko/20100101 Firefox/54.0

Verified as fixed on Windows 10 x 64, Mac OS X 10.10 and Ubuntu 16.04 x64 on Firefox Nightly 54.0a1.
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
Comment on attachment 8838024 [details]
Bug 1333008 - Don't toggle play or pause when button is focused.

Fix an issue related to video play/pause control by pressing space key and was verified. Aurora53+.
Attachment #8838024 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Setting qe-verify- based on Ray's assessment on manual testing needs (see Comment 13) and the fact that this fix has automated coverage.

However, this is a good candidate for the community. Flagging it accordingly.
QA Whiteboard: [good first verify]
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.