1781830 - [macOS] PiP is fullscreen when launched from a full screen window

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P3)

Description

[Asif Youssuff]

From https://www.reddit.com/r/firefox/comments/w8kjli/firefox_1030_see_all_new_features_updates_and/ihuor8t/

Steps to reproduce:

1. Navigate to https://www.youtube.com/watch?v=E31fNqvEWTk
2. Click "full screen" button in Firefox window (green window decoration)
3. Play video
4. Click PiP button overlay in Firefox

What happens:

The PiP window opens in a new fullscreen window and doesn't act like a PiP window (doesn't overlay other windows if moved to a different screen, for example).

Expected result:

As prior to regression.

19:05.56 INFO: Narrowed inbound regression window from [430c8dbe, 8e63cdd5] (3 builds) to [430c8dbe, 709f48d5] (2 builds) (~1 steps left)
19:05.56 INFO: No more inbound revisions, bisection finished.
19:05.56 INFO: Last good revision: 430c8dbe41323dd546c10160706b52d118ef6361
19:05.56 INFO: First bad revision: 709f48d5b8373bf1547db9566f120fbddc5bddf4
19:05.56 INFO: Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=430c8dbe41323dd546c10160706b52d118ef6361&tochange=709f48d5b8373bf1547db9566f120fbddc5bddf4

I tested this on macOS 10.15.7 (19H1922) - I have not tried other macOS versions, so the behavior may be different elsewhere.

Comment 1

[Alice0775 White]

Bug 1781483 seems duplicated

Comment 2

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

Set release status flags based on info from the regressing bug 1700674

Comment 3

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

:KrisWright, since you are the author of the regressor, bug 1700674, could you take a look?
For more information, please visit auto_nag documentation.

Comment 4

[Asif Youssuff]

Someone is seeing this on macOS 11.6 as well: https://www.reddit.com/r/firefox/comments/w9lkua/picture_in_picture_mode_goes_into_full_screen/

Comment 5

[kpatenio]

Attachment: Screen Recording 2022-07-28 at 10.28.35.mov

Attaching a recording of behaviour that I believe to be linked to this issue. This was tested on MacOS 11.6 with a 2 screen setup, with the source video open in a fullscreen window (via green button). (Video was also cropped to reduce file size)

Comment 7

[Donal Meehan [:dmeehan]]

Tracking for 103 dot release potential - S2 severity.
Is backing out the regressor an option for release?

Comment 8

[Stephen A Pohl [:spohl]]

