1466335 - Automatically enable the dark theme in macOS 10.14 dark mode

Status: RESOLVED FIXED

(Firefox :: Theme, enhancement, P1)

Description

[Dão Gottwald [:dao]]

+++ 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.

Comment 1

[Stephen A Pohl [:spohl]]

Attachment: 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?
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?

Comment 2

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3088429a825c687d3a13f9c7607806e0e853378

Comment 3

[Stephen A Pohl [:spohl]]

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

Comment 4

[Markus Stange [:mstange]]

(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 5

[:Felipe Gomes (needinfo for replies!)]

Comment on attachment 8985727 [details] [diff] [review]
Patch

I'm not the best person to review this

Comment 6

[Dão Gottwald [:dao]]

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.

Comment 7

[Stephen A Pohl [:spohl]]

Attachment: Patch

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.

Comment 8

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bf1e70a0666c0b0e5ea7fb74f75d7ec02cf2ea6

Comment 9

[Tim Nguyen :ntim]

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

Comment 10

[Markus Stange [:mstange]]

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.

Comment 11

[Stephen A Pohl [:spohl]]

(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.

Comment 12

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48c6319673a142ecbda6304174990bfcc7a29c2d

Comment 13

[Stephen A Pohl [:spohl]]

Attachment: 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 14

[Dão Gottwald [:dao]]

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.

Comment 15

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=00847ccd9d24412cb89725074c985eab985f99b4

Comment 16

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed review feedback.

Comment 17

[Dão Gottwald [:dao]]

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.

Comment 18

[Stephen A Pohl [:spohl]]

Attachment: Patch

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 19

[Stephen A Pohl [:spohl]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b6c30ff87f476e6f55c9dd638506108fe1d850b

Comment 20

[Dão Gottwald [:dao]]

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...

Comment 21

[Stephen A Pohl [:spohl]]

Attachment: Patch

Addressed feedback. Carrying over r+.

Comment 22

[Stephen A Pohl [:spohl]]

Attachment: Patch

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

Comment 23

[Stephen A Pohl [:spohl]]

Attachment: Properly assign dark theme on startup, if necessary

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 24

[Dão Gottwald [:dao]]

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...

Comment 25

[Stephen A Pohl [:spohl]]

Attachment: Properly assign dark theme on startup, if necessary

Good points, thanks!

Comment 26

[Stephen A Pohl [:spohl]]

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

Comment 27

[Cosmin Sabou [:CosminS]]

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

Comment 28

[Brunno Pleffken [:bpleffken]]

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

Comment 29

[Stephen A Pohl [:spohl]]

(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?

Comment 30

[Brunno Pleffken [:bpleffken]]

(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.

Comment 31

[Stephen A Pohl [:spohl]]

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?

Comment 32

[Brunno Pleffken [:bpleffken]]

(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 = [e-mail]

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.

Comment 33

[Stephen A Pohl [:spohl]]

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?

Comment 34

[Brunno Pleffken [:bpleffken]]

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.

Comment 35

[Stephen A Pohl [:spohl]]

(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!

Comment 36

[Brunno Pleffken [:bpleffken]]

(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

Comment 37

[Pascal Chevrel:pascalc]

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

Comment 38

[Dão Gottwald [:dao]]

(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.

Comment 39

[Pascal Chevrel:pascalc]

(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.

Comment 40

[Pascal Chevrel:pascalc]

(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

Comment 41

[Dão Gottwald [:dao]]

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

Comment 42

[Stephen A Pohl [:spohl]]

Attachment: Combined patch for beta

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 43

[Liz Henry (:lizzard) (relman/hg->git project)]

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.

Comment 44

[Arthur Iakab [arthur_iakab]]

https://hg.mozilla.org/releases/mozilla-beta/rev/287d12a9f2ea

Comment 45

[Bogdan Maris, Desktop Test Engineering]

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

Comment 46

[Dão Gottwald [:dao]]

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

Comment 48

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Added to Firefox 62 release notes.

Comment 49

[Joe Borg]

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.

Comment 50

[Dão Gottwald [:dao]]

Correct, it doesn't at the moment.

Comment 51

[eboyee]

Firefox 68 and context menu is still light in dark mode for me. Any news on this?

Comment 52

[Stephen A Pohl [:spohl]]

(In reply to eboyee from comment #51)

Firefox 68 and context menu is still light in dark mode for me. Any news on this?

See bug 1475520 where this will be addressed.

Comment 53

[ricardo]

Is there any workaround for this? At the moment Firefox is unusable for Dark Mode users. I'm in Mojave 10.14.6 and still got this issue.

Comment 54

[Markus Stange [:mstange]]

You can go to Hamburger menu -> Customize -> Themes -> Dark.

Comment 55

[ricardo]

I ended up posting here via other issues referring this one. I was referring to the context menu, which with Dark Mode on has a white background with white text on it.