3

Add a keyboard shortcut for PiP full screen mode

 1 year ago
source link: https://bugzilla.mozilla.org/show_bug.cgi?id=1724915
Go to the source link to view the article. You can view the picture content, updated content and better typesetting reading experience. If the link is broken, please click the button below to view the snapshot at that time.
neoserver,ios ssh client
Closed Bug 1724915 Opened 2 years ago Closed 8 days ago

Add a keyboard shortcut for PiP full screen mode

Categories

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

Toolkit ▾
Picture-in-Picture ▾

Firefox 91

Tracking

(bug RESOLVED as FIXED awaiting an answer on a request for information)

RESOLVED FIXED

114 Branch

Tracking Status
firefox114 --- fixed

People

(Reporter: aros, Assigned: bnasar, NeedInfo)

References

Details

(Whiteboard: [fidefe-pip3])

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

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)
You need to log in before you can comment on or make changes to this bug.

About Joyk


Aggregate valuable and interesting links.
Joyk means Joy of geeK