Popup handling in BrowserViews doesn't work with WebContentsView
PalmerAL opened this issue · 4 comments
Preflight Checklist
- I have read the Contributing Guidelines for this project.
- I agree to follow the Code of Conduct that this project adheres to.
- I have searched the issue tracker for a feature request that matches the one I want to file, without success.
Problem Description
Previously described in #41432 (comment)
#41432 added an API to set a custom callback on popup window creation. This is necessary to, for example, have popups open in a BrowserView, rather than a new window.
With a BrowserView, this looks like:
mainWindow.webContents.setWindowOpenHandler((details) => {
return {
action: 'allow',
createWindow: (options) => {
const browserView = new BrowserView(options)
mainWindow.addBrowserView(browserView)
browserView.setBounds({ x: 0, y: 0, width: 640, height: 480 })
return browserView.webContents
}
}
})
options contains a WebContents, which is passed through to the BrowserView constructor.
The problem is that WebContentsView does not support passing in a WebContents during it's construction, which makes this API impossible to use.
For now, I'm continuing to use a BrowserView in my app, but as BrowserView is deprecated, I'm concerned that once it's removed, I won't have any migration path to a newer version.
Proposed Solution
- Option 1: Allow passing in a WebContents to the WebContentsView constructor. This is already supported internally, and used by the BrowserView compatibility layer, it just isn't publicly exposed: https://github.com/electron/electron/blob/main/lib/browser/api/browser-view.ts#L13-L17. I'm not sure what the reason for hiding it was.
- Option 2: Change the
setWindowOpenHandlerAPI to work around this somehow. I expect that would be harder; I'm not sure exactly what the correct API would look like in that case.
Alternatives Considered
None.
Additional Information
No response
Hiding the ability to construct a WebContentsView with a pre-existing WebContents was an oversight, not an intentional design decision. Let's fix it :)
My proposal would be:
diff --git a/docs/api/web-contents-view.md b/docs/api/web-contents-view.md
index b4d6b4c93c..66bb257cf0 100644
--- a/docs/api/web-contents-view.md
+++ b/docs/api/web-contents-view.md
@@ -35,10 +35,11 @@ Process: [Main](../glossary.md#main-process)
### `new WebContentsView([options])`
* `options` Object (optional)
* `webPreferences` [WebPreferences](structures/web-preferences.md) (optional) - Settings of web page's features.
+ * `webContents` [WebContents](web-contents.md) (optional) - If present, the given WebContents will be adopted by the WebContentsView. A WebContents may only be presented in one WebContentsView at a time.
-Creates an empty WebContentsView.
+Creates a WebContentsView.That proposal looks good to me; I believe it matches how BrowserView works.
To resolve this the following BrowserWindow code can be copied: shell\browser\api\electron_api_browser_window.cc
i.e.: In the 31-x-x branch....
// Copy the webContents option to webPreferences.
v8::Local<v8::Value> value;
if (options.Get("webContents", &value)) {
web_preferences.SetHidden("webContents", value);
}
in shell\browser\api\electron_api_web_contents_view.cc at line 157
This allows us to simply pass the options argument of createWindow straight into as follows: new WebContentsView(options)
Not sure who can add this.