Fix fullscreen not working + menu refactor (fix #1206) (#1210)

* Catch promise errors better

* Move subFunctions to bottom of createNewWindow

* Use parents when creating child BrowserWindow instances

* Some about:blank pages have an anchor (for some reason)

* Inject browserWindowOptions better

* Interim refactor to MainWindow object

* Split up the window functions/helpers/events some

* Further separate out window functions + tests

* Add a mock for unit testing functions that access electron

* Add unit tests for onWillPreventUnload

* Improve windowEvents tests

* Add the first test for windowHelpers

* Move WebRequest event handling to node

* insertCSS completely under test

* clearAppData completely under test

* Fix contextMenu require bug

* More tests + fixes

* Fix + add to createNewTab tests

* Convert createMainWindow back to func + work out gremlins

* Move setupWindow away from main since its shared

* Make sure contextMenu is handling promises

* Fix issues with fullscreen not working + menu refactor

* Run jest against app/dist so that we can hit app unit tests as well

* Requested PR changes

app/src/components/mainWindow.ts

@@ -10,21 +10,12 @@ import {
   getCounterValue,
   isOSX,
   nativeTabsSupported,
-  openExternal,
 } from '../helpers/helpers';
 import { setupNativefierWindow } from '../helpers/windowEvents';
 import {
-  clearAppData,
   clearCache,
-  getCurrentURL,
   getDefaultWindowOptions,
-  goBack,
-  goForward,
-  goToURL,
   hideWindow,
-  zoomIn,
-  zoomOut,
-  zoomReset,
 } from '../helpers/windowHelpers';
 import { initContextMenu } from './contextMenu';
 import { createMenu } from './menu';
@@ -47,12 +38,10 @@ type SessionInteractionResult = {
 
 /**
  * @param {{}} nativefierOptions AppArgs from nativefier.json
- * @param {function} onAppQuit
  * @param {function} setDockBadge
  */
 export async function createMainWindow(
   nativefierOptions,
-  onAppQuit: () => void,
   setDockBadge: (value: number | string, bounce?: boolean) => void,
 ): Promise<BrowserWindow> {
   const options = { ...nativefierOptions };
@@ -74,8 +63,7 @@ export async function createMainWindow(
     y: options.y,
     autoHideMenuBar: !options.showMenuBar,
     icon: getAppIcon(),
-    // set to undefined and not false because explicitly setting to false will disable full screen
+    fullscreen: options.fullScreen,
-    fullscreen: options.fullScreen ?? undefined,
     // Whether the window should always stay on top of other windows. Default is false.
     alwaysOnTop: options.alwaysOnTop,
     titleBarStyle: options.titleBarStyle,
@@ -96,8 +84,7 @@ export async function createMainWindow(
   if (options.tray === 'start-in-tray') {
     mainWindow.hide();
   }
-
+  createMenu(options, mainWindow);
-  createMainMenu(options, mainWindow, onAppQuit);
   createContextMenu(options, mainWindow);
   setupNativefierWindow(options, mainWindow);
 
@@ -180,30 +167,6 @@ function setupCounter(
   });
 }
 
-function createMainMenu(
-  options: any,
-  window: BrowserWindow,
-  onAppQuit: () => void,
-) {
-  const menuOptions = {
-    nativefierVersion: options.nativefierVersion,
-    appQuit: onAppQuit,
-    clearAppData: () => clearAppData(window),
-    disableDevTools: options.disableDevTools,
-    getCurrentURL,
-    goBack,
-    goForward,
-    goToURL,
-    openExternal,
-    zoomBuildTimeValue: options.zoom,
-    zoomIn,
-    zoomOut,
-    zoomReset,
-  };
-
-  createMenu(menuOptions);
-}
-
 function setupNotificationBadge(
   options,
   window: BrowserWindow,
@@ -275,7 +238,7 @@ function setupSessionInteraction(options, window: BrowserWindow): void {
           result.value = window.webContents.session[request.property];
         } else {
           // Why even send the event if you're going to do this? You're just wasting time! ;)
-          throw Error(
+          throw new Error(
             'Received neither a func nor a property in the request. Unable to process.',
           );
         }

app/src/components/menu.test.ts

@@ -0,0 +1,159 @@
+import { BrowserWindow } from 'electron';
+
+jest.mock('../helpers/helpers');
+import { isOSX } from '../helpers/helpers';
+import { generateMenu } from './menu';
+
+describe('generateMenu', () => {
+  let window: BrowserWindow;
+  const mockIsOSX: jest.SpyInstance = isOSX as jest.Mock;
+  let mockIsFullScreen: jest.SpyInstance;
+  let mockIsFullScreenable: jest.SpyInstance;
+  let mockIsSimpleFullScreen: jest.SpyInstance;
+  let mockSetFullScreen: jest.SpyInstance;
+  let mockSetSimpleFullScreen: jest.SpyInstance;
+
+  beforeEach(() => {
+    window = new BrowserWindow();
+    mockIsOSX.mockReset();
+    mockIsFullScreen = jest.spyOn(window, 'isFullScreen');
+    mockIsFullScreenable = jest.spyOn(window, 'isFullScreenable');
+    mockIsSimpleFullScreen = jest.spyOn(window, 'isSimpleFullScreen');
+    mockSetFullScreen = jest.spyOn(window, 'setFullScreen');
+    mockSetSimpleFullScreen = jest.spyOn(window, 'setSimpleFullScreen');
+  });
+
+  afterAll(() => {
+    mockIsFullScreen.mockRestore();
+    mockIsFullScreenable.mockRestore();
+    mockIsSimpleFullScreen.mockRestore();
+    mockSetFullScreen.mockRestore();
+    mockSetSimpleFullScreen.mockRestore();
+  });
+
+  test('does not have fullscreen if not supported', () => {
+    mockIsOSX.mockReturnValue(false);
+    mockIsFullScreenable.mockReturnValue(false);
+
+    const menu = generateMenu(
+      {
+        nativefierVersion: '1.0.0',
+        zoomBuildTimeValue: 1.0,
+        disableDevTools: false,
+      },
+      window,
+    );
+
+    const editMenu = menu.filter((item) => item.label === '&View');
+
+    const fullscreen = (editMenu[0].submenu as any[]).filter(
+      (item) => item.label === 'Toggle Full Screen',
+    );
+
+    expect(fullscreen).toHaveLength(1);
+    expect(fullscreen[0].enabled).toBe(false);
+    expect(fullscreen[0].visible).toBe(false);
+
+    expect(mockIsOSX).toHaveBeenCalled();
+    expect(mockIsFullScreenable).toHaveBeenCalled();
+  });
+
+  test('has fullscreen no matter what on mac', () => {
+    mockIsOSX.mockReturnValue(true);
+    mockIsFullScreenable.mockReturnValue(false);
+
+    const menu = generateMenu(
+      {
+        nativefierVersion: '1.0.0',
+        zoomBuildTimeValue: 1.0,
+        disableDevTools: false,
+      },
+      window,
+    );
+
+    const editMenu = menu.filter((item) => item.label === '&View');
+
+    const fullscreen = (editMenu[0].submenu as any[]).filter(
+      (item) => item.label === 'Toggle Full Screen',
+    );
+
+    expect(fullscreen).toHaveLength(1);
+    expect(fullscreen[0].enabled).toBe(true);
+    expect(fullscreen[0].visible).toBe(true);
+
+    expect(mockIsOSX).toHaveBeenCalled();
+    expect(mockIsFullScreenable).toHaveBeenCalled();
+  });
+
+  test.each([true, false])(
+    'has a fullscreen menu item that toggles fullscreen',
+    (isFullScreen) => {
+      mockIsOSX.mockReturnValue(false);
+      mockIsFullScreenable.mockReturnValue(true);
+      mockIsFullScreen.mockReturnValue(isFullScreen);
+
+      const menu = generateMenu(
+        {
+          nativefierVersion: '1.0.0',
+          zoomBuildTimeValue: 1.0,
+          disableDevTools: false,
+        },
+        window,
+      );
+
+      const editMenu = menu.filter((item) => item.label === '&View');
+
+      const fullscreen = (editMenu[0].submenu as any[]).filter(
+        (item) => item.label === 'Toggle Full Screen',
+      );
+
+      expect(fullscreen).toHaveLength(1);
+      expect(fullscreen[0].enabled).toBe(true);
+      expect(fullscreen[0].visible).toBe(true);
+
+      expect(mockIsOSX).toHaveBeenCalled();
+      expect(mockIsFullScreenable).toHaveBeenCalled();
+
+      fullscreen[0].click(null, window);
+
+      expect(mockSetFullScreen).toHaveBeenCalledWith(!isFullScreen);
+      expect(mockSetSimpleFullScreen).not.toHaveBeenCalled();
+    },
+  );
+
+  test.each([true, false])(
+    'has a fullscreen menu item that toggles simplefullscreen as a fallback on mac',
+    (isFullScreen) => {
+      mockIsOSX.mockReturnValue(true);
+      mockIsFullScreenable.mockReturnValue(false);
+      mockIsSimpleFullScreen.mockReturnValue(isFullScreen);
+
+      const menu = generateMenu(
+        {
+          nativefierVersion: '1.0.0',
+          zoomBuildTimeValue: 1.0,
+          disableDevTools: false,
+        },
+        window,
+      );
+
+      const editMenu = menu.filter((item) => item.label === '&View');
+
+      const fullscreen = (editMenu[0].submenu as any[]).filter(
+        (item) => item.label === 'Toggle Full Screen',
+      );
+
+      expect(fullscreen).toHaveLength(1);
+      expect(fullscreen[0].enabled).toBe(true);
+      expect(fullscreen[0].visible).toBe(true);
+
+      expect(mockIsOSX).toHaveBeenCalled();
+      expect(mockIsFullScreenable).toHaveBeenCalled();
+
+      fullscreen[0].click(null, window);
+
+      expect(mockSetSimpleFullScreen).toHaveBeenCalledWith(!isFullScreen);
+      expect(mockSetFullScreen).not.toHaveBeenCalled();
+    },
+  );
+});

app/src/components/menu.ts

@@ -1,39 +1,60 @@
 import * as fs from 'fs';
 import path from 'path';
 
-import { clipboard, Menu, MenuItemConstructorOptions } from 'electron';
+import {
+  BrowserWindow,
+  clipboard,
+  Menu,
+  MenuItem,
+  MenuItemConstructorOptions,
+} from 'electron';
 import * as log from 'loglevel';
 
+import { isOSX, openExternal } from '../helpers/helpers';
+import {
+  clearAppData,
+  getCurrentURL,
+  goBack,
+  goForward,
+  goToURL,
+  zoomIn,
+  zoomOut,
+  zoomReset,
+} from '../helpers/windowHelpers';
+
 type BookmarksLink = {
   type: 'link';
   title: string;
   url: string;
   shortcut?: string;
 };
+
 type BookmarksSeparator = {
   type: 'separator';
 };
+
 type BookmarkConfig = BookmarksLink | BookmarksSeparator;
+
 type BookmarksMenuConfig = {
   menuLabel: string;
   bookmarks: BookmarkConfig[];
 };
 
-export function createMenu({
+export function createMenu(options, mainWindow: BrowserWindow): void {
-  nativefierVersion,
+  log.debug('createMenu', { options, mainWindow });
-  appQuit,
+  const menuTemplate = generateMenu(options, mainWindow);
-  zoomIn,
+
-  zoomOut,
+  injectBookmarks(menuTemplate);
-  zoomReset,
+
-  zoomBuildTimeValue,
+  const menu = Menu.buildFromTemplate(menuTemplate);
-  goBack,
+  Menu.setApplicationMenu(menu);
-  goForward,
+}
-  getCurrentURL,
+
-  goToURL,
+export function generateMenu(
-  clearAppData,
+  options,
-  disableDevTools,
+  mainWindow: BrowserWindow,
-  openExternal,
+): MenuItemConstructorOptions[] {
-}): void {
+  const { nativefierVersion, zoomBuildTimeValue, disableDevTools } = options;
   const zoomResetLabel =
     zoomBuildTimeValue === 1.0
       ? 'Reset Zoom'
@@ -69,8 +90,7 @@ export function createMenu({
         label: 'Copy Current URL',
         accelerator: 'CmdOrCtrl+L',
         click: () => {
-          const currentURL = getCurrentURL();
+          clipboard.writeText(getCurrentURL());
-          clipboard.writeText(currentURL);
         },
       },
       {
@@ -90,7 +110,19 @@ export function createMenu({
       },
       {
         label: 'Clear App Data',
-        click: clearAppData,
+        click: (item: MenuItem, focusedWindow: BrowserWindow) => {
+          log.debug('Clear App Data.click', {
+            item,
+            focusedWindow,
+            mainWindow,
+          });
+          if (!focusedWindow) {
+            focusedWindow = mainWindow;
+          }
+          clearAppData(focusedWindow).catch((err) =>
+            log.error('clearAppData ERROR', err),
+          );
+        },
       },
     ],
   };
@@ -100,11 +132,7 @@ export function createMenu({
     submenu: [
       {
         label: 'Back',
-        accelerator: (() => {
+        accelerator: isOSX() ? 'CmdOrAlt+Left' : 'Alt+Left',
-          const backKbShortcut =
-            process.platform === 'darwin' ? 'Cmd+Left' : 'Alt+Left';
-          return backKbShortcut;
-        })(),
         click: goBack,
       },
       {
@@ -116,11 +144,7 @@ export function createMenu({
       },
       {
         label: 'Forward',
-        accelerator: (() => {
+        accelerator: isOSX() ? 'Cmd+Right' : 'Alt+Right',
-          const forwardKbShortcut =
-            process.platform === 'darwin' ? 'Cmd+Right' : 'Alt+Right';
-          return forwardKbShortcut;
-        })(),
         click: goForward,
       },
       {
@@ -132,27 +156,32 @@ export function createMenu({
       },
       {
         label: 'Reload',
-        accelerator: 'CmdOrCtrl+R',
+        role: 'reload',
-        click: (item, focusedWindow) => {
-          if (focusedWindow) {
-            focusedWindow.reload();
-          }
-        },
       },
       {
         type: 'separator',
       },
       {
         label: 'Toggle Full Screen',
-        accelerator: (() => {
+        accelerator: isOSX() ? 'Ctrl+Cmd+F' : 'F11',
-          if (process.platform === 'darwin') {
+        enabled: mainWindow.isFullScreenable() || isOSX(),
-            return 'Ctrl+Cmd+F';
+        visible: mainWindow.isFullScreenable() || isOSX(),
+        click: (item: MenuItem, focusedWindow: BrowserWindow) => {
+          log.debug('Toggle Full Screen.click()', {
+            item,
+            focusedWindow,
+            isFullScreen: focusedWindow?.isFullScreen(),
+            isFullScreenable: focusedWindow?.isFullScreenable(),
+          });
+          if (!focusedWindow) {
+            focusedWindow = mainWindow;
           }
-          return 'F11';
+          if (focusedWindow.isFullScreenable()) {
-        })(),
-        click: (item, focusedWindow) => {
-          if (focusedWindow) {
             focusedWindow.setFullScreen(!focusedWindow.isFullScreen());
+          } else if (isOSX()) {
+            focusedWindow.setSimpleFullScreen(
+              !focusedWindow.isSimpleFullScreen(),
+            );
           }
         },
       },
@@ -202,16 +231,13 @@ export function createMenu({
       },
       {
         label: 'Toggle Developer Tools',
-        accelerator: (() => {
+        accelerator: isOSX() ? 'Alt+Cmd+I' : 'Ctrl+Shift+I',
-          if (process.platform === 'darwin') {
+        click: (item: MenuItem, focusedWindow: BrowserWindow) => {
-            return 'Alt+Cmd+I';
+          log.debug('Toggle Developer Tools.click()', { item, focusedWindow });
+          if (!focusedWindow) {
+            focusedWindow = mainWindow;
           }
-          return 'Ctrl+Shift+I';
-        })(),
-        click: (item, focusedWindow) => {
-          if (focusedWindow) {
           focusedWindow.webContents.toggleDevTools();
-          }
         },
       },
     );
@@ -241,13 +267,21 @@ export function createMenu({
       {
         label: `Built with Nativefier v${nativefierVersion}`,
         click: () => {
-          openExternal('https://github.com/nativefier/nativefier');
+          openExternal('https://github.com/nativefier/nativefier').catch(
+            (err) =>
+              log.error(
+                'Built with Nativefier v${nativefierVersion}.click ERROR',
+                err,
+              ),
+          );
         },
       },
       {
         label: 'Report an Issue',
         click: () => {
-          openExternal('https://github.com/nativefier/nativefier/issues');
+          openExternal('https://github.com/nativefier/nativefier/issues').catch(
+            (err) => log.error('Report an Issue.click ERROR', err),
+          );
         },
       },
     ],
@@ -255,7 +289,7 @@ export function createMenu({
 
   let menuTemplate: MenuItemConstructorOptions[];
 
-  if (process.platform === 'darwin') {
+  if (isOSX()) {
     const electronMenu: MenuItemConstructorOptions = {
       label: 'E&lectron',
       submenu: [
@@ -287,7 +321,7 @@ export function createMenu({
         {
           label: 'Quit',
           accelerator: 'Cmd+Q',
-          click: appQuit,
+          role: 'quit',
         },
       ],
     };
@@ -305,43 +339,50 @@ export function createMenu({
     menuTemplate = [editMenu, viewMenu, windowMenu, helpMenu];
   }
 
-  try {
+  return menuTemplate;
+}
+
+function injectBookmarks(menuTemplate: MenuItemConstructorOptions[]): void {
   const bookmarkConfigPath = path.join(__dirname, '..', 'bookmarks.json');
-    if (fs.existsSync(bookmarkConfigPath)) {
+
+  if (!fs.existsSync(bookmarkConfigPath)) {
+    return;
+  }
+
+  try {
     const bookmarksMenuConfig: BookmarksMenuConfig = JSON.parse(
       fs.readFileSync(bookmarkConfigPath, 'utf-8'),
     );
     const bookmarksMenu: MenuItemConstructorOptions = {
       label: bookmarksMenuConfig.menuLabel,
       submenu: bookmarksMenuConfig.bookmarks.map((bookmark) => {
-          if (bookmark.type === 'link') {
+        switch (bookmark.type) {
+          case 'link':
             if (!('title' in bookmark && 'url' in bookmark)) {
-              throw Error(
+              throw new Error(
                 'All links in the bookmarks menu must have a title and url.',
               );
             }
             try {
               new URL(bookmark.url);
-            } catch (_) {
+            } catch {
-              throw Error('Bookmark URL "' + bookmark.url + '"is invalid.');
+              throw new Error('Bookmark URL "' + bookmark.url + '"is invalid.');
-            }
-            let accelerator = null;
-            if ('shortcut' in bookmark) {
-              accelerator = bookmark.shortcut;
             }
             return {
               label: bookmark.title,
               click: () => {
-                goToURL(bookmark.url);
+                goToURL(bookmark.url).catch((err) =>
+                  log.error(`${bookmark.title}.click ERROR`, err),
+                );
               },
-              accelerator: accelerator,
+              accelerator: 'shortcut' in bookmark ? bookmark.shortcut : null,
             };
-          } else if (bookmark.type === 'separator') {
+          case 'separator':
             return {
               type: 'separator',
             };
-          } else {
+          default:
-            throw Error(
+            throw new Error(
               'A bookmarks menu entry has an invalid type; type must be one of "link", "separator".',
             );
         }
@@ -349,11 +390,7 @@ export function createMenu({
     };
     // Insert custom bookmarks menu between menus "View" and "Window"
     menuTemplate.splice(menuTemplate.length - 2, 0, bookmarksMenu);
-    }
   } catch (err) {
     log.error('Failed to load & parse bookmarks configuration JSON file.', err);
   }
-
-  const menu = Menu.buildFromTemplate(menuTemplate);
-  Menu.setApplicationMenu(menu);
 }

app/src/helpers/windowHelpers.ts

@@ -137,16 +137,16 @@ export function getDefaultWindowOptions(
     ...(options.browserwindowOptions?.webPreferences ?? {}),
   };
 
-  const defaultOptions = {
+  const defaultOptions: BrowserWindowConstructorOptions = {
-    // Convert dashes to spaces because on linux the app name is joined with dashes
+    fullscreenable: true,
-    title: options.name,
     tabbingIdentifier: nativeTabsSupported() ? options.name : undefined,
+    title: options.name,
     webPreferences: {
       javascript: true,
-      plugins: true,
       nodeIntegration: false, // `true` is *insecure*, and cause trouble with messenger.com
-      webSecurity: !options.insecure,
       preload: path.join(__dirname, 'preload.js'),
+      plugins: true,
+      webSecurity: !options.insecure,
       zoomFactor: options.zoom,
       ...webPreferences,
     },

app/src/main.ts

@@ -263,11 +263,7 @@ if (shouldQuit) {
 }
 
 async function onReady(): Promise<void> {
-  const mainWindow = await createMainWindow(
+  const mainWindow = await createMainWindow(appArgs, setDockBadge);
-    appArgs,
-    app.quit.bind(this),
-    setDockBadge,
-  );
 
   createTrayIcon(appArgs, mainWindow);
 

app/src/mocks/electron.ts

@@ -42,9 +42,29 @@ class MockBrowserWindow extends EventEmitter {
     return window ?? new MockBrowserWindow();
   }
 
+  isSimpleFullScreen(): boolean {
+    return undefined;
+  }
+
+  isFullScreen(): boolean {
+    return undefined;
+  }
+
+  isFullScreenable(): boolean {
+    return undefined;
+  }
+
   loadURL(url: string, options?: any): Promise<void> {
     return Promise.resolve(undefined);
   }
+
+  setFullScreen(flag: boolean): void {
+    return;
+  }
+
+  setSimpleFullScreen(flag: boolean): void {
+    return;
+  }
 }
 
 class MockDialog {

package.json

@@ -101,14 +101,14 @@
       "<rootDir>/src.*",
       "<rootDir>/node_modules.*",
       "<rootDir>/app/src.*",
-      "<rootDir>/app/dist.*",
+      "<rootDir>/app/lib.*",
       "<rootDir>/app/node_modules.*"
     ],
     "watchPathIgnorePatterns": [
       "<rootDir>/src.*",
       "<rootDir>/tsconfig.json",
       "<rootDir>/app/src.*",
-      "<rootDir>/app/dist.*",
+      "<rootDir>/app/lib.*",
       "<rootDir>/app/tsconfig.json"
     ]
   },