1620899 - Arrow keys don't work in picture-in-picture window after tabbing unless play/pause is pressed

Status: VERIFIED FIXED

(Toolkit :: Video/Audio Controls, defect, P3)

Description

[James Teh [:Jamie]]

STR:

1. Open https://www.youtube.com/watch?v=dQw4w9WgXcQ
2. Start playing (and enjoying) the video.
3. Press control+shift+] to switch to picture-in-picture. The picture-in-picture window gets focus.
4. Press right arrow to fast forward.
   • Observe: It works. Yay!
5. Press tab.
6. Press right arrow to fast forward.
   • Observe: It doesn't work.
7. Tab a few more times to get back to the pause button.
8. Press right arrow to fast forward.
   • Expected: It should fast forward.
   • Actual: Nothing happens.
9. Press space to pause the video.
10. Press space again to play.
11. Press right arrow.
    • Observe: It works. Yay!

Not supporting this when not focused on the play/pause button (step 6) is okay, though I'm not sure if this restriction is really necessary and it could make life easier if it weren't thus restricted. That said, we obviously can't support the space shortcut in this case, since that would break expected activation of the other buttons when they're focused.
However, the real problem here is that even after returning to the play/pause button, the arrow keys don't work. Aside from being annoying, it can leave users with the impression (as it did initially for me) that the keyboard shortcuts don't work at all (or not reliably), since a user won't necessarily think to try pressing play/pause before using them.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Hey gl, do you have time to look at this? I guess we should probably only be opting to redirect space / return when focusing moves to another button in the player, but the rest of the keyboard shortcuts should get sent down to the underlying video.

Comment 2

[Gabriel Luong [:gl] (ΦωΦ)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #1)

Hey gl, do you have time to look at this? I guess we should probably only be opting to redirect space / return when focusing moves to another button in the player, but the rest of the keyboard shortcuts should get sent down to the underlying video.

Hey Mike, I will assign myself and take a look when I get a cycle.

Comment 3

[Asa Dotzler [:asa]]

Updating the Accessibility Team's impact assessment to conform with the new triage guidelines. See https://wiki.mozilla.org/Accessibility/Triage for descriptions of these whiteboard flags.

Comment 4

[Firefox Bug Husbandry Bot]

Because this bug's Severity has not been changed from the default since it was filed, and it's Priority is P3 (Backlog,) indicating it has been triaged, the bug's Severity is being updated to S3 (normal.)

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Hey Reid,

By default, the keyboard shortcuts for seeking the video are disabled. You'll need to enable them first. Type about:config in the URL bar, press enter, click past the warning, then search for this preference:

media.videocontrols.picture-in-picture.keyboard-controls.enabled

and toggle it to true to turn on the keyboard controls.

Here is the handler for the keydown event for the PiP player window: https://searchfox.org/mozilla-central/rev/8f97d6d022bd18d4c3138bf5f13f4144ac334d82/toolkit/components/pictureinpicture/content/player.js#199-221

I think the logic here needs to be updated: https://searchfox.org/mozilla-central/rev/8f97d6d022bd18d4c3138bf5f13f4144ac334d82/toolkit/components/pictureinpicture/content/player.js#211-213

Let me explain what we're doing here. We've got two modes of keyboard interaction:

1. Tabbing around to the different buttons in the UI. This is named "keying" mode, for some reason.
2. Keyboard shortcuts that have nothing to do with the buttons, that are proxied down to the underlying <video> (for example, seeking)

The condition in the code that I highlighted above tries to make it so that we don't send commands down to the underlying <video> when we're in "keying" mode. I think we're being too restrictive here - we only want to avoid sending down those keyboard commands in "keying" mode if they were the spacebar. Others (like cursors) are okay to go through.

As part of this patch, I think it'd also be really prudent to add some keyboard shortcut tests. We don't have a lot of coverage for the keyboard shortcuts in the player window. Let's get the problem fixed, and then I'll guide you on how to write a test for it (and the rest of the keyboard shortcuts).

Comment 6

[Reid Shinabarker]

Attachment: Bug 1620899 - Arrow keys don't work in picture-in-picture window after tabbing unless play/pause is pressed. r?mconley,Gijs

Comment 7

[Reid Shinabarker]

Attachment: Error I get after a rebase

```

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Hey Reid,

Can you try doing ./mach build (instead of ./mach build faster), and see if that helps?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/45929f0dbda0
Arrow keys don't work in picture-in-picture window after tabbing unless play/pause is pressed. r=mconley,Gijs

Comment 10

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/45929f0dbda0

Comment 11

[Alin Ilea, Desktop Test Engineering]

Reproduced the initial issue using an old Nightly build 75.0a1 on Windows 10.
Verified - Fixed in latest Nightly 84.0a1 84.0a1 (2020-10-22) and Beta 83.0b3 (build id: 20201022171613).