1739615 - Focused tab and search dropdown not visible when restarting with Troubleshoot Mode from about:support

Status: VERIFIED FIXED

(WebExtensions :: Themes, defect)

Description

[Giorgia Nichita, Release Desktop QA]

Attachment: Troubleshoot mode Win.gif

Affected versions

• Nightly 96.a1
• Beta 95.0b2

Affected platforms

• Windows 10x64

Preconditions

• Dark mode enabled at OS level.

Steps to reproduce

1. Open a Firefox with a clean profile.

2. Install a theme from Recommended Themes (I used A color Within Another Color, rusty and Purple and Gold Marble)

3. Go to about:support and click on "Troubleshoot Mode..." (make sure you have two-three tabs opened).

4. Click on Restart.

Expected result

• All open tabs are visible. All suggestions in the search dropdown are visible on hover.

Actual result

• Only the focused tab is not visible. Suggestions are not visible on hover.

Suggested Severity

• S2 - because it visually affects users on a very used area and it's a recent regression since it's not reproducible on Release 94.0.

Regression range

• https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=1ff04e10cd2318b494c0f05777cd80291289d127&tochange=d39b23efefe7e7d89d8dfd0ba188ad6af2b0a9ed

Additional notes

• Screen cast attached to observe the issue.

Comment 1

[Giorgia Nichita, Release Desktop QA]

@Emilio, it seems that Bug 1733569 caused the regression, can you please have a look?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

This is a pre-existing issue (also happens on macOS dark mode), can you confirm?

So this is because the we don't enable the default dark theme when the OS theme is dark. So we need to either force browser.theme.toolbar-theme=1 on windows and macOS in safe mode (or behave like such), or allow enabling the default dark theme there.

Gijs, do you know where is the right place to fix it?

Comment 3

[Emilio Cobos Álvarez (:emilio)]

Ah, so the theme does get enabled if it's the default theme already, it's only if you have some other theme installed that we don't run it (which I guess it's why we haven't seen this before).

So forcing chrome color-scheme to light when in safe mode like I was thinking:

diff --git a/widget/nsXPLookAndFeel.cpp b/widget/nsXPLookAndFeel.cpp
--- a/widget/nsXPLookAndFeel.cpp
+++ b/widget/nsXPLookAndFeel.cpp
@@ -1066,17 +1066,28 @@ static bool ShouldUseStandinsForNativeCo
   }

   return false;
 }

 ColorScheme LookAndFeel::sChromeColorScheme;
 ColorScheme LookAndFeel::sContentColorScheme;

+extern bool gSafeMode;
+
 auto LookAndFeel::ColorSchemeSettingForChrome() -> ChromeColorSchemeSetting {
+#ifndef MOZ_WIDGET_GTK
+  // On Linux, the default theme follows the system so we don't need this. But
+  // on macOS and Windows we want to force light color-scheme as we don't load
+  // the dark theme, which is an extension.
+  if (gSafeMode) {
+    return ChromeColorSchemeSetting::Light;
+  }
+#endif
+
   switch (StaticPrefs::browser_theme_toolbar_theme()) {
     case 0:  // Dark
       return ChromeColorSchemeSetting::Dark;
     case 1:  // Light
       return ChromeColorSchemeSetting::Light;
     default:
       return ChromeColorSchemeSetting::System;
   }

Won't cut it, unless we also force the [e-mail] to be used even in dark mode. So I think this at least needs work on the front-end or web-extension code (and I'm not very familiar about how that and safe mode interact).

Comment 4

[Giorgia Nichita, Release Desktop QA]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

This is a pre-existing issue (also happens on macOS dark mode), can you confirm?

Yes, I can confirm it's also happening on macOS. I filled Bug 1739131 for macOS because there is a slight difference in how it behaves and has a different regression range.

Comment 5

