From fea3366bc7d702b3cbff46fd5fa2791d2fb5bc99 Mon Sep 17 00:00:00 2001 From: Samuel Attard Date: Mon, 30 Mar 2020 15:39:50 -0700 Subject: [PATCH] fix: propagate preferred color scheme to the renderer (#22896) * fix: do not crash if the window is closed syncronously with a nativeTheme change * fix: propogate preferred color scheme to the renderer and keep it up to date --- .../browser/api/electron_api_native_theme.cc | 26 ++++++++++++------- shell/browser/api/electron_api_native_theme.h | 7 +++-- shell/browser/electron_browser_client.cc | 5 ++++ shell/browser/native_window.h | 1 + shell/browser/native_window_mac.h | 2 +- shell/browser/native_window_mac.mm | 6 ++--- spec-main/api-native-theme-spec.ts | 26 ++++++++++++++++++- 7 files changed, 56 insertions(+), 17 deletions(-) diff --git a/shell/browser/api/electron_api_native_theme.cc b/shell/browser/api/electron_api_native_theme.cc index 643f0d6d286f..634bed735848 100644 --- a/shell/browser/api/electron_api_native_theme.cc +++ b/shell/browser/api/electron_api_native_theme.cc @@ -23,14 +23,16 @@ namespace electron { namespace api { -NativeTheme::NativeTheme(v8::Isolate* isolate, ui::NativeTheme* theme) - : theme_(theme) { - theme_->AddObserver(this); +NativeTheme::NativeTheme(v8::Isolate* isolate, + ui::NativeTheme* ui_theme, + ui::NativeTheme* web_theme) + : ui_theme_(ui_theme), web_theme_(web_theme) { + ui_theme_->AddObserver(this); Init(isolate); } NativeTheme::~NativeTheme() { - theme_->RemoveObserver(this); + ui_theme_->RemoveObserver(this); } void NativeTheme::OnNativeThemeUpdatedOnUI() { @@ -44,7 +46,8 @@ void NativeTheme::OnNativeThemeUpdated(ui::NativeTheme* theme) { } void NativeTheme::SetThemeSource(ui::NativeTheme::ThemeSource override) { - theme_->set_theme_source(override); + ui_theme_->set_theme_source(override); + web_theme_->set_theme_source(override); #if defined(OS_MACOSX) // Update the macOS appearance setting for this new override value UpdateMacOSAppearanceForOverrideValue(override); @@ -59,15 +62,15 @@ void NativeTheme::SetThemeSource(ui::NativeTheme::ThemeSource override) { } ui::NativeTheme::ThemeSource NativeTheme::GetThemeSource() const { - return theme_->theme_source(); + return ui_theme_->theme_source(); } bool NativeTheme::ShouldUseDarkColors() { - return theme_->ShouldUseDarkColors(); + return ui_theme_->ShouldUseDarkColors(); } bool NativeTheme::ShouldUseHighContrastColors() { - return theme_->UsesHighContrastColors(); + return ui_theme_->UsesHighContrastColors(); } #if defined(OS_MACOSX) @@ -92,8 +95,11 @@ bool NativeTheme::ShouldUseInvertedColorScheme() { // static v8::Local NativeTheme::Create(v8::Isolate* isolate) { - ui::NativeTheme* theme = ui::NativeTheme::GetInstanceForNativeUi(); - return gin::CreateHandle(isolate, new NativeTheme(isolate, theme)).ToV8(); + ui::NativeTheme* ui_theme = ui::NativeTheme::GetInstanceForNativeUi(); + ui::NativeTheme* web_theme = ui::NativeTheme::GetInstanceForWeb(); + return gin::CreateHandle(isolate, + new NativeTheme(isolate, ui_theme, web_theme)) + .ToV8(); } // static diff --git a/shell/browser/api/electron_api_native_theme.h b/shell/browser/api/electron_api_native_theme.h index 8f82f86f5756..ca9d30e2487a 100644 --- a/shell/browser/api/electron_api_native_theme.h +++ b/shell/browser/api/electron_api_native_theme.h @@ -22,7 +22,9 @@ class NativeTheme : public gin_helper::EventEmitter, v8::Local prototype); protected: - NativeTheme(v8::Isolate* isolate, ui::NativeTheme* theme); + NativeTheme(v8::Isolate* isolate, + ui::NativeTheme* ui_theme, + ui::NativeTheme* web_theme); ~NativeTheme() override; void SetThemeSource(ui::NativeTheme::ThemeSource override); @@ -40,7 +42,8 @@ class NativeTheme : public gin_helper::EventEmitter, void OnNativeThemeUpdatedOnUI(); private: - ui::NativeTheme* theme_; + ui::NativeTheme* ui_theme_; + ui::NativeTheme* web_theme_; DISALLOW_COPY_AND_ASSIGN(NativeTheme); }; diff --git a/shell/browser/electron_browser_client.cc b/shell/browser/electron_browser_client.cc index ace78c56861f..7907fa803568 100644 --- a/shell/browser/electron_browser_client.cc +++ b/shell/browser/electron_browser_client.cc @@ -524,6 +524,11 @@ void ElectronBrowserClient::OverrideWebkitPrefs( prefs->picture_in_picture_enabled = false; #endif + ui::NativeTheme* native_theme = ui::NativeTheme::GetInstanceForNativeUi(); + prefs->preferred_color_scheme = native_theme->ShouldUseDarkColors() + ? blink::PreferredColorScheme::kDark + : blink::PreferredColorScheme::kLight; + SetFontDefaults(prefs); // Custom preferences of guest page. diff --git a/shell/browser/native_window.h b/shell/browser/native_window.h index a0077f3104c1..6f3a87d493d3 100644 --- a/shell/browser/native_window.h +++ b/shell/browser/native_window.h @@ -198,6 +198,7 @@ class NativeWindow : public base::SupportsUserData, #if defined(OS_MACOSX) virtual void SetTrafficLightPosition(const gfx::Point& position) = 0; virtual gfx::Point GetTrafficLightPosition() const = 0; + virtual void RedrawTrafficLights() = 0; #endif // Touchbar API diff --git a/shell/browser/native_window_mac.h b/shell/browser/native_window_mac.h index 11bb4b008a97..8cf446a1dd44 100644 --- a/shell/browser/native_window_mac.h +++ b/shell/browser/native_window_mac.h @@ -150,7 +150,7 @@ class NativeWindowMac : public NativeWindow, public ui::NativeThemeObserver { void SetWindowLevel(int level); // Custom traffic light positioning - void RedrawTrafficLights(); + void RedrawTrafficLights() override; void SetExitingFullScreen(bool flag); void SetTrafficLightPosition(const gfx::Point& position) override; gfx::Point GetTrafficLightPosition() const override; diff --git a/shell/browser/native_window_mac.mm b/shell/browser/native_window_mac.mm index 21614b9bca1f..3e0a588060c8 100644 --- a/shell/browser/native_window_mac.mm +++ b/shell/browser/native_window_mac.mm @@ -700,9 +700,9 @@ void NativeWindowMac::SetExitingFullScreen(bool flag) { } void NativeWindowMac::OnNativeThemeUpdated(ui::NativeTheme* observed_theme) { - base::PostTask(FROM_HERE, {content::BrowserThread::UI}, - base::BindOnce(&NativeWindowMac::RedrawTrafficLights, - base::Unretained(this))); + base::PostTask( + FROM_HERE, {content::BrowserThread::UI}, + base::BindOnce(&NativeWindow::RedrawTrafficLights, GetWeakPtr())); } bool NativeWindowMac::IsEnabled() { diff --git a/spec-main/api-native-theme-spec.ts b/spec-main/api-native-theme-spec.ts index 46c0d2b06841..9ec267971944 100644 --- a/spec-main/api-native-theme-spec.ts +++ b/spec-main/api-native-theme-spec.ts @@ -1,10 +1,12 @@ import { expect } from 'chai'; -import { nativeTheme, systemPreferences } from 'electron'; +import { nativeTheme, systemPreferences, BrowserWindow } from 'electron'; import * as os from 'os'; +import * as path from 'path'; import * as semver from 'semver'; import { delay, ifdescribe } from './spec-helpers'; import { emittedOnce } from './events-helpers'; +import { closeAllWindows } from './window-helpers'; describe('nativeTheme module', () => { describe('nativeTheme.shouldUseDarkColors', () => { @@ -18,6 +20,8 @@ describe('nativeTheme module', () => { nativeTheme.themeSource = 'system'; // Wait for any pending events to emit await delay(20); + + closeAllWindows(); }); it('is system by default', () => { @@ -62,6 +66,26 @@ describe('nativeTheme module', () => { expect(systemPreferences.appLevelAppearance).to.equal('light'); }); }); + + const getPrefersColorSchemeIsDark = async (w: Electron.BrowserWindow) => { + const isDark: boolean = await w.webContents.executeJavaScript( + 'matchMedia("(prefers-color-scheme: dark)").matches' + ); + return isDark; + }; + + it('should override the result of prefers-color-scheme CSS media query', async () => { + const w = new BrowserWindow({ show: false }); + await w.loadFile(path.resolve(__dirname, 'fixtures', 'blank.html')); + const originalSystemIsDark = await getPrefersColorSchemeIsDark(w); + nativeTheme.themeSource = 'dark'; + expect(await getPrefersColorSchemeIsDark(w)).to.equal(true); + nativeTheme.themeSource = 'light'; + expect(await getPrefersColorSchemeIsDark(w)).to.equal(false); + nativeTheme.themeSource = 'system'; + expect(await getPrefersColorSchemeIsDark(w)).to.equal(originalSystemIsDark); + w.close(); + }); }); describe('nativeTheme.shouldUseInvertedColorScheme', () => {