Closed Bug 1545924 Opened 7 months ago Closed 2 months ago

Picture-in-Picture toggle icon should not display if set media.videocontrols.picture-in-picture.video-toggle.enabled = false

Categories

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

68 Branch
Desktop
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- wontfix
firefox68 --- wontfix
firefox69 - wontfix
firefox70 + verified
firefox71 --- verified

People

(Reporter: alice0775, Assigned: mconley)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

Reproduce: always

Steps To Reproduce:

  1. set media.videocontrols.picture-in-picture.enabled to false
  2. Open any video

Actual Results:
Picture-in-Picture toggle icon display

Expected Results:
Picture-in-Picture toggle icon should not display

Blocks: videopip
Priority: -- → P3

On 69 release version with both media.videocontrols.picture-in-picture.enabled and media.videocontrols.picture-in-picture.video-toggle.enabled set to false:

While any video loads, an empty transparent player of default size is shown. In that player the blue toggle is visible, may be moused-over (it expands to the left) and clicked (if you're fast enough) which displays a PiP notification instead of the video ("this video...") and opens a broken empty window in the task bar.
Once the video loads in, the toggle is gone and I have no other way of accessing PiP functionality (as it should be). I suspect the toggle is shown and loaded regardless of the about:config flags and is simply behind the video instead of being completely disabled. Please remove it while PiP is disabled.

Thanks David,

Yes, I can reproduce this when viewing http://commondatastorage.googleapis.com/gtv-videos-bucket/sample/BigBuckBunny.mp4 with those prefs set to false.

Priority: P3 → P2

[Tracking Requested - why for this release]:

This is an unfortunate visual glitch where we'll show the "Picture-in-Picture" toggle for a split second when videos are still in the midst of loading, even when the PiP feature is disabled.. This is probably exacerbated on slower connections where it takes longer to load the video.

Assignee: nobody → mconley
Priority: P2 → P1

The good news is that, from what I can tell, this only affects videos that have controls="true", so doesn't seem to impact popular video sites like YouTube, Facebook, Amazon Prime, Twitch or Netflix.

Does that match your experience, David?

Flags: needinfo?(durlag.trolltoeter)

Yes, I've only noticed it either loading video files directly in a new tab or on sites that embed using the default player. Of those you mentioned I only use YouTube and I haven't seen the blue toggle show up there at all. And yes, loading time is a significant factor. When I play a local file it loads instantly and doesn't show the toggle. My connection isn't great, but I've noticed it more when the host is slow to respond. Once the file is locally cached on second viewing it also loads instantly, not revealing the toggle.

For reference: When I look at the source code of a site where I notice the toggle the embedding for a webm looks like
<video controls="" loop="" autoplay="" src=[...]></video>

Flags: needinfo?(durlag.trolltoeter)

This patch centralizes the logic where the videocontrols UAWidget removes the
"hidden" attribute from the Picture-in-Picture toggle, and makes sure we only
do this if the PiP toggle is enabled.

[Tracking Requested - why for this release]:

See comment 4.

This bug kinda got away from the original summary / description, so I'll clone it out.

Summary: Picture-in-Picture toggle icon should not display if set media.videocontrols.picture-in-picture.enabled = false → Picture-in-Picture toggle icon should not display if set media.videocontrols.picture-in-picture.video-toggle.enabled = false

Cloned this bug to bug 1580488 for the original issue.

Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4418fc5c5efd
Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r=JSON_voorhees
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Comment on attachment 9091451 [details]
Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

Beta/Release Uplift Approval Request

  • User impact if declined: Users may see the Picture-in-Picture toggle on video elements briefly when a video is still loading, even if the feature is not enabled.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: No
  • If yes, steps to reproduce:
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): This is a simple, well-understood patch that centralizes the logic for displaying (or hiding) the Picture-in-Picture toggle on videos, where before it was spread out over two places (and the second place allowed the toggle to display sometimes).
  • String changes made/needed: None
Attachment #9091451 - Flags: approval-mozilla-beta?

Comment on attachment 9091451 [details]
Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

Avoids showing a control if the feature is disabled. OK for uplift for beta 7.

Attachment #9091451 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Verified - fixed on latest Nightly 71.0a1 (2019-09-15) (64-bit) on Windows 10 x64.
Setting the media.videocontrols.picture-in-picture.video-toggle.enabled to false will disable the toggle for PiP. Enforced slow internet connectivity using network throttling.

Waiting for uplift to Beta7.

QA Whiteboard: [qa-triaged]

Verified - fixed on latest Beta 70.0b17 on Windows 10 x64.
Closing this as verified - fixed.

Status: RESOLVED → VERIFIED
QA Whiteboard: [qa-triaged]
Flags: qe-verify+

Is this something we should fix on ESR68 still?

Flags: needinfo?(mconley)

Bah, I apologize - I forgot that this bug doesn't depend on PiP being enabled.

This isn't a critical security or performance fix, so I don't think it qualifies as something we'd fix for ESR. I'm going to suggest we WONTFIX.

I spoke with RyanVM over IRC, and actually considering how early we are in the ESR release cycle, we're willing to take this. I'll request uplift.

Comment on attachment 9091451 [details]
Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: We're still early enough in the ESR release cycle (and before the auto-upgrades from 60), and we also feel that the patch is low-risk enough to warrant consideration.
  • User impact if declined: Videos with the controls attribute on them can, for a small period of time while loading, show the Picture-in-Picture toggle if the mouse is hovering the video, despite that feature not being enabled.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a simple patch that's been on Nightly for quite a while, with no recurrence of the problem being observed or reported.
  • String or UUID changes made by this patch: None.
Attachment #9091451 - Flags: approval-mozilla-esr68?

Comment on attachment 9091451 [details]
Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

Low risk fix to avoid a visual glitch with PiP mode. Approved for 68.2esr.

Attachment #9091451 - Flags: approval-mozilla-esr68? → approval-mozilla-esr68+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

The patch fails to apply. Bug 1547795 and bug 1555834 have not landed on (ESR) 68. Please provide a patch.

Flags: needinfo?(mconley)

Comment on attachment 9091451 [details]
Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

Per IRC discussion with mconley, it's not worth the rebase effort to fix this on ESR68.

Flags: needinfo?(mconley)
Attachment #9091451 - Flags: approval-mozilla-esr68+ → approval-mozilla-esr68-

Removing qe flags based on comment 26.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.