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: pend bounds change when moving BrowserWindows #33288
Conversation
A couple of questions and comments. First, it looks like this is only implemented for Windows? If that's intentional, can it be mentioned in the description and release notes?
|
@dsanders11 you're right, i ifdef'd the queue itself and whiffed on the queue fill part. I've also corrected the queue pop logic. |
Is there a reason we even need a queue, or should we just remember whatever the latest setBounds call was and just apply that? Why do we need to move the window 20 or whatever times in a row? |
@pushkin- i also thought about that. i'd be fine with either using a queue or just applying the the most recent one, though you may be right in that simply applying the most recent one is likely more intuitive. |
ab8a15f
to
47c4c64
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.
It looks like true queuing wasn't implemented. Not sure if it was intentional or not.
Release Notes Persisted
|
I was unable to backport this PR to "16-x-y" cleanly; |
I was unable to backport this PR to "17-x-y" cleanly; |
I have automatically backported this PR to "18-x-y", please check out #33375 |
@codebytere has manually backported this PR to "17-x-y", please check out #33543 |
@codebytere has manually backported this PR to "16-x-y", please check out #33544 |
* fix: ensure bounds changes apply when moving windows * chore: remove unused queue include
* fix: ensure bounds changes apply when moving windows * chore: remove unused queue include
Description of Change
Closes #33279.
Refs CRBUG:251813
It's the case at present that
widget()->SetBounds(bounds)
does not take effect if the window is currently being moved or resized. This is an open issue on Chromium since 2013, with no real solution implemented upstream but several options suggested, including:In a Chromium context, this might not be ideal, but in an Electron context I believe this is the most expected behavior from a development standpoint. As a result of this PR, the bounds update correctly once the user completes the resize or the drag movements to reflect the new bounds set programmatically. Now, we take the last call to
setBounds
and apply it once the user has finished the move or drag operation.Tested with https://gist.github.com/6b6ea5cdaa1883656a8bd0bf89195283.
This is unfortunately not directly testable via spec, given that it's dependent on user-initiated movement.
Checklist
npm test
passesRelease Notes
Notes: Fixed an issue where new bounds set via
setBounds
was not correctly applied if the user was moving or resizing the window concurrently on Windows.