1666739 - PiP player is enabled when clicking Skip Ads button while PiP description is shown and the firefox window is smaller

Status: VERIFIED FIXED

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

Description

[Alexandru Trif, Desktop QA [:atrif]]

Affected versions

• 83.0a1 (20200922154306)
• 82.0b2 (20200922183749)

Affected platforms

• Windows 7/10x64
• macOS 10.12
• Ubuntu 18.04

Steps to reproduce

1. Open Firefox with a new profile and go to a random Youtube video.
2. If an ad is playing, click the skip ads button while the PiP description is shown.

Expected result

• The ad is skipped.

Actual result

• PiP Player is opened.

Regression range

• I will search for one ASAP.

Notes

• Attached a screen recording: link.
• The issue can be reproducible even when the firefox window is smaller and Skip Ads button overlaps the PiP button (see the attached screen recording)
• When I like this ad/ I dislike this ad buttons are present the PiP button is underneath them when the size of the window is smaller (the issue is seen in the screen recording as well).

Suggested Severity: S3

Comment 1

[Peter Magyari (Desktop QA)]

This issue doesn't seem to be a regression, I was able to reproduce it even on Firefox 73.0a1 (even when the PiP Icon was smaller)

Comment 2

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

Thanks atrif,

We might want to consider moving the toggle slightly on YouTube. What do you think, emanuela?

Comment 3

[emanuela [ux]]

We are considering some solutions for this problem.

Comment 5

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

We're going to see if we can make it so that sites can have individual hittest thresholds for the overlay opacities.

Hey emilio, do you recall https://bugzilla.mozilla.org/show_bug.cgi?id=1600372? I'm wondering what the best way would be to plumb through a specific opacity threshold that we care about, starting from nsIDOMWindowUtils.nodesFromRect.

Perhaps I could make the opacity threshold an optional second argument to nsDisplayListBuilder::SetHitTestIsForVisibility... but what's the best way to get that value passed down that far? Is it too sloppy to just append an optional float from ::NodesFromRect all the way down to nsLayoutUtils::GetFramesForArea? Or is there a better, preferred way?

Comment 6

[Emilio Cobos Álvarez (:emilio)]

I think changing the functions so that instead of passing EnumSet<FrameForPointOption> we pass some sort of const FrameForPointOptions& which can store both the flags and the extra opacity would be cleaner than that.

But if that's too much work or it gets complicated for any reason, plumbing down an extra argument is probably ok.

Comment 7

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

Can I presume by FrameForPointOptions&, you're implying that I use some kind of chrome-webidl dictionary to pass this information around?

Comment 8

[Emilio Cobos Álvarez (:emilio)]

No, I just meant something like:

struct FrameForPointOptions {
  EnumSet<FrameForPointOption> mFlags; // Probably rename FrameForPointOption -> FrameForPointFlag or something.
  float mVisibleOpacityThreshold = 0.0f; // Better name welcome.
};

Comment 9

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

Ah, okay good - I think that will be much simpler than what I had inferred in comment 7. :) Thanks!

Comment 10

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

Attachment: Bug 1666739 - Add site-specific PiP toggle visibility threshold to the WebCompat add-on. r?mstriemer!,twisniewski!

This also adds the first threshold of 0.9 for YouTube, which allows us to avoid
hittest false positives on the PiP toggle when the user has one of the YouTube
player menus open.

Comment 12

[Alin Ilea, Desktop QA]

Attachment: 2020-10-15_13h13_42.mp4

A similar behavior is happening on https://www.cdc.gov/. When trying to close the "More videos" section, the PiP window in opened (see the video attachment). Please let me know if I should open a new bug for this or its the same issue as in this bug.
Thanks.

Comment 13

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

Thanks, Alin. This is ultimately the same underlying issue, but we should open a separate bug that one so that once we figure out this opacity threshold stuff, we can weigh adding a site-specific rule for cdc.gov.

Comment 14

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

Okay, so my next approach is go deeper and try to teach nsIDOMWindowUtils.nodesFromRect to cull items from the list that occur after a node that exceeds some kind of opacity threshold. I believe this will allow us to handle all of the ways that a thing can be transparent.

