STR:

1. Start firefox

Expected:
Between when the browser launches and the tabs/webcontent are rendered, an empty solid colored window is shown. This window should match the user's selected dark/light mode preference when HCM is not enabled.

Actual:
The window is always "light" themed (white) because the background system color defaults to white on windows.

Users can reset this behaviour by changing the background color or not using system colors (both options are in the colors dialog)

Filing in layout because this might be a canvas issue, but could also be a CSS one so feel free to re-triage.

Ah okay I think this is because tabpanels use canvas colors, which are technically HCM aware because they use presShell vals, but not theme aware.

So we should either adapt canvas colors to consider light/dark theming, and then fallback to presShell colors, or we should change the tabpanels css to only override with the system colors if HCM is enabled.

I'm a little confused by the css to begin with, because it reads "if we're using system colors, set background color to canvas. otherwise, if FF HCM is always on (so, regardless of OS HCM), set background color to whatever is set from the colors dialog pref". I think something like this would make more sense:

      // By default, tabpanels are styled by client theme. We handle
      // styling in the presence of FF high contrast mode here.
      if (Services.prefs.getIntPref("browser.display.document_color_use") == 2) {
        if (Services.prefs.getBoolPref("browser.display.use_system_colors")) {
          // If FF high contrast mode is on, and system colors are being used,
          // we should style tab panels with these colors instead of the OS
          // theme colors, since this represents a more specific user preference.
          this.tabpanels.style.backgroundColor = "-moz-default-background-color";
        } else {
          // Otherwise if HCM is on but system colors is off, use the value
          // set for background color in the colors dialog.
          this.tabpanels.style.backgroundColor = Services.prefs.getCharPref(
            "browser.display.background_color"
          );
        }
      }

This won't handle the case where FF HCM is set to "with high contrast themes" and OS HCM is enabled, but AFAICT there's no way for us to get that information in js.
This is sort of an odd intersection of HCM / OS theming. I'm not convinced background color on its own is the right thing to be using here.

cc emilio, mstange -- it looks like that relevant TODO was written when y'all did some color-scheme work earlier this year, do you have thoughts on this issue?

Yeah, so that TODO should probably be fixed, but this should work if windows actually had dark system color support, see bug 1727371.

So this is basically pending dark color-scheme support on Windows, which IIRC dholbert was going to work on sometime on H2.

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

Yeah, so that TODO should probably be fixed, but this should work if windows actually had dark system color support, see bug 1727371.

So this is basically pending dark color-scheme support on Windows, which IIRC dholbert was going to work on sometime on H2.

ah cool !
in the meantime, I think it might be good to put up a css fix to protect photosensitive users who didn't have this issue pre-system-colors-change.
I'll do that and ask gijs or someone else on frontend to take a look :)

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

reposting this phab comment here for documentation, in case anyone From The Future needs it:

! In D126289#4101644, @Gijs wrote:
I am confused by this patch because the initial comment in bug 1731678 appears to be about the about:blank window that is painted initially, not about the web content area:

Between when the browser launches and the tabs/webcontent are rendered, an empty solid colored window is shown. This window should match the user's selected dark/light mode preference when HCM is not enabled.

Sorry, I should've been clearer: this issue occurs after the browser chrome has rendered, but before web content or tab content (like about:newtab) has rendered. This is an issue specific to the web area, not the blank chrome window shown before any firefox-related UI is rendered.

Can you clarify what exactly the problem is and why this patch fixes it? If the real problem is the entire browser window (including chrome), then I don't think this fix will help. If the real problem is the web content area, then I suspect this will help after startup but possibly not during startup, because we would need to teach the skeleton UI code some more tricks (it will have its own logic about painting the web content part of the stub/skeleton UI it paints).

So as above, this is an issue with the web area's default background color. This is controlled by the styling of tab panels in tabbrowser.js, which starts off with the default values here.
With no HCM enabled, and with 'use system colors' UNchecked, tab panels are styled according to the firefox theme detected from browser.theme.toolbar-theme. If we detect a dark theme, the panels get a background color of #2B2A33, otherwise we set --tabpanel-background-color: #f9f9fa;. This means users with a light theme see a light web area, and users with a dark theme see a dark web area.