[:Gijs (he/him)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #2)

This is a pre-existing issue (also happens on macOS dark mode), can you confirm?

So this is because the we don't enable the default dark theme when the OS theme is dark. So we need to either force browser.theme.toolbar-theme=1 on windows and macOS in safe mode (or behave like such), or allow enabling the default dark theme there.

Gijs, do you know where is the right place to fix it?

It used to be sensible to disable all themes in safe mode, because "full" themes (in the pre-Firefox-52, or perhaps even earlier, days) could change XBL bindings being applied and thus affect functionality - they were pretty dangerous from that perspective.

However, the world has changed, and so I suspect that at this point, the add-ons manager shouldn't be disabling the built-in themes in safe mode. Rob, does that sound like a reasonable way of addressing this?

(We could also decide not to disable any theme add-ons, but that feels riskier because third-party themes could make some stuff illegible on purpose or somesuch - we've set the quality bar higher for our own stuff so keeping builtin themes feels like it should be OK.

Comment 7

[Rob Wu [:robwu]]

We do allow built-in extensions, including themes in safe mode: https://searchfox.org/mozilla-central/rev/364aa1d41eb9b5ea583fa6cedc56fb5343e3514f/toolkit/mozapps/extensions/internal/XPIProvider.jsm#302-315

Theme management by the add-on manager is complicated, e.g. when a theme add-on is installed it is not the active theme by default. We store that in a pref. I'm not sure if everything works correctly with Safe mode. What we don't want is to overwrite the theme preference during safe mode (since that would persist to the regular profile after restarting in normal mode).

Redirecting needinfo to Shane, so he can take a look at how themes and safe mode interact, and whether that's desired.

P.S. I'm clearing the Severity flag since it being set prevents the extension team from seeing the bug during the weekly bug triage.

Comment 8

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

Set release status flags based on info from the regressing bug 1733569

Comment 9

[Pascal Chevrel:pascalc (PTO until April 26)]

Emilio, this is our last week of beta, I am guessing that this is wontfix for 95, correct? Thanks

Comment 10

[Emilio Cobos Álvarez (:emilio)]

This is a pre-existing issue on macOS and I don't have a good sense about how easy is it to fix on the add-ons side... I'd ask Shane or Rob, but yeah looks like it otherwise.

Comment 11

[Shane Caraveo (:mixedpuppy)]

This only happens with 3rd party themes. If the selected theme is a builtin it works fine. Given this is a safe mode issue, I wouldn't rate the priority that high, but it is a bit annoying. So a fix is to address this in theme code where using the dark them is determined, but I don't have the cycles this week to write tests/etc for this. A quick hack below addresses it enough to make things visible, if not great, in safe mode (at least on osx).

Can you take this Emilio?

https://searchfox.org/mozilla-central/rev/70b32246fce5ca1f53af573a21c1939df58cb969/toolkit/modules/LightweightThemeConsumer.jsm#267-276

diff --git a/toolkit/modules/LightweightThemeConsumer.jsm b/toolkit/modules/LightweightThemeConsumer.jsm
--- a/toolkit/modules/LightweightThemeConsumer.jsm
+++ b/toolkit/modules/LightweightThemeConsumer.jsm
@@ -259,25 +259,25 @@ LightweightThemeConsumer.prototype = {
         break;
     }
   },
 
   _update(themeData) {
     this._lastData = themeData;
 
     // In Linux, the default theme picks up the right colors from dark GTK themes.
-    const useDarkTheme =
+    let useDarkTheme =
       themeData.darkTheme &&
       this.darkThemeMediaQuery?.matches &&
       (themeData.darkTheme.id != DEFAULT_THEME_ID ||
         AppConstants.platform != "linux");
-
     let theme = useDarkTheme ? themeData.darkTheme : themeData.theme;
     if (!theme) {
       theme = { id: DEFAULT_THEME_ID };
+      useDarkTheme = this.darkThemeMediaQuery?.matches;
     }
 
     let active = (this._active = Object.keys(theme).length);
 
     let root = this._doc.documentElement;
 
     if (active && theme.headerURL) {
       root.setAttribute("lwtheme-image", "true");

Comment 12

[Shane Caraveo (:mixedpuppy)]

I'm guessing that would need to be

useDarkTheme = this.darkThemeMediaQuery?.matches && AppConstants.platform != "linux";

Comment 13

[Emilio Cobos Álvarez (:emilio)]

Ah, that diff gave me a good idea of how to fix this.

Comment 14

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1739615 - Force a light toolbar color when using the system theme on non-Linux platforms. r=#desktop-theme-reviewers

Not sure what's the best way to test this.

Comment 15

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

The severity field is not set for this bug.
:mixedpuppy, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b17c1312fa2e
Force a light toolbar color when using the system theme on non-Linux platforms. r=desktop-theme-reviewers,dao

Comment 17

[Sandor Molnar[:smolnar]]

Backed out changeset b17c1312fa2e (bug 1739615) for causing mochitest failures in browser_startup_syncIPC.

Backout link: https://hg.mozilla.org/integration/autoland/rev/b0e20232f5882f6d6bc0150ab84ee2f9921d14c2

Push with failures

Failure log

 INFO - TEST-PASS | browser/base/content/test/performance/browser_startup_syncIPC.js | sync IPC PCompositorBridge::Msg_FlushRendering happened as many times as expected before handling user events - 
[task 2021-12-01T12:02:28.809Z] 12:02:28     INFO - known sync IPC before becoming idle:
[task 2021-12-01T12:02:28.809Z] 12:02:28     INFO -   PCompositorBridge::Msg_NotifyChildCreated - at most 1 times
[task 2021-12-01T12:02:28.809Z] 12:02:28     INFO -   PCompositorBridge::Msg_FlushRendering - at most 1 times
[task 2021-12-01T12:02:28.810Z] 12:02:28     INFO - Buffered messages finished
[task 2021-12-01T12:02:28.810Z] 12:02:28     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/performance/browser_startup_syncIPC.js | unexpected PWebRenderBridge::Msg_EnsureConnected sync IPC before becoming idle - 
[task 2021-12-01T12:02:28.810Z] 12:02:28     INFO - Stack trace:

Comment 18

[Emilio Cobos Álvarez (:emilio)]

I see, so this is basically bug 1715331. My guess is that we can avoid the failure by avoiding the change (by tweaking the default pref value to be 1 on macOS and Windows), but maybe the message should be allowlisted, it seems like basically anything that invalidates paint would cause this message. Andrew, thoughts?

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Yep, avoiding the pref change helps: https://treeherder.mozilla.org/jobs?repo=try&revision=22c7b50d8c4347e901803ad6a622801c2100c260

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c2567f65e5f
Force a light toolbar color when using the system theme on non-Linux platforms. r=desktop-theme-reviewers,dao

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Yes, the EnsureConnected sync API happens whenever we initialize a WebRenderLayerManager -- once per creation of a new tab/window/popup. The timing can be a bit erratic sometimes, so I think we can be generous on fuzzy that particular message.

Comment 22

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/3c2567f65e5f

Comment 23

[Giorgia Nichita, Release Desktop QA]

Verified as fixed on 96.0a1 (20211201214955), on both Windows 10x64 and macOS 10.15. All tabs are visible.