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

feat: add immersive dark mode on windows #33624

Merged
merged 12 commits into from Jun 14, 2022
Merged

Conversation

mlaurencin
Copy link
Contributor

@mlaurencin mlaurencin commented Apr 5, 2022

Description of Change

Closes #32913, closes #18119.

A refactored implementation of dark mode for Windows, using official API for Windows 11 and undocumented flag on Win 10 >= 20H1. It allows Electron apps to set a dark title bar based on the system preference.

cc @dsanders11 @deermichel @ckerr

Checklist

Release Notes

Notes: Added immersive dark mode on Windows.

@electron-cation electron-cation bot added the new-pr 🌱 PR opened in the last 24 hours label Apr 5, 2022
@dsanders11 dsanders11 requested a review from ckerr April 6, 2022 02:01
@dsanders11
Copy link
Member

Been waiting for this. 😄

I've built it and played with it on Windows 10. However, it seems to suffer from the same problem outlined in #25373:

There is still a wart: Changing themeSource dynamically when the window is already shown isn't perfect: the property is set, but the window still has to be poked before the change takes effect, e.g. changing focus or minimizing/restoring the window. The usual programmatic ways of forcing a window refresh don't seem to work here; perhaps this is my error but let's see about getting guidance for the right recipe to use.

Using the fiddle from our docs on dark mode I can confirm that's the behavior I see, the titlebar doesn't change until something forces a window refresh like resizing it, minimizing, blurring it, etc. It does however refresh correctly when it's set to system and you toggle the system setting.

Also, can release notes be updated to clarify it's for Windows?

@deermichel
Copy link
Contributor

deermichel commented Apr 6, 2022

@dsanders11 yup that's a known issue - I spend quite some time trying to find a fix but wasn't able to come up with a working programmatic "force-redraw" (RedrawWindow API etc.). Feel free to give it a try ;). For some reason I only hit that in vanilla Electron, not in Skype - also it's only an issue when setting dark mode from within the app; afaik it works when changing the system dark mode in OS settings.

The fact that the title bar changes after moving a window shows that the property is already set correctly (so it's nothing missing on our side). I wouldn't be surprised if it is an actual Windows bug.

build/fuses/fuses.json5 Outdated Show resolved Hide resolved
@deermichel
Copy link
Contributor

@mlaurencin after merging, we should also add the new fuse to @electron/fuses :)

@deermichel deermichel changed the title feat: add immersive dark mode feat: add immersive dark mode on windows Apr 6, 2022
@dsanders11
Copy link
Member

dsanders11 commented Apr 7, 2022

Current implementation doesn't change the titlebar for DevTools windows, but I was able to get that working wtih a small change, see 63e4ef2 for how I implemented it. EDIT: That commit has problems, so just a reference on how it might be done, not a shippable solution.

@deermichel
Copy link
Contributor

@dsanders11 Thanks! I'll check your comments tmw or next week; unfortunately i cannot guarantee for anything but the initially proposed changes, because they were tested in the wild 🤞🏽

@dsanders11
Copy link
Member

@deermichel, disregard the commit about doing all windows to get the DevTools windows. I tested it again and it didn't work like it did before. 🤔 I also realized that it wouldn't work for the case where you open DevTools after the theme change, since it wouldn't re-run that code when the DevTools opens. So, disregard that. 🙂 I'll just open a bug once this lands that DevTools won't have dark mode titlebar (would be nice, but not too important).

@deermichel
Copy link
Contributor

@dsanders11 @mlaurencin Let me suggest a path for going forward here:

  • On Win11, there is a public API (that seems to work reliably) - so I wouldn't put the feature behind a flag, but enable it for all.
  • On Win10, we can offer a fuse for users to enable it there with the risk of private/undocumented API usage
  • or we don't offer the option on Win10 at all (not sure if Win11 adoption is that widespread tbh)

@mlaurencin
Copy link
Contributor Author

@dsanders11 @mlaurencin Let me suggest a path for going forward here:

  • On Win11, there is a public API (that seems to work reliably) - so I wouldn't put the feature behind a flag, but enable it for all.
  • On Win10, we can offer a fuse for users to enable it there with the risk of private/undocumented API usage
  • or we don't offer the option on Win10 at all (not sure if Win11 adoption is that widespread tbh)

If I am following correctly, the API is currently public for Win11 and private for Win10, so it would be a matter of converting the fuse to only apply to Win10 if it were to be kept, right?

@MarshallOfSound
Copy link
Member

or we don't offer the option on Win10 at all (not sure if Win11 adoption is that widespread tbh)

I much prefer this option, it keeps the code cleaner and makes more sense for consumers. "This is a Windows 11 feature".

@dsanders11
Copy link
Member