With no HCM enabled, and with 'use system colors' checked (as is the default on windows), the above styling gets overridden. Instead of using the FF-theme value, we use -moz-default-background-color, under the assumption that that -moz-default-background-color maps to the system background color AND the system background color correctly reflects the user's system theme (now we're talking about windows dark mode vs. windows light mode). If we trace it through, it looks like -moz-default-background-color gets mapped to the Canvas color which then in turn gets mapped to mDefaultBackgroundColor which is finally set in PresShell according to the value of usePrefColors.

So, with 'use system colors' enabled (assuming standins are off) we'll assign -moz-default-background-color = Canvas = mDefaultBackground = ColorID::WindowBackground. But! When we fetch ColorID::WindowBackground, we fetch that color with respect to the given scheme, as decided here. In this example, that scheme value will mirror what's set in browser.theme.toolbar-theme (which sounds correct) however, unlike Android, MacOS, and Linux, Windows ultimately doesn't take scheme into account when returning a color. Fetching the default ::WindowBackground with HCM on, however, seems to return the user's HCM background color. Why this is true, I'm not entirely sure, but that's what my testing seems to show. I'll verify this again. I'd have to learn more about how windows does color overriding internally to explain exactly what's happening here 🤕 Seems like this is a known issue though.

Anyway ultimately, when system colors is on, we ask for the user's current theme, throw it away, and then get the default window color which is from the light theme when HCM isn't present. :(

So what this patch does is it ensures we only use use_system_colors to style tabpanels in the case that FFHCM is enabled. That way, windows users without HCM will still see their tabpanel respond to their FF-theme, and winodws HCM users will see their HCM theme colors reflected iff they've also enabled FF HCM.
Behaviour on other platforms should stay the same, since they'll stay considering system theme in system color overriding.

To be more accurate, this should look something like:

if (use_system_colors) {
  if (platform != windows) || (windows_HCM_enabled) {
    this.tabpanels.style.backgroundColor = -moz-default-background-color;
  }
}

but I couldn't figure out how to do that in frontend :/

Maybe a better fallback, instead of what I have, is to just consider the OS, like:

if (use_system_colors) {
  if (platform != windows) {
    this.tabpanels.style.backgroundColor = -moz-default-background-color;
  }
}

Let me know if this makes sense!

We can potentially also add reduced support for Windows dark mode right now, at least for the default foreground / background colors, if that would be simpler? It seems better long term too. Let me know if you want me to take it.

Changing severity to S2 because it's a rather annoying user-visible behavior.

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

We can potentially also add reduced support for Windows dark mode right now, at least for the default foreground / background colors, if that would be simpler? It seems better long term too. Let me know if you want me to take it.

That's probably the more correct thing to do here. If its straightforward to implement, this bug is yours 😀 but I'm also happy to land a temp CSS fix. I poked around search fox and don't see any other areas where we're doing a similar system colors check in frontend (though could definitely be a lurking issue other places...).
I'll push the updated CSS fix to phab as an option. Up to you! I'm happy either way

I can probably take over the week.

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

I can probably take over the week.

sounds good.
whichever we end up landing, we should probably uplift IMO

Eventually we should support all of them, but for now they are only used
by native code (unless you enable the color-scheme pref) and these will
be enough.

(In reply to Morgan Reschenberg [:morgan] from comment #12)

whichever we end up landing, we should probably uplift IMO

Agreed.

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

This change seems to cause (when using dark theme in Windows) the text to be white on white background on many windows. For example the "Password" popup and "Page info" windows.

(In reply to dgjunk from comment #17)

This change seems to cause (when using dark theme in Windows) the text to be white on white background on many windows. For example the "Password" popup and "Page info" windows.

[Edit] Ah, regression 1733354 is already mentioned.

:emilio, is there anything we can uplift for this or are the changes too risky?
I'm wondering if some of the fixes for regressions in 95 should get uplifted to 94

The main patch and partial backout landed for 94 didn't it? Most of them (all except bug 1734767) should be fixed in 94.

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

The main patch and partial backout landed for 94 didn't it? Most of them (all except bug 1734767) should be fixed in 94.

Ah okay, yeah I was looking at bug 1733354 and bug 1734297, but I see now that one was disabled in 94 :) All good! Thanks 😁

Er, wait this comment seems to indicate it'd fix other things if we uplifted that patch? 🤔 Do you have any context for what they mean by "other theme issues"?

I believe "other theme issues" Is mostly bug 1734767. But I'll try to write a more scoped patch for beta.