1750932 - Allow themes to override global color scheme heuristics.

Status: RESOLVED FIXED

(WebExtensions :: Themes, enhancement, P3)

Comment 1

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750932 - Add color_scheme / content_color_scheme properties to theme API. r=robwu!,dao!

This allows themes to override our light / dark theme heuristics if they
choose to, so that we don't have to complicate the heuristics too much.

This is specially useful for themes with images, where the image might
be "light", but still have enough contrast with light text. A good
example is the theme mentioned in bug 1749837 comment 0.

The semantics are:

• color_scheme: If set, overrides the general "toolbar theme" (so
  window and context menu appearance and so on), otherwise we fall back
  to heuristics.

• content_color_scheme: If set, overrides the color scheme for the
  content area. Otherwise we fall back to color_scheme if present, or
  heuristics otherwise.

One thing that I didn't include was a sort of "system" option, which
might be useful to say "this theme is neutral, and works both for light
and dark themes". Let me know if you think that's a must-have, otherwise
I think it's probably worth deferring to a follow-up if it's needed at
all.

Comment 2

[Rob Wu [:robwu]]

Thanks for the patch. I'll discuss this with my team next week, but meanwhile I have some questions:

• Themes/extensions can declare theme and dark_theme, and the choice is based on some heuristics at https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/toolkit/modules/LightweightThemeConsumer.jsm#237-241. Would it make sense to default to the "dark" value for color_scheme/content_color_scheme for color properties in the dark_theme theme?
• The theme.update API supports window-specific themes. What is the desired (and actual) behavior when color_scheme / content_color_scheme is used? Could you add some unit tests that check the behavior when browser.theme.update is used with window-specific themes?
  • If window-specific themes cannot be supported for technical reasons, could you describe the current behavior, so we can evaluate whether it makes sense?
  • Even if window-specific themes can be supported, I wonder whether edge cases such as tabs moving between windows are accounted for (the content color scheme would then have to update based on the window's theme).

Comment 3

[Emilio Cobos Álvarez (:emilio)]

(In reply to Rob Wu [:robwu] from comment #2)

Thanks for the patch. I'll discuss this with my team next week, but meanwhile I have some questions:

• Themes/extensions can declare theme and dark_theme, and the choice is based on some heuristics at https://searchfox.org/mozilla-central/rev/d4b9c457db637fde655592d9e2048939b7ab2854/toolkit/modules/LightweightThemeConsumer.jsm#237-241. Would it make sense to default to the "dark" value for color_scheme/content_color_scheme for color properties in the dark_theme theme?

That seems reasonable, will update the patch and add a test.

• The theme.update API supports window-specific themes. What is the desired (and actual) behavior when color_scheme / content_color_scheme is used? Could you add some unit tests that check the behavior when browser.theme.update is used with window-specific themes?
  • If window-specific themes cannot be supported for technical reasons, could you describe the current behavior, so we can evaluate whether it makes sense?
  • Even if window-specific themes can be supported, I wonder whether edge cases such as tabs moving between windows are accounted for (the content color scheme would then have to update based on the window's theme).

In terms of "desired" behavior, in an ideal world that would be supported just fine, but in practice we can't for various reasons:

• The main one is that GTK doesn't allow us to switch native appearance in a performant way, so in practice all windows need to share the same native theme. Note that the theme still works of course, this mostly affects stuff like context menus / XUL trees / dialogs / etc.

• The increase in complexity in the platform would be significant (as you mentioned we'd need to track the window the current browsing-context is in and so on, and react to changes to it).

So I believe the current behavior is that both browser and content color scheme depend on the last theme.update call, in practice. This behavior is not changed by this patch at all, and this new API shouldn't prevent us from supporting the desired behavior in the future if we decide it's worth the complexity.

Comment 4

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1750932 - Explicitly opt in dark and light mode to dark/light color-schemes. r=dao

This doesn't change behavior yet, but in bug 1736218 we'll stop using
heuristics to choose color-scheme, and thus this would be needed to preserve
the content color-scheme on those themes.

Comment 5

[Rob Wu [:robwu]]

With the removal of heuristics in bug 1736218, what is the purpose of this bug and the patches?

I'm concerned about the volatility of the extension APIs, as that is a maintenance burden (see also https://phabricator.services.mozilla.com/D140375?id=551311#inline-772583).

Comment 6

[Emilio Cobos Álvarez (:emilio)]

(In reply to Rob Wu [:robwu] from comment #5)

With the removal of heuristics in bug 1736218, what is the purpose of this bug and the patches?

I'm concerned about the volatility of the extension APIs, as that is a maintenance burden (see also https://phabricator.services.mozilla.com/D140375?id=551311#inline-772583).

You can think of those changes as basically making the heuristic "always system theme unless theme specifies otherwise". And thus, the theme still needs a way to specify the different color schemes, so we still need this.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8cd7e0a57fa3
Add color_scheme / content_color_scheme properties to theme API. r=robwu,dao
https://hg.mozilla.org/integration/autoland/rev/944dd654f1e8
Explicitly opt in dark and light mode to dark/light color-schemes. r=dao

Comment 8

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/8cd7e0a57fa3
https://hg.mozilla.org/mozilla-central/rev/944dd654f1e8

Comment 9

[Anon Ymity]

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

Created attachment 9259712 [details]
Bug 1750932 - Add color_scheme / content_color_scheme properties to theme API. r=robwu!,dao!

This allows themes to override our light / dark theme heuristics if they
choose to, so that we don't have to complicate the heuristics too much.

This is specially useful for themes with images, where the image might
be "light", but still have enough contrast with light text. A good
example is the theme mentioned in bug 1749837 comment 0.

The semantics are:

• color_scheme: If set, overrides the general "toolbar theme" (so
  window and context menu appearance and so on), otherwise we fall back
  to heuristics.

• content_color_scheme: If set, overrides the color scheme for the
  content area. Otherwise we fall back to color_scheme if present, or
  heuristics otherwise.

One thing that I didn't include was a sort of "system" option, which
might be useful to say "this theme is neutral, and works both for light
and dark themes". Let me know if you think that's a must-have, otherwise
I think it's probably worth deferring to a follow-up if it's needed at
all.

Windows 10 64-Bit (21H2- Build 19044.1586)
Firefox Nightly 100a1 (2022-03-18)
Website appearance: Nightly theme

I tried adding the "color_scheme" property to my theme.
https://addons.mozilla.org/de/firefox/addon/tiger-eyes-turquoise/

I discovered a problem when debugging the changed theme.
I can see that windows (content, about pages, library) follow the color scheme. I can make them light or dark with the theme.
The context menu and the menu of the menu bar do not follow the value. They are always dark.

Comment 10

[Emilio Cobos Álvarez (:emilio)]

On which OS? Can you file a separate bug for this and link it from here? I can take a look.

Comment 11

[Anon Ymity]

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

On which OS? Can you file a separate bug for this and link it from here? I can take a look.

The OS is Windows 10 64-Bit.
Bug 1760317

Comment 12

[CenCerberon]

I think this bug also fixes these 2 bugs:
Bug 1702427
Bug 1705501

A theme with bright text causes various menus to be dark themed.

Comment 14

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

Do we want to add a relnote for this?

Comment 15

[Emilio Cobos Álvarez (:emilio)]

Presumably, yeah. Maybe along with bug 1736218? The changes are described here. Which bug do you prefer the relnote request to be?

Comment 16

[Dianna Smith [:diannaS] - on Leave, back on August 9th]

I think bug 1736218

Comment 17

[jscher2000]

I filed an issue on MDN but am not comfortable actually making the edit: https://github.com/mdn/content/issues/15794

Comment 18

[Richard Bloor]

Content (Manifest theme key update #16568 ) & BCD (Manifest theme key update #16390) updates are ready for review.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Looks great, thanks!

Comment 20

[Richard Bloor]

Are you able approve the GitHub PRs?

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Not in the MDN repos, no.

Comment 22

[Rob Wu [:robwu]]

I have approved the mdn updates.