1724915 - Add a keyboard shortcut for PiP full screen mode

Status: RESOLVED FIXED

(Toolkit :: Picture-in-Picture, enhancement, P4)

Description

[Artem S. Tashkinov]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101 Firefox/91.0

Steps to reproduce:

It would be great if it were possible to go full screen from picture-in-picture mode right away without 1) disabling picture in picture mode 2) going back to the original web page 3) hitting go full screen - these are 3 steps instead of one.

And while we're at it, it would be great if "F" or "Alt + Enter" shortcuts did that as well.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

The Bugbug bot thinks this bug should belong to the 'Toolkit::Video/Audio Controls' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 2

[kpatenio]

We now support viewing a PiP video in full screen mode. This can be done by either 1) double clicking on the PiP window or 2) selecting the full screen button if you are using Nightly and/or have the pref media.videocontrols.picture-in-picture.improved-video-controls.enabled.

All that said, I think adding a new keyboard shortcut for full screen mode would be a pretty interesting addition. I'll rename this bug to reflect that request.

Comment 3

[kpatenio]

Writing some detailed steps on how we could add support for the shortcut.

I thought more about how we can implement it. It may just be a matter of simply adding logic for toggling on/off full screen mode within this code block. Here are my suggested steps:

1. Verify that the event keyCode is for the F key (see KeyEvent)
2. If the F key is detected, call this.fullscreenModeToggle

We should also add a test to make sure the shortcut works as expected. Let's use this existing test as reference. The most important steps are the following:

1. First load a test page
2. Enable PiP for video on the page
3. Ensure PiP becomes full screen after pressing the F key via promiseFullscreenEntered. The async function we pass should be EventUtils.synthesizeKey (maybe we can just use "f" as the key like this?)
4. Verify that pipWin.document.fullscreenElement is the PiP window
5. Then ensure PiP is no longer full screen after pressing F again, this time via promiseFullscreenExited.
6. Verify that pipWin.document.fullscreenElement is no longer set
7. Clean up the test by closing the PiP window and tab

Comment 4

[Bilal]

Attachment: Bug 1724915 - Added a keyboard shortcut to enable full screen mode in PiP. r?#pip-reviewers

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b2afadd8b2fc
Added a keyboard shortcut to enable full screen mode in PiP. r=pip-reviewers,kpatenio

Comment 6

[Noemi Erli[:noemi_erli]]

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

Comment 7

[Daniel Bodea [:danibodea]]

Now that the shortcut has been added, maybe the button's hover tooltip should be changed to "Fullscreen (f)" or "Fullscreen (double-click/f)".
QA suggests the first variant as double-click is the commonly known (instinctively expected) action to go into or out of full-screen mode.
"Exit fullscreen (double-click)" should also be changed along to match.

What do you think, Niklas?

Comment 8

[Ania]

Hi Flod,
Would you have concerns about the localization of the tooltip hover:
"Fullscreen (double-click or F)"? It seems like we're using "Cmd + latin script letter" shortcuts for many features, but hoped to double check before we add it.

Comment 9

[Francesco Lodolo [:flod]]

We tend to always expose shortcuts, in case they need to be localized. I don't expect there will be a need for a letter like F, but having the flexibility is a safeguard. Instead, this bug hard-coded F (KeyEvent.DOM_VK_F).

Strings like

pictureinpicture-fullscreen-btn =
  .aria-label = Fullscreen
  .tooltip = Fullscreen (double-click)

Will need to be changed to use a variable for the shortcut. At that point, I'd suggest exposing the shortcut as well.

As for your question on having a letter without modifiers as shortcut, I feel that's more a UX question that localization.

Comment 10

[Pascal Chevrel:pascalc]

Ania, should we mention that in the 114 beta release notes?

Comment 11

[Ania]

We probably should hold off until we finish [https://bugzilla.mozilla.org/show_bug.cgi?id=1831966](this ticket).
I'll make sure to nominate that one for release notes!