Automatically enable the dark theme in macOS 10.14 dark mode

RESOLVED FIXED in Firefox 62

Status

()

enhancement
P1
major
RESOLVED FIXED
Last year
23 days ago

People

(Reporter: dao, Assigned: spohl)

Tracking

(Depends on 1 bug, Blocks 1 bug, {feature})

Trunk
Firefox 63
Unspecified
macOS
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(relnote-firefox 62+, firefox62+ fixed, firefox63+ fixed)

Details

Attachments

(3 attachments, 7 obsolete attachments)

Reporter

Description

Last year
+++ 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.
Reporter

Updated

Last year
Blocks: mojave

Updated

Last year
Whiteboard: [ntim-backlog]
Assignee

Updated

Last year
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Assignee

Comment 1

Last year
Posted patch Patch (obsolete) — Splinter Review
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?
Attachment #8985727 - Flags: review?(mstange)
Attachment #8985727 - Flags: review?(felipc)
(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
Attachment #8985727 - Flags: review?(felipc) → review?(dao+bmo)
Reporter

Comment 6

Last year
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.
Attachment #8985727 - Flags: review?(dao+bmo)
Assignee

Comment 7

Last year
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8985727 - Attachment is obsolete: true
Attachment #8985727 - Flags: review?(mstange)
Attachment #8986234 - Flags: review?(mstange)
Attachment #8986234 - Flags: review?(dao+bmo)
-moz-system-dark-theme might be a better name
Reporter

Updated

Last year
Blocks: 1368808
See Also: 1368808
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.
Attachment #8986234 - Flags: review?(mstange) → review+
Assignee

Comment 11

Last year
(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.
Assignee

Comment 13

Last year
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8986234 - Attachment is obsolete: true
Attachment #8986234 - Flags: review?(dao+bmo)
Attachment #8986351 - Flags: review?(dao+bmo)
Reporter

Comment 14

Last year
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.
Attachment #8986351 - Flags: review?(dao+bmo) → review-
Assignee

Comment 16

Last year
Posted patch Patch (obsolete) — Splinter Review
Addressed review feedback.
Attachment #8986351 - Attachment is obsolete: true
Attachment #8986482 - Flags: review?(dao+bmo)

Updated

Last year
Whiteboard: [ntim-backlog]
Reporter

Comment 17

Last year
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.
Attachment #8986482 - Flags: review?(dao+bmo) → review-
Assignee

Comment 18

Last year
Posted patch Patch (obsolete) — Splinter Review
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.
Attachment #8986482 - Attachment is obsolete: true
Attachment #8986543 - Flags: review?(dao+bmo)
Reporter

Comment 20

Last year
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...
Attachment #8986543 - Flags: review?(dao+bmo) → review+
Assignee

Comment 21

Last year
Posted patch Patch (obsolete) — Splinter Review
Addressed feedback. Carrying over r+.
Attachment #8986543 - Attachment is obsolete: true
Attachment #8987947 - Flags: review+
Assignee

Updated

Last year
Attachment #8987947 - Attachment is obsolete: true
Attachment #8987947 - Flags: review+
Assignee

Comment 22

Last year
Posted patch PatchSplinter Review
This is the patch I meant to upload. Addressed feedback. Carrying over r+.
Attachment #8987976 - Flags: review+
Assignee

Comment 23

Last year
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.
Attachment #8987980 - Flags: review?(dao+bmo)
Reporter

Comment 24

Last year
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...
Attachment #8987980 - Flags: review?(dao+bmo)
Assignee

Comment 25

Last year
Good points, thanks!
Attachment #8987980 - Attachment is obsolete: true
Attachment #8988193 - Flags: review?(dao+bmo)
Reporter

Updated

Last year
Attachment #8988193 - Flags: review?(dao+bmo) → review+
Assignee

Comment 26

Last year
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

Last year
bugherder
https://hg.mozilla.org/mozilla-central/rev/24fe98c45aae
https://hg.mozilla.org/mozilla-central/rev/760705f94da4
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
The "Default" theme (dark tab and light address bar) doesn't work anymore.
Now is either Light or Dark.
Assignee

Comment 29

Last year
(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?
Flags: needinfo?(brunno.pleffken)
(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.
Flags: needinfo?(brunno.pleffken)
Assignee

Comment 31

Last year
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?
Flags: needinfo?(brunno.pleffken)
(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.
Flags: needinfo?(brunno.pleffken)
Assignee

Comment 33

Last year
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?
Flags: needinfo?(brunno.pleffken)
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.
Flags: needinfo?(brunno.pleffken)
Assignee

Comment 35

Last year
(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!
Assignee

Updated

Last year
Blocks: 1471950
(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
Keywords: feature
Reporter

Comment 38

Last year
(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
Reporter

Comment 41

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

Updated

Last year
No longer blocks: 1471950
Depends on: 1471950
Assignee

Comment 42

Last year
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
Attachment #8988763 - Flags: review+
Attachment #8988763 - Flags: approval-mozilla-beta?
Depends on: 1472272
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.
Attachment #8988763 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Is there something blocking us verifying this bug using the free beta build of mac10.14, or we should wait for the official release?
Flags: needinfo?(dao+bmo)
Reporter

Comment 46

11 months ago
No, let's get this verified now so we can fix potential issues before 10.14 is released.
Flags: needinfo?(dao+bmo)
Reporter

Updated

11 months ago
Depends on: 1474591

Updated

10 months ago
Duplicate of this bug: 1483418
Added to Firefox 62 release notes.
Flags: qe-verify-
Reporter

Updated

8 months ago
Depends on: 1502096
Reporter

Updated

8 months ago
Blocks: 1494034
Depends on: 1488384
Reporter

Updated

6 months ago
No longer depends on: 1488384
See Also: → 1488384

Comment 49

6 months ago
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.
Flags: needinfo?(dao+bmo)
Reporter

Comment 50

6 months ago
Correct, it doesn't at the moment.
Flags: needinfo?(dao+bmo)

Updated

4 months ago
Blocks: 1524660

Comment 51

23 days ago

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

Assignee

Comment 52

23 days ago

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

Depends on: 1475520
You need to log in before you can comment on or make changes to this bug.