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

Add experimental popup window support #1608

Merged
merged 7 commits into from
Jun 10, 2021
Merged

Add experimental popup window support #1608

merged 7 commits into from
Jun 10, 2021

Conversation

PalmerAL
Copy link
Collaborator

This adds support for opening popup windows from webpages, using this new feature of Electron. Implementing this would fix most of the compatibility issues mentioned in #140

Known issues:

  • ctrl+clicking on links to open them in a new tab no longer works. To fix that, we need feat: add more info in setWindowOpenHandler details electron/electron#28518, which requires upgrading to Electron 14 (or maybe 13?)
  • Occasionally the browser crashes when closing the popup window.
    • It would be really helpful to find a consistent way to reproduce this - if anyone finds one while testing, please let me know.

@PalmerAL
Copy link
Collaborator Author

CC @blackgwe - this fixes the issue with the hypothes.is login page.

@blackgwe
Copy link
Contributor

@PalmerAL Thank you very much. It works for me. Unfortunately, I don't know any other page with pop-up windows where I can test the browser crash. If I can help, pls let me know!

I tested minbrowser with current electron 14.0.0-nightly.20210413 successfully, but the "ctrl+clicking on links" bug does not disappear.

Another idea: The "ctrl+clicking on links" bug shows that it's possible to support multiple minbrowser windows (of course it's confusing to have tasks, tabs and windows…) But maybe you can have a config value in the min://settings) to enable "windows support" which means, there is a menu item "File Menu" → "Open new window".
-> #1582
-> #1573

@PalmerAL
Copy link
Collaborator Author

Great!

Once we upgrade, I think we need to add a windowOpenHandler (https://www.electronjs.org/docs/api/web-contents#contentssetwindowopenhandlerhandler) and then it should probably work.

Multiple windows would be nice, but there's a lot of features of Min that won't work without substantial work to modify them (I listed some of them here: #169 (comment)). Fixing all of those problems hasn't been a priority, and so I don't think it makes sense to have a hidden setting for something that doesn't really work.

@PalmerAL
Copy link
Collaborator Author

PalmerAL commented Jun 6, 2021

ctrl+clicking is fixed now, and I'm not seeing any other crashes (or any other issues really), so I'm hoping this is done.

I'll probably test this for a little bit, and assuming no issues, merge and release a beta version with this change.

@PalmerAL PalmerAL merged commit cf4a2c9 into master Jun 10, 2021
@PalmerAL PalmerAL changed the title [WIP] Add experimental popup window support Add experimental popup window support Jun 10, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants