1742585 - PIP button z-index not respected with semi-transparent div

Status: VERIFIED FIXED

(Toolkit :: Picture-in-Picture, defect, P3)

Description

[Chocobozzz]

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

Steps to reproduce:

Create a video element, and add a semi-transparent div on the right.
Click on the PiP button that is below the div.

Actual results:

The PiP window is opened.

Expected results:

The PiP window should not be opened since there is a div with an higher z-index on top of the PiP button.

Comment 1

[Chocobozzz]

Attachment: Reproducing test case

Comment 2

[Chocobozzz]

It causes issues with custom settings dialog displayed on top of video elements (for example PeerTube: https://framatube.org/w/kkGMgK9ZtnKfYAgnEtQxbv). Weirdly, youtube doesn't seem affected. I don't know how they manage to not trigger the PiP button when the settings dialog is opened.

Comment 3

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

The Bugbug bot thinks this bug should belong to the 'Core::Widget: Gtk' component, and is moving the bug to that component. Please revert this change in case you think the bot is wrong.

Comment 4

[Martin Stránský [:stransky] (ni? me)]

Mike, do we even support that?
Thanks.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

IIRC this is by design.

Comment 6

[Emilio Cobos Álvarez (:emilio)]

https://searchfox.org/mozilla-central/rev/f574930f8d5f65fc856783ff27674ada781f4570/browser/extensions/pictureinpicture/data/picture_in_picture_overrides.js#56-58 is why it works on YT I guess. The default is 1 so we only allow opaque stuff to overlay over the video element. Maybe the default should be something a bit smaller?

Comment 7

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

Possibly it should be smaller. We have to balance carefully - some sites use semi-transparent overlays to represent the "paused" state of their videos, which we still want users to be able to use PiP through.

I did a quick test using a default visibility threshold of 0.8. This allows the testcase to not cause the toggle to work, while still keeping the toggle working on YouTube, Vimeo, DailyMotion, Apple Trailers, Twitch and a few news sites that I did a cursory test on.

If we did decide to decrease the threshold a little bit, we'd probably want this to be a pref in case we need to quickly rollout a fix.

What do you think, rtestard?

Test builds with 0.8 toggle visibility threshold:

Linux 64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/f_dpjJDBSout6uHy3OQbyA/runs/0/artifacts/public/build/target.tar.bz2
macOS: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/ILynuEEBQvqkBV8HwKOhiQ/runs/0/artifacts/public/build/target.dmg
Win 64: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/KJ2nSK15T3-vWS0JsU1E-w/runs/1/artifacts/public/build/target.zip

Comment 8

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

Hi Ania, this might be of some interest to you, too.

Comment 9

[Ania]

I am hesitant to introduce a pref for threshold transparency, as I imagine that users' knee-jerk reaction to the non-clickable PiP in the paused state button won't be going to Settings.

A cursory look into SUMO and Reddit doesn't show that users struggle with the current situation - from what I understand, at its worst, it creates a need for the secondary click on a setting that directly overlaps with the PiP button.

We can roll out an experiment with a default visibility threshold of 0.8 (thank you for the build, Mike, the threshold looks good) and see if it affects PiP usage. Still, since we don't have granular analytics, we'll have to rely on user reports to understand what video websites it breaks.

Mike, how did we initially decide on the default threshold? I wonder if rolling out the 0.9 override for youtube for everyone could be a compromise?

Comment 10

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

I am hesitant to introduce a pref for threshold transparency

To be clear, when I said "pref", I didn't mean to expose it in about:preferences. It would be something in about:config, but the primary advantage is that we could roll out changes to it via RemoteSettings.

Mike, how did we initially decide on the default threshold? I wonder if rolling out the 0.9 override for youtube for everyone could be a compromise?

The default threshold of 1.0 was chosen, as it granted the user the most power with the toggle (with the trade-off being sites like YouTube needing a special threshold for their semi-transparent menus).

So how about we:

1. Default to a 0.9 threshold
2. Store that value in a way that we could easily rollback using RemoteSettings

and then deploy. If we're happy with everyone being at 0.9, then we can remove the YouTube-specific threshold rule.

That compromise works for me. Does it work for you?

Comment 11

[Ania]

My bad, being able to control the value via RemoteSettings would be great.

Yes, that works great for me.

Comment 12

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

Hey Ania,

Did you have a sense of what priority this should be at?

Comment 13

[Rashelle]

Attachment: Bug 1742585 - Add PiP toggle visibility threshold preference r?kpatenio,mtigley

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ba0ff402e454
Add PiP toggle visibility threshold preference r=mtigley

Comment 15

[Iulian Moraru]

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

Comment 16

[Ciprian Georgiu, Desktop QA]

Hi rhopkinsdev! I'm trying to verify this fix on Beta 97.0b8 to see if it works as expected. This fix contain chances on the UI side? I'm not seeing any differences when comparing an affected build (Nightly 96.0a1, 20211123215113)) and a fixed one. I've opened the test case from comment 1, and looked over the PIP toggle button transparency.

Maybe, I'm not correctly understating what was fixed here, can you please help with this?

Comment 17

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

(In reply to Ciprian Georgiu [:ciprian_georgiu], Release Desktop QA from comment #16)

I'm not seeing any differences when comparing an affected build (Nightly 96.0a1, 20211123215113)) and a fixed one. I've opened the test case from comment 1, and looked over the PIP toggle button transparency.

Maybe, I'm not correctly understating what was fixed here, can you please help with this?

I believe the fix here was to provide a preference that determines some visibility threshold value (that defaults to 0.9) that allows the PiP to be opened or not. We decided this value would 0.9 and see if this would affect PiP usage on other major sites, but it can adjusted from about:config. The example in comment 1, though, required a threshold of 0.8 and I think to verify this fix we'd have to change it to that.

Unfortunately, it's very difficult (if not impossible), to find a value that will work on test cases. So we need to compromise and find a value that won't cause as much breakage. And if we need to make an exception, we should create an site-specific override for that.

Ania, is this assessment of what we should expect from this fix accurate?

Comment 18

[Ania]

Micah, thank you for the summary, it's correct.

Ciprian, testing-wise, what would be a satisfactory minimal test for this fix - verifying the new threshold is 0.9 and that it can be changed from about:config?

Comment 19

[Ciprian Georgiu, Desktop QA]

(In reply to amininkova from comment #18)

Micah, thank you for the summary, it's correct.

Ciprian, testing-wise, what would be a satisfactory minimal test for this fix - verifying the new threshold is 0.9 and that it can be changed from about:config?

Yes, I can confirm that the pref value (media.videocontrols.picture-in-picture.video-toggle.visibility-threshold) is 0.9, and can be successfully changed. Tested with Beta 97.0b9, under macOS 11, Win 10 and Ubuntu 18.04 x64.