1679174 - [PiP] The Ctrl + Shift + ] keyboard shortcut tries to open PiP on <video> with a NaN duration

Status: VERIFIED FIXED

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

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: shortcut-not-opening-PiP.gif

[Affected versions]:

• Firefox Release 83
• Firefox Beta 84.0b4
• Firefox Nightly 85.0a1

[Affected platforms]:

• Windows 10 x64
• Ubuntu 20.04 x64
• macOS 10.15

[Prerequisites]:

• Have the Firefox browser installed.

[Steps to reproduce]:

1. Open the Firefox browser and navigate to the https://www.pornhub.com page.
2. Open a video.
3. Make sure the video is paused.
4. Press "Ctrl+Shift+]" keys.
5. Observe what happens next.

[Expected result]:

• The PiP window is opened.

[Actual result]:

• The PiP window is not opened.

[Notes]:

• The PiP button changes its state as if the PiP window was opened.
• I didn't notice this issue on other video platforms such as Youtube, Twitch, Netflix, Hulu, HBO, or Amazon Prime.
• Attached a screen recording of the issue.

Comment 1

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

The problem here is that a number of the <video> elements on the site are "lazy", and so the video metadata hasn't loaded, and the durations are NaN when sorting here: https://searchfox.org/mozilla-central/rev/168c45a7acc44e9904cfd4eebcb9eb080e05699c/toolkit/actors/PictureInPictureChild.jsm#156-158

This sorting puts the NaN durations before any videos with durations. Thanks, JavaScript!

I think what we'd want to do is filter out any videos that have no durations before we do any of these calculations.

To whoever from MSU looks at this - you can reproduce this without needing to go to PornHub. I believe you can do this by putting a <video> element with no src attribute set earlier in the DOM than the "target" video.

Comment 2

[Micah [:mtigley] (she/her)]

Attachment: test_bug1679174.html

Another test case based on the suggestion in comment 1

Comment 3

[Micah [:mtigley] (she/her)]

Attachment: test_bug1679174_vid_with_no_src.html

Here's another test case that actually has a <video> with no src attribute set before the target video.

Interestingly, we can also reproduce the issue in the first test case I attached though. The first <video> there also has a NaN duration, so the proposed solution should also filter that out.

Comment 4

[Swapnik Katkoori]

Attachment: Bug 1679174 - Filter out videos with NaN duration. r=mtigley,mhowell

Comment 5

[Swapnik Katkoori]

Attachment: Bug 1679174 - Add test case. r=mtigley,mhowell

Comment 6

[Swapnik Katkoori]

Attachment: Bug 1679174 - Add line to test case. r=mtigley,mhowell

Depends on D105065

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e6062efb8f08
Filter out videos with NaN duration. r=mtigley

Comment 8

[Atila Butkovits]

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

Comment 9

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

Since the status are different for nightly and release, what's the status for beta?
For more information, please visit auto_nag documentation.

Comment 10

[Carmen Fat [:cfat] - Ecosystem QA]

• I have verified that this issue is no longer reproducible with the latest Firefox Beta (87.0b6 Build ID - 20210304190020) installed, on Windows 10 x64, Ubuntu 20.04 x64, and Mac 11.1.
• I can confirm that the Pip window is now opened using the shortcut when the videos attached are paused.