1405577 - [Mac] Unable to go to previous window after opening a new private window in fullscreen mode

Status: RESOLVED FIXED

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

Description

[aash]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20170925150345

Steps to reproduce:

1 Open firefox
2. Go to full-screen mode
3. Open a new private window
4. Switch desktops to go to the normal firefox window. 


Actual results:

New private window opens in full screen mode but I am unable to go to the previous window which is already in full screen mode


Expected results:

New private window should open in another screen which can be navigated by (ctrl+left or ctrl+right)

Comment 1

[u591696]

I have the same issue. Both with normal windows and with private windows.
When I have a Firefox window in fullscreen mode on my Mac, and I open new window, it opens in front of the existing window.
But it should, of course, open a new fullscreen window.

Comment 2

[Tracy Walker [:tracy]]

Confirmed on latest Nightly, 20171024.  A new Window (private or normal) opened from a Window in full screen, opens is slightly less than full screen.  I can see the initial window at far left.  However, the initial window can not be brought to the front.  Two ways to get the window in front;  Take the window out of full screen mode or close the new window.

You should be able to click on the initial window to bring it to the foreground.

Requests that step 3 open a new full screen window is a feature request, not what this bug is about.

This is a regression as I cannot reproduce this on Firefox release 56.

Comment 4

[Stephen A Pohl [:spohl]]

This was introduced by adding the undocumented NSDisablePersistence key in our plist in bug 967970 and affects all new windows, not just new private windows.

Comment 6

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ce10f96e9f2f249f231270b1ddf4900336414bae

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

The undocumented NSDisablePersistence key in our plist (introduced in bug 967970) had the unintended consequence that when a window was in fullscreen, a newly created window would no longer open in another space in fullscreen mode. Instead, the new window would appear in front of the fullscreen window in the same space. This caused issues such as bug 1406032 because the z-ordering of popup windows in the new window would appear to be behind the new window.

This patch removes the NSDisablePersistence from the plist but also makes sure that we don't leak window titles to disk in private browsing mode. Checking on irc it appears that Tor sets browser.privatebrowsing.autostart, so Tor should automatically be covered by this patch as well.

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8932899 [details] [diff] [review]
Patch

Review of attachment 8932899 [details] [diff] [review]:
-----------------------------------------------------------------

::: xpfe/appshell/nsAppShellService.cpp
@@ +749,2 @@
>  
> +  bool isParentPrivateBrowsingWindow = false;

We need to start differentiating between this window being private vs. the parent window here. If we didn't do this, a new, non-private window opened from a private window would set NSWindow's restorable to NO instead of YES.

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8932899 [details] [diff] [review]
Patch

Review of attachment 8932899 [details] [diff] [review]:
-----------------------------------------------------------------

Do you have an idea about why a setting about saving window state affects how windows open?

Comment 10

[Stephen A Pohl [:spohl]]

(In reply to Markus Stange [:mstange] from comment #9)
> Comment on attachment 8932899 [details] [diff] [review]
> Patch
> 
> Review of attachment 8932899 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you have an idea about why a setting about saving window state affects
> how windows open?

That is a very good question. Unfortunately, I have no idea. I wouldn't be surprised if this actually revealed a bug in macOS itself, but since this is an undocumented key I'm not very confident that Apple would put much of a priority on a bug report about it.

Comment 11

[Markus Stange [:mstange]]

True.

Comment 12

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84bcb5742e0c0d8d49c8c9605405a5e1dce671e0
Bug 1405577: Restore the ability to open new windows in new spaces when a window is already in fullscreen mode on macOS. r=mstange

Comment 13

[Markus Stange [:mstange]]

Have you tested what happens if you call setRestorable:NO and disableSnapshotRestoration indiscriminately on all windows? We don't properly integrate with restoration anyway and have our own session restore mechanisms, so I think doing it indiscriminately would be a valid thing to do and lead to simpler code. So I would prefer that unless it brings back the bug we're trying to fix here.

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/84bcb5742e0c

Comment 15

[Stephen A Pohl [:spohl]]

Attachment: Minimal patch

This patch sets restorable to NO and disableSnapshotRestoration indiscriminately. This still fixes the bug. The reason I was leaning towards the more involved patch is that just because we don't properly integrate with macOS resume at this time, we might want to do so in the future and this would prepare us for it. I'm happy to go either route, however, and will defer to you to make a decision.

Regarding your earlier question about why setting the NSDisablePersistence key messes with z-ordering and windows in fullscreen: I noticed that when we set the NSDisablePersistence key, the OS doesn't write anything out to ~/Library/Saved Application State. If we set restorable to NO instead, the OS will start writing data to this directory related to our application, although not window titles or other potentially sensitive data. Specifically, windows.plist will contain an entry for NSWindowZOrder, which may come into play here. It is conceivable that the OS relies on this to properly order popup windows in fullscreen mode etc. These are just guesses, however.

Comment 16

[Stephen A Pohl [:spohl]]

This actually affects Firefox 30+. But I don't see us uplifting this to ESR.

Comment 17

[Julien Cristau [:jcristau]]

Given the age of this issue I guess this fix can ride the trains.

Comment 18

[Stephen A Pohl [:spohl]]

Comment on attachment 8933319 [details] [diff] [review]
Minimal patch

Switching this to r? in case we want this patch instead of the one that has already landed (I would back out the previous one and land this one instead). See comment 15 for more detail.

Comment 19

[Markus Stange [:mstange]]

Comment on attachment 8933319 [details] [diff] [review]
Minimal patch

Review of attachment 8933319 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Stephen A Pohl [:spohl] from comment #15)
> The reason I was leaning towards
> the more involved patch is that just because we don't properly integrate
> with macOS resume at this time, we might want to do so in the future and
> this would prepare us for it.

I see. I think it's better to leave this code out of the code base as long as we don't need it. Once we do add support for macOS resume, we can recover it from the patch on this bug.

Thanks!

Comment 20

[Stephen A Pohl [:spohl]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab70879b57341260cb450528b721998dddd8223b
Backout 84bcb5742e0c (bug 1405577) to replace with a slimmer patch. r=me

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c2c9bbf9fccd746f3e61afc8da3acc8d0b8e98a
Bug 1405577: Restore the ability to open new windows in new spaces when a window is already in fullscreen mode on macOS. r=mstange

Comment 21

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/5c2c9bbf9fcc