1435216 - Return something useful for theme.getCurrent() when the default theme is applied

Status: NEW

(WebExtensions :: Themes, enhancement, P3)

Description

[Tim Nguyen :ntim]

Right now we just return an empty object for the default theme is applied, but we could return some actual values from the browser css.

Comment 1

[David Durst [:ddurst]]

If we agree that we should get values for other built-in themes (dark, light), why would we not get values from default as well?

Comment 3

[Shane Caraveo (:mixedpuppy)]

getCurrent should always return theme data that an extension can use to theme itself, there is no reason not to.

Comment 4

[Tim Nguyen :ntim]

(In reply to Shane Caraveo (:mixedpuppy) from comment #3)
> getCurrent should always return theme data that an extension can use to
> theme itself, there is no reason not to.

Well, the default theme(s) does more than what the theming api can do, which makes it hard to return theme data.

Comment 5

[David Durst [:ddurst]]

(In reply to Tim Nguyen :ntim from comment #4)
> > getCurrent should always return theme data that an extension can use to
> > theme itself, there is no reason not to.
> 
> Well, the default theme(s) does more than what the theming api can do, which
> makes it hard to return theme data.

Can't we just return values that the theming API *can* do? I would expect that would be the case for all three built-ins, since I assume they all do more than what the theming API can do...

Comment 6

[Tim Nguyen :ntim]

(In reply to David Durst [:ddurst] (REO for 63) from comment #5)
> (In reply to Tim Nguyen :ntim from comment #4)
> > > getCurrent should always return theme data that an extension can use to
> > > theme itself, there is no reason not to.
> > 
> > Well, the default theme(s) does more than what the theming api can do, which
> > makes it hard to return theme data.
> 
> Can't we just return values that the theming API *can* do? I would expect
> that would be the case for all three built-ins, since I assume they all do
> more than what the theming API can do...

Sure, but that wouldn't necessarily make a representative result of how the theme is displayed.

An example is the default theme on macOS, right now querying `frame/accentcolor` would return #eeeeee (light gray) even though we use dark vibrancy (black blended with whatever is behind the window), which is obviously wrong. Of course, we can always return hardcoded values, but that also maintaining them over the releases.

Comment 7

[David Durst [:ddurst]]

Personally, I think that's OK.

Because a developer can only apply what's available in the theming API, when/if we're looking to leverage the theming API to make other extension UI look consistent (sidebars, etc), being able to get the existing values is substantially better than guessing at all the values that are involved.

If your point is that those values, once retrieved, don't actually provide the visual result -- I would argue it's still closer than giving nothing. That ".get-in-order-to-.set elsewhere" idea was the impetus behind this request for Light and Dark in the first place (with Default following on because it seems weird to leave it out).

From the extensions POV, these values are in a css file, so we could use those -- and we've talked about being able to update/parameterize that css file based on the user's selection of another built-in (since its current state is matching/derived from "default"). This would allow the browser_style property to be more meaningful. If that were the case, I would probably care less about getting theming API analogue values from getCurrent; it would seem inconsistent (to me), but for extensions the goal would be achievable.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to David Durst [:ddurst] (REO for 63) from comment #7)

> That ".get-in-order-to-.set elsewhere" idea was the impetus behind this
> request for Light and Dark in the first place (with Default following on
> because it seems weird to leave it out).

I would take that a few steps further...it's not weird, it's broken.  "default" can mean different things on different platforms/etc.  Having a consistent programmatic way to get relatively consistent appearance is a good thing.

Comment 9

[David Durst [:ddurst]]

Adding 1455960 to see also because of the browser_style impact.

Comment 13

[Ugi]

Hey, I want to bring attention to this bug again. Please consider it, as the number of users with the system default theme enabled has increased since the introduction of the Mica effect for Windows (widget.windows.mica), which requires that theme selection to function properly. And this default theme selection has started to cause addons that use theme.getCurrent() to break.

Comment 14

[Rob Wu [:robwu]]

I'm not sure if the default theme can fully be expressed in terms of the theme API. There is certainly some definition for the default theme at https://searchfox.org/mozilla-central/rev/2fcb225dc34b2183d7807e595d43a6f0f382d1b7/toolkit/mozapps/extensions/default-theme/manifest.json

The mica effect is implemented in bug 1764822. It is not clear to me how that would translate to output of the theme API.

Comment 15

[Ugi]

For the widget.windows.mica flag to work properly, the theme needs to be set to "system default"—not dark or light, but default (as mentioned in step 3 of this blog, I couldn't find any word by Mozilla about Mica). Not sure about the logic behind it but it works when followed and doesn't when it isn't.

The connection to this bug is that setting it to system default breaks theme.getCurrent(), which is used in many add-ons. The issue lies within the theme API rather than Mica itself; Mica is simply an example of why I am compelled to use system default as my theme.

Comment 16

[Rob Wu [:robwu]]

Relevant WECG topic: Proposal: theme.getCurrent() should always return an effective theme
https://github.com/w3c/webextensions/issues/877

Comment 18

[aafn]

Hello, see https://bugzilla.mozilla.org/show_bug.cgi?id=2005430#c11, this enhancement is direly needed, as my add-on behavior is now broken, following the promotion of https://bugzilla.mozilla.org/show_bug.cgi?id=1993056 in 146.0

null is now returned for the built-in dark theme, and my add-on has no way to find out which colors to use for the "sidebar" and "sidebar_text" coming from the current theme.

Comment 19

[Rob Wu [:robwu]]

(In reply to aafn from comment #18)

Hello, see https://bugzilla.mozilla.org/show_bug.cgi?id=2005430#c11, this enhancement is direly needed, as my add-on behavior is now broken, following the promotion of https://bugzilla.mozilla.org/show_bug.cgi?id=1993056 in 146.0

Bug 1993056 (specifically this part: https://phabricator.services.mozilla.com/D267850) changed the Firefox internal implementation, to rely on the (dark/light) theme stylesheet (browser-colors.css) instead of a custom (dark/light) theme declaration in the manifest when the dark/light theme is selected in about:addons. As a result of this change, getCurrent() stopped returning the theme colors from the custom dark/light theme (since it is no longer encoded in manifest.json's theme property).

It was already possible to have null for a dark theme when the default theme was selected (if the system theme is dark), now it is consistently "broken" in returning null whenever a default theme is used (whether system, light, dark), except for alpenglow theme, which is currently still formatted as a theme in manifest.json.

@aafn Did your extension work before, for all default themes? Or did it indeed not work in some cases as I described?

A potential short-term work-around to the regression could be to hardcode the manifest that was removed in bug 1993056 as a return value for getCurrent(), until we figure out a way to implement the feature requested here generally. But with hardcoding such a manifest, I worry that we enter the territory of browser_style, where the intent was for the browser to provide a way to automatically style extensions, but in reality turned out into an outdated stylesheet because of the unclear impact on extensions that specify it (resulting in us to drop it from MV3 in bug 1827910).

Comment 20

[aafn]

(looks like my comment was lost, not sure how, so let me answer again)

Yes, you are correct, there were some cases where my add-on theme mimic-ing was already broken, like when default system theme, and system theme is dark. Because the default theme of my add-on is light, and when the colors property is null, then I am using that default them.

Now, the problem is wider, because default dark is also returning null. It is still working with alpenglow, but this one will break also when it becomes "default" and return "null" like the others.

For now, I have a hardcoding workaround, which consists in reproducing the dark theme colors in my add-on when I can parse "dark" in the theme. But this is not a very good thing to do, as soon as they will change, the add-on will be off ..

Thank you for all the details above, I will try to use them to hardcode things a little better, while waiting for this enhancement to happen.