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

fix: properly order out child windows on window.hide() #29821

Merged
merged 1 commit into from
Jun 24, 2021

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 21, 2021

Description of Change

Closes #29820.

Properly orders out child windows when window.hide() is called on a parent window. This should not change any end user behavior, but Chromium specifically DCHECKs for it so we should handle it properly.

Checklist

Release Notes

Notes: none.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/12-x-y labels Jun 21, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 21, 2021
@codebytere codebytere changed the title fix: properly order out child windows fix: properly order out child windows on window.hide() Jun 21, 2021
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.

Hm if I'm reading this right this is an annoying behavior change, where you can't open a child window and have it remain visible while hiding the parent. (E.g. Think about a certain app that has a calls window it opens as a child and then hides the parent).

Not sure what the correct solution is here but this will definitely affect existing apps

@codebytere
Copy link
Member Author

codebytere commented Jun 21, 2021

@MarshallOfSound I thought so too, but this behavior is the same for end users at present - if you add for example

const child = new BrowserWindow({
    width: 200,
    height: 200,
    parent: mainWindow
  })

  child.show()
  setTimeout(() => {
    mainWindow.hide()
  }, 5000)

to the default fiddle and run with e.g. 12.0.10 you'll see that when the setTimeout callback executes, both windows are hidden. Also, if you don't set the timeout and run:

child.show()
mainWindow.hide()

you end up with a ghost main window (look at the transparent window):

Screen Shot 2021-06-21 at 10 29 22 PM

which this PR also ameliorates.

Copy link
Contributor

@zcbenz zcbenz left a comment

Choose a reason for hiding this comment

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

I'm 👍 with this since it is fixing a DCHECK in Chromium.

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 22, 2021
@codebytere
Copy link
Member Author

Crash report failure is not relevant.

@codebytere codebytere merged commit f9bfd1d into main Jun 24, 2021
@codebytere codebytere deleted the fix-child-hide-dcheck branch June 24, 2021 18:43
@release-clerk
Copy link

release-clerk bot commented Jun 24, 2021

No Release Notes

@trop
Copy link
Contributor

trop bot commented Jun 24, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 24, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 24, 2021

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

@trop trop bot removed the target/13-x-y label Jun 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: DCHECK in remote_cocoa/app_shim/native_widget_ns_window_bridge.mm
4 participants