emilio and miko tell me this is probably possible in the nsDisplayList::HitTest chunk of the code, which figures out which frames are beneath a particular coordinate.

So I've gone ahead and added some plumbing so that we can pipe an optional opacity threshold to nsIDOMWindowUtils.nodesFromRect, and from what I understand, this allows us to skip considering all children of nodes that are below our visibility threshold, so I guess that's good from a performance standpoint.

The next step is to go through the z-ordered list of frames and somehow figure out which one tips us over from "transparent" to "opaque enough to care about". I think this means examining each nsDisplayItem in the list and somehow getting its opacity value and ... summing them? Originally, I thought multiplying would make sense, but it doesn't - two <div>'s with opacity 0.5 stacked on top of one another are ultimately more opaque - multiplying their values would give me 0.25 which... is not right.

So do I sum them? Two frames with 0.5 opacity gives us 1.0, so as soon as we cross that threshold, we can cull the rest of the frame list here?:

https://searchfox.org/mozilla-central/rev/40b561b9accbff54387edcba2b8b4a3c3828110e/layout/painting/nsDisplayList.cpp#2771-2778

Am I at all on the right track, emilio? Please be advised, I have no idea what I'm doing, so you might really have to spell it out for me.

Comment 15

[Emilio Cobos Álvarez (:emilio)]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #14)

Okay, so my next approach is go deeper and try to teach nsIDOMWindowUtils.nodesFromRect to cull items from the list that occur after a node that exceeds some kind of opacity threshold. I believe this will allow us to handle all of the ways that a thing can be transparent.

emilio and miko tell me this is probably possible in the nsDisplayList::HitTest chunk of the code, which figures out which frames are beneath a particular coordinate.

So I've gone ahead and added some plumbing so that we can pipe an optional opacity threshold to nsIDOMWindowUtils.nodesFromRect, and from what I understand, this allows us to skip considering all children of nodes that are below our visibility threshold, so I guess that's good from a performance standpoint.

At least the current opaqueness check works as an after-the-fact thing, so it doesn't skip much work. But if you're rejiggering stuff to make that work differently that's fine too.

The next step is to go through the z-ordered list of frames and somehow figure out which one tips us over from "transparent" to "opaque enough to care about". I think this means examining each nsDisplayItem in the list and somehow getting its opacity value and ... summing them? Originally, I thought multiplying would make sense, but it doesn't - two <div>'s with opacity 0.5 stacked on top of one another are ultimately more opaque - multiplying their values would give me 0.25 which... is not right.

It is right, isn't it? The bottom div here is 0.25 opaque unlike the one at the top or the middle.

<!doctype html>
<div style="background: black; height: 100px"></div>
<div style="opacity: .5">
  <div style="background: black; height: 100px"></div>
</div>
<div style="opacity: .5">
  <div style="opacity: .5">
    <div style="background: black; height: 100px"></div>
  </div>
</div>

I guess you mean if all the divs with opacity have their own background? If so you need to sum the multiplied values, so if you have:

<!doctype html>
<style>
  div { background: rgba(0, 0, 0, .5); }
</style>
<div style="height: 100px"></div>
<div>
  <div style="height: 100px"></div>
  <div>
    <div style="height: 100px"></div>
  </div>
</div>

For each of the <div style="height: 100px">:

• The first should get an opacity of .5 (the one from the background)
• The second should get an opacity of .5 + .5 * .5
• The third should get .5 + .5 * .5 + .5 * .5 * .5

I think that math is right :)

