+++ This bug was initially created as a clone of Bug #1368808 +++ Apparently macOS 10.14 will support a dark mode, or at least Apple appears to be working on this. I'm not sure how exactly this will work, but presumably it won't magically make Firefox dark the way we want it; most likely apps will have to explicitly support this. We already have a dark theme that we should enable in that mode.

This works as expected, but I have two questions/concerns: 1. This patch currently writes the theme pref value directly from nsNativeThemeCocoa.mm. Is there a better way to communicate this theme change up to the frontend rather than writing it out like this and having LightweightThemeManager.jsm observe the pref? 2. If a user selects a theme in Firefox, then changes the system preference to either dark mode, or back to regular mode, we will disregard the user's theme choice in Firefox and switch to the appropriate theme based on the currently selected system preference. Personally, I think this might be the behavior that we want since we might otherwise be the only app that doesn't change its appearance after the system pref has changed and appear to be 'broken' by not responding to the change. Furthermore, even if the user has previously selected a Firefox theme, he might still expect for the system pref change to supersede the previously selected Firefox theme. I'm sure there will be a select number of users who would rather keep their previously selected Firefox theme when the system pref changes, but I'm inclined to think that this will be a smaller number of users than the opposite. What are your thoughts?

Try build, in case you would like to take it for a spin: https://queue.taskcluster.net/v1/task/Z-3nFR8fQoSDISDgGaWpEg/runs/0/artifacts/public/build/target.dmg

