Closed Bug 1611634 Opened 4 years ago Closed 4 years ago

Picture-in-Picture keyboard shortcut should open the video currently playing if there's multiple videos on a page

Categories

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

74 Branch
Desktop
Linux
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: sdk, Assigned: sapoliakaran, Mentored, NeedInfo)

References

(Blocks 1 open bug)

Details

(Keywords: good-first-bug, nightly-community, Whiteboard: [lang=js])

Attachments

(1 file)

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

  1. Open a page with multiple video on it (e.g https://mozilla.github.io/open-leadership-training-series/articles/introduction-to-open-leadership/getting-to-know-mozilla-and-the-leadership-network/)
  2. Play the second video
  3. Open Picture-in-Picture

Actual results:

Picture-in-Picture opens and shows the first video in it's window

Expected results:

Picture-in-Picture should open the video currently playing in it's window

Blocks: videopip
OS: Unspecified → Linux
Hardware: Unspecified → Desktop
Version: 72 Branch → 74 Branch

I'm unable to reproduce this on Windows. The expected video moves to a PiP when clicking on the blue button that overlays the video. Are you entering PiP through a different method? Can you try with Firefox Nightly since this may have been already fixed ( http://nightly.mozilla.org/ )?

Flags: needinfo?(contact)

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #1)

I'm unable to reproduce this on Windows. The expected video moves to a PiP when clicking on the blue button that overlays the video. Are you entering PiP through a different method?

Aha. I tried with the toggle button and it works. So, it seems like this issue only happens if I use the keyboard shortcut.

Can you try with Firefox Nightly since this may have been already fixed ( http://nightly.mozilla.org/ )?

I'm always on Nightly ;)

Flags: needinfo?(contact)

(In reply to Danny Colin [:sdk] from comment #2)

Can you try with Firefox Nightly since this may have been already fixed ( http://nightly.mozilla.org/ )?

I'm always on Nightly ;)

Ah, I asked because comment #0 mentions Firefox 72 in the user agent string.

With the keyboard shortcut we can probably choose the first video that is playing at least.

Ah, I asked because comment #0 mentions Firefox 72 in the user agent string.

I wasn't on my main laptop when I filed this bugreport ;).

With the keyboard shortcut we can probably choose the first video that is playing at least.

Don't sure I'm following you. Do you mean that with the shortcut you can't choose the video
currently playing but only a specific <video> tag in the DOM (in this case the first one being the
default)?

What I'm trying to say is that instead of doing something roughly like document.querySelector("video") to find the video that should be moved to the new window, we can do Array.from(document.querySelectorAll("video")).filter(v => !v.paused) || document.querySelector("video").

This way if the keyboard shortcut is used we will pop out the first non-paused video if one exists, otherwise just the first video we find. Of course there is more nuance to it, in that we should only pop out videos that meet the criteria to be moved to a PiP window. You can see the criteria here, https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/toolkit/content/widgets/videocontrols.js#126-161

Summary: Picture-in-Picture should open the video currently playing if there's multiple videos on a page → Picture-in-Picture keyboard shortcut should open the video currently playing if there's multiple videos on a page
Mentor: jaws
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: good-first-bug
Whiteboard: [lang=js]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)

Would you like to submit a patch?

Sadly, I don't have enough free time to set a build environment and work on a patch currently.
Thanks for triaging it tho. Hopefully, with the good-first-bug this issue will gain more visibility ;).

Flags: needinfo?(contact)
Priority: -- → P5

Hi, I'd like to work on this, is that still open ?

Hi Julien! Yes, this bug is unassigned. Do you have a local build of Firefox set up to work on?

Flags: needinfo?(julienpan1234)

The code for this is at https://searchfox.org/mozilla-central/rev/2e355fa82aaa87e8424a9927c8136be184eeb6c7/toolkit/actors/PictureInPictureChild.jsm#1397-1400.

Would you like to submit a patch?

Hello Jared, I created the patch as suggested.
The suggested patch has one issue though.

If you pause the currently playing video in PiP mode, and then use the keyboard shortcut to close the PiP window, the first video in the page opens up in the PiP window instead of the PiP window closing. I think the user expects the window to close instead. Also, I think this happens because after using the shortcut to close the PiP window, the keyboard focus immediately goes to the body element and so our patch picks up the first video immediately, insted of closing the window. Let me know if we can address that within this patch, or should we create a separate bug for that. I have attached a gif of the case: https://gfycat.com/wetcleverkingfisher

Also, I think we should give keyboard focus the top priority. With the patch, when all videos are paused, only the first video in the page opens in PiP, even if someone has put keyboard focus on the second or third video in the page. For a person using keyboard as their primary input source, keyboard focus is the way of selecting things in the page. It might be unintuitive for them if the have used the tab key to put focus on a subsequent video, and on using the keyboard shortcut, the first video on the page opens up.

If a page has multiple videos, PiP keyboard shortcut opens the currently playing video.
Opens the first video, if no video is playing.

Assignee: nobody → sapoliakaran
Status: NEW → ASSIGNED

Kept this patch limited to opening first of current playing videos only, so as to keep the 1 bug per issue focus. Would like to discuss (and implement, if useful) keyboard focus priority with everyone's inputs :)

Someone can give me the link to download the local version of Firefox, please.

Flags: needinfo?(contact)
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0aa4f9852887
Picture-in-Picture keyboard shortcut should open currently playing video. r=jaws
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
Flags: needinfo?(contact)
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: