Skip to content
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.

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

docs: add <webview> new-window event removal to breaking-changes.md #36985

Merged
merged 1 commit into from May 31, 2023

Conversation

miniak
Copy link
Contributor

@miniak miniak commented Jan 21, 2023

Description of Change

Fix #34678. Follow-up to #34526

Checklist

  • PR description included and stakeholders cc'd
  • npm test passes
  • relevant documentation, tutorials, templates and examples are changed or added

Release Notes

Notes: none

@electron-cation electron-cation bot added documentation 📓 semver/patch backwards-compatible bug fixes new-pr 🌱 PR opened in the last 24 hours labels Jan 21, 2023
@miniak miniak added target/22-x-y PR should also be added to the "22-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. labels Jan 21, 2023
@miniak miniak mentioned this pull request Jan 21, 2023
1 task
@miniak miniak marked this pull request as ready for review January 21, 2023 20:34
@miniak miniak force-pushed the miniak/webview-new-window-event branch from c38c7d5 to e87dafc Compare January 21, 2023 20:37
@miniak
Copy link
Contributor Author

miniak commented Jan 21, 2023

@nornagon I am wondering whether we should re-implement the new-window event on <webview> using the new API somehow as it's kind of complicated to handle this compared to quite straightforward replacing of the webContents new-window event with setWindowOpenHandler(). On the other handle the renderer cannot do event.preventDefault() anyway, so it's not that useful. What do you think?

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jan 22, 2023
@@ -146,6 +146,15 @@ webContents.setWindowOpenHandler((details) => {
})
```

### Removed: `<webview>` `new-window` event

The `new-window` event of `<webview>` has been removed. There is no direct replacement.
Copy link
Member

Choose a reason for hiding this comment

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

We should probably give some guidance for how to move forward for apps that depend on this. Mention setWindowOpenHandler and link to docs, but note that there is no automatic forwarding to the embedder?

Copy link
Contributor Author

@miniak miniak Jan 23, 2023

Choose a reason for hiding this comment

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

the problem here is that the forwarding is kind of complicated to do on app-level, so mentioning setWindowOpenHandler without showing how to do it is not that useful

Copy link
Member

Choose a reason for hiding this comment

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

it ought to be possible to figure out which webview an open handler comes from, right...?

Copy link
Contributor Author

@miniak miniak Jan 24, 2023

Choose a reason for hiding this comment

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

you need to get the webContents of the <webview> first, set setWindowOpenHandler there and then you would need to send an IPC to a preload script in the <webview>, which would then call ipcRenderer.sendToHost() to get it as ipc-message event on the <webview> element, which is routed via the main process again

Copy link
Contributor Author

@miniak miniak Jan 24, 2023

Choose a reason for hiding this comment

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

something like this
main.js

mainWindow.webContents.on('will-attach-webview', (event, webPreferences) => {
  webPreferences.preload = path.join(__dirname, 'preload.js');
});

mainWindow.webContents.on('did-attach-webview', (event, wc) => {
  wc.setWindowOpenHandler(details => {
    wc.send('new-window', details);
    return { action: 'allow' };
  });
});

preload.js

const { ipcRenderer } = require('electron');

ipcRenderer.on('new-window', (event, data) => {
  ipcRenderer.sendToHost('new-window', data);
});

index.js

const webview = document.querySelector('webview')
webview.addEventListener('ipc-message', (event) => {
  if (event.channel === 'new-window') {
    const [details] = event.args;
    console.log(details);
  }
});

Copy link
Contributor Author

@miniak miniak Jan 25, 2023

Choose a reason for hiding this comment

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

But then you would send the IPC normally instead of getting it via the webview element and it will have to be passed through the contextBridge

Copy link
Member

Choose a reason for hiding this comment

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

you can definitely emit an event on a webview from the preload without a sendToHost IPC! it's just an EventTarget, you can make up an event and fire it however you like.

here's an example: https://gist.github.com/408586025cefd8a0396869e3aa5a1fa4

one snag i ran into is that it's not possible to call webview methods from the preload, in particular wv.getWebContentsId() would have been useful to make an example that would match the event to the right webview by querySelectorAll('webview').forEach(wv => wv.getWebContentsId() === wcId && wv.dispatchEvent(new Event('new-window'))) or so. this seems kind of like an oversight, but doesn't fundamentally prevent the technique from working.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nornagon can we merge this for now and address a better code sample in a follow-up?

Copy link
Member

Choose a reason for hiding this comment

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

@miniak no. I'm specifically commenting on the content of this documentation change with a request to improve the documentation. I've given a specific example of code which I think is better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added your example as fiddle

@jkleinsc jkleinsc added the target/24-x-y PR should also be added to the "24-x-y" branch. label Feb 9, 2023
@miniak miniak self-assigned this Feb 10, 2023
@codebytere
Copy link
Member

@nornagon is this still blocked re #36985 (comment)?

@jkleinsc jkleinsc requested a review from nornagon March 9, 2023 02:49
@nornagon
Copy link
Member

nornagon commented Mar 9, 2023

@codebytere I believe the PR hasn't changed since that comment, so yes.

@miniak
Copy link
Contributor Author

miniak commented Mar 10, 2023

@nornagon could you please help with the docs? I got quite busy with some Teams internal stuff and didn’t have time for this PR :(

Copy link
Contributor

@jkleinsc jkleinsc left a comment

Choose a reason for hiding this comment

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

Blocking this until #36985 (comment) is resolved.

@jkleinsc jkleinsc added the target/25-x-y PR should also be added to the "25-x-y" branch. label Apr 6, 2023
@miniak miniak force-pushed the miniak/webview-new-window-event branch from e87dafc to 8e3e689 Compare May 25, 2023 13:36
@miniak miniak requested a review from a team as a code owner May 25, 2023 13:36
@miniak miniak requested a review from jkleinsc May 25, 2023 16:20
@miniak miniak force-pushed the miniak/webview-new-window-event branch from 8e3e689 to 39cf439 Compare May 25, 2023 16:27
@jkleinsc jkleinsc merged commit 67f273a into main May 31, 2023
17 checks passed
@jkleinsc jkleinsc deleted the miniak/webview-new-window-event branch May 31, 2023 14:47
@release-clerk
Copy link

release-clerk bot commented May 31, 2023

No Release Notes

@trop
Copy link
Contributor

trop bot commented May 31, 2023

I have automatically backported this PR to "25-x-y", please check out #38523

@trop trop bot removed the target/25-x-y PR should also be added to the "25-x-y" branch. label May 31, 2023
@trop
Copy link
Contributor

trop bot commented May 31, 2023

I have automatically backported this PR to "22-x-y", please check out #38524

@trop
Copy link
Contributor

trop bot commented May 31, 2023

I have automatically backported this PR to "24-x-y", please check out #38525

@trop
Copy link
Contributor

trop bot commented May 31, 2023

I have automatically backported this PR to "23-x-y", please check out #38526

@trop trop bot added in-flight/22-x-y merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch merged/22-x-y PR was merged to the "22-x-y" branch. merged/25-x-y PR was merged to the "25-x-y" branch. and removed target/22-x-y PR should also be added to the "22-x-y" branch. target/24-x-y PR should also be added to the "24-x-y" branch. target/23-x-y PR should also be added to the "23-x-y" branch. in-flight/23-x-y in-flight/25-x-y labels May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📓 merged/22-x-y PR was merged to the "22-x-y" branch. merged/23-x-y PR was merged to the "23-x-y" branch. merged/24-x-y PR was merged to the "24-x-y" branch merged/25-x-y PR was merged to the "25-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Please fix the webview docs
5 participants