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: macOS maximize button shouldn't be disabled just because the window is non-fullscreenable #40705

Merged
merged 3 commits into from Jan 5, 2024

Conversation

tzahola
Copy link
Contributor

@tzahola tzahola commented Dec 5, 2023

Description of Change

This PR fixes this bug: #39833

An earlier PR (cc @codebytere) incorrectly disabled NSWindowZoomButton if the window was non-fullscreenable. However, if a macOS window is non-fullscreenable, the green zoom button turns into a maximize button (with a plus sign instead of the full-screen arrows), so it is still useful and shouldn't be disabled.

Here I'm enabling the button if the window is fullscreenable OR (resizable AND maximizable). I'm also ensuring that this invariant holds whenever any of these 3 flags change (i.e. fullscreenable, resizable, maximizable).

Checklist

Release Notes

Notes: Fixed the enabled/disabled behavior of the maximize/fullscreen button of macOS windows

Copy link
Member

@ckerr ckerr left a comment

Choose a reason for hiding this comment

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

The code in this PR makes sense to me. And thanks for adding a test!

Holding back on an Approve for now because I'd like feedback from @codebytere. The comment in her previous PR about handling fullsize + maximize + resizable in a way that's consistent cross-platform is tricky so I'd like a second opinion.

@ckerr ckerr requested a review from codebytere December 6, 2023 17:01
codebytere
codebytere previously approved these changes Dec 11, 2023
@codebytere
Copy link
Member

@tzahola missed this initially: this test is failing.

@tzahola tzahola force-pushed the fix-macOS-window-maximize-button branch from 86738a8 to bdac14e Compare December 11, 2023 16:08
@tzahola
Copy link
Contributor Author

tzahola commented Dec 11, 2023

@tzahola missed this initially: this test is failing.

Fixed now. Although something still caused a test job to fail with SIGSEGV, but that shouldn't be related. ¯_(ツ)_/¯

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/27-x-y PR should also be added to the "27-x-y" branch. target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. labels Dec 11, 2023
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Dec 11, 2023
@nornagon
Copy link
Member

Tray icon related failures are probably fixed by 6602703, merged recently. So, unrelated to this PR :)

@tzahola tzahola force-pushed the fix-macOS-window-maximize-button branch from bdac14e to f3c10b8 Compare December 13, 2023 12:14
@tzahola
Copy link
Contributor Author

tzahola commented Dec 13, 2023

Tray icon related failures are probably fixed by 6602703, merged recently. So, unrelated to this PR :)

Rebased!

@jkleinsc jkleinsc merged commit cc1b64e into electron:main Jan 5, 2024
17 checks passed
Copy link

release-clerk bot commented Jan 5, 2024

Release Notes Persisted

Fixed the enabled/disabled behavior of the maximize/fullscreen button of macOS windows

@trop
Copy link
Contributor

trop bot commented Jan 5, 2024

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

@trop trop bot added the in-flight/27-x-y label Jan 5, 2024
@trop trop bot removed the target/27-x-y PR should also be added to the "27-x-y" branch. label Jan 5, 2024
@trop
Copy link
Contributor

trop bot commented Jan 5, 2024

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

@trop
Copy link
Contributor

trop bot commented Jan 5, 2024

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

@trop trop bot added in-flight/28-x-y in-flight/29-x-y merged/27-x-y PR was merged to the "27-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. and removed target/28-x-y PR should also be added to the "28-x-y" branch. target/29-x-y PR should also be added to the "29-x-y" branch. in-flight/27-x-y in-flight/29-x-y in-flight/28-x-y labels Jan 5, 2024
@miniak
Copy link
Contributor

miniak commented Jan 11, 2024

@codebytere I am afraid this fix is causing a regression, just run the default Fiddle test-app and make the window fullscreen by clicking the green semaphore button. You can no longer exit fullscreen as the green semaphore button gets disabled.

Screenshot 2024-01-12 at 12 01 02 AM

Before this PR:
Screenshot 2024-01-12 at 12 01 06 AM

After this PR:
Screenshot 2024-01-12 at 12 01 19 AM

@tzahola
Copy link
Contributor Author

tzahola commented Jan 15, 2024

@codebytere I am afraid this fix is causing a regression, just run the default Fiddle test-app and make the window fullscreen by clicking the green semaphore button. You can no longer exit fullscreen as the green semaphore button gets disabled.

Fix: #40994

@tzahola tzahola deleted the fix-macOS-window-maximize-button branch January 15, 2024 13:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged/27-x-y PR was merged to the "27-x-y" branch. merged/28-x-y PR was merged to the "28-x-y" branch. merged/29-x-y PR was merged to the "29-x-y" branch. semver/patch backwards-compatible bug fixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants