feat: enable nativeWindowOpen by default (#28552)

nornagon · web-flow · commit f8bdef5349a0 · 2021-04-13T15:36:38.000-04:00
* feat: enable nativeWindowOpen by default

* set nativeWindowOpen: false on spec/ main window

* update snapshots

* fix tests

* fix test

* fix webview test missing allowpopups

* fix other test

* update default
diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md
@@ -349,9 +349,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
       context in the dev tools by selecting the 'Electron Isolated Context'
       entry in the combo box at the top of the Console tab.
     * `nativeWindowOpen` Boolean (optional) - Whether to use native
-      `window.open()`. Defaults to `false`. Child windows will always have node
-      integration disabled unless `nodeIntegrationInSubFrames` is true. **Note:** This option is currently
-      experimental.
+      `window.open()`. Defaults to `true`. Child windows will always have node
+      integration disabled unless `nodeIntegrationInSubFrames` is true.
     * `webviewTag` Boolean (optional) - Whether to enable the [`<webview>` tag](webview-tag.md).
       Defaults to `false`. **Note:** The
       `preload` script configured for the `<webview>` will have node integration
diff --git a/docs/api/window-open.md b/docs/api/window-open.md
@@ -6,16 +6,15 @@ untrusted content within a renderer. Windows can be created from the renderer in
 * clicking on links or submitting forms adorned with `target=_blank`
 * JavaScript calling `window.open()`
 
-In non-sandboxed renderers, or when `nativeWindowOpen` is false (the default), this results in the creation of a
-[`BrowserWindowProxy`](browser-window-proxy.md), a light wrapper around
-`BrowserWindow`.
+For same-origin content, the new window is created within the same process,
+enabling the parent to access the child window directly. This can be very
+useful for app sub-windows that act as preference panels, or similar, as the
+parent can render to the sub-window directly, as if it were a `div` in the
+parent. This is the same behavior as in the browser.
 
-However, when the `sandbox` (or directly, `nativeWindowOpen`) option is set, a
-`Window` instance is created, as you'd expect in the browser. For same-origin
-content, the new window is created within the same process, enabling the parent
-to access the child window directly. This can be very useful for app sub-windows that act
-as preference panels, or similar, as the parent can render to the sub-window
-directly, as if it were a `div` in the parent.
+When `nativeWindowOpen` is set to false, `window.open` instead results in the
+creation of a [`BrowserWindowProxy`](browser-window-proxy.md), a light wrapper
+around `BrowserWindow`.
 
 Electron pairs this native Chrome `Window` with a BrowserWindow under the hood.
 You can take advantage of all the customization available when creating a
@@ -69,49 +68,18 @@ window.open('https://github.com', '_blank', 'top=500,left=200,frame=false,nodeIn
 
 To customize or cancel the creation of the window, you can optionally set an
 override handler with `webContents.setWindowOpenHandler()` from the main
-process. Returning `false` cancels the window, while returning an object sets
-the `BrowserWindowConstructorOptions` used when creating the window. Note that
-this is more powerful than passing options through the feature string, as the
-renderer has more limited privileges in deciding security preferences than the
-main process.
-
-### `BrowserWindowProxy` example
-
-```javascript
-
-// main.js
-const mainWindow = new BrowserWindow()
-
-mainWindow.webContents.setWindowOpenHandler(({ url }) => {
-  if (url.startsWith('https://github.com/')) {
-    return { action: 'allow' }
-  }
-  return { action: 'deny' }
-})
-
-mainWindow.webContents.on('did-create-window', (childWindow) => {
-  // For example...
-  childWindow.webContents.on('will-navigate', (e) => {
-    e.preventDefault()
-  })
-})
-```
-
-```javascript
-// renderer.js
-const windowProxy = window.open('https://github.com/', null, 'minimizable=false')
-windowProxy.postMessage('hi', '*')
-```
+process. Returning `{ action: 'deny' }` cancels the window. Returning `{
+action: 'allow', overrideBrowserWindowOptions: { ... } }` will allow opening
+the window and setting the `BrowserWindowConstructorOptions` to be used when
+creating the window. Note that this is more powerful than passing options
+through the feature string, as the renderer has more limited privileges in
+deciding security preferences than the main process.
 
 ### Native `Window` example
 
 ```javascript
 // main.js
-const mainWindow = new BrowserWindow({
-  webPreferences: {
-    nativeWindowOpen: true
-  }
-})
+const mainWindow = new BrowserWindow()
 
 // In this example, only windows with the `about:blank` url will be created.
 // All other urls will be blocked.
@@ -135,3 +103,33 @@ mainWindow.webContents.setWindowOpenHandler(({ url }) => {
 const childWindow = window.open('', 'modal')
 childWindow.document.write('<h1>Hello</h1>')
 ```
+
+### `BrowserWindowProxy` example
+
+```javascript
+
+// main.js
+const mainWindow = new BrowserWindow({
+  webPreferences: { nativeWindowOpen: false }
+})
+
+mainWindow.webContents.setWindowOpenHandler(({ url }) => {
+  if (url.startsWith('https://github.com/')) {
+    return { action: 'allow' }
+  }
+  return { action: 'deny' }
+})
+
+mainWindow.webContents.on('did-create-window', (childWindow) => {
+  // For example...
+  childWindow.webContents.on('will-navigate', (e) => {
+    e.preventDefault()
+  })
+})
+```
+
+```javascript
+// renderer.js
+const windowProxy = window.open('https://github.com/', null, 'minimizable=false')
+windowProxy.postMessage('hi', '*')
+```
diff --git a/docs/breaking-changes.md b/docs/breaking-changes.md
@@ -28,6 +28,16 @@ ensure your code works with this property enabled.  It has been enabled by defau
 
 You will be affected by this change if you use either `webFrame.executeJavaScript` or `webFrame.executeJavaScriptInIsolatedWorld`. You will need to ensure that values returned by either of those methods are supported by the [Context Bridge API](api/context-bridge.md#parameter--error--return-type-support) as these methods use the same value passing semantics.
 
+### Default Changed: `nativeWindowOpen` defaults to `true`
+
+Prior to Electron 14, `window.open` was by default shimmed to use
+`BrowserWindowProxy`. This meant that `window.open('about:blank')` did not work
+to open synchronously scriptable child windows, among other incompatibilities.
+`nativeWindowOpen` is no longer experimental, and is now the default.
+
+See the documentation for [window.open in Electron](api/window-open.md)
+for more details.
+
 ## Planned Breaking API Changes (13.0)
 
 ### API Changed: `session.setPermissionCheckHandler(handler)`
diff --git a/shell/browser/web_contents_preferences.cc b/shell/browser/web_contents_preferences.cc
@@ -141,7 +141,7 @@ WebContentsPreferences::WebContentsPreferences(
   SetDefaultBoolIfUndefined(options::kDisableHtmlFullscreenWindowResize, false);
   SetDefaultBoolIfUndefined(options::kWebviewTag, false);
   SetDefaultBoolIfUndefined(options::kSandbox, false);
-  SetDefaultBoolIfUndefined(options::kNativeWindowOpen, false);
+  SetDefaultBoolIfUndefined(options::kNativeWindowOpen, true);
   SetDefaultBoolIfUndefined(options::kContextIsolation, true);
   SetDefaultBoolIfUndefined(options::kJavaScript, true);
   SetDefaultBoolIfUndefined(options::kImages, true);
@@ -474,7 +474,7 @@ void WebContentsPreferences::OverrideWebkitPrefs(
   GetPreloadPath(&prefs->preload);
 
   // Check if nativeWindowOpen is enabled.
-  prefs->native_window_open = IsEnabled(options::kNativeWindowOpen);
+  prefs->native_window_open = IsEnabled(options::kNativeWindowOpen, true);
 
   // Check if we have node integration specified.
   prefs->node_integration = IsEnabled(options::kNodeIntegration);
diff --git a/spec-main/api-browser-window-spec.ts b/spec-main/api-browser-window-spec.ts
@@ -3075,19 +3075,7 @@ describe('BrowserWindow module', () => {
       expect(url).to.equal('http://example.com/test');
       expect(frameName).to.equal('');
       expect(disposition).to.equal('foreground-tab');
-      expect(options).to.deep.equal({
-        show: true,
-        width: 800,
-        height: 600,
-        webPreferences: {
-          contextIsolation: true,
-          nodeIntegration: false,
-          webviewTag: false,
-          nodeIntegrationInSubFrames: false,
-          openerId: options.webPreferences!.openerId
-        },
-        webContents: undefined
-      });
+      expect(options).to.be.an('object').not.null();
       expect(referrer.policy).to.equal('strict-origin-when-cross-origin');
       expect(referrer.url).to.equal('');
       expect(additionalFeatures).to.deep.equal([]);
diff --git a/spec-main/chromium-spec.ts b/spec-main/chromium-spec.ts
@@ -744,7 +744,9 @@ describe('chromium features', () => {
     }
 
     it('disables node integration when it is disabled on the parent window for chrome devtools URLs', async () => {
-      const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } });
+      // NB. webSecurity is disabled because native window.open() is not
+      // allowed to load devtools:// URLs.
+      const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, webSecurity: false } });
       w.loadURL('about:blank');
       w.webContents.executeJavaScript(`
         { b = window.open('devtools://devtools/bundled/inspector.html', '', 'nodeIntegration=no,show=no'); null }
@@ -755,8 +757,8 @@ describe('chromium features', () => {
     });
 
     it('disables JavaScript when it is disabled on the parent window', async () => {
-      const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true } });
-      w.webContents.loadURL('about:blank');
+      const w = new BrowserWindow({ show: true, webPreferences: { nodeIntegration: true } });
+      w.webContents.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
       const windowUrl = require('url').format({
         pathname: `${fixturesPath}/pages/window-no-javascript.html`,
         protocol: 'file',
@@ -783,7 +785,7 @@ describe('chromium features', () => {
         targetURL = `file://${fixturesPath}/pages/base-page.html`;
       }
       const w = new BrowserWindow({ show: false });
-      w.loadURL('about:blank');
+      w.webContents.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
       w.webContents.executeJavaScript(`{ b = window.open(${JSON.stringify(targetURL)}); null }`);
       const [, window] = await emittedOnce(app, 'browser-window-created');
       await emittedOnce(window.webContents, 'did-finish-load');
@@ -792,7 +794,7 @@ describe('chromium features', () => {
 
     it('defines a window.location setter', async () => {
       const w = new BrowserWindow({ show: false });
-      w.loadURL('about:blank');
+      w.webContents.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
       w.webContents.executeJavaScript('{ b = window.open("about:blank"); null }');
       const [, { webContents }] = await emittedOnce(app, 'browser-window-created');
       await emittedOnce(webContents, 'did-finish-load');
@@ -803,7 +805,7 @@ describe('chromium features', () => {
 
     it('defines a window.location.href setter', async () => {
       const w = new BrowserWindow({ show: false });
-      w.loadURL('about:blank');
+      w.webContents.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html'));
       w.webContents.executeJavaScript('{ b = window.open("about:blank"); null }');
       const [, { webContents }] = await emittedOnce(app, 'browser-window-created');
       await emittedOnce(webContents, 'did-finish-load');
@@ -1035,7 +1037,7 @@ describe('chromium features', () => {
           // We are testing whether context (3) can access context (2) under various conditions.
 
           // This is context (1), the base window for the test.
-          const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, webviewTag: true, contextIsolation: false } });
+          const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, webviewTag: true, contextIsolation: false, nativeWindowOpen: false } });
           await w.loadURL('about:blank');
 
           const parentCode = `new Promise((resolve) => {
diff --git a/spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt b/spec-main/fixtures/snapshots/proxy-window-open.snapshot.txt
@@ -2,9 +2,7 @@
   [
     "top=5,left=10,resizable=no",
     {
-      "sender": "[WebContents]",
-      "frameId": 1,
-      "processId": "placeholder-process-id"
+      "sender": "[WebContents]"
     },
     "about:blank",
     "frame-name",
@@ -20,10 +18,11 @@
       "y": 5,
       "webPreferences": {
         "contextIsolation": true,
+        "nativeWindowOpen": true,
         "nodeIntegration": false,
         "webviewTag": false,
         "nodeIntegrationInSubFrames": false,
-        "openerId": "placeholder-opener-id"
+        "openerId": null
       },
       "webContents": "[WebContents]"
     },
@@ -37,9 +36,7 @@
   [
     "zoomFactor=2,resizable=0,x=0,y=10",
     {
-      "sender": "[WebContents]",
-      "frameId": 1,
-      "processId": "placeholder-process-id"
+      "sender": "[WebContents]"
     },
     "about:blank",
     "frame-name",
@@ -54,10 +51,11 @@
       "webPreferences": {
         "zoomFactor": "2",
         "contextIsolation": true,
+        "nativeWindowOpen": true,
         "nodeIntegration": false,
         "webviewTag": false,
         "nodeIntegrationInSubFrames": false,
-        "openerId": "placeholder-opener-id"
+        "openerId": null
       },
       "webContents": "[WebContents]"
     },
@@ -71,9 +69,7 @@
   [
     "backgroundColor=gray,webPreferences=0,x=100,y=100",
     {
-      "sender": "[WebContents]",
-      "frameId": 1,
-      "processId": "placeholder-process-id"
+      "sender": "[WebContents]"
     },
     "about:blank",
     "frame-name",
@@ -85,10 +81,11 @@
       "backgroundColor": "gray",
       "webPreferences": {
         "contextIsolation": true,
+        "nativeWindowOpen": true,
         "nodeIntegration": false,
         "webviewTag": false,
         "nodeIntegrationInSubFrames": false,
-        "openerId": "placeholder-opener-id",
+        "openerId": null,
         "backgroundColor": "gray"
       },
       "x": 100,
@@ -105,9 +102,7 @@
   [
     "x=50,y=20,title=sup",
     {
-      "sender": "[WebContents]",
-      "frameId": 1,
-      "processId": "placeholder-process-id"
+      "sender": "[WebContents]"
     },
     "about:blank",
     "frame-name",
@@ -121,10 +116,11 @@
       "title": "sup",
       "webPreferences": {
         "contextIsolation": true,
+        "nativeWindowOpen": true,
         "nodeIntegration": false,
         "webviewTag": false,
         "nodeIntegrationInSubFrames": false,
-        "openerId": "placeholder-opener-id"
+        "openerId": null
       },
       "webContents": "[WebContents]"
     },
@@ -138,9 +134,7 @@
   [
     "show=false,top=1,left=1",
     {
-      "sender": "[WebContents]",
-      "frameId": 1,
-      "processId": "placeholder-process-id"
+      "sender": "[WebContents]"
     },
     "about:blank",
     "frame-name",
@@ -155,10 +149,11 @@
       "y": 1,
       "webPreferences": {
         "contextIsolation": true,
+        "nativeWindowOpen": true,
         "nodeIntegration": false,
         "webviewTag": false,
         "nodeIntegrationInSubFrames": false,
-        "openerId": "placeholder-opener-id"
+        "openerId": null
       },
       "webContents": "[WebContents]"
     },
diff --git a/spec/static/main.js b/spec/static/main.js
@@ -109,7 +109,8 @@ app.whenReady().then(async function () {
       backgroundThrottling: false,
       nodeIntegration: true,
       webviewTag: true,
-      contextIsolation: false
+      contextIsolation: false,
+      nativeWindowOpen: false
     }
   });
   window.loadFile('static/index.html', {
diff --git a/spec/webview-spec.js b/spec/webview-spec.js
@@ -501,7 +501,8 @@ describe('<webview> tag', function () {
   describe('new-window event', () => {
     it('emits when window.open is called', async () => {
       loadWebView(webview, {
-        src: `file://${fixtures}/pages/window-open.html`
+        src: `file://${fixtures}/pages/window-open.html`,
+        allowpopups: true
       });
       const { url, frameName } = await waitForEvent(webview, 'new-window');
 
@@ -511,7 +512,8 @@ describe('<webview> tag', function () {
 
     it('emits when link with target is called', async () => {
       loadWebView(webview, {
-        src: `file://${fixtures}/pages/target-name.html`
+        src: `file://${fixtures}/pages/target-name.html`,
+        allowpopups: true
       });
       const { url, frameName } = await waitForEvent(webview, 'new-window');
 