(In reply to Stephen A Pohl [:spohl] from comment #1) > Created attachment 8985727 [details] [diff] [review] > Patch > > This works as expected, but I have two questions/concerns: > > 1. This patch currently writes the theme pref value directly from > nsNativeThemeCocoa.mm. Is there a better way to communicate this theme > change up to the frontend rather than writing it out like this and having > LightweightThemeManager.jsm observe the pref? You could add a new media selector "(-moz-mac-dark-theme)", analogous to (-moz-windows-default-theme) and (-moz-overlay-scrollbars), and then observe the change using window.matchMedia("(-moz-mac-dark-theme)").addListener(...). If you're adding the systemMetricsChanged notification observer as in the current patch, this will automatically cause the matchMedia listener to be called when the theme is changed. I'm not sure how the UI should work. It'd be easier if Firefox just had a light theme and a dark theme: we could just add a third option, "system", which would automatically pick between the two. Or we could remove the UI altogether and have "system" be the default behavior. But Firefox doesn't just have two existing themes, it has three: Default, Light, and Dark. Maybe we should change the behavior of the Default theme? Make it look like the current Default if the system theme is light, and make it look like the current dark theme if the system theme is dark? But then you could never get the current Default look while your system is set to dark... but maybe that's good.

Comment on attachment 8985727 [details] [diff] [review] Patch I'm not the best person to review this

Comment on attachment 8985727 [details] [diff] [review] Patch (In reply to Markus Stange [:mstange] (away from June 20 to July 2) from comment #4) > (In reply to Stephen A Pohl [:spohl] from comment #1) > > Created attachment 8985727 [details] [diff] [review] > > Patch > > > > This works as expected, but I have two questions/concerns: > > > > 1. This patch currently writes the theme pref value directly from > > nsNativeThemeCocoa.mm. Is there a better way to communicate this theme > > change up to the frontend rather than writing it out like this and having > > LightweightThemeManager.jsm observe the pref? > > You could add a new media selector "(-moz-mac-dark-theme)", analogous to > (-moz-windows-default-theme) and (-moz-overlay-scrollbars), and then observe > the change using window.matchMedia("(-moz-mac-dark-theme)").addListener(...). > > If you're adding the systemMetricsChanged notification observer as in the > current patch, this will automatically cause the matchMedia listener to be > called when the theme is changed. Sounds good to me. I would maybe make the media query name OS-agnostic so we can re-use it for bug 1368808. > I'm not sure how the UI should work. It'd be easier if Firefox just had a > light theme and a dark theme: we could just add a third option, "system", > which would automatically pick between the two. Or we could remove the UI > altogether and have "system" be the default behavior. But Firefox doesn't > just have two existing themes, it has three: Default, Light, and Dark. > Maybe we should change the behavior of the Default theme? Make it look like > the current Default if the system theme is light, and make it look like the > current dark theme if the system theme is dark? But then you could never get > the current Default look while your system is set to dark... but maybe > that's good. I'm thinking we should switch automatically, and only when default is selected.

This patch adds the platform agnostic media selector and changes the way our themes behave as follows: If the default Firefox theme is selected, Firefox will match the system appearance (current default theme in light mode, dark theme in dark mode). Note that about:addons will continue to show "default" as the selected theme, even when it is technically using the dark theme under the hood to match the system's dark mode. If any Firefox theme other than "default" is selected in about:addons, Firefox will not change themes when the system appearance changes. This has the downside that it isn't possible to select the current default theme in Firefox when macOS is in dark mode, but :dao suggested on IRC that there isn't really a good use case for users wanting the default Firefox theme when the OS is in dark mode. This greatly simplifies things.

-moz-system-dark-theme might be a better name

Comment on attachment 8986234 [details] [diff] [review] Patch Review of attachment 8986234 [details] [diff] [review]: ----------------------------------------------------------------- I prefer the name that ntim suggested, -moz-system-dark-theme. r+ with that change. I didn't review the JS parts, I'm leaving those to dao.

(In reply to Markus Stange [:mstange] (away from June 20 to July 2) from comment #10) > I prefer the name that ntim suggested, -moz-system-dark-theme. r+ with that > change. Made this change locally. Try seems to show at least one failure that I'll be looking into. The fix will come in the form of a separate patch.

This changes the name of the media selector and fixes at least one try failure by only calling the initial LightweightThemeManager.systemThemeChanged in browser-compacttheme.js if the system is indeed in dark mode. Carrying over r+ from mstange.

Comment on attachment 8986351 [details] [diff] [review] Patch >--- a/browser/base/content/browser-compacttheme.js >+++ b/browser/base/content/browser-compacttheme.js >@@ -29,16 +29,22 @@ var CompactTheme = { > }, > > init() { > Services.obs.addObserver(this, "lightweight-theme-styling-update"); > > if (this.isThemeCurrentlyApplied) { > this._toggleStyleSheet(true); > } >+ window.matchMedia("(-moz-system-dark-theme)") >+ .addListener(LightweightThemeManager.systemThemeChanged); >+ if (window.matchMedia("(-moz-system-dark-theme)").matches) { >+ LightweightThemeManager.systemThemeChanged( >+ window.matchMedia("(-moz-system-dark-theme)")); >+ } > }, When changing the OS theme with n windows open, this will call LightweightThemeManager.systemThemeChanged n times. Presumably we only want to call it once. nit: you're calling window.matchMedia("(-moz-system-dark-theme)") three times here. Just call it once and reuse the result. > uninit() { > Services.obs.removeObserver(this, "lightweight-theme-styling-update"); > this.styleSheet = null; >+ window.matchMedia("(-moz-system-dark-theme)") >+ .removeListener(LightweightThemeManager.systemThemeChanged); matchMedia will give you a distinct MediaQueryList each time it's called, so I don't think you can remove the listener like this. Not sure that you need to remove it in the first place. >+ _mDefaultThemeIsInDarkMode: false, >+ >+ get defaultThemeIsInDarkMode() { >+ return this._mDefaultThemeIsInDarkMode; >+ }, >+ >+ set defaultThemeIsInDarkMode(aInDarkMode) { >+ return this._mDefaultThemeIsInDarkMode = aInDarkMode; >+ }, Please make this a global variable instead, similar to _themeIDBeingEnabled.

Addressed review feedback.

Comment on attachment 8986482 [details] [diff] [review] Patch >--- a/browser/base/content/browser-compacttheme.js >+++ b/browser/base/content/browser-compacttheme.js > uninit() { > Services.obs.removeObserver(this, "lightweight-theme-styling-update"); > this.styleSheet = null; >+ window.matchMedia("(-moz-system-dark-theme)") >+ .removeListener(LightweightThemeManager.systemThemeChanged); > } > }; Forgot to remove this... >--- a/toolkit/modules/LightweightThemeConsumer.jsm >+++ b/toolkit/modules/LightweightThemeConsumer.jsm >+ let darkThemeMediaQuery = this._win.matchMedia("(-moz-system-dark-theme)"); >+ darkThemeMediaQuery.addListener( >+ temp.LightweightThemeManager.systemThemeChanged(darkThemeMediaQuery)); This looks broken. Did you mean ...systemThemeChanged instead of ...systemThemeChanged(darkThemeMediaQuery)? Actually, can you just pass temp.LightweightThemeManager here, implement handleEvent in LightweightThemeManager and call systemThemeChanged from there? This way you can use 'this' instead of 'LightweightThemeManager' in systemThemeChanged.

Ugh, sorry about the churn here. Confirmed with a local build that this is working as expected on 10.14 this time. Try run coming up.

Comment on attachment 8986543 [details] [diff] [review] Patch >+ let themeToSwitchTo = aData; >+ if (aData && aData.id == DEFAULT_THEME_ID) { >+ if (_defaultThemeIsInDarkMode) { >+ themeToSwitchTo = LightweightThemeManager.getUsedTheme( >+ DARK_THEME_ID); >+ } else { >+ themeToSwitchTo = LightweightThemeManager.getUsedTheme( >+ DEFAULT_THEME_ID); >+ } The else branch seems unnecessary since themeToSwitchTo is already set to aData. >+ systemThemeChanged(aMediaQueryList) { >+ let themeToSwitchTo = null; >+ if (aMediaQueryList.matches && !_defaultThemeIsInDarkMode) { Can you also check aMediaQueryList.media here to make sure this won't active the dark theme for unrelated queries? >+ /** >+ * Handles system theme changes. >+ * >+ * @param aEvent >+ * The MediaQueryList associated with the system theme change. >+ */ >+ handleEvent(aEvent) { >+ this.systemThemeChanged(aEvent); Technically aEvent is going to be a MediaQueryListEvent here rather than an MediaQueryList, I believe...

Addressed feedback. Carrying over r+.

This is the patch I meant to upload. Addressed feedback. Carrying over r+.

Just before landing the patch in this bug I noticed that the dark theme wasn't getting applied properly if Firefox is started when the system is already in dark mode. This patch fixes this by fixing up browser-compacttheme.js to use LightweightThemeManager.currentThemeForDisplay instead of LightweightThemeManager.currentTheme, which will properly return the dark theme.

Comment on attachment 8987980 [details] [diff] [review] Properly assign dark theme on startup, if necessary >--- a/toolkit/mozapps/extensions/LightweightThemeManager.jsm >+++ b/toolkit/mozapps/extensions/LightweightThemeManager.jsm >@@ -148,16 +148,20 @@ var LightweightThemeManager = { > try { > if (data[key] && _prefs.getBoolPref("persisted." + key)) > data[key] = _getLocalImageURI(PERSIST_FILES[key]).spec > + "?" + data.id + ";" + _version(data); > } catch (e) {} > } > } > >+ if (_defaultThemeIsInDarkMode && data && data.id == DEFAULT_THEME_ID) { >+ data = this.getUsedTheme(DARK_THEME_ID); >+ } Can we merge this with the _fallbackThemeData stuff earlier in this method? Something like this: if (!data || data.id == DEFAULT_THEME_ID) { if (_fallbackThemeData) { data = _fallbackThemeData; } else if (_defaultThemeIsInDarkMode) { data = this.getUsedTheme(DARK_THEME_ID); } } Also not sure if the PERSIST_ENABLED stuff makes any sense at all for these cases...

Good points, thanks!

https://hg.mozilla.org/integration/mozilla-inbound/rev/24fe98c45aae15108532297e7fceb70486888e24 Bug 1466335: Automatically switch to the appropriate Firefox theme based on the macOS dark mode system preference. r=dao,mstange https://hg.mozilla.org/integration/mozilla-inbound/rev/760705f94da446d77749544cd5bd8602cb7367a9 Bug 1466335: Automatically apply the dark theme on startup if the OS is in dark mode. r=dao

https://hg.mozilla.org/mozilla-central/rev/24fe98c45aae https://hg.mozilla.org/mozilla-central/rev/760705f94da4

The "Default" theme (dark tab and light address bar) doesn't work anymore. Now is either Light or Dark.

(In reply to Brunno Pleffken from comment #28) > The "Default" theme (dark tab and light address bar) doesn't work anymore. > Now is either Light or Dark. What OS are you on? In about:addons, under themes, which theme is selected?

(In reply to Stephen A Pohl [:spohl] from comment #29) > What OS are you on? In about:addons, under themes, which theme is selected? I'm on macOS 10.13.5 High Sierra. In about:addons, the "Default" theme is selected. Selecting Light works properly, but the Default is automatically set as Dark.

Hmm, I'm on the same OS and I'm unable to reproduce. In about:config, what's the string assigned to the lightweightThemes.selectedThemeID pref when the default theme is selected?

(In reply to Stephen A Pohl [:spohl] from comment #31) > Hmm, I'm on the same OS and I'm unable to reproduce. In about:config, what's > the string assigned to the lightweightThemes.selectedThemeID pref when the > default theme is selected? When the Default theme is selected... lightweightThemes.selectedThemeID = default-theme@mozilla.org The settings are all looking fine, but appears as Dark to me. Maybe trying to Refresh Nightly (I still didn't), I don't know if anyone else is going through the same problem.

Could you post three screenshots, one for each theme (default/light/dark), showing the top section of the browser with the titlebar and URL bar?

I got something!! I have "Use dark menu bar and Dock" selected on Preferences. When it's checked, Default is forcing the use of the Dark theme, but disabling it on Preferences (the default OS setting), all the themes works fine again. Is the Default theme now working as "Auto"? Then, perhaps, adding another option to force the Default theme (light address + dark tab) even though "Dark menu bar and dock" is selected on Preferences.

(In reply to Brunno Pleffken from comment #34) > I got something!! > > I have "Use dark menu bar and Dock" selected on Preferences. When it's > checked, Default is forcing the use of the Dark theme, but disabling it on > Preferences (the default OS setting), all the themes works fine again. Interesting. It looks like "AppleInterfaceStyle" was an existing key that Apple repurposed for dark mode on 10.14. I will file a follow-up bug to restrict this to 10.14 and above. > Is the Default theme now working as "Auto"? Then, perhaps, adding another > option to force the Default theme (light address + dark tab) even though > "Dark menu bar and dock" is selected on Preferences. This was discussed in comment 6 and comment 7. Supporting the "light" version of the default theme is not currently something that we plan to do for dark mode on 10.14+. However, I don't believe that we should be switching to the dark theme on 10.13 and below to match the behavior of other apps. Thanks for reporting this!

(In reply to Stephen A Pohl [:spohl] from comment #35) > This was discussed in comment 6 and comment 7. Supporting the "light" > version of the default theme is not currently something that we plan to do > for dark mode on 10.14+. Oh... This is sad! :( I like this blend of light+dark SO MUCH! The menu bar and tab bar background seems like one, and it makes Firefox unique! e.g.: https://imgur.com/a/UloQfCj

adding the relnote flag as we may want to add this to release notes for 63

(In reply to Pascal Chevrel:pascalc from comment #37) > adding the relnote flag as we may want to add this to release notes for 63 I'm not sure there's something to advertise here that will be meaningful to end users, as 10.14 isn't released yet and this is irrelevant to users on older versions.

(In reply to Dão Gottwald [::dao] from comment #38) > (In reply to Pascal Chevrel:pascalc from comment #37) > > adding the relnote flag as we may want to add this to release notes for 63 > > I'm not sure there's something to advertise here that will be meaningful to > end users, as 10.14 isn't released yet and this is irrelevant to users on > older versions. 10.14 should be released around September/October and Firefox 53 release date is October 23.

(In reply to Pascal Chevrel:pascalc from comment #39) > 10.14 should be released around September/October and Firefox 53 release > date is October 23. typo, I meant 63

In that case we should probably uplift this such that it will already be part of release Firefox whenever 10.14 is out.

Approval Request Comment [Feature/Bug causing the regression]: New OS feature introduced in macOS 10.14 (Mojave) [User impact if declined]: We will not support macOS dark mode when macOS 10.14 is released in September/October 2018. [Is this code covered by automated tests?]: Our themes are covered by tests, but the automatic switching of themes on macOS 10.14 introduced in this patch is not. [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: No [List of other uplifts needed for the feature/fix]: Bug 1471950 [Is the change risky?]: No [Why is the change risky/not risky?]: Although we touch some cross-platform code related to our themes, the main change only impacts macOS 10.14 which is not yet released. [String changes made/needed]: none

Comment on attachment 8988763 [details] [diff] [review] Combined patch for beta Theme switching changes, let's aim this at beta 6. Should only affect the upcoming new MacOS.

Is there something blocking us verifying this bug using the free beta build of mac10.14, or we should wait for the official release?

No, let's get this verified now so we can fix potential issues before 10.14 is released.

Added to Firefox 62 release notes.

Just to confirm, although this does address the generic theme of Firefox in Mojave, it doesn't seem to respect right-click menus, file dialogs, lookup etc. I'm running 64.0 and still see light theme for these.

Correct, it doesn't at the moment.