Closed Bug 1724915 Opened 3 years ago Closed 1 year ago

Add a keyboard shortcut for PiP full screen mode

Categories

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

Firefox 91
enhancement

Tracking

()

RESOLVED FIXED
114 Branch
Tracking Status
firefox114 --- fixed

People

(Reporter: aros, Assigned: bnasar)

References

Details

(Whiteboard: [fidefe-pip3])

Attachments

(1 file)

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.

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.

Component: Untriaged → Video/Audio Controls
Product: Firefox → Toolkit
Severity: -- → S4
Component: Video/Audio Controls → Picture-in-Picture
Priority: -- → P4

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.

Summary: Going full screen from picture-in-picture mode → Add a keyboard shortcut for PiP full screen mode
Assignee: nobody → bnasar
Status: UNCONFIRMED → NEW
Ever confirmed: true
Whiteboard: [fidefe-pip3]

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
Pushed by bnasar@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/b2afadd8b2fc
Added a keyboard shortcut to enable full screen mode in PiP. r=pip-reviewers,kpatenio
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 114 Branch

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?

Flags: needinfo?(nbaumgardner)

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.

Flags: needinfo?(francesco.lodolo)

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.

Flags: needinfo?(francesco.lodolo)
Blocks: 1831966

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

Flags: needinfo?(asafko)

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!

Flags: needinfo?(asafko)
Flags: needinfo?(nbaumgardner)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: