8000 feat: add webContents.setWindowOpenHandler API by loc · Pull Request #24517 · electron/electron · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

feat: add webContents.setWindowOpenHandler API #24517

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 54 commits into from
Nov 10, 2020
Merged

feat: add webContents.setWindowOpenHandler API #24517

merged 54 commits into from
Nov 10, 2020

Conversation

loc
Copy link
Contributor
@loc loc commented Jul 10, 2020 •
edited by nornagon
Loading

API changes:

  • Deprecate webContents new-window event in favor of webContents.setWindowOpenHandler(handler) and did-create-window

    new-window is fired after the contents is already created by Chrome. This is a problem when the application wants to prevent that window from being opened, as killing the window during initialization causes many problems and leads to the inevitable crash. Instead, we'll allow cancellation/customization of the new window before it's created. In congress with a change to set WebPreferences via IPC instead of the command line, this allows us to completely* change the child window's BrowserWindowConstructorOptions, even for in-process windows.

    *With the exception of a few outstanding preferences that we need early in initialization, like offscreen and nodeIntegrationInWorker. These could be fixed with some effort.

    Old way:

    mainWindow.webContents.on('new-window', (event, url, frameName, disposition, options, additionalFeatures) => {
    Object.assign(options, {
      modal: true,
    });
    childWindow = new BrowserWindow(options);
    event.newGuest = childWindow;
});

// or

mainWindow.webContents.on('new-window', (event, url, frameName, disposition, options, additionalFeatures) => {
    event.preventDefault();
});

    New way:

    mainWindow.webContents.setWindowOpenHandler(({ url, frameName }) => {
  return {
    action: 'allow',
    overrideBrowserWindowOptions: {
      modal: true,  
    }
  }
});
mainWindow.webContents.on('did-create-window', (win, { url, frameName, options, disposition, additionalFeatures, referrer, postData }) => {
  childWindow = win;
});

// or

mainWindow.webContents.setWindowOpenHandler(({ url, frameName }) => {
  return { action: 'deny' };
});

  • (internal API) webFrame.getWebPreference(prefName)

    Previously, preferences were stuffed into command line switches so that the renderer could look and see what settings it was supposed to be using. However, this wouldn't work for in-process child windows. Instead, we'll stuff Electron preferences into the struct that Chrome already uses for this sort of thing and add an API for the renderer to retreive those preferences. At the moment, only Electron-specific WebPreferences are available for lookup, but it would be easy to add Chromium-defined prefs like defaultFontSize, for example.

    const { webFrame } = require('electron');
const isContextIsolationEnabled = webFrame.getWebPreference('contextIsolation');

