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: pend bounds change when moving BrowserWindows #33288

Merged
merged 2 commits into from Mar 22, 2022
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Mar 16, 2022

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:

Drag completes ignoring the bounds update but the bounds update is queued and then executed after drag completes.

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

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

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/16-x-y labels Mar 16, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Mar 16, 2022
@dsanders11
Copy link
Member

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?

  • If Windows only, can we #ifdef the changes? It looks to me like with the current code the queue will be filled on Linux and SetBounds while moving/resizing ignored and the queue will never be read
  • This is implemented as a queue, but only one item is popped when the move/resize ends. What happens if you change the repro gist to use setinterval instead of settimeout, and keep dragging during that time? Won't multiple bounds changes be queued up, but only one read? Seems that would affect future moves/resizes - it'll start playing back the full queue, but only each time you move/resize the window.

@codebytere
Copy link
Member Author

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

@pushkin-
Copy link

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?

@codebytere
Copy link
Member Author

codebytere commented Mar 17, 2022

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

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Mar 17, 2022
Copy link
Contributor

@jkleinsc jkleinsc left a 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.

shell/browser/native_window_views.h Outdated Show resolved Hide resolved
@codebytere codebytere changed the title fix: queue bounds changes when moving BrowserWindows fix: pend bounds change when moving BrowserWindows Mar 21, 2022
@zcbenz zcbenz merged commit f511263 into main Mar 22, 2022
@zcbenz zcbenz deleted the handle-bounds-dragging branch March 22, 2022 06:07
@release-clerk
Copy link

release-clerk bot commented Mar 22, 2022

Release Notes Persisted

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.

@trop
Copy link
Contributor

trop bot commented Mar 22, 2022

I was unable to backport this PR to "16-x-y" cleanly;
you will need to perform this backport manually.

@trop
Copy link
Contributor

trop bot commented Mar 22, 2022

I was unable to backport this PR to "17-x-y" cleanly;
you will need to perform this backport manually.

@trop trop bot removed the target/17-x-y label Mar 22, 2022
@trop
Copy link
Contributor

trop bot commented Mar 22, 2022

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

@trop
Copy link
Contributor

trop bot commented Mar 31, 2022

@codebytere has manually backported this PR to "17-x-y", please check out #33543

@trop
Copy link
Contributor

trop bot commented Mar 31, 2022

@codebytere has manually backported this PR to "16-x-y", please check out #33544

bavulapati pushed a commit to bavulapati/electron that referenced this pull request Apr 29, 2022
* fix: ensure bounds changes apply when moving windows

* chore: remove unused queue include
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* fix: ensure bounds changes apply when moving windows

* chore: remove unused queue include
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]:setBounds is not work when dragging the window all the way
5 participants