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

Status: VERIFIED FIXED

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

Description

[Alice0775 White]

Attachment: Picture-in-Picture toggle icon

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

Comment 2

[David]

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.

Comment 3

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

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.

Comment 4

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

[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.

Comment 5

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

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?

Comment 6

[David]

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>

Comment 7

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

Attachment: Bug 1545924 - Make sure to never unhide the Picture-in-Picture toggle if the feature is disabled. r?JSON_Voorhees

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.

Comment 8

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

[Tracking Requested - why for this release]:

See comment 4.

Comment 9

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

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

Comment 10

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

Cloned this bug to bug 1580488 for the original issue.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
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

Comment 12

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/4418fc5c5efd

Comment 13

[Mike Conley (: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

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

Comment 14

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

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.

Comment 15

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/releases/mozilla-beta/rev/ceee9e229566

Comment 16

[Timea Cernea [:tbabos][inactive]]

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.

Comment 17

[Timea Cernea [:tbabos][inactive]]

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

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Is this something we should fix on ESR68 still?

Comment 19

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

Nope - Picture-in-Picture is not available for ESR68.

Comment 20

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

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

Comment 21

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

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.

Comment 22

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

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 23

[Mike Conley (: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

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.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

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

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 27

[Timea Cernea [:tbabos][inactive]]

Removing qe flags based on comment 26.