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

Jump to bottom

feat: Added ability to configure if window should close when opener closes #31314

Merged
merged 9 commits into from Feb 23, 2022
Merged

feat: Added ability to configure if window should close when opener closes #31314

merged 9 commits into from Feb 23, 2022

Conversation

t57ser
Copy link
Contributor

@t57ser t57ser commented Oct 6, 2021 •
edited

Description of Change

Fixes: #11128

Currently electron automatically closes child windows when the opener window closes.
This behavior is a left over from the Atom text editor and does not conform with standard browser behavior.
The PR adds the ability to control this behavior, since it is not possible to deactivate this without hacks.
Note: This is not a breaking change since the flag is not required and the default stays the same as it used to be.

Checklist

Release Notes

Notes: Added ability to configure if window should close when opener closes

@t57ser t57ser requested a review from a team as a code owner October 6, 2021 09:08
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Oct 6, 2021
@t57ser t57ser marked this pull request as draft October 6, 2021 14:09
@t57ser t57ser marked this pull request as ready for review October 7, 2021 07:36
@t57ser
Copy link
Contributor Author

t57ser commented Oct 7, 2021

I believe this one would also be for @nornagon. Since he is on leave I am tagging @codebytere

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Oct 13, 2021
@zcbenz zcbenz added api-review/requested 🗳 semver/minor backwards-compatible functionality labels Oct 21, 2021
@t57ser t57ser requested review from a team as code owners October 27, 2021 08:00
jkleinsc
jkleinsc requested changes Oct 28, 2021
View reviewed changes
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.

The documentation isn't clear - I've made a suggestion but feel free to improve it with something else.

docs/api/window-open.md Outdated Show resolved Hide resolved
@t57ser t57ser requested a review from jkleinsc November 9, 2021 07:20
@t57ser
Copy link
Contributor Author

t57ser commented Nov 25, 2021

Am I missing something?

@zcbenz zcbenz requested a review from a team December 8, 2021 08:00
@zcbenz
Copy link
Member

zcbenz commented Dec 8, 2021

People who are familiar with this API are happen to be either not available or busy with other things, and this month I believe this organization still won't get much activity, so sorry this might have to wait for review until next month.

@t57ser
Copy link
Contributor Author

t57ser commented Dec 8, 2021

Thank you for the info.

nornagon
nornagon reviewed Jan 25, 2022
View reviewed changes
Copy link
Member

@nornagon nornagon left a comment

Choose a reason for hiding this comment

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

This seems like a reasonable addition; it's a little regrettable that we have this default behavior left over from the early days.

I'd like to bikeshed a little on the name of the option as closeWithOpener is kind of a mindbender, and also it's generally preferable to have the default of a boolean be false. Perhaps outlivesParent or remainsWhenParentCloses? parent is sort of an imprecise term here but I'd rather avoid a name with both the words "open" and "close" in it. Or maybe closeWhenParentCloses (though this is the opposite sense & the default would be true)?

docs/api/window-open.md Outdated Show resolved Hide resolved
lib/browser/guest-window-manager.ts Outdated Show resolved Hide resolved
@nornagon
Copy link
Member

nornagon commented Feb 3, 2022

API LGTM

@jkleinsc jkleinsc added semver/minor backwards-compatible functionality and removed semver/minor backwards-compatible functionality labels Feb 8, 2022
jkleinsc
jkleinsc approved these changes Feb 8, 2022
View reviewed changes
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.

API LGTM

@zcbenz zcbenz changed the title feat: Added ability to configure if window should close when opener closes feat: Added ability to configure if window should close when opener closes FIXCI Feb 23, 2022
@zcbenz zcbenz changed the title feat: Added ability to configure if window should close when opener closes FIXCI feat: Added ability to configure if window should close when opener closes Feb 23, 2022
@zcbenz zcbenz merged commit 41b2945 into electron:main Feb 23, 2022
@release-clerk
Copy link

release-clerk bot commented Feb 23, 2022

Release Notes Persisted

Added ability to configure if window should close when opener closes

@commit-lint commit-lint bot mentioned this pull request Apr 14, 2022
Open
bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022
@t57ser @nornagon @bavulapati
feat: add ability to configure if window should close when opener clo…
9b1c58e 
…ses (electron#31314)

* feat: Added ability to configure if window should close when opener closes

* fix: check if embedder is destroyed

* fix: correctly take over closeWithOpener property

* chore: Added documentation

* Update docs/api/window-open.md

Co-authored-by: John Kleinschmidt <jkleinsc@github.com>

* chore: refactor

Co-authored-by: Jeremy Rose <nornagon@nornagon.net>

* chore: changed property name from `closeWithOpener` to `outlivesOpener`

* dummy change to kick lint

* undo above

Co-authored-by: John Kleinschmidt <jkleinsc@github.com>
Co-authored-by: Jeremy Rose <nornagon@nornagon.net>
@nornagon nornagon mentioned this pull request Nov 29, 2022
Merged
@ErikSin ErikSin mentioned this pull request Jan 26, 2023
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pthompson-asana pthompson-asana pthompson-asana left review comments

@nornagon nornagon nornagon approved these changes

@jkleinsc jkleinsc jkleinsc approved these changes

Assignees
No one assigned
Labels
api-review/approved ✅ api-review/requested 🗳 no-backport semver/minor backwards-compatible functionality
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Opened windows will be closed when the opener window closes.
5 participants
@t57ser @zcbenz @nornagon @jkleinsc @pthompson-asana