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: handle async nature of [NSWindow -toggleFullScreen] #25470

Merged
merged 2 commits into from
Apr 21, 2021

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Sep 15, 2020

Description of Change

Closes #25426.
Closes #25427.

[NSWindow -toggleFullScreen] is an asynchronous operation, which means that it's possible to call it while a fullscreen transition is currently in process. As such, this can create weird behavior (incl. phantom windows and missing traffic lights) and so we want to queue up transitions until the previous ones have completed. We do this by tracking when fullscreen transitions begin and end, and scheduling those transitions to execute in order after any current transitions complete.

cc @MarshallOfSound @nornagon @zcbenz

Checklist

Release Notes

Notes: Fixed an issue where multiple calls to window.setFullScreen could cause problems.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Sep 15, 2020
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.

the docs don't mention anything about not calling this twice.

shell/browser/ui/cocoa/electron_ns_window.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.h Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
shell/browser/native_window_mac.h Outdated Show resolved Hide resolved
shell/browser/native_window_mac.mm Outdated Show resolved Hide resolved
@codebytere codebytere force-pushed the block-toggle-fullscreen branch 5 times, most recently from 01ebb6d to 3ddd100 Compare September 16, 2020 14:25
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Sep 16, 2020
@codebytere codebytere force-pushed the block-toggle-fullscreen branch 2 times, most recently from 3474b8a to b617395 Compare September 16, 2020 22:46
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.

I think we could write some tests for this!

e.g.:

requestFullScreen immediately followed by cancelFullScreen should produce the appropriate events and the window should ultimately end up not in fullscreen mode. request -> cancel -> request should end up in fullscreen mode and likewise generate the relevant events.

shell/browser/native_window_mac.h Outdated Show resolved Hide resolved
@codebytere
Copy link
Member Author

Holding this for a bit to explore a few edge cases

@trop
Copy link
Contributor

trop bot commented Apr 21, 2021

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

@trop
Copy link
Contributor

trop bot commented Apr 21, 2021

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

@trop
Copy link
Contributor

trop bot commented Apr 22, 2021

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

@trop
Copy link
Contributor

trop bot commented Apr 22, 2021

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

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
4 participants