(In reply to Donal Meehan [:dmeehan] from comment #7)

Is backing out the regressor an option for release?

This seems like an option to me, but the best person to answer this question would be the author of bug 1700674, Kris.

Comment 9

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

The bug is marked as tracked for firefox103 (release). However, the bug still isn't assigned.

:gcp, could you please find an assignee for this tracked bug? Given that it is a regression and we know the cause, we could also simply backout the regressor. If you disagree with the tracking decision, please talk with the release managers.

For more information, please visit auto_nag documentation.

Comment 10

[Kris Wright :KrisWright]

Based on prior discussion it looks like the best fix until we know what's going on here is to back out bug 1700674.

Comment 11

[Kris Wright :KrisWright]

Attachment: Bug 1781830 - Backout bug 1700674. r?mhowell

Comment 12

[Kris Wright :KrisWright]

Adding leave-open since this backout doesn't address the underlying issue caused in bug 1700674.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ddf86a7ed4d8
Backout bug 1700674. r=mhowell

Comment 15

[Kris Wright :KrisWright]

Comment on attachment 9287977 [details]
Bug 1781830 - Backout bug 1700674. r?mhowell

Beta/Release Uplift Approval Request

• User impact if declined: Users will continue experiencing picture-in-picture issue
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce:

1. Navigate to https://www.youtube.com/watch?v=E31fNqvEWTk
2. Click "full screen" button in Firefox window (green window decoration)
3. Play video
4. Click PiP button overlay in Firefox

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This patch simply reverts the regressing patch to address regressions until we know what's going on.
• String changes made/needed:
• Is Android affected?: No

Comment 16

[Andreea Pavel [:apavel]]

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

Comment 17

[Dianna Smith [:diannaS]]

Comment on attachment 9287977 [details]
Bug 1781830 - Backout bug 1700674. r?mhowell

Approved for 104.0b5

Comment 18

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/32ea08279419

Comment 19

[Donal Meehan [:dmeehan]]

Comment on attachment 9287977 [details]
Bug 1781830 - Backout bug 1700674. r?mhowell

Approved for 103.0.2, thanks.

Comment 20

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/687ffa64bbca

Comment 22

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

The severity field for this bug is relatively low, S3. However, the bug has 3 duplicates.
:KrisWright, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 23

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

The leave-open keyword is there and there is no activity for 6 months.
:KrisWright, maybe it's time to close this bug?
For more information, please visit BugBot documentation.

Comment 24

[Celso]

Attachment: screen_recording.mp4

This issue seems to be back on Firefox Nightly 125.

Currently using 125.0a1 (2024-02-24) (64-bit) on a Macbook Pro 16" with the M1 Max running macOS Sonoma 14.3.1 (23D60).

Comment 25

[Celso]

(In reply to Celso from comment #24)

Created attachment 9382482 [details]
screen_recording.mp4

This is now happening on beta: 125.0b2 (64-bit)

Comment 26

[Celso]

(In reply to Celso from comment #24)

Created attachment 9382482 [details]
screen_recording.mp4

This issue seems to be back on Firefox Nightly 125.

Currently using 125.0a1 (2024-02-24) (64-bit) on a Macbook Pro 16" with the M1 Max running macOS Sonoma 14.3.1 (23D60).

Now happening on 125.0...

Comment 27

[Alexandru Trif, Desktop Test Engineering [:atrif] [PTO 15-29 Aug, please ni? :mboldan]]

(In reply to Celso from comment #26)

(In reply to Celso from comment #24)

Created attachment 9382482 [details]
screen_recording.mp4

This issue seems to be back on Firefox Nightly 125.

Currently using 125.0a1 (2024-02-24) (64-bit) on a Macbook Pro 16" with the M1 Max running macOS Sonoma 14.3.1 (23D60).

Now happening on 125.0...

Thank you!
Confirming this. It seems that this issue came back after bug 1880558. Brad, can you please take a look?

Comment 28

[Brad Werth [:bradwerth]]

I'll fix this.

Comment 29

[citizenoftheweb]

Still present on 126.0b3 fyi.

Comment 30

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

Hey bradwerth,

We keep getting reports about this one on various channels... are you still working on this?

Comment 31

[Brad Werth [:bradwerth]]

This is a weird one. The initial call to show the PiP window triggers a macOS internal codepath that eventually calls a private method _enterAutomaticFullScreen, which we want to prevent. Since it's private, and the path to it is in code we can't inspect, then I'll have to experiment a bit to determine what conditions are involved. Whatever those conditions are, we'll probably want to shut them off for any alwaysOnTop windows, any dialogs or popups. None of those should begin life in fullscreen.

Comment 32

[Stephen A Pohl [:spohl]]

Would it be possible to swizzle _enterAutomaticFullScreen and skip it for alwaysOnTop windows?

Comment 33

[Brad Werth [:bradwerth]]

(In reply to Stephen A Pohl [:spohl] from comment #32)

Would it be possible to swizzle _enterAutomaticFullScreen and skip it for alwaysOnTop windows?

Swizzle meaning override it and supply our own no-op behavior? Maybe, but that makes us dependent on the naming of a private method, which feels fragile. Alternatively, we can just temporarily manipulate the collectionBehavior which is what was affected by the changes in Bug 1880558. I've built a patch that does that and we'll see how reviewers feel about it.

Comment 34

[Brad Werth [:bradwerth]]

I think we could also argue that opening new fullscreen windows from fullscreen windows is current behavior and shouldn't be special-cased for PiP windows. Once we "fix" this, I suspect we're going to get bug reports that the new PiP window doesn't appear in the same space as the fullscreen window, which is impossible given the way fullscreen spaces work in macOS.

Comment 35

[Brad Werth [:bradwerth]]

Attachment: Bug 1781830: Prevent PiP windows from opening in fullscreen.

Though this is standard behavior for other windows, we want to
special-case this for PiP windows, which users expect to open in
resizable popups. This patch also prevents other popups and
alerts/dialogs from opening in fullscreen mode, which would be a bad
user experience.

Comment 36

[[e-mail]]

please fix this. Its been 2 years. Thanks.

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e78266dc8300
Prevent PiP windows from opening in fullscreen. r=mac-reviewers,mstange

Comment 39

[tszentpeteri]

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

Comment 40

[Negritas Sergiu, Desktop QA]

Hello! I can confirm that the issue is fixed with firefox 128.0b1 on MacOS 12.6, I will update the status and the flags of this issue.

Have a nice day!