Fix regressions in opening windows/tabs, update browser versions (#1284)

* Update electron to 13.4.0 + update browser versions * Fix injectCSS preventing new windows from opening * Fix some window/tab opening weirdness/bugs * Fix unit tests * Switch to using onResponseStarted to avoid the issues with callbacks in the future * Clear up comments on onResponseStarted
2021-09-24 22:03:03 -04:00 · 2021-09-24 22:03:03 -04:00 · 24115bb3bd
parent d759695e5a
commit 24115bb3bd
10 changed files with 101 additions and 221 deletions
--- a/.github/manual-test
+++ b/.github/manual-test
@ -64,6 +64,8 @@ printf '\n***** SMOKE TEST 1: Test checklist *****
 - Injected css: should make npmjs all blue
 - Internal links open internally
 - External links open in browser
+- Context menu -> Open Link In New Window works
+- MAC ONLY: Context menu -> Open Link In New Tab works
 - Keyboard shortcuts: {back, forward, zoom in/out/zero} work
 - Console: no Electron runtime deprecation warnings/error logged'
 launch_app "$tmp_dir" "$name"
--- a/app/package-lock.json
+++ b/app/package-lock.json
@ -17,7 +17,7 @@
        "source-map-support": "^0.5.19"
      },
      "devDependencies": {
-        "electron": "^13.1.7"
+        "electron": "^13.4.0"
      }
    },
    "node_modules/@electron/get": {
--- a/app/package.json
+++ b/app/package.json
@ -20,6 +20,6 @@
    "source-map-support": "^0.5.19"
  },
  "devDependencies": {
-    "electron": "^13.1.7"
+    "electron": "^13.4.0"
  }
 }
