Closed Bug 1408118 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Widget: Cocoa, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: standard8, Assigned: standard8)

References

Details

Attachments

(1 file)

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.
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.
Component: Bookmarks & History → Widget: Cocoa
Product: Firefox → Core
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.
Flags: needinfo?(mstange)
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.
Flags: needinfo?(standard8)
Adding the applicationWillFinishLaunching delegate method around here should work, I think: http://searchfox.org/mozilla-central/rev/ed1d5223adcdc07e9a2589ee20f4e5ee91b4df10/toolkit/xre/MacApplicationDelegate.mm#299
(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.
Flags: needinfo?(standard8)
The good news is, the snippet works and fixes both cases. I'll post a patch in a couple of minutes.
Flags: needinfo?(mstange)
Assignee: nobody → standard8
Status: NEW → ASSIGNED
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 *)
Attachment #8918184 - Flags: review?(spohl.mozilla.bugs) → review+
Pushed by mbanner@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/33f88f277d0b
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Duplicate of this bug: 1303294
You need to log in before you can comment on or make changes to this bug.