1817885 - Bookmarks menu icons on macOS use bright icons if OS theme isn't dark but Fx theme is (incl. private browsing windows in default theme)

Status: ASSIGNED

(Firefox :: Menus, defect, P3)

Description

[:Gijs (he/him)]

Attachment: Screenshot of the problem

This means white icons on a light background, so the icons are hard to see. The icons, on macOS with its global menubar, should not depend on the Firefox theme, only on the OS theme.

Comment 1

[:Gijs (he/him)]

I just noticed this affects private browsing windows in the default configuration, which seems bad, so I'm bumping severity on this.

Comment 2

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

I'm investigating this bug, and I can't manage to reproduce it except in the case of a private browsing window, as mentioned in comment 0.

After quite a bit of flailing, I noticed that, when the browser.theme.dark-private-windows pref is set to true, LightweightThemeConsumer._update() applies the dark theme to private windows, but bails out early:

(from https://searchfox.org/mozilla-central/source/toolkit/modules/LightweightThemeConsumer.sys.mjs#235-240)

      // When applying the dark theme for a PBM window we need to skip calling
      // _determineToolbarAndContentTheme, because it applies the color scheme
      // globally for all windows. Skipping this method also means we don't
      // switch the content theme to dark.
      updateGlobalThemeData = false;
      return true;

A few lines below this spot, the _update function goes on to call _setDarkModeAttributes(this._doc, root, theme._processedColors). If I add that just before the return true above, the bookmarks menu svg fill color is fixed, but the toolbars for the window are themed using the light theme, not the dark theme--a much worse and more noticeable bug.

I've noticed this bug cannot be reproduced if I install and uninstall a theme during the browsing session. I don't know what sort of clue that provides.

It seems like I might be getting somewhere, but any thoughts on whether this is the right sort of fix for this bug? Should I keep going down this path and try to clean up the PBM-theming logic in LightweightThemeConsumer, or should I instead add an exception in CSS directly, maybe something like adding a rule to override the fill color applied when lwtheme-brighttext is true and we're in a private window?

Comment 3

[:Gijs (he/him)]

(In reply to Jared Hirsch [:jhirsch] (he/him) (Needinfo please) from comment #2)

I'm investigating this bug, and I can't manage to reproduce it except in the case of a private browsing window, as mentioned in comment 0.

I don't really understand why this would be, because reproduction is still trivial for me:

1. clean profile on macOS on current nightly. macOS is using light theme.
2. open about:addons with cmd-shift-a
3. switch to themes view
4. enable dark theme
5. click bookmarks menu

and hey presto, the icons are wrong.

Does this not reproduce for you?

I think any fix probably shouldn't be specific to PB mode.

Comment 4

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Whoa, great! I hadn't found those steps to reproduce, thanks. I'll keep digging.

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Using the STR from comment 3, mozregression narrowed down to a build window in June 12-13, 2021: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=0a5c47d4a336cfffcfd09465ff6d699a4b364dc4&tochange=f531f12e5c35223e49b1e657300ae83e0cf696a8

These changes are old enough that they're no longer available on taskcluster, so I tried to build locally.

I first ran into the mach build error ImportError: cannot import name 'Iterable' from 'collections'.

It turns out this happened because python 3.10 changed collections to collections.abc, and I was running 3.11 locally. I installed pyenv to get an old enough python (3.9) that mach build works, which fixed this error.

Unfortunately, I ran into another error I haven't been able to fix--I can't seem to get the toolchain to install:

]$ ./mach build
0:02.62 Updating bootstrapped toolchain in /Users/jaredhirsch/.mozbuild/clang
0:03.97 ERROR!!!!!! Could not find artifacts for a toolchain build named toolchain-macosx64-aarch64-clang. Local commits, dirty/stale files, and other changes in your checkout may cause this error. Make sure you are on a fresh, current checkout of mozilla-central. Beware that commands like mach bootstrap and mach artifact are unlikely to work on any versions of the code besides recent revisions of mozilla-central.
0:05.04 ERROR: Command '['/bin/sh', '/Users/jaredhirsch/codez/mozilla/mozilla-unified/mach', '--log-no-times', 'artifact', 'toolchain', '--from-build', 'toolchain-macosx64-aarch64-clang']' returned non-zero exit status 1.
0:05.04 ERROR: If you can't fix the above, retry with --disable-bootstrap.

The error message suggestion to use --disable-bootstrap seems to be an argument supported by some subscript that mach invokes, but mach doesn't accept it.

After reading through the 100+ comment history of bug 1636797, and trying various one-liners like ./mach clobber python, PYTHONDONTWRITEBYTECODE=1 ./mach build, and find . -name "*.pyc" | xargs rm, I still can't figure out how to get past this error.

I'm not really sure how to proceed, and the bug is minor enough that it's not a huge issue on its own--but it really seems like it should be possible to build older versions of Firefox, and I'd like to figure out how.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

Based on comment #5, this bug contains a bisection range found by mozregression. However, the Regressed by field is still not filled.

:jhirsch, if possible, could you fill the Regressed by field and investigate this regression?

For more information, please visit auto_nag documentation.