I much prefer leaving it possible to use on Win10 - which is going to be around for a long time to come. Not having the functionality at the moment is an eyesore, especially since the menu bar does respect dark mode, making the out of the box experience on Windows feel unpolished.

@deermichel
Copy link
Contributor

I agree with @dsanders11.

  • Dark mode has been around for a long time with widespread usage on Win10 - users might not understand why this should be a Win11-only feature now.
  • Our Win11 userbase is around 10% (compared to all Windows versions) and a quick google reveals adoption doesn't look much better on a larger scale. Do we want to withhold a "great" experience from 80%+ of Windows users?

For Win11, enabling the feature per default is a no-brainer. For Win10, putting it behind a fuse seems like a reasonable way to make this feature available to the public without risking too much (it is super unlikely the API will change on Win10 anyway as it is in maintenance mode & would even break msft apps).

@electron-cation electron-cation bot removed the new-pr 🌱 PR opened in the last 24 hours label Apr 12, 2022
@hipstersmoothie
Copy link

How close are we to getting this landed? I really don't want to create my own custom build of electron

@dsanders11
Copy link
Member

@deermichel, take a look at my branch, hopefully we can grab these changes and finish this PR off: https://github.com/dsanders11/electron/commits/immersive-dark-mode-add-2

I changed the fuse to mention Win10 specifically, added a runtime check for Win11, and separated out the implementations so it's cleaner and clear which one is using private APIs.

@deermichel
Copy link
Contributor

@dsanders11 thank you very much, i'll take a look next week

@deermichel deermichel added the semver/minor backwards-compatible functionality label May 17, 2022
@jkleinsc jkleinsc deleted the immersive-dark-mode-add branch June 14, 2022 16:27
@release-clerk
Copy link

release-clerk bot commented Jun 14, 2022

Release Notes Persisted

Added immersive dark mode on Windows.

trop bot pushed a commit that referenced this pull request Jun 14, 2022
* feat: add immersive dark mode

* fix syntax and add header

* add me

* Update fuses.json5

* fix: redraw title bar on dark mode change

* chore: SetWindowTheme doesn't seem to be needed

* chore: separate out Win 10 dark mode implementation

* final touches

* final touches

* chore: limit Win 10 to >= 20H1 and drop fuse

* fix types

* fix lint

Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
trop bot pushed a commit that referenced this pull request Jun 14, 2022
* feat: add immersive dark mode

* fix syntax and add header

* add me

* Update fuses.json5

* fix: redraw title bar on dark mode change

* chore: SetWindowTheme doesn't seem to be needed

* chore: separate out Win 10 dark mode implementation

* final touches

* final touches

* chore: limit Win 10 to >= 20H1 and drop fuse

* fix types

* fix lint

Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
trop bot pushed a commit that referenced this pull request Jun 14, 2022
* feat: add immersive dark mode

* fix syntax and add header

* add me

* Update fuses.json5

* fix: redraw title bar on dark mode change

* chore: SetWindowTheme doesn't seem to be needed

* chore: separate out Win 10 dark mode implementation

* final touches

* final touches

* chore: limit Win 10 to >= 20H1 and drop fuse

* fix types

* fix lint

Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
@trop
Copy link
Contributor

trop bot commented Jun 14, 2022

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

@trop
Copy link
Contributor

trop bot commented Jun 14, 2022

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

@trop
Copy link
Contributor

trop bot commented Jun 14, 2022

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

zcbenz pushed a commit that referenced this pull request Jun 20, 2022
feat: add immersive dark mode on windows (#33624)

* feat: add immersive dark mode

* fix syntax and add header

* add me

* Update fuses.json5

* fix: redraw title bar on dark mode change

* chore: SetWindowTheme doesn't seem to be needed

* chore: separate out Win 10 dark mode implementation

* final touches

* final touches

* chore: limit Win 10 to >= 20H1 and drop fuse

* fix types

* fix lint

Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>

Co-authored-by: Michaela Laurencin <35157522+mlaurencin@users.noreply.github.com>
Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
@damienallen
Copy link

Very excited to test this out in the next release!

khalwa pushed a commit to solarwindscloud/electron that referenced this pull request Feb 22, 2023
* feat: add immersive dark mode

* fix syntax and add header

* add me

* Update fuses.json5

* fix: redraw title bar on dark mode change

* chore: SetWindowTheme doesn't seem to be needed

* chore: separate out Win 10 dark mode implementation

* final touches

* final touches

* chore: limit Win 10 to >= 20H1 and drop fuse

* fix types

* fix lint

Co-authored-by: Micha Hanselmann <micha.hanselmann@gmail.com>
Co-authored-by: David Sanders <dsanders11@ucsbalum.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api-review/requested 🗳 semver/minor backwards-compatible functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request]: Immersive Dark mode Windows chrome and context menu should be aware of dark mode setting
9 participants