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: maximized state calculation for non-resizable windows #30989

Merged
merged 2 commits into from Sep 21, 2021

Conversation

codebytere
Copy link
Member

Description of Change

Partially addresses #30979.

Fixes an issue where non-resizable non-fullscreenable windows with aspect ratios set could return incorrect results for isMaximized(). macOS calls windowWillUseStandardFrame:defaultFrame with what it refers to as the "best fit" frame for a
given zoom (see Discussion Section). This means that even if an aspect ratio is set, macOS might adjust it to better fit the screen. Thus, we can't just calculate the maximized aspect ratio'd sizing from the current visible screen and compare that to the current window's frame to determine whether a window is maximized in isMaximized.

Checklist

Release Notes

Notes: Fixes an issue where non-resizable non-fullscreenable windows with aspect ratios set could return incorrect results for isMaximized().

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/13-x-y labels Sep 15, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 15, 2021
@BlackHole1
Copy link
Member

BlackHole1 commented Sep 16, 2021

Thank you PR, I did the checkout locally but found it only solved the second problem and not the first major one.

When resizable is true (the default), I leave the window maximized while moving it around a bit and you will see that this fix does not do anything.

You can use the gist address (gist/BlackHole1/b67e544) mentioned in #30979 to reproduce the problem locally.

@codebytere
Copy link
Member Author

@BlackHole1 yes, that is why this specifically says "partially addresses". The primary issue may not be fixable owing to the way that Cocoa calculates window maximization, but i will continue to try.

@BlackHole1
Copy link
Member

BlackHole1 commented Sep 16, 2021

@codebytere thx for your answer.

List of questions: #30979 (comment)

What I'm going to say next is in response to the first question which has nothing to do with this PR.
Since they fall under one category, I'm commenting here and also hope @zcbenz and @MarshallOfSound and @samuelmaddock will participate.

If we're going to try to solve the first problem, I think we need to establish one thing.

Is maximizing after the AspectRatio restriction a "real maximization"?

  • If it's not "real maximization", then what we're doing now should actually be considered correct.
  • If it is a "real maximization", then the current behavior should be wrong.

Only when this concept is clarified should we consider whether to fix the first problem.


By the behavior of NsWindow.isZoomed, we can infer two possibilities.

  1. macOS thinks that after setting AspectRatio and performing maximization, the state of the window is not maximized ("fake maximization")
  2. This is not the expected behavior of macOS, it is an NsWindow.isZoomed bug

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 16, 2021
Copy link
Member

@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.

According to the documentation of isZoomed:

The zoomed state of the window is determined using the following steps:

If the delegate or the window class implements windowWillUseStandardFrame(_:defaultFrame:), it is invoked to obtain the zoomed frame of the window. The value of isZoomed is then determined by whether or not the current window frame is equal to the zoomed frame.

So I think the correct fix should be tweaking windowWillUseStandardFrame:defaultFrame: to return the correct size of maximized window, instead of doing the calculation in NativeWindowMac::IsMaximized.

Also the current implementation of NativeWindowMac::IsMaximized has a special case for unresizable window, which should also be done in windowWillUseStandardFrame.

@codebytere
Copy link
Member Author

codebytere commented Sep 20, 2021

@zcbenz can you elaborate a bit more on what you mean? I'm not sure that would work for non-resizable windows, since -[NSWindow isZoomed] only works if the zoom button is enabled and in this scenario it's not. The slightly altered size returned from windowWillUseStandardFrame:defaultFrame: is correct in this case in that it's what is necessary for the window to be flush to screen edges as users would expect.

@zcbenz
Copy link
Member

zcbenz commented Sep 21, 2021

since -[NSWindow isZoomed] only works if the zoom button is enabled and in this scenario it's not

I didn't know that, this change makes sense to me now 👍

@codebytere codebytere merged commit 629d891 into main Sep 21, 2021
@codebytere codebytere deleted the fix-aspect-ratio-maximize branch September 21, 2021 10:04
@release-clerk
Copy link

release-clerk bot commented Sep 21, 2021

Release Notes Persisted

Fixes an issue where non-resizable non-fullscreenable windows with aspect ratios set could return incorrect results for isMaximized().

@trop
Copy link
Contributor

trop bot commented Sep 21, 2021

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

@trop
Copy link
Contributor

trop bot commented Sep 21, 2021

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

@trop
Copy link
Contributor

trop bot commented Sep 21, 2021

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

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.

None yet

3 participants