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.

Sign up for GitHub

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

Jump to bottom

fix: html fullscreen transitions stacking #32905

Merged
merged 2 commits into from Jun 7, 2022
Merged

fix: html fullscreen transitions stacking #32905

merged 2 commits into from Jun 7, 2022

Conversation

codebytere
Copy link
Member

Description of Change

Closes #32894.

Addresses improper fullscreen transition stacking when the fullscreen is HTML-initiated as a follow-up to #25470. As a result of this, the full window transition queue resolves prior to window destruction.

Checklist

Release Notes

Notes: Fixes an issue with fullscreen transitions when HTML fullscreen is requested.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/17-x-y labels Feb 15, 2022
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Feb 15, 2022
@codebytere codebytere force-pushed the fix-fs-transitions-for-chrome-fs branch 4 times, most recently from a39fc70 to 93420d9 Compare February 15, 2022 17:18
@codebytere codebytere marked this pull request as draft February 15, 2022 18:09
@codebytere codebytere marked this pull request as ready for review February 15, 2022 19:26
@codebytere codebytere force-pushed the fix-fs-transitions-for-chrome-fs branch 5 times, most recently from 08190ce to 5e10965 Compare February 16, 2022 09:42
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Feb 16, 2022
@codebytere codebytere force-pushed the fix-fs-transitions-for-chrome-fs branch 2 times, most recently from 8576f08 to 3892f46 Compare March 30, 2022 13:13
@zcbenz
Copy link
Member

zcbenz commented Mar 31, 2022

I wonder if we should just disable the " requestFullscreen from webview pressing ESC should emit the leave-html-full-screen event" test for mac arm64, the test has been flaky and we are unlikely to get a m1 MacBook in a short time to fix it.

@jkleinsc
Copy link
Contributor

@zcbenz I think that test has been more stable. It appears that something in this PR is triggering it consistently.

jkleinsc
jkleinsc requested changes Mar 31, 2022
View reviewed changes
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.

macOS arm64 failures are new and should be addressed.

@codebytere
Copy link
Member Author

codebytere commented May 5, 2022 •
edited

@jkleinsc they're not new - what happened is similar to your comment here:

electron/spec-main/webview-spec.ts

Line 439 in 5fc761d

// TODO(jkleinsc) fix this test on arm64 macOS. It causes the tests following it to fail/be flaky

where further subsequent tests fail if that one is run on arm64. I added a new test above the test that's failing, and thereby caused the same thing to happen since the newly-failing test is being affected. This is also true on main if i do something similar, though, and so isn't caused by this PR. I've pushed a commit to disable the flakes and will prioritize figuring it out in a follow up :D

@codebytere codebytere force-pushed the fix-fs-transitions-for-chrome-fs branch from 3892f46 to 5fc761d Compare May 5, 2022 08:09
@trop
Copy link
Contributor

trop bot commented Jun 7, 2022

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

@trop
Copy link
Contributor

trop bot commented Jun 7, 2022

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

@trop trop bot mentioned this pull request Jun 7, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Jun 7, 2022

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

@trop trop bot removed the target/19-x-y label Jun 7, 2022
@trop
Copy link
Contributor

trop bot commented Jun 7, 2022

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

@codebytere codebytere mentioned this pull request Jul 13, 2022
Merged
@trop
Copy link
Contributor

trop bot commented Jul 13, 2022

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

@codebytere codebytere mentioned this pull request Jul 13, 2022
Closed
@trop
Copy link
Contributor

trop bot commented Jul 13, 2022

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

@codebytere codebytere mentioned this pull request Jul 28, 2022
Merged
4 tasks
@commit-lint commit-lint bot mentioned this pull request Aug 9, 2022
Open
khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
@codebytere @khalwa
fix: html fullscreen transitions stacking (electron#32905)
cd13c61 
* fix: html fullscreen transitions stacking

* spec: move webview test to spec-main
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jkleinsc jkleinsc jkleinsc approved these changes

@VerteDinde VerteDinde VerteDinde approved these changes

@clavin clavin clavin approved these changes

@zcbenz zcbenz Awaiting requested review from zcbenz

Assignees
No one assigned
Labels
semver/patch backwards-compatible bug fixes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Sometimes fullscreened window stays alive after being destroyed
5 participants
@codebytere @zcbenz @jkleinsc @VerteDinde @clavin