Closed Bug 1466335 Opened 6 years ago Closed 6 years ago

Automatically enable the dark theme in macOS 10.14 dark mode

Categories

(Firefox :: Theme, enhancement, P1)

Unspecified
macOS
enhancement

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
relnote-firefox --- 62+
firefox62 + fixed
firefox63 + fixed

People

(Reporter: dao, Assigned: spohl)

References

(Blocks 1 open bug)

Details

(Keywords: feature)

Attachments

(3 files, 7 obsolete files)

+++ 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.
Blocks: mojave
Whiteboard: [ntim-backlog]
Assignee: nobody → spohl.mozilla.bugs
Status: NEW → ASSIGNED
Priority: P3 → P1
Attached patch Patch (obsolete) (deleted) — 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)
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)
Attached patch Patch (obsolete) (deleted) — 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
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+
(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.
Attached patch Patch (obsolete) (deleted) — 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)
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-
Attached patch Patch (obsolete) (deleted) — Splinter Review
Addressed review feedback.
Attachment #8986351 - Attachment is obsolete: true
Attachment #8986482 - Flags: review?(dao+bmo)
Whiteboard: [ntim-backlog]
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-
Attached patch Patch (obsolete) (deleted) — 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)
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+
Attached patch Patch (obsolete) (deleted) — Splinter Review
Addressed feedback. Carrying over r+.
Attachment #8986543 - Attachment is obsolete: true
Attachment #8987947 - Flags: review+
Attachment #8987947 - Attachment is obsolete: true
Attachment #8987947 - Flags: review+
Attached patch Patch (deleted) — Splinter Review
This is the patch I meant to upload. Addressed feedback. Carrying over r+.
Attachment #8987976 - Flags: review+
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)
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)
Good points, thanks!
Attachment #8987980 - Attachment is obsolete: true
Attachment #8988193 - Flags: review?(dao+bmo)
Attachment #8988193 - Flags: review?(dao+bmo) → review+
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
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
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.
(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)
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)
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)
(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
Keywords: feature
(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.
No longer blocks: 1471950
Depends on: 1471950
Attached patch Combined patch for beta (deleted) — Splinter Review
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)
No, let's get this verified now so we can fix potential issues before 10.14 is released.
Flags: needinfo?(dao+bmo)
Depends on: 1474591
Flags: qe-verify-
Depends on: 1502096
Blocks: 1494034
Depends on: 1488384
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)
Correct, it doesn't at the moment.
Flags: needinfo?(dao+bmo)
Blocks: 1524660

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

(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

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.

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

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.

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

Attachment

General

Created:
Updated:
Size: