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

Use system theme via matchMedia for auto dark mode option #372

Closed
shgysk8zer0 opened this issue Nov 9, 2020 · 13 comments · Fixed by #663
Closed

Use system theme via matchMedia for auto dark mode option #372

shgysk8zer0 opened this issue Nov 9, 2020 · 13 comments · Fixed by #663

Comments

@shgysk8zer0
Copy link

Describe the feature or problem you’d like to solve

GitHub Desktop (at least on Mac) supports system preferences for dark/light mode (see desktop#5037). This seems to rely on some systemPreferences APIs, and this does not seem to be a cross-platform solution, and is definitely not working for me on Fedora. Conversations surrounding the issue and feature seem exclusive to macOS despite dark/light being something that exists in some form on nearly every modern OS.

I'm not sure if this is an issue for Desktop itself, Electron, or this fork. It's a feature request here, something already implemented on GitHub Desktop, and seemingly a bug in Electron.

Using GitHub Desktop 2.5.7 on Fedora 29 (I know... A bit outdated).

Proposed solution

Instead of relying on systemPreferences, favor matchMedia('(prefers-color-scheme: dark)') for implementing system preferences.

How will it benefit Desktop and its users?

Using a standard API instead of an Electron one should improve compatibility and maintainability.

Additional context

I've done some experimenting with this approach. query.matches works (mostly) correctly, but query.addListener() does not behave as expected. matchMedia seems to be fixed at launch time, such that query.matches is determined by system theme when first launched.

  1. Open GitHub Desktop with "Light Mode"
  2. matchMedia('(prefers-color-scheme: dark)').matches is false
  3. Switch to "Dark Mode"
  4. matchMedia('(prefers-color-scheme: dark)').matches is still false and event is not dispatched
  5. Close and re-open GitHub Desktop
  6. matchMedia('(prefers-color-scheme: dark)').matches is true now
  7. Switch to "Light Mode"
  8. matchMedia('(prefers-color-scheme: dark)').matches is still true and event is not dispatched
// If theme preference is set to system/auto...
const query = matchMedia('(prefers-color-scheme: dark)');

if (query.matches) {
  document.body.classList.add('theme-dark');
} else {
    document.body.classList.add('theme-light');
}

query.addListener(({ target }) => {
  // Seems this portion is never run
  document.body.classList.toggle('theme-dark', target.matches);
  document.body.classList.toggle('theme-light', ! target.matches);
});

Add any other context like screenshots or mockups are helpful, if applicable.

@shiftkey
Copy link
Owner

shiftkey commented Nov 9, 2020

@shgysk8zer0 thanks for the feedback. The option I've had my eye on for a while is Electron's nativeThem module which mentions Windows and macOS support:

https://www.electronjs.org/docs/api/native-theme#nativethemeshouldusedarkcolors-readonly

This is now supported in recent versions of macOS and Windows, and Desktop itself has some detection available:

export function supportsDarkMode() {
if (__DARWIN__) {
return isMacOsAndMojaveOrLater()
} else if (__WIN32__) {
// Its technically possible this would still work on prior versions of Windows 10 but 1809
// was released October 2nd, 2018 that the feature can just be "attained" by upgrading
// See https://github.com/desktop/desktop/issues/9015 for more
return isWindows10And1809Preview17666OrLater()
}
return false
}

It'd be interesting to see what sort of support is there for Linux users in Chromium itself before we try out alternative approaches, as it may work already.

@shgysk8zer0
Copy link
Author

@shiftkey nativeTheme looks to be or responsible for prefers-color-scheme matching, so I think that'd be equivalent. I came across the general electron/dark-mode document that gives recommended handling for automatic or manual theme switching.

I think that either the media query option or nativeTheme module would have the same results. CSS and use of prefers-color-scheme media query instead of theme-dark or theme-light classes would be best but would require more changes.

If you want to manually switch between light/dark modes, you can do this by setting the desired mode in the themeSource property of the nativeTheme module. This property's value will be propagated to your Renderer process. Any CSS rules related to prefers-color-scheme will be updated accordingly.

The only thing I've not seen is detecting if system theme is even an option on an OS. Linux has too many distros for the UA sniffing route to be feasible.

@shiftkey
Copy link
Owner

With this change and running the app locally:

diff --git a/app/src/ui/lib/dark-theme.ts b/app/src/ui/lib/dark-theme.ts
index 31451ec9f..5c898852b 100644
--- a/app/src/ui/lib/dark-theme.ts
+++ b/app/src/ui/lib/dark-theme.ts
@@ -12,6 +12,9 @@ export function supportsDarkMode() {
     // was released October 2nd, 2018 that the feature can just be "attained" by upgrading
     // See https://github.com/desktop/desktop/issues/9015 for more
     return isWindows10And1809Preview17666OrLater()
+  } else if (__LINUX__) {
+    // TODO: let's see if this works at all
+    return true
   }
 
   return false

I see these changes:

  • you can check the box for "Automatically switch theme to match system theme" and it updated to match dark/light
  • restarting the app after changing the theme (this is on Ubuntu and switching between Yaru light and dark themes) detects the new theme and chooses to match
  • updating the theme while the app is running updates the window frame, but we don't see this event emitted that would update the in-app theme:

private onThemeNotificationFromOS = (event: string, userInfo: any) => {
const darkModeEnabled = isDarkModeEnabled()
const theme = darkModeEnabled
? ApplicationTheme.Dark
: ApplicationTheme.Light
this.emitThemeChanged(theme)
}

So it's close to how the other platforms work, but maybe there's no change monitor support in Chromium for Linux to signal this (or it's not working how I expect). It's also not clear to me how broadly this is supported.

@shgysk8zer0
Copy link
Author

@shiftkey This is consistent with my experience in Fedora, though my dark/light themes are Adwaita instead or Yaru. I expect similar results for anything on Gnome and hopefully beyond.

Based on my understanding and assumptions, I think we could gauge support by having other run matchMedia('(prefers-color-scheme: dark)').matches in console.

And I suspect that the lack of the change event being emitted stems from themes themselves being unofficial and kinda hacks in any distro I'm aware of.

@shiftkey
Copy link
Owner

Based on my understanding and assumptions, I think we could gauge support by having other run matchMedia('(prefers-color-scheme: dark)').matches in console.

I had a play around with this and it seems to work as expected, but I wanted to add an event listener like this to monitor when it changes:

window.matchMedia('(prefers-color-scheme: dark)')
      .addEventListener('change', event => {
  if (event.matches) {
    //dark mode
  } else {
    //light mode
  }
})

But it doesn't seem to fire when I change the theme in the OS. Without that, using matchMedia gives us the same feature set as is available inbuilt in Electron today.

@shgysk8zer0
Copy link
Author

It is my understanding per Electron documentation of nativeTheme.themeSource that they're ultimately the same thing. It describes the media query matching being changed and a change event being dispatched on changes to themeSource.

nativeTheme.themeSource = 'light';
matchMedia('(prefers-color-scheme: dark)').matches // false

nativeTheme.themeSource = 'dark';
matchMedia('(prefers-color-scheme: dark)').matches // true

nativeTheme.themeSource = 'system';
matchMedia('(prefers-color-scheme: dark)').matches // depends on OS theme

Is the current implementation using nativeTheme when toggling theme preferences? I would expect a change event to be dispatched on changes to GitHub Desktop's theme via preferences, but it is not.

@shiftkey
Copy link
Owner

Is the current implementation using nativeTheme when toggling theme preferences? I would expect a change event to be dispatched on changes to GitHub Desktop's theme via preferences, but it is not.

It is not, but even when I enabled this and ran the app locally I wasn't seeing the change event fire (e.g. switching between Yaru-dark and Yaru-light on Ubuntu)

@say25
Copy link

say25 commented Feb 22, 2021

In desktop#11629 I moved Electron to use nativeTheme, this would at least let you verify what the behavior is with it.

@shiftkey
Copy link
Owner

@say25 thanks for the update. We're up to date with the Electron 11 upgrade so this was something I was hoping to revisit.

@shiftkey
Copy link
Owner

I've re-tested this behaviour with Electron 11 but the lack of the updated event being reported is still a blocker on this. I found electron/electron#25925 which seems to be the same issue, and left a comment there.

The workarounds like polling for theme changes feel unsatisfactory here, so I'm going to leave this open in the meantime.

@shgysk8zer0
Copy link
Author

@shiftkey May I request that startup light/dark mode be regarded as a separate issue from updating theme post-launch? Switching themes while Desktop is open is pretty minor, and I'd personally rather re-open it if I change system themes and want Desktop's theme to reflect that than manually change anything in preferences every time.

@shiftkey
Copy link
Owner

The main project has updated to Electron 16 and 17 last week, so I have a build out that should have proper support for this - 2.9.9-linux1. This is probably worth revisiting as I'm not aware of any blockers to ensuring nativeTheme works as expected for us.

@shiftkey
Copy link
Owner

Please test out the 2.9.9-linux2 and confirm this is resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants