1566183 - Picture-in-Picture player window can't be accessed with keyboard only

Status: VERIFIED FIXED

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

Description

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

Steps to reproduce:

1. Launch Nightly
2. Go to Youtube https://youtube.com
3. Select any video to play
4. Use only the keyboard to enable the Picture-in-Picture window using the context menu or the toggle.

Expected Result:
The PiP should be accessible with keyboard only.

Actual Result:
The PiP can't be enabled using keyboard only.

Comment 1

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

Attachment: Bug 1566183 - Add a keyboard shortcut to enter Picture-in-Picture for first video of focused window. r?NeilDeakin,JSON_Voorhees

Comment 2

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

I have a patch up here that adds a keyboard shortcut, Ctrl-Alt-P / Cmd-Alt-P to enter Picture-in-Picture for the first found video in the focused frame of the selected tab. I'm in the process of reaching out to UX to find out whether or not this keyboard shortcut needs to be run by anyone first - UX, localization, etc. Once I have that information, I'll request review.

Comment 3

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

Hi Asa, you were suggested as a potential source of feedback for the keyboard shortcut here. Does my selection of Ctrl/Cmd-Alt-P make sense, or should we go for something else?

Comment 4

[Asa Dotzler [:asa]]

Though this shortcut would overlap with the shortcut for presentation mode for PDFs in Firefox, practically I don't think that would be a significant problem.

I've looked around (macOS and Chrome PiP modes) and don't see any existing shortcuts to emulate for PiP.

I've also checked and this shortcut doesn't seem to be used very often in other apps. It does show up some, for example in Word's switch to print layout view and OneNote for play audio recordings, but I don't think that should cause any significant confusion.

Shorlander had some good questions:

• Does it conflict with another popular or expected shortcut? No.
  -- Related: Is there already a commonly accepted shortcut for this action? No.
• Does it work across operating systems? Yes.
• Is it somewhat easy to activate? (i.e. how much do you have to contort your hand to do it?) Requires two hands so not the most convenient but that's true of many of our current shortcuts.

I think we can go with this.

Comment 5

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

ni?ing Neil about https://phabricator.services.mozilla.com/D40082#1216193

Comment 6

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

Hey Asa,

According to Neil in https://phabricator.services.mozilla.com/D40082#1215968, our original keyboard shortcut proposal (Ctrl+Alt+P) may not be workable because Ctrl+Alt+P maps to AltGr+P on some keyboards, which is a printable character.

Is Ctrl+Shift+V acceptable instead?

Comment 7

[Asa Dotzler [:asa]]

Ctrl+Shift+V is an existing Firefox shortcut for paste without formatting. Unless I'm missing something obvious, a reasonable assumption :) I can imagine scenarios where a page had a video and a text form that could make re-using this shortcut problematic.

Comment 8

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

(In reply to Asa Dotzler [:asa] from comment #7)

Ctrl+Shift+V is an existing Firefox shortcut for paste without formatting.

Shoot, good point. :( Alright let me see if I can find another...

Comment 9

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

Ctrl+Shift+} doesn't appear taken. Can I use that? (I'm basically picking characters at random though, unless anybody has a better suggestion.)

Comment 10

[Asa Dotzler [:asa]]

(In reply to Mike Conley (:mconley) (:⚙️) (Catching up from PTO) from comment #9)

Ctrl+Shift+} doesn't appear taken. Can I use that? (I'm basically picking characters at random though, unless anybody has a better suggestion.)

ctrl+shift+] is "bring to front" for some popular design applications so it's kind of on point. I'm looking into a possible conflict with JAWS, the popular screen reader. Will report back soon.

Comment 11

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

Hi Asa, any updates on whether ctrl+shift+] makes sense as the keyboard shortcut?

Comment 12

[Asa Dotzler [:asa]]

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

Hi Asa, any updates on whether ctrl+shift+] makes sense as the keyboard shortcut?

I think this makes sense. Note, for some people using the JAWS screen reader, the keyboard shortcut will be usurped for screen reader functionality.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bb08767934a8
Add a keyboard shortcut to enter Picture-in-Picture for first video of focused window. r=NeilDeakin,JSON_voorhees,flod

Comment 14

[Razvan Maries]

Backed out for causing perma fails on browser_duplicateIDs.js

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=265182262&resultStatus=testfailed%2Cbusted%2Cexception&revision=bb08767934a86eabb6127ce3759e70d3fa083771

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=265182262&repo=autoland&lineNumber=2158

[task 2019-09-05T17:01:20.607Z] 17:01:20 INFO - TEST-START | browser/base/content/test/general/browser_duplicateIDs.js
[task 2019-09-05T17:01:20.745Z] 17:01:20 INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_toggleReaderMode should be unique -
[task 2019-09-05T17:01:20.745Z] 17:01:20 INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_togglePictureInPicture should be unique -
[task 2019-09-05T17:01:20.745Z] 17:01:20 INFO - Buffered messages finished
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/general/browser_duplicateIDs.js | key_togglePictureInPicture should be unique -
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - Stack trace:
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochikit/content/browser-test.js:test_ok:1580
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js:test/<:7
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochitests/content/browser/browser/base/content/test/general/browser_duplicateIDs.js:test:3
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1387
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochikit/content/browser-test.js:nextTest/<:1213
[task 2019-09-05T17:01:20.746Z] 17:01:20 INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:805
[task 2019-09-05T17:01:20.747Z] 17:01:20 INFO - TEST-PASS | browser/base/content/test/general/browser_duplicateIDs.js | key_reload should be unique -

Backout: https://hg.mozilla.org/integration/autoland/rev/40265561197b8f035773576eed117dc2d1321ae6

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbcd1724fe42
Add a keyboard shortcut to enter Picture-in-Picture for first video of focused window. r=NeilDeakin,JSON_voorhees,flod

Comment 16

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/dbcd1724fe42

Comment 17

[Cristian Craciun]

Build ID 20190906094324
User Agent Mozilla/5.0 (Windows NT 10.0; rv:71.0) Gecko/20100101 Firefox/71.0

Verified as fixed on the latest Nightly build.