fix: hiding window menu should work on startup (#21436)

* fix: menu visibility should not be overwritten on startup

* fix: removing menu for window without global menubar

* test: setMenu tests are not for mac

lib/browser/init.ts

@@ -195,13 +195,14 @@ app.on('window-all-closed', () => {
   }
 })
 
-Promise.all([
-  import('@electron/internal/browser/default-menu'),
-  app.whenReady()
-]).then(([{ setDefaultApplicationMenu }]) => {
-  // Create default menu
-  setDefaultApplicationMenu()
-})
+const { setDefaultApplicationMenu } = require('@electron/internal/browser/default-menu')
+
+// Create default menu.
+//
+// Note that the task must be added before loading any app, so we can make sure
+// the call is maded before any user window is created, otherwise the default
+// menu may show even when user explicitly hides the menu.
+app.once('ready', setDefaultApplicationMenu)
 
 if (packagePath) {
   // Finally load app's main.js and transfer control to C++.

shell/browser/native_window_views.cc

@@ -1002,21 +1002,23 @@ void NativeWindowViews::SetFocusable(bool focusable) {
 
 void NativeWindowViews::SetMenu(AtomMenuModel* menu_model) {
 #if defined(USE_X11)
-  if (menu_model == nullptr) {
+  // Remove global menu bar.
+  if (global_menu_bar_ && menu_model == nullptr) {
     global_menu_bar_.reset();
     root_view_->UnregisterAcceleratorsWithFocusManager();
     return;
   }
 
-  if (!global_menu_bar_ && ShouldUseGlobalMenuBar())
-    global_menu_bar_ = std::make_unique<GlobalMenuBarX11>(this);
-
   // Use global application menu bar when possible.
-  if (global_menu_bar_ && global_menu_bar_->IsServerStarted()) {
+  if (ShouldUseGlobalMenuBar()) {
+    if (!global_menu_bar_)
+      global_menu_bar_ = std::make_unique<GlobalMenuBarX11>(this);
+    if (global_menu_bar_->IsServerStarted()) {
       root_view_->RegisterAcceleratorsWithFocusManager(menu_model);
       global_menu_bar_->SetMenu(menu_model);
       return;
     }
+  }
 #endif
 
   // Should reset content size when setting menu.

spec-main/api-menu-spec.ts

@@ -1,8 +1,14 @@
+import * as cp from 'child_process'
+import * as path from 'path'
 import { expect } from 'chai'
 import { BrowserWindow, Menu, MenuItem } from 'electron'
 import { sortMenuItems } from '../lib/browser/api/menu-utils'
+import { emittedOnce } from './events-helpers'
+import { ifit } from './spec-helpers'
 import { closeWindow } from './window-helpers'
 
+const fixturesPath = path.resolve(__dirname, 'fixtures')
+
 describe('Menu module', function () {
   this.timeout(5000)
   describe('Menu.buildFromTemplate', () => {
@@ -864,5 +870,27 @@ describe('Menu module', function () {
       Menu.setApplicationMenu(null)
       expect(Menu.getApplicationMenu()).to.be.null('application menu')
     })
+
+    ifit(process.platform !== 'darwin')('does not override menu visibility on startup', async () => {
+      const appPath = path.join(fixturesPath, 'api', 'test-menu-visibility')
+      const appProcess = cp.spawn(process.execPath, [appPath])
+
+      let output = ''
+      appProcess.stdout.on('data', data => { output += data })
+
+      await emittedOnce(appProcess, 'close')
+      expect(output).to.include('Window has no menu')
+    })
+
+    ifit(process.platform !== 'darwin')('does not override null menu on startup', async () => {
+      const appPath = path.join(fixturesPath, 'api', 'test-menu-null')
+      const appProcess = cp.spawn(process.execPath, [appPath])
+
+      let output = ''
+      appProcess.stdout.on('data', data => { output += data })
+
+      await emittedOnce(appProcess, 'close')
+      expect(output).to.include('Window has no menu')
+    })
   })
 })

spec-main/fixtures/api/test-menu-null/main.js

@@ -0,0 +1,17 @@
+const { app, BrowserWindow } = require('electron')
+
+let win
+app.on('ready', function () {
+  win = new BrowserWindow({})
+  win.loadURL('about:blank')
+  win.setMenu(null)
+
+  setTimeout(() => {
+    if (win.isMenuBarVisible()) {
+      console.log('Window has a menu')
+    } else {
+      console.log('Window has no menu')
+    }
+    app.quit()
+  })
+})

spec-main/fixtures/api/test-menu-null/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "electron-test-menu",
+  "main": "main.js"
+}

spec-main/fixtures/api/test-menu-visibility/main.js

@@ -0,0 +1,17 @@
+const { app, BrowserWindow } = require('electron')
+
+let win
+app.on('ready', function () {
+  win = new BrowserWindow({})
+  win.loadURL('about:blank')
+  win.setMenuBarVisibility(false)
+
+  setTimeout(() => {
+    if (win.isMenuBarVisible()) {
+      console.log('Window has a menu')
+    } else {
+      console.log('Window has no menu')
+    }
+    app.quit()
+  })
+})

spec-main/fixtures/api/test-menu-visibility/package.json

@@ -0,0 +1,4 @@
+{
+  "name": "electron-test-menu",
+  "main": "main.js"
+}