fix: invoke the window open handler for _blank links (#28498)

* fix: invoke the window open handler for _blank links * add test
electron · Apr 6, 2021 · 62b3881 · 62b3881
1 parent 3ed8da0
commit 62b3881
Show file tree

Hide file tree

Showing 3 changed files with 80 additions and 24 deletions.
diff --git a/lib/browser/api/web-contents.ts b/lib/browser/api/web-contents.ts
@@ -433,7 +433,11 @@ WebContents.prototype._callWindowOpenHandler = function (event: Electron.Event,
     event.preventDefault();
     return null;
   } else if (response.action === 'allow') {
-    if (typeof response.overrideBrowserWindowOptions === 'object' && response.overrideBrowserWindowOptions !== null) { return response.overrideBrowserWindowOptions; } else { return {}; }
+    if (typeof response.overrideBrowserWindowOptions === 'object' && response.overrideBrowserWindowOptions !== null) {
+      return response.overrideBrowserWindowOptions;
+    } else {
+      return {};
+    }
   } else {
     event.preventDefault();
     console.error('The window open handler response must be an object with an \'action\' property of \'allow\' or \'deny\'.');
@@ -565,19 +569,22 @@ WebContents.prototype._init = function () {
     // Make new windows requested by links behave like "window.open".
     this.on('-new-window' as any, (event: ElectronInternal.Event, url: string, frameName: string, disposition: string,
       rawFeatures: string, referrer: Electron.Referrer, postData: PostData) => {
-      openGuestWindow({
-        event,
-        embedder: event.sender,
-        disposition,
-        referrer,
-        postData,
-        overrideBrowserWindowOptions: {},
-        windowOpenArgs: {
-          url,
-          frameName,
-          features: rawFeatures
-        }
-      });
+      const options = this._callWindowOpenHandler(event, url, frameName, rawFeatures);
+      if (!event.defaultPrevented) {
+        openGuestWindow({
+          event,
+          embedder: event.sender,
+          disposition,
+          referrer,
+          postData,
+          overrideBrowserWindowOptions: options || {},
+          windowOpenArgs: {
+            url,
+            frameName,
+            features: rawFeatures
+          }
+        });
+      }
     });
 
     let windowOpenOverriddenOptions: BrowserWindowConstructorOptions | null = null;

diff --git a/lib/browser/guest-window-manager.ts b/lib/browser/guest-window-manager.ts
@@ -42,7 +42,6 @@ export function openGuestWindow ({ event, embedder, guest, referrer, disposition
   windowOpenArgs: WindowOpenArgs,
 }): BrowserWindow | undefined {
   const { url, frameName, features } = windowOpenArgs;
-  const isNativeWindowOpen = !!guest;
   const { options: browserWindowOptions, additionalFeatures } = makeBrowserWindowOptions({
     embedder,
     features,
@@ -74,11 +73,14 @@ export function openGuestWindow ({ event, embedder, guest, referrer, disposition
     webContents: guest,
     ...browserWindowOptions
   });
-  if (!isNativeWindowOpen) {
+  if (!guest) {
     // We should only call `loadURL` if the webContents was constructed by us in
     // the case of BrowserWindowProxy (non-sandboxed, nativeWindowOpen: false),
     // as navigating to the url when creating the window from an existing
     // webContents is not necessary (it will navigate there anyway).
+    // This can also happen if we enter this function from OpenURLFromTab, in
+    // which case the browser process is responsible for initiating navigation
+    // in the new window.
     window.loadURL(url, {
       httpReferrer: referrer,
       ...(postData && {

diff --git a/spec-main/guest-window-manager-spec.ts b/spec-main/guest-window-manager-spec.ts
@@ -112,16 +112,21 @@ describe('webContents.setWindowOpenHandler', () => {
     const { browserWindowOptions } = testConfig[testName];
 
     describe(testName, () => {
-      beforeEach((done) => {
+      beforeEach(async () => {
         browserWindow = new BrowserWindow(browserWindowOptions);
-        browserWindow.loadURL('about:blank');
-        browserWindow.on('ready-to-show', () => { browserWindow.show(); done(); });
+        await browserWindow.loadURL('about:blank');
+        browserWindow.show();
       });
 
       afterEach(closeAllWindows);
 
-      it('does not fire window creation events if an override returns action: deny', (done) => {
-        browserWindow.webContents.setWindowOpenHandler(() => ({ action: 'deny' }));
+      it('does not fire window creation events if an override returns action: deny', async () => {
+        const denied = new Promise((resolve) => {
+          browserWindow.webContents.setWindowOpenHandler(() => {
+            setTimeout(resolve);
+            return { action: 'deny' };
+          });
+        });
         browserWindow.webContents.on('new-window', () => {
           assert.fail('new-window should not to be called with an overridden window.open');
         });
@@ -132,9 +137,51 @@ describe('webContents.setWindowOpenHandler', () => {
 
         browserWindow.webContents.executeJavaScript("window.open('about:blank') && true");
 
-        setTimeout(() => {
-          done();
-        }, 500);
+        await denied;
+      });
+
+      it('is called when clicking on a target=_blank link', async () => {
+        const denied = new Promise((resolve) => {
+          browserWindow.webContents.setWindowOpenHandler(() => {
+            setTimeout(resolve);
+            return { action: 'deny' };
+          });
+        });
+        browserWindow.webContents.on('new-window', () => {
+          assert.fail('new-window should not to be called with an overridden window.open');
+        });
+
+        browserWindow.webContents.on('did-create-window', () => {
+          assert.fail('did-create-window should not to be called with an overridden window.open');
+        });
+
+        await browserWindow.webContents.loadURL('data:text/html,<a target="_blank" href="http://example.com" style="display: block; width: 100%; height: 100%; position: fixed; left: 0; top: 0;">link</a>');
+        browserWindow.webContents.sendInputEvent({ type: 'mouseDown', x: 10, y: 10, button: 'left', clickCount: 1 });
+        browserWindow.webContents.sendInputEvent({ type: 'mouseUp', x: 10, y: 10, button: 'left', clickCount: 1 });
+
+        await denied;
+      });
+
+      it('is called when shift-clicking on a link', async () => {
+        const denied = new Promise((resolve) => {
+          browserWindow.webContents.setWindowOpenHandler(() => {
+            setTimeout(resolve);
+            return { action: 'deny' };
+          });
+        });
+        browserWindow.webContents.on('new-window', () => {
+          assert.fail('new-window should not to be called with an overridden window.open');
+        });
+
+        browserWindow.webContents.on('did-create-window', () => {
+          assert.fail('did-create-window should not to be called with an overridden window.open');
+        });
+
+        await browserWindow.webContents.loadURL('data:text/html,<a href="http://example.com" style="display: block; width: 100%; height: 100%; position: fixed; left: 0; top: 0;">link</a>');
+        browserWindow.webContents.sendInputEvent({ type: 'mouseDown', x: 10, y: 10, button: 'left', clickCount: 1, modifiers: ['shift'] });
+        browserWindow.webContents.sendInputEvent({ type: 'mouseUp', x: 10, y: 10, button: 'left', clickCount: 1, modifiers: ['shift'] });
+
+        await denied;
       });
 
       it('fires handler with correct params', (done) => {