1644212 - [macOS] Picture in Picture opens as full screen if the parent window is in full screen

Status: VERIFIED FIXED

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

Description

[daditarik]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:79.0) Gecko/20100101 Firefox/79.0

Steps to reproduce:

Opened Firefox latest Nightly on MacOS. Put the Firefox window in full screen. Went to a whatever website containing a video (youtube for example). Played a video. Clicked on the pip button.

Actual results:

The Picture-in-Picture video opened as a new full screen video.

Expected results:

The video should have been floating over my firefox window.

Comment 1

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

Bugbug thinks this bug should belong to this component, but please revert this change in case of error.

Comment 2

[:Gijs (he/him)]

[Tracking Requested - why for this release]:
This sort of defeats the point of PiP, and we haven't shipped this regression yet, so would be good to get it fixed before we do.

Comment 3

[:Gijs (he/him)]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=4266d097e223610178159331d3ffea2fbacf4ec7&tochange=d08ab5b0522d85939b5f6484c4321667fbfa8792

Xidorn, do you have time to take a look? It seems like the PiP window now inherits fullscreen state from the main browser window, but we explicitly don't want it to.

Comment 4

[:Gijs (he/him)]

I think the issue is that we don't want to set + newBehavior |= NSWindowCollectionBehaviorFullScreenPrimary; if we're mAlwaysOnTop. Trying that now.

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

I think the issue is that we don't want to set + newBehavior |= NSWindowCollectionBehaviorFullScreenPrimary; if we're mAlwaysOnTop. Trying that now.

Sounds reasonable.

Most of the issues observed from that change was due to timing change, but this seems like something unrelated to timing but instead the OS behavior, which I don't really have more insight.

I can have a look into it this weekend if needed, so keep ni? for now.

Comment 7

[:Gijs (he/him)]

The thing in comment #4 seems to work, so I'm going to put up a patch.

Comment 8

[:Gijs (he/him)]

Attachment: Bug 1644212 - reintroduce an attribute to limit which windows allow full screen, r?xidorn,haik

Comment 9

[daditarik]

Small comment, maybe not relevant for this bug, but this forced fullscreen phenomenon happens also when opening (while in fullscreen) an website information or the bookmark library.

Comment 10

[:Gijs (he/him)]

(In reply to daditarik from comment #9)

Small comment, maybe not relevant for this bug, but this forced fullscreen phenomenon happens also when opening (while in fullscreen) an website information or the bookmark library.

Thanks for pointing this out!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2b85a2e15ba3
reintroduce an attribute to limit which windows allow full screen, r=xidorn,haik

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/2b85a2e15ba3

Comment 13

[Julien Cristau [:jcristau]]

Gijs, are we ready to get this on 78?

Comment 14

[:Gijs (he/him)]

Comment on attachment 9155616 [details]
Bug 1644212 - reintroduce an attribute to limit which windows allow full screen, r?xidorn,haik

Beta/Release Uplift Approval Request

• User impact if declined: Broken PiP behaviour when used in full screen on macOS
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment #0
• List of other uplifts needed: n/a
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): It re-introduces some of the code that was removed, with some slight tweaks - so it's a partial revert, ie we've shipped (something very similar to) this code before.
• String changes made/needed: no

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9155616 [details]
Bug 1644212 - reintroduce an attribute to limit which windows allow full screen, r?xidorn,haik

fix a PiP issue on mac, approved for 78.0b9

Comment 16

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9bbde470de57

Comment 17

[Timea Cernea [:tbabos][inactive]]

Reproduced the issue on affected Nightly from 06-08-2020 on MacOS 10.13.
Verified-fixed on the latest Nightly 79.0a1 (06-08-2020) on the same MacOS 10.13.

Waiting for the fix to land in Beta.0b9 to verify further.

Comment 18

[Timea Cernea [:tbabos][inactive]]

Verified-fixed on latest Beta.0b9 on MacOS 10.13