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: don't assign NSAlert to window which is not visible #22672
fix: don't assign NSAlert to window which is not visible #22672
Conversation
Without this change it's possible to create message box which can't be dismissed on mac.
03852dd
to
713e82d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified locally - thanks for the fix!
@CezaryKulakowski this appears to be causing a crash on Linux. |
@codebytere this fix changes only |
@CezaryKulakowski misread - you're correct. Thought it was a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we document this behavior and tell folks to avoid this scenario to call window.show()
before opening the modal?
@MarshallOfSound in that case, would we want to show the window ourselves if it's hidden before invoking the message box? I wanted to avoid the breaking change in this PR but it might be something to consider |
@MarshallOfSound what do you mean by documenting this behavior and telling folks to avoid this scenario to call window.show() before opening the modal? Leaving it as it is now and adding some information to documentation? The scenario leading to it isn't very unusual and it breaks the ui. |
Yes exactly that |
@MarshallOfSound @CezaryKulakowski Maybe we can do this fix and document it explicitly? The |
@@ -98,7 +98,7 @@ int ShowMessageBoxSync(const MessageBoxSettings& settings) { | |||
|
|||
// Use runModal for synchronous alert without parent, since we don't have a | |||
// window to wait for. | |||
if (!settings.parent_window) | |||
if (!settings.parent_window || !settings.parent_window->IsVisible()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should leave a comment on why IsVisible
is checked here.
@CezaryKulakowski Can you also document this behavior? (On macOS showing message box on invisible parent window would behave the same with no parent window.) @MarshallOfSound Are you OK with both documenting this scenario and having a workaround? |
@zcbenz I've pushed a fixup which updates documentation. I hope this is what you meant. |
Release Notes Persisted
|
I have automatically backported this PR to "8-x-y", please check out #23088 |
I have automatically backported this PR to "7-2-x", please check out #23089 |
I have automatically backported this PR to "9-x-y", please check out #23090 |
Closes #22671.
Fixes an issue with possible creation of a messageBox which cannot be dismissed on macOS.
Checklist
npm test
passesRelease Notes
Notes: Fixed an issue with possible creation of a messageBox which cannot be dismissed on macOS.