In terms of implementing it, with nested elements with opacity it seems fairly straight-forward (you'd just keep track of the "current" opacity and you'd increment / decrement in nsDisplayOpacity::HitTest as needed.

Depending on the behavior that you want with overlaid elements / backgrounds, you might want different implementation strategies... What behavior do you want in that case? Which elements should remain in the results and which elements shouldn't?

Comment 18

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

Sorry for the delay, finally had some time to think this over. Wow, this was a major refresher course in the complexities of stacking contexts. I had forgotten about those.

So ultimately we're producing a list of DOM elements that are going to be returned to nodesFromRect. What I'm looking for is for nodesFromRect to truncate the list at the point when the stack of nodes on top have exceeded some kind of transparency threshold. And the reason for this is I want to determine if the <video> is sufficiently visible at the point where the user clicked.

Suppose we have this DOM:

<html>
  <body>
     <section>
       <div A>
         <div B>
           <video>
         </div>
       </div>
      </section>
     <div C> <!-- Pretend <div C> overlaps everything by way of CSS -->
     </div>
  </body>
</html>

I think that means nodesFromRect will return something like this:

[<html>, <body>, <section>, <div C>, <div A>, <div B>, <video>]

Suppose <div's> are all semi-transparent, such that the total opacity when overlapping the <video> is greater than, say, 0.9. At that point, I would consider the <video> not visible, and want it truncated it from the list.

To be clear, I don't really care about the complexity of the DOM itself. What I care about it ultimately how visible the <video> is in relation to all of the stuff on top of it. If we can somehow compute a final value for how transparent everything is that's on top of the <video>, that would help.

Does that make it clearer what I'm looking for?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Yes, but I'm not sure how hard is it to write the "right" check.

The simplest thing would be something like the kind of check we're doing already for hitting opaque items (GetOpaqueRegion etc), with some sort of extension to allow semi-transparency.

The right check of detecting multiple, non-nested, semi-transparent elements overlapping, and adding up some sort of opacity to each of them seems like a much more annoying thing to compute. Not sure how precise you need to be here...

Matt may have some ideas on how to get best results, but I hope my initial (somewhat naive) idea should be good enough...

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1666739 - Add an optional opacity threshold for visibility hit-test. r=mconley!,miko!

This is a best-effort thing of course, but so is the rest of the
visibility threshold stuff in practice and this should be good enough.

Comment 24

[Emilio Cobos Álvarez (:emilio)]

Does the attached patch do what you want, roughly? It seems to fix the YT context-menu issue, etc, for example.

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e922805de1e
Add an optional opacity threshold for visibility hit-test. r=mconley,miko

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/626e701b2a8c
Fix android build bustage.

Comment 27

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/3e922805de1e
https://hg.mozilla.org/mozilla-central/rev/626e701b2a8c

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/404bfa9f415b
Add site-specific PiP toggle visibility threshold to the WebCompat add-on. r=mstriemer,webcompat-reviewers,denschub,twisniewski

Comment 29

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/404bfa9f415b

Comment 30

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

[Alexandru Trif, Desktop QA [:atrif]]

Attachment: pip_ad_00.gif

Hello! I tried verifying the issue on Windows 10x64 with Firefox 85.0a1 (20201208213457) but I noticed that sometimes I can still reproduce the initial issue if the firefox window is smaller and an add is played while clicking the Skip Add button with ads longer than 45 seconds(see screen recording). I've changed the value of media.videocontrols.picture-in-picture.video-toggle.min-video-secs: 1 to make the recording. The issue is intermittent because sometimes the ad is skipped and the pip window is not shown.

Should we close this as verified and open another issue given the fact that this is an edge case and the Youtube Settings menu can be clicked on the PiP button area without enabling the PiP window? Thank you!

Comment 32

[Emilio Cobos Álvarez (:emilio)]

301 Mike :)

Comment 33

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

Hey atrif,

Hm, yes, let's get a new bug on file for that case. Sorry, I thought this would have taken care of that problem to boot. :/

Comment 34

[Alexandru Trif, Desktop QA [:atrif]]

(In reply to Mike Conley (:mconley) (:⚙️) from comment #33)

Hey atrif,

Hm, yes, let's get a new bug on file for that case. Sorry, I thought this would have taken care of that problem to boot. :/

Thank you Mike!

Based on the above the issue is verified fixed with Firefox 85.0a1 (20201209213504) on Windows 10x64, macOS 11 and Ubuntu 18.04. I opened bug 1681663 for the remaining issue.