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: aspect ratio when max width/height is set #29101

Merged
merged 1 commit into from Nov 22, 2021

Conversation

CezaryKulakowski
Copy link
Contributor

@CezaryKulakowski CezaryKulakowski commented May 11, 2021

Add the native frame border size to the minimum and maximum size if
the view reports its size as the client size. It allows to enlarge
window to proper values when aspect ratio and max width/height are
set. It also fixes DCHECK which was triggered when user tried to
enlarge window above dimensions set during creation of the
BrowserWindow.

Fixes #29100

Description of Change

Checklist

Release Notes

Notes: fixed respecting aspect ratio when maximum size is set on BrowserWindow

@CezaryKulakowski CezaryKulakowski requested a review from a team as a code owner May 11, 2021 11:18
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label May 11, 2021
@codebytere codebytere added the semver/patch backwards-compatible bug fixes label May 14, 2021
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label May 14, 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.

I think the patch itself is good, can you also try to upstream this patch to Chromium?

This PR requires review from @electron/wg-upgrades.

@CezaryKulakowski
Copy link
Contributor Author

I'll try.

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.

Patch is pretty small and self-contained, but there are no tests. We periodically sweep our patch list and remove patches that don't cause tests to fail when deleted, so this would be liable to be removed without a test.

@codebytere codebytere changed the title fix: fix aspect ratio when max width/height is set fix: aspect ratio when max width/height is set Sep 2, 2021
@CezaryKulakowski
Copy link
Contributor Author

@nornagon it would be hard (if possible) to create test for it as current testing environment does't allow to perform user initiated window resize (i.e. resize with mouse and window's edge). It would require browser test functionality and generating winapi events WM_SIZING.

@CezaryKulakowski
Copy link
Contributor Author

I was trying to get a repro on clean chromium to be able to upstream the fix to them. Unfortunately the only use case in chromium which uses this code (at least the only one I was able to find) is html5 video's pip: https://developer.mozilla.org/en-US/docs/Web/API/HTMLVideoElement/requestPictureInPicture. The problem is that this window is frameless so its content size is equal to its window size and check which I added is not needed for such case (it also wouldn't break anything but we won't be able to get a repro).

@CezaryKulakowski
Copy link
Contributor Author

I've just checked that I should be able to create spec test for it. I'll handle it tomorrow and I'all also rebase the changes to the newest main.

Add the native frame border size to the minimum and maximum size if
the view reports its size as the client size. It allows to enlarge
window to proper values when aspect ratio and max width/height are
set. It also fixes DCHECK which was triggered when user tried to
enlarge window above dimensions set during creation of the
BrowserWindow.
@CezaryKulakowski
Copy link
Contributor Author

I've pushed branch rebased to the newest main. I've also added a test.

@zcbenz
Copy link
Member

zcbenz commented Nov 19, 2021

Test has been added and I think this PR should be ready to go, we still need approval from @electron/wg-upgrades.

Copy link
Member

@deepak1556 deepak1556 left a comment

Choose a reason for hiding this comment

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

LGTM since it now addresses #29101 (review)

@deepak1556 deepak1556 dismissed nornagon’s stale review November 19, 2021 06:01

Tests have been added

@zcbenz zcbenz merged commit 557e586 into electron:main Nov 22, 2021
@release-clerk
Copy link

release-clerk bot commented Nov 22, 2021

Release Notes Persisted

fixed respecting aspect ratio when maximum size is set on BrowserWindow

@trop
Copy link
Contributor

trop bot commented Nov 22, 2021

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

@trop
Copy link
Contributor

trop bot commented Nov 22, 2021

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

@trop
Copy link
Contributor

trop bot commented Nov 22, 2021

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

@trop
Copy link
Contributor

trop bot commented Nov 22, 2021

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

t57ser pushed a commit to t57ser/electron that referenced this pull request Jan 25, 2022
Add the native frame border size to the minimum and maximum size if
the view reports its size as the client size. It allows to enlarge
window to proper values when aspect ratio and max width/height are
set. It also fixes DCHECK which was triggered when user tried to
enlarge window above dimensions set during creation of the
BrowserWindow.
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]: maximum size isn't respected correctly when aspect ratio is enabled
7 participants