Implementation details:

  • Allowing for in-process WebPreference changes

    Prior to this change, WebPreferences were set via the command line, which meant that in-process child windows were forced to inherit WebPreferences from their parent. Now, we overload the Chrome WebPreference struct with our own properties so they are accessible in the renderer (via a new webFrame.getWebPreference).

    window-open

  • Some behavior changes that make window opening less, um, surprising.

    • Non-security related BrowserWindowConstructorOptions are no longer inherited from the parent. This was confusing when you had to "undo" preferences that may have been set in a different part of your app.
    • window.open features keys without values are now interpreted as true, to w3c spec.
    • We no longer check for cycles in BrowserWindowOptions. (Handle cycles when merging browser window options #8340)

Checklist

Release Notes

Notes: Add setWindowOpenHandler API for renderer-created child windows, and deprecate new-window event.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened recently label Jul 10, 2020
MarshallOfSound
MarshallOfSound reviewed Jul 11, 2020
View reviewed changes
Copy link
Member
@MarshallOfSound MarshallOfSound left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome stuff, love the diagram 😄

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened recently label Jul 11, 2020
@loc loc force-pushed the in-process-web-prefs branch from f14d641 to a2f17c6 Compare July 21, 2020 17:22
@loc loc force-pushed the in-process-web-prefs branch from a2f17c6 to ab954be Compare July 21, 2020 17:37
loc
loc commented Jul 21, 2020
View reviewed changes
@loc loc marked this pull request as ready for review July 21, 2020 19:45
@loc loc requested a review from a team as a code owner July 21, 2020 19:45
@loc loc requested a review from nornagon July 21, 2020 19:50
@loc
Copy link
Contributor Author
loc commented Jul 21, 2020

A couple tests are still failing, but I'm going to get the review ball rolling while I address them.

@loc loc mentioned this pull request Jul 23, 2020
Closed
6 tasks
@loc loc requested a review from MarshallOfSound July 23, 2020 15:32
@nornagon no 8000 rnagon mentioned this pull request Oct 27, 2020
Merged
2 tasks
@jkleinsc
Copy link
Member

I'm in favor of approach (4). Mutating the event feels clunky even if there is precedence to do so. I also like the declarative nature of the return object on (4).

@jkleinsc
Copy link
Member

The API WG approved this PR at our Nov 9, 2020 meeting

@nornagon nornagon merged commit 0b85fdf into master Nov 10, 2020
@release-clerk
Copy link
release-clerk bot commented Nov 10, 2020

Release Notes Persisted

Add setWindowOpenHandler API for renderer-created child windows, and deprecate new-window event.

@nornagon nornagon deleted the in-process-web-prefs branch November 10, 2020 17:06
@sentialx
Copy link
Contributor

Is it now possible to set e.newGuest to a BrowserView?

@codebykat codebykat mentioned this pull request Nov 15, 2020
Closed
This was referenced Nov 17, 2020
Closed
Merged
@stewartlord
Copy link
Contributor

@nornagon Is there a way to influence how the new window is created? Specifically, we are looking at how to open into a BrowserView

@stewartlord
Copy link
Contributor

@loc @nornagon Just wanted to share an idea that @sentialx and I discussed to add support for browser views. We tried to think of a way to do this without changing too much about the new setWindowOpenHandler() API. I'm not sure it's ideal, but it looks like the design is being driven by a requirement to leave creation of the new guest up to electron internally? With that in mind:

mainWindow.webContents.setWindowOpenHandler(({ url, frameName }) => {
  return {
    action: 'allow',
    guestType: 'browserView', // default: 'window'
    overrideBrowserViewOptions: {
      ...
    }
  }
});
mainWindow.webContents.on('did-create-browser-view', (view, { url, frameName, options, disposition, additionalFeatures, referrer, postData }) => {
  browserWindow.addBrowserView(view)
  ...
});

I believe there might be an outstanding issue re: 'close'? Eryk was going to think about this a bit more and refine the idea, but I wanted to get some initial feedback on the above.

@nornagon
Copy link
Contributor

@stewartlord hm, that proposal raises a few concerns for me, specifically it seems difficult to correctly correlate the window open handler invocation with the did-create-browser-view event.

However, I like the direction this is going in and to collect discussion I'd encourage you to open a proposal over at https://github.com/electron/governance/tree/master/wg-api/spec-documents (see https://github.com/electron/governance/blob/master/wg-api/spec-documents/api-spec-template.md for a template)

@stewartlord
Copy link
Contributor

@nornagon Thanks for the feedback. Eryk had a different idea and opened a PR that should solve the issue with less effort:
#26802

@nornagon nornagon mentioned this pull request Dec 15, 2020
Closed
3 tasks
@pushkin-
Copy link

Is there a way with this new paradigm to pend window creation? I only want to create the window if the URL is in my allowlist (whitelist). Currently, I'm preventing the new-window event, checking my allowlist asynchronously, and then creating the window.

Is the way to do this using the new paradigm to create the window in a hidden state, then after it's been created, check with the allowlist and close the window if the URL doesn't pass the check; otherwise, show the window? I presume I can do that bit in mainWindow.webContents.on('did-create-window' since I expect that can be asynchronous.

@nornagon
Copy link
Contributor

There is not. Chromium code forces that a decision about whether to create the window or not be made synchronously.

@ericblade ericblade mentioned this pull request Mar 3, 2021
Closed
@tgxn tgxn mentioned this pull request Mar 5, 2021
Merged
2 tasks
@bvaughn bvaughn mentioned this pull request Apr 27, 2021
Merged
SpacingBat3 added a commit to SpacingBat3/WebCord that referenced this pull request May 7, 2021
@SpacingBat3
Switch from deprecated event to newer function.
2a7cae1 
Switch from event 'new-window' to function 'setWindowOpenHandler()'.
For more information, see:
electron/electron#24517
@miniak miniak mentioned this pull request Aug 9, 2021
Closed
3 tasks
@VerteDinde VerteDinde mentioned this pull request Sep 20, 2021
Merged
5 tasks
@ErikSin ErikSin mentioned this pull request Jan 25, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@miniak miniak miniak left review comments

@nornagon nornagon nornagon approved these changes

@MarshallOfSound MarshallOfSound Awaiting requested review from MarshallOfSound

Assignees
No one assigned
Labels
api-review/approved ✅
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@loc @jkleinsc @nornagon @sentialx @stewartlord @pushkin- @miniak @MarshallOfSound
0