--- a/app/src/components/mainWindow.ts
+++ b/app/src/components/mainWindow.ts
@ -14,6 +14,7 @@ import {
 import { onNewWindow, setupNativefierWindow } from '../helpers/windowEvents';
 import {
  clearCache,
+  createNewTab,
  getDefaultWindowOptions,
  hideWindow,
 } from '../helpers/windowHelpers';
@ -88,9 +89,11 @@ export async function createMainWindow(
  if (options.tray === 'start-in-tray') {
    mainWindow.hide();
  }
+
+  const windowOptions = outputOptionsToWindowOptions(options);
  createMenu(options, mainWindow);
  createContextMenu(options, mainWindow);
-  setupNativefierWindow(outputOptionsToWindowOptions(options), mainWindow);
+  setupNativefierWindow(windowOptions, mainWindow);

  // .on('new-window', ...) is deprected in favor of setWindowOpenHandler(...)
  // We can't quite cut over to that yet for a few reasons:
@ -102,13 +105,13 @@ export async function createMainWindow(
  // users are being pointed to use setWindowOpenHandler.
  // E.g., https://github.com/electron/electron/issues/28374

-  // Note it is important to add this handler only to the *main* window,
+  // Note it is important to add these handlers only to the *main* window,
  // else we run into weird behavior like opening tabs twice
  mainWindow.webContents.on(
    'new-window',
    (event, url, frameName, disposition) => {
      onNewWindow(
-        outputOptionsToWindowOptions(options),
+        windowOptions,
        setupNativefierWindow,
        event,
        url,
@ -117,6 +120,16 @@ export async function createMainWindow(
      ).catch((err) => log.error('onNewWindow ERROR', err));
    },
  );
+  // @ts-expect-error new-tab isn't in the type definition, but it does exist
+  mainWindow.on('new-tab', () => {
+    createNewTab(
+      windowOptions,
+      setupNativefierWindow,
+      options.targetUrl,
+      true,
+      mainWindow,
+    );
+  });

  if (options.counter) {
    setupCounter(options, mainWindow, setDockBadge);
--- a/app/src/helpers/windowEvents.ts
+++ b/app/src/helpers/windowEvents.ts
@ -190,15 +190,4 @@ export function setupNativefierWindow(
  window.webContents.on('will-prevent-unload', onWillPreventUnload);

  sendParamsOnDidFinishLoad(options, window);
-
-  // @ts-expect-error new-tab isn't in the type definition, but it does exist
-  window.on('new-tab', () => {
-    createNewTab(
-      options,
-      setupNativefierWindow,
-      options.targetUrl,
-      true,
-      window,
-    );
-  });
 }
--- a/app/src/helpers/windowHelpers.test.ts
+++ b/app/src/helpers/windowHelpers.test.ts
@ -1,9 +1,4 @@
-import {
-  dialog,
-  BrowserWindow,
-  HeadersReceivedResponse,
-  WebContents,
-} from 'electron';
+import { dialog, BrowserWindow, WebContents } from 'electron';
 jest.mock('loglevel');
 import { error } from 'loglevel';
 import { WindowOptions } from '../../../shared/src/options/model';
@ -143,7 +138,7 @@ describe('injectCSS', () => {
    expect(mockWebContentsInsertCSS).not.toHaveBeenCalled();
  });

-  test('will inject on did-navigate + onHeadersReceived', (done) => {
+  test('will inject on did-navigate + onResponseStarted', () => {
    mockGetCSSToInject.mockReturnValue(css);
    const window = new BrowserWindow();

@ -154,62 +149,17 @@ describe('injectCSS', () => {
    window.webContents.emit('did-navigate');
    // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
    // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-    window.webContents.session.webRequest.send(
-      'onHeadersReceived',
-      { responseHeaders, webContents: window.webContents },
-      (result: HeadersReceivedResponse) => {
-        expect(mockWebContentsInsertCSS).toHaveBeenCalledWith(css);
-        expect(result.cancel).toBe(false);
-        expect(result.responseHeaders).toBe(responseHeaders);
-        done();
-      },
-    );
+    window.webContents.session.webRequest.send('onResponseStarted', {
+      responseHeaders,
+      webContents: window.webContents,
+    });
+
+    expect(mockWebContentsInsertCSS).toHaveBeenCalledWith(css);
  });

-  test('will catch errors inserting CSS', (done) => {
-    mockGetCSSToInject.mockReturnValue(css);
-
-    mockWebContentsInsertCSS.mockReturnValue(
-      Promise.reject('css insertion error'),
-    );
-
-    const window = new BrowserWindow();
-
-    injectCSS(window);
-
-    expect(mockGetCSSToInject).toHaveBeenCalled();
-
-    window.webContents.emit('did-navigate');
-    // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-    window.webContents.session.webRequest.send(
-      'onHeadersReceived',
-      { responseHeaders, webContents: window.webContents },
-      (result: HeadersReceivedResponse) => {
-        expect(mockWebContentsInsertCSS).toHaveBeenCalledWith(css);
-        expect(mockLogError).toHaveBeenCalledWith(
-          'injectCSSIntoResponse ERROR',
-          'css insertion error',
-        );
-        expect(result.cancel).toBe(false);
-        expect(result.responseHeaders).toBe(responseHeaders);
-        done();
-      },
-    );
-  });
-
-  test.each<string | jest.DoneCallback>([
-    'application/json',
-    'font/woff2',
-    'image/png',
-  ])(
+  test.each<string>(['application/json', 'font/woff2', 'image/png'])(
    'will not inject for content-type %s',
-    // @ts-expect-error because TypeScript can't recognize that
-    // '(contentType: string, done: jest.DoneCallback) => void'
-    // and
-    // '(...args: (string | DoneCallback)[]) => any'
-    // are actually compatible.
-    (contentType: string, done: jest.DoneCallback) => {
+    (contentType: string) => {
      mockGetCSSToInject.mockReturnValue(css);
      const window = new BrowserWindow();

@ -223,32 +173,19 @@ describe('injectCSS', () => {
      mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined);
      // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      window.webContents.session.webRequest.send(
-        'onHeadersReceived',
-        {
-          responseHeaders,
-          webContents: window.webContents,
-          url: `test-${contentType}`,
-        },
-        (result: HeadersReceivedResponse) => {
-          // insertCSS will still run once for the did-navigate
-          expect(mockWebContentsInsertCSS).not.toHaveBeenCalled();
-          expect(result.cancel).toBe(false);
-          expect(result.responseHeaders).toBe(responseHeaders);
-          done();
-        },
-      );
+      window.webContents.session.webRequest.send('onResponseStarted', {
+        responseHeaders,
+        webContents: window.webContents,
+        url: `test-${contentType}`,
+      });
+      // insertCSS will still run once for the did-navigate
+      expect(mockWebContentsInsertCSS).not.toHaveBeenCalled();
    },
  );

-  test.each<string | jest.DoneCallback>(['text/html'])(
+  test.each<string>(['text/html'])(
    'will inject for content-type %s',
-    // @ts-expect-error because TypeScript can't recognize that
-    // '(contentType: string, done: jest.DoneCallback) => void'
-    // and
-    // '(...args: (string | DoneCallback)[]) => any'
-    // are actually compatible.
-    (contentType: string, done: jest.DoneCallback) => {
+    (contentType: string) => {
      mockGetCSSToInject.mockReturnValue(css);
      const window = new BrowserWindow();

@ -262,36 +199,19 @@ describe('injectCSS', () => {
      mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined);
      // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      window.webContents.session.webRequest.send(
-        'onHeadersReceived',
-        {
-          responseHeaders,
-          webContents: window.webContents,
-          url: `test-${contentType}`,
-        },
-        (result: HeadersReceivedResponse) => {
-          expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1);
-          expect(result.cancel).toBe(false);
-          expect(result.responseHeaders).toBe(responseHeaders);
-          done();
-        },
-      );
+      window.webContents.session.webRequest.send('onResponseStarted', {
+        responseHeaders,
+        webContents: window.webContents,
+        url: `test-${contentType}`,
+      });
+
+      expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1);
    },
  );

-  test.each<string | jest.DoneCallback>([
-    'image',
-    'script',
-    'stylesheet',
-    'xhr',
-  ])(
+  test.each<string>(['image', 'script', 'stylesheet', 'xhr'])(
    'will not inject for resource type %s',
-    // @ts-expect-error because TypeScript can't recognize that
-    // '(contentType: string, done: jest.DoneCallback) => void'
-    // and
-    // '(...args: (string | DoneCallback)[]) => any'
-    // are actually compatible.
-    (resourceType: string, done: jest.DoneCallback) => {
+    (resourceType: string) => {
      mockGetCSSToInject.mockReturnValue(css);
      const window = new BrowserWindow();

@ -303,33 +223,20 @@ describe('injectCSS', () => {
      mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined);
      // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      window.webContents.session.webRequest.send(
-        'onHeadersReceived',
-        {
-          responseHeaders,
-          webContents: window.webContents,
-          resourceType,
-          url: `test-${resourceType}`,
-        },
-        (result: HeadersReceivedResponse) => {
-          // insertCSS will still run once for the did-navigate
-          expect(mockWebContentsInsertCSS).not.toHaveBeenCalled();
-          expect(result.cancel).toBe(false);
-          expect(result.responseHeaders).toBe(responseHeaders);
-          done();
-        },
-      );
+      window.webContents.session.webRequest.send('onResponseStarted', {
+        responseHeaders,
+        webContents: window.webContents,
+        resourceType,
+        url: `test-${resourceType}`,
+      });
+      // insertCSS will still run once for the did-navigate
+      expect(mockWebContentsInsertCSS).not.toHaveBeenCalled();
    },
  );

-  test.each<string | jest.DoneCallback>(['html', 'other'])(
+  test.each<string>(['html', 'other'])(
    'will inject for resource type %s',
-    // @ts-expect-error because TypeScript can't recognize that
-    // '(contentType: string, done: jest.DoneCallback) => void'
-    // and
-    // '(...args: (string | DoneCallback)[]) => any'
-    // are actually compatible.
-    (resourceType: string, done: jest.DoneCallback) => {
+    (resourceType: string) => {
      mockGetCSSToInject.mockReturnValue(css);
      const window = new BrowserWindow();

@ -341,21 +248,13 @@ describe('injectCSS', () => {
      mockWebContentsInsertCSS.mockReset().mockResolvedValue(undefined);
      // @ts-expect-error this function doesn't exist in the actual electron version, but will in our mock
      // eslint-disable-next-line @typescript-eslint/no-unsafe-call
-      window.webContents.session.webRequest.send(
-        'onHeadersReceived',
-        {
-          responseHeaders,
-          webContents: window.webContents,
-          resourceType,
-          url: `test-${resourceType}`,
-        },
-        (result: HeadersReceivedResponse) => {
-          expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1);
-          expect(result.cancel).toBe(false);
-          expect(result.responseHeaders).toBe(responseHeaders);
-          done();
-        },
-      );
+      window.webContents.session.webRequest.send('onResponseStarted', {
+        responseHeaders,
+        webContents: window.webContents,
+        resourceType,
+        url: `test-${resourceType}`,
+      });
+      expect(mockWebContentsInsertCSS).toHaveBeenCalledTimes(1);
    },
  );
 });
--- a/app/src/helpers/windowHelpers.ts
+++ b/app/src/helpers/windowHelpers.ts
@ -3,10 +3,9 @@ import {
  BrowserWindow,
  BrowserWindowConstructorOptions,
  Event,
-  HeadersReceivedResponse,
  MessageBoxReturnValue,
-  OnHeadersReceivedListenerDetails,
  WebPreferences,
+  OnResponseStartedListenerDetails,
 } from 'electron';

 import log from 'loglevel';
@ -67,8 +66,12 @@ export function createAboutBlankWindow(
  setupWindow: (options: WindowOptions, window: BrowserWindow) => void,
  parent?: BrowserWindow,
 ): BrowserWindow {
-  const window = createNewWindow(options, setupWindow, 'about:blank', parent);
-  window.hide();
+  const window = createNewWindow(
+    { ...options, show: false },
+    setupWindow,
+    'about:blank',
+    parent,
+  );
  window.webContents.once('did-stop-loading', () => {
    if (window.webContents.getURL() === 'about:blank') {
      window.close();
@ -213,50 +216,33 @@ export function injectCSS(browserWindow: BrowserWindow): void {
        log.error('browserWindow.webContents.insertCSS', err),
      );

-    // We must inject css early enough; so onHeadersReceived is a good place.
-    // Will run multiple times, see `did-finish-load` event on the window
-    // that unsets this handler.
-    browserWindow.webContents.session.webRequest.onHeadersReceived(
+    // We must inject css early enough; so onResponseStarted is a good place.
+    browserWindow.webContents.session.webRequest.onResponseStarted(
      { urls: [] }, // Pass an empty filter list; null will not match _any_ urls
-      (
-        details: OnHeadersReceivedListenerDetails,
-        callback: (headersReceivedResponse: HeadersReceivedResponse) => void,
-      ) => {
-        const contentType =
-          details.responseHeaders && 'content-type' in details.responseHeaders
-            ? details.responseHeaders['content-type'][0]
-            : undefined;
-
-        log.debug('onHeadersReceived', {
-          contentType,
+      (details: OnResponseStartedListenerDetails): void => {
+        log.debug('onResponseStarted', {
          resourceType: details.resourceType,
          url: details.url,
        });
-
-        injectCSSIntoResponse(details, contentType, cssToInject)
-          .then((responseHeaders) => {
-            callback({
-              cancel: false,
-              responseHeaders,
-            });
-          })
-          .catch((err) => {
-            log.error('injectCSSIntoResponse ERROR', err);
-            callback({
-              cancel: false,
-              responseHeaders: details.responseHeaders,
-            });
-          });
+        injectCSSIntoResponse(details, cssToInject).catch((err: unknown) => {
+          log.error('injectCSSIntoResponse ERROR', err);
+        });
      },
    );
  });
 }

-async function injectCSSIntoResponse(
-  details: OnHeadersReceivedListenerDetails,
-  contentType: string | undefined,
+function injectCSSIntoResponse(
+  details: OnResponseStartedListenerDetails,
  cssToInject: string,
-): Promise<Record<string, string[]> | undefined> {
+): Promise<string | undefined> {
+  const contentType =
+    details.responseHeaders && 'content-type' in details.responseHeaders
+      ? details.responseHeaders['content-type'][0]
+      : undefined;
+
+  log.debug('injectCSSIntoResponse', { details, cssToInject, contentType });
+
  // We go with a denylist rather than a whitelist (e.g. only text/html)
  // to avoid "whoops I didn't think this should have been CSS-injected" cases
  const nonInjectableContentTypes = [
@ -280,7 +266,7 @@ async function injectCSSIntoResponse(
        details.resourceType
      } and content-type ${contentType as string}`,
    );
-    return details.responseHeaders;
+    return Promise.resolve(undefined);
  }

  log.debug(
@ -288,9 +274,7 @@ async function injectCSSIntoResponse(
      details.resourceType
    } and content-type ${contentType as string}`,
  );
-  await details.webContents.insertCSS(cssToInject);
-
-  return details.responseHeaders;
+  return details.webContents.insertCSS(cssToInject);
 }

 export function sendParamsOnDidFinishLoad(
--- a/app/src/mocks/electron.ts
+++ b/app/src/mocks/electron.ts
@ -126,22 +126,13 @@ class MockWebRequest {
    this.emitter = new InternalEmitter();
  }

-  onHeadersReceived(
+  onResponseStarted(
    filter: unknown,
-    listener:
-      | ((
-          details: unknown,
-          callback: (headersReceivedResponse: unknown) => void,
-        ) => void)
-      | null,
+    listener: ((details: unknown) => void) | null,
  ): void {
    if (listener) {
-      this.emitter.addListener(
-        'onHeadersReceived',
-        (
-          details: unknown,
-          callback: (headersReceivedResponse: unknown) => void,
-        ) => listener(details, callback),
+      this.emitter.addListener('onResponseStarted', (details: unknown) =>
+        listener(details),
      );
    }
  }
--- a/shared/src/options/model.ts
+++ b/shared/src/options/model.ts
@ -205,6 +205,7 @@ export type WindowOptions = {
  internalUrls?: string | RegExp;
  name: string;
  proxyRules?: string;
+  show?: boolean;
  targetUrl: string;
  userAgent?: string;
  zoom: number;
--- a/src/constants.ts
+++ b/src/constants.ts
@ -5,7 +5,8 @@ export const DEFAULT_APP_NAME = 'APP';
 // Update both DEFAULT_ELECTRON_VERSION and DEFAULT_CHROME_VERSION together,
 // and update app / package.json / devDeps / electron to value of DEFAULT_ELECTRON_VERSION
 export const DEFAULT_ELECTRON_VERSION = '13.4.0';
-export const DEFAULT_CHROME_VERSION = '91.0.4472.124';
+// https://atom.io/download/atom-shell/index.json
+export const DEFAULT_CHROME_VERSION = '91.0.4472.164';

 // Update each of these periodically
 // https://product-details.mozilla.org/1.0/firefox_versions.json
@ -13,9 +14,9 @@ export const DEFAULT_FIREFOX_VERSION = '92.0';

 // https://en.wikipedia.org/wiki/Safari_version_history
 export const DEFAULT_SAFARI_VERSION = {
-  majorVersion: 14,
-  version: '14.1.2',
-  webkitVersion: '611.3.10.1.5',
+  majorVersion: 15,
+  version: '15.0',
+  webkitVersion: '605.1.15',
 };

 export const ELECTRON_MAJOR_VERSION = parseInt(