Closed Bug 1581117 Opened 2 months ago Closed 2 months ago

Switching dark mode does not update some add-on toolbar icons

Categories

(Toolkit :: Add-ons Manager, defect)

71 Branch
Desktop
macOS
defect
Not set

Tracking

()

VERIFIED FIXED
mozilla71
Tracking Status
firefox-esr60 --- unaffected
firefox-esr68 --- verified
firefox67 --- unaffected
firefox68 --- wontfix
firefox69 --- wontfix
firefox70 --- verified
firefox71 --- verified

People

(Reporter: TheOne, Assigned: emilio)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

When switching native macOS dark mode, for some add-ons, their toolbar icon doesn't doesn't honor that switch, it stays in the old mode.

STR:

  1. Switch to macOS dark mode.
  2. Install https://addons.mozilla.org/en-US/firefox/addon/facebook-container/ .
  3. Switch to macOS light mode.

Expected results:

Dark add-on toolbar icon.

Actual results:

Light toolbar icon (see screenshot, which shows the hover state, in non-hover state, the icon is invisible).

Flags: needinfo?(emilio)

So I thought this was a layout bug, but turns out it's not.

Way to reproduce on Linux, which makes it obvious what's going on:

  • Clean profile, light theme.
  • Install Facebook container or the Gecko profiler.
  • Run the following into the browser toolbox:
const { LightweightThemeManager } = ChromeUtils.import("resource://gre/modules/LightweightThemeManager.jsm");
let c = new LightweightThemeConsumer(document);
c.darkThemeMediaQuery = matchMedia("all");
c._update(LightweightThemeManager.themeData);

// To revert and see the effect:
c.darkThemeMediaQuery = matchMedia("not all");
c._update(LightweightThemeManager.themeData);

The issue is that the list style image is still not right because LightweightThemeConsumer._update(), which is what runs when dark mode changes via:

https://searchfox.org/mozilla-central/rev/053579099d936e26393ac10b809b14fb5841c0f0/toolkit/modules/LightweightThemeConsumer.jsm#220

Doesn't end up triggering the thing that sets / unsets the brighttext attribute on the toolbar: https://searchfox.org/mozilla-central/rev/053579099d936e26393ac10b809b14fb5841c0f0/browser/base/content/browser.js#9319

And thus we don't end up using inverted icons.

Blame log for this seems to be bug 1525762.

Component: Layout → Theme
Flags: needinfo?(emilio)
Product: Core → Firefox
Regressed by: 1525762
Summary: Mac switching dark mode does not update add-on toolbar icons → Switching dark mode does not update some add-on toolbar icons

Just this._update() is not enough to update everything that changing the theme
implies. Instead, use the observer mechanism so all the pieces of code involved
update properly.

I guess it's pretty easy to fix anyway.

I tested the fix on Linux with:

diff --git a/toolkit/modules/LightweightThemeConsumer.jsm b/toolkit/modules/LightweightThemeConsumer.jsm
index cf68e690257f..facbe4d85b50 100644
--- a/toolkit/modules/LightweightThemeConsumer.jsm
+++ b/toolkit/modules/LightweightThemeConsumer.jsm
@@ -185,10 +185,10 @@ function LightweightThemeConsumer(aDocument) {
   // We're responsible for notifying LightweightThemeManager when the OS is in
   // dark mode so it can activate the dark theme. We don't want this on Linux
   // as the default theme picks up the right colors from dark GTK themes.
-  if (AppConstants.platform != "linux") {
-    this.darkThemeMediaQuery = this._win.matchMedia("(-moz-system-dark-theme)");
+  // if (AppConstants.platform != "linux") {
+    this.darkThemeMediaQuery = this._win.matchMedia("(min-width: 1000px)");
     this.darkThemeMediaQuery.addListener(this);
-  }
+  // }
 
   const { LightweightThemeManager } = ChromeUtils.import(
     "resource://gre/modules/LightweightThemeManager.jsm"
@@ -216,7 +216,7 @@ LightweightThemeConsumer.prototype = {
   },
 
   handleEvent(aEvent) {
-    if (aEvent.media == "(-moz-system-dark-theme)") {
+    if (aEvent.media && aEvent.srcElement == this.darkThemeMediaQuery) {
       Services.obs.notifyObservers(this._lastData, "lightweight-theme-styling-update");
       return;
     }

And resizing the browser.

Assignee: nobody → emilio
Component: Theme → Add-ons Manager
Product: Firefox → Toolkit
Pushed by ealvarez@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a76fde9d9b8e
Properly invalidate theme data from the media query change in LightweightThemeConsumer.jsm. r=kmag,dao
Blocks: 1583941
Status: NEW → RESOLVED
Closed: 2 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla71

Hello,

Verified the fix using the latest Nightly (71.0a1/20190926213542) under MacOS 10.15.

Switching between Light and Dark mode will correctly update the add-on toolbar icons so as they are properly visible.

Status: RESOLVED → VERIFIED

Maybe best to let this ride the trains with 71. Just let me know if you disagree.

I think it's a very low-effort fix, so may be worth uplifting even though I think we've been shipping with it for a while... Dao, any strong opinion?

Flags: needinfo?(dao+bmo)

I wouldn't bother uplifting.

Flags: needinfo?(dao+bmo)

Emilio, can we request uplift to ESR for this bug? Invisible icons seems a visually bad bug to me that I don't think ESR users should stay with for several months until the next ESR is released.

Flags: needinfo?(emilio)

Sure, I think this is pretty low risk, if it applies cleanly (just a one-liner that makes us invalidate more when the native theme changes).

I told Andreas how to file the request form :)

Flags: needinfo?(emilio)

Comment on attachment 9092811 [details]
Bug 1581117 - Properly invalidate theme data from the media query change in LightweightThemeConsumer.jsm.

ESR Uplift Approval Request

  • If this is not a sec:{high,crit} bug, please state case for ESR consideration: Invisible toolbar icons is a significant visual glitch. Especially on Mac, where some people use scheduled light/dark mode switches might experience this issue regularly. Since the next ESR version is still some time out, I don't think we want to keep exposing users to this bug for the next several months.
  • User impact if declined: Users might experience invisible toolbar icons until they restart Firefox.
  • Fix Landed on Version: 71
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): It's a two-line patch that fires an observer in order to notify about the light/dark mode change.
  • String or UUID changes made by this patch: None
Attachment #9092811 - Flags: approval-mozilla-esr68?

Comment on attachment 9092811 [details]
Bug 1581117 - Properly invalidate theme data from the media query change in LightweightThemeConsumer.jsm.

Minor fix to avoid ugly icons on macOS. Approved for 70.0b14 and 68.2esr.

Attachment #9092811 - Flags: approval-mozilla-esr68?
Attachment #9092811 - Flags: approval-mozilla-esr68+
Attachment #9092811 - Flags: approval-mozilla-beta+
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

Hi,

Verified the fix using the latest Beta (70.0b14/20191010142853) under MacOS Catalina 10.15.

Switching between Light and Dark mode will correctly update the add-on toolbar icons so as they are properly visible.

Hello,

Verified the fix using the latest ESR (68.2.0esr/20191016163237) under MacOS Catalina 10.15.

Switching between Light and Dark mode will correctly update the add-on toolbar icons so as they are properly visible.

QA Whiteboard: [qa-triaged]
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.