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: child window alwaysOnTop level persistence #29813

Merged
merged 4 commits into from Jun 23, 2021
Merged

Conversation

codebytere
Copy link
Member

@codebytere codebytere commented Jun 21, 2021

Description of Change

Fixes an issue found while debugging #29732 - when a child window is attached to a parent NSWindow, the child window's level will be reset. E.g if one calls childWindow.setAlwaysOnTop(true, 'screen-saver'), this will not be honored. This fixes that by preserving the window level before it's attached to the parent and then restoring it.

Tested with https://gist.github.com/f2a3a5e8d9130223f18b8ddade390f20.

A test can't be added here because isAlwaysOnTop is true for all child windows, and while setAlwaysOnTop differentiates between different window levels the getter does not, and so it's not possible at present to tell whether or not a window level is for ex. screen-saver or pop-up-menu since isAlwaysOnTop would be true for both.

Checklist

Release Notes

Notes: Fixed an issue where the setAlwaysOnTop value would sometimes not be preserved for child windows on macOS.

@codebytere codebytere added semver/patch backwards-compatible bug fixes target/12-x-y labels Jun 21, 2021
@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Jun 21, 2021
@codebytere codebytere changed the title fix: child window alwaysOnTop level fix: child window alwaysOnTop level persistence Jun 21, 2021
@nornagon
Copy link
Member

should we make a getter to make this testable?

@codebytere
Copy link
Member Author

@nornagon i considered that but i wasn't sure if we wanted to pursue that given that if we modify the existing isAlwaysOnTop getter that'd be semver-major, and a semver-minor change that would address this would be sort of strange given that we'd be adding basically another getter for alwaysOnTop information so we'd have one setter and two getters. We could potentially just leave it undocumented, but......that feels kind of janky 🤔

@nornagon
Copy link
Member

I think it could make sense to add another getter for alwaysOnTopLevel (and maybe also for relativeLevel?)

I've also been considering a class of tests that could make use of desktopCapturer and uniquely-colored windows to test things like this (esp. been thinking of this for the pdf viewer). In that case, you could have a test that tested the outcome of the always-on-top-ness, rather than the values in memory, i.e. that the higher layer appears above the lower layer when they overlap, even when the lower-layer window is focused. Probably a bit much for this PR though.

@zcbenz
Copy link
Member

zcbenz commented Jun 22, 2021

We could potentially just leave it undocumented, but......that feels kind of janky 🤔

Another choice would be wrapping it in #if DCHECK_IS_ON() so it is only available when running tests. There are a few places using this pattern.

@codebytere
Copy link
Member Author

codebytere commented Jun 22, 2021

@zcbenz @nornagon i added a new undocumented method getAlwaysOnTopLevel which enables testing as well as a test. Since the granular value set is really only relevant on macOS it's only defined there for now but in the future we could consider:

  1. Documented and exposing this method in a follow-up semver-minor PR
  2. Moving this to be defined only for testing builds as @zcbenz mentioned with a DCHECK_IS_ON guard as well as another potentially undocumented method for checking relative window level.

My goal for the new method i added here is to enable minimum viable testability more than anything else but I'm happy to modify this or take on item(s) numbered above if you all think that does fall within scope of this PR!

@codebytere codebytere force-pushed the fix-child-window-leveling branch 2 times, most recently from cbbfc77 to fb9131b Compare June 22, 2021 08:57
@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Jun 22, 2021
@zcbenz zcbenz merged commit 129f92e into main Jun 23, 2021
@zcbenz zcbenz deleted the fix-child-window-leveling branch June 23, 2021 06:09
@release-clerk
Copy link

release-clerk bot commented Jun 23, 2021

Release Notes Persisted

Fixed an issue where the setAlwaysOnTop value would sometimes not be preserved for child windows on macOS.

@trop
Copy link
Contributor

trop bot commented Jun 23, 2021

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

@trop trop bot removed the target/12-x-y label Jun 23, 2021
@trop
Copy link
Contributor

trop bot commented Jun 23, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 23, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

@trop
Copy link
Contributor

trop bot commented Jun 30, 2021

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

BlackHole1 pushed a commit to BlackHole1/electron that referenced this pull request Aug 30, 2021
* fix: child window alwaysOnTop level

* chore: add undocumented getAlwaysOnTopLevel

* test: add test for level persistence

* Address feedback from review
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.

None yet

3 participants