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

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Andreas Wagner [:TheOne] [use NI]]

Attachment: screenshot of the issue.png

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

Comment 1

[Emilio Cobos Álvarez (: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.

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1581117 - Properly invalidate theme data from the media query change in LightweightThemeConsumer.jsm.

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a76fde9d9b8e
Properly invalidate theme data from the media query change in LightweightThemeConsumer.jsm. r=kmag,dao

Comment 5

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/a76fde9d9b8e

Comment 6

[Alex Cornestean]

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.

Comment 7

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

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

Comment 8

[Emilio Cobos Álvarez (:emilio)]

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?

Comment 9

[Dão Gottwald [:dao]]

I wouldn't bother uplifting.

Comment 10

[Andreas Wagner [:TheOne] [use NI]]

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.

Comment 11

[Emilio Cobos Álvarez (: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 :)

Comment 12

[Andreas Wagner [:TheOne] [use NI]]

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

Comment 13

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 14

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/91ac17bc71ea

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-esr68/rev/9cd8fc96dfa7

Comment 16

[Alex Cornestean]

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.

Comment 17

[Alex Cornestean]

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.