1408118 - The view menu of the library window has two full screen options listed

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement)

Description

[Mark Banner (:standard8)]

In the view menu, the library window has two full-screen options listed:

- "Enter Full Screen" (with shortcut cmd-ctrl-F, but disabled)
- "Enter Full Screen" (enabled)

According to http://searchfox.org/mozilla-central/rev/31606bbabc50b08895d843b9f5f3da938ccdfbbf/browser/base/content/browser.js#1922 and the fact we've had a bug like bug 1406331, I think we should only have the disabled option in the menu.

Comment 1

[Mark Banner (:standard8)]

I've done some digging - changing our localised value didn't change the second menu option. Which lead me to start thinking that Mac is injecting the second option.

Eventually I came across:

https://developer.apple.com/library/content/releasenotes/AppKit/RN-AppKitOlderNotes/#10_11FullScreen

(search for NSFullScreenMenuItemEverywhere).

The document there states:

======
AppKit automatically creates an "Enter Full Screen" menu item after the application finishes launching if an equivalent menu item isn't found. If this menu item should not be created for your app, before NSApplicationDidFinishLaunchingNotification is sent you may set the NSFullScreenMenuItemEverywhere default to NO.

- (void)applicationWillFinishLaunching:(nonnull NSNotification *)notification {
    [[NSUserDefaults standardUserDefaults] setBool:NO forKey:@"NSFullScreenMenuItemEverywhere"];
}
======

However, we don't seem to have any cocoa code relating to that, so I'm not quite sure where this would be located. I'm guessing it is probably widget related, so moving there for a first stab.

Comment 2

[Mark Banner (:standard8)]

Marcus, if you have ideas on where we could put this, I'm willing to give it a try.

I am a bit puzzled that it doesn't show up in the main browser window though.

Comment 3

[Stephen A Pohl [:spohl]]

What is the "library" window?

With a regular browser window, I've noticed that this doesn't occur when first opening the View menu. After cmd-tabbing to another application and coming back, the View menu now shows two fullscreen entries.

Comment 4

[Markus Stange [:mstange]]

Adding the applicationWillFinishLaunching delegate method around here should work, I think: http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/toolkit/xre/MacApplicationDelegate.mm#299

Comment 5

[Mark Banner (:standard8)]

(In reply to Stephen A Pohl [:spohl] from comment #3)
> What is the "library" window?

Bookmarks -> Show All Bookmarks (or one other ones, e.g. History, Downloads, they're all the same window).

Also, the About window does this as well.

> With a regular browser window, I've noticed that this doesn't occur when
> first opening the View menu. After cmd-tabbing to another application and
> coming back, the View menu now shows two fullscreen entries.

Wow, never noticed that before. Interesting. I suspect the root cause is probably the same though.

Comment 6

[Mark Banner (:standard8)]

The good news is, the snippet works and fixes both cases. I'll post a patch in a couple of minutes.

Comment 7

[Mark Banner (:standard8)]

Attachment: Bug 1408118 - Disable Mac's insertion of its own full screen menu item as we already provide one.

Review commit: https://reviewboard.mozilla.org/r/189054/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/189054/

Comment 8

[Stephen A Pohl [:spohl]]

Comment on attachment 8918184 [details]
Bug 1408118 - Disable Mac's insertion of its own full screen menu item as we already provide one.

https://reviewboard.mozilla.org/r/189054/#review194476

With nit addressed. Thank you!

::: toolkit/xre/MacApplicationDelegate.mm:300
(Diff revision 1)
>    return menu;
>  
>    NS_OBJC_END_TRY_ABORT_BLOCK_NIL;
>  }
>  
> +- (void)applicationWillFinishLaunching:(nonnull NSNotification *)notification {

nit: s/(nonnull NSNotification *)/(NSNotification*)/

(no space between NSNotification and *)

Comment 9

[Mark Banner (:standard8)]

Comment on attachment 8918184 [details]
Bug 1408118 - Disable Mac's insertion of its own full screen menu item as we already provide one.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/189054/diff/1-2/

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/33f88f277d0b
Disable Mac's insertion of its own full screen menu item as we already provide one. r=spohl

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/33f88f277d0b