1386004 - Use theming API for Photon Dark and Light themes

Status: RESOLVED DUPLICATE of bug 1525762

(Firefox :: Theme, enhancement, P3)

Description

[Tim Nguyen :ntim]

Right now, Compact Dark and Compact Light have some wizardry associated to them. It would be nice to switch them to normal themes.

Comment 1

[Tim Nguyen :ntim]

Attachment: Bug 1386004 - Use theming API for Photon Dark and Light themes.

Review commit: https://reviewboard.mozilla.org/r/210014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/210014/

Comment 2

[Tim Nguyen :ntim]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210014/diff/1-2/

Comment 3

[Tim Nguyen :ntim]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210014/diff/2-3/

Comment 4

[Tim Nguyen :ntim]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/210014/diff/3-4/

Comment 5

[Tim Nguyen :ntim]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

This patch ships the light and dark themes as system add-ons (WebExtension Themes). This approach would remove a load of CSS, and a special mechanism (browser-compacttheme.js) that watches the lwt observer and injects the stylesheet, for which I'm super happy about. Another advantage is that the themes now work with `browser.theme.getCurrent()`.

I'm wondering if this is the right approach though, here's specifically why:

1) Localization (for the theme description and name) is surprisingly the most tricky part of this bug. Traditionally, the en-US version of the locales are located in mozilla-central, and the other locales are located in another repo (mozilla-central-l10n or something).

This patch introduces WebExtension-style localization, which makes it really hard to integrate with the current l10n system. Firefox Screenshots also uses the same system, and all the localization is done inside the Screenshots github repo, then the moz.build is automatically generated by their build script. However, this seems overkill for the Light/Dark themes.

Another possibility is to use the built-in l10n system *somehow* with this extension. I'm not sure how that would work though.

2) The themes don't appear in the customization mode. This is a problem in general for all WebExtension themes, that should be fixed anyway when we replace lightweight themes with static themes. This is because lightweight themes and WebExtensions themes are managed by different modules... IMO, this is not a big problem as it should be fixed generally anyway.

3) The two themes can't be done exclusively with the theming API properties (Mac vibrancy, customize mode changes, Windows 7 special layout), it will need support for the experimental section (bug 1347207), which will be a CSS file or a set of CSS rules that works on Nightly/DevEd/Mozilla add-ons. We'll probably need pre-processing somewhere for platform specific styles. I don't know what this approach will be.


:aswan, :rhelmer, Does the overall approach in the patch look right? Also, what do you think about 1) ?

:jaws, :dao, What do you think about 2) and 3) ?

Comment 6

[Tim Nguyen :ntim]

Another issue with l10n that I've just noticed is that this doesn't respect the "vendorShortName" string (Mozilla).

Comment 7

[Andrew Swan [:aswan]]

Getting rid of a bunch of special case handling for compact themes does seem like a nice win, thanks for working on this.  I'm kind of surprised that all the stuff in compacttheme.inc.css can be replaced by a webextension theme, but I'll leave it to jaws and dao to look that part over.

One nit, it looks like the system themes have icons defined but those icons aren't in the patch?

My bigger concern is about the fact that we haven't used system addons for themes before, I think we'll need to be explicit about some of the things this implies, such as:
1. system themes need to be disabled by default, unlike system extensions (actually why isn't this an issue with the current patch?)
2. system themes are not hidden by default (it looks like your patch adds some logic to implement this, that's not a problem per se but I have a reflexive negative reaction to that sort of very specialized logic deeply embedded in the addons manager)

I would also encourage you to do a talos run to see if this has any impact on startup performance.

Finally, re localization, the browser l10n team has a proposal for letting system addon strings be included in language packs (obviously this doesn't work for strings that need to be updated on the fly, but for the name and description of the built-in themes, I think that should be fine).  I believe this proposal is dependent on some of the ongoing work with fluent though, I'm not sure how their timeline would fit with when you are shooting to get this landed.

Comment 8

[Tim Nguyen :ntim]

(In reply to Andrew Swan [:aswan] from comment #7)
> Getting rid of a bunch of special case handling for compact themes does seem
> like a nice win, thanks for working on this.  I'm kind of surprised that all
> the stuff in compacttheme.inc.css can be replaced by a webextension theme,
> but I'll leave it to jaws and dao to look that part over.

This is mainly a proof of concept, but the plan is to replace the compacttheme CSS with:
- changes to the theming API system (bug 1418605, bug 1404688)
- more properties when it makes sense (bug 1348034)
- extra CSS for the remaining stuff like Mac Vibrancy, Windows 7 theme, see bug 1347207

I haven't taken care of this here yet, but this stuff is actually the easy part to solve once those bugs are fixed. 

> One nit, it looks like the system themes have icons defined but those icons
> aren't in the patch?

I've done an `hg move` in this patch that moves the icons from the old to the new directory (I've double checked the review request, and it's there)

> My bigger concern is about the fact that we haven't used system addons for
> themes before, I think we'll need to be explicit about some of the things
> this implies, such as:
> 1. system themes need to be disabled by default, unlike system extensions
> (actually why isn't this an issue with the current patch?)

Only one theme can be enabled by default. The mechanism ensuring this seems to work fine in this context.

> 2. system themes are not hidden by default (it looks like your patch adds
> some logic to implement this, that's not a problem per se but I have a
> reflexive negative reaction to that sort of very specialized logic deeply
> embedded in the addons manager)

Yes, I was unsure about that part, but it seems like all the themes we currently ship by default are all visible in the UI somehow (Light/Dark/Recommended themes in the customization mode).

> I would also encourage you to do a talos run to see if this has any impact
> on startup performance.

Good idea.

> Finally, re localization, the browser l10n team has a proposal for letting
> system addon strings be included in language packs (obviously this doesn't
> work for strings that need to be updated on the fly, but for the name and
> description of the built-in themes, I think that should be fine).  I believe
> this proposal is dependent on some of the ongoing work with fluent though,
> I'm not sure how their timeline would fit with when you are shooting to get
> this landed.

There's no rush really, this patch still depends on some theming API bugs to be fixed to be land-able. I'm just asking for overall feedback on this.

The main user-visible benefit of this is that it avoids visual regressions because of some forgotten code in compacttheme.css when doing large redesigns (Photon/Australis for Windows 10). But since we just shipped Photon, it's fine as it is for now.

Comment 9

[Dão Gottwald [:dao]]

(In reply to Tim Nguyen :ntim from comment #5)
> 3) The two themes can't be done exclusively with the theming API properties
> (Mac vibrancy, customize mode changes, Windows 7 special layout), it will
> need support for the experimental section (bug 1347207), which will be a CSS
> file or a set of CSS rules that works on Nightly/DevEd/Mozilla add-ons.
> We'll probably need pre-processing somewhere for platform specific styles. I
> don't know what this approach will be.

(In reply to Tim Nguyen :ntim from comment #8)
> The main user-visible benefit of this is that it avoids visual regressions
> because of some forgotten code in compacttheme.css when doing large
> redesigns (Photon/Australis for Windows 10). But since we just shipped
> Photon, it's fine as it is for now.

The experimental section that bug 1347207 proposes just looks like JSONified CSS. I don't understand how reformatting and moving compacttheme.css makes it more maintainable.

Comment 10

[Tim Nguyen :ntim]

(In reply to Dão Gottwald [::dao] from comment #9)
> The experimental section that bug 1347207 proposes just looks like JSONified
> CSS. I don't understand how reformatting and moving compacttheme.css makes
> it more maintainable.

There's a possibility that the experimental section becomes a reference to a CSS file rather than JSONified CSS. So no reformatting is needed. I still need to discuss that with :jaws and :mikedeboer, but that would be my preferred option for the experimental section.

Also, using the theming API will allow removing some parts of compacttheme.css

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

https://reviewboard.mozilla.org/r/210014/#review216886

I think this can work, thank you for taking the time to work on this! We would have to implement the experimental section first (as you know). In this patch I don't see what that included experimental CSS file would look like, perhaps you haven't done that part yet or it doesn't show up in mozreview?

::: toolkit/components/extensions/schemas/theme.json:505
(Diff revision 4)
>              "optional": true,
>              "patternProperties": {
>                "^[1-9]\\d*$": { "type": "string" }
>              }
> +          },
> +          "default_locale": {

Why is this needed? Isn't this covered by https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/toolkit/components/extensions/schemas/i18n.json#12 ?

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8939706 [details]
Bug 1386004 - Use theming API for Photon Dark and Light themes.

https://reviewboard.mozilla.org/r/210014/#review217962

Seems fine to me in principle, although I don't know that we'd get a lot of practical benefit from having these separated into add-ons that ship with Firefox versus having the theme CSS live in the omni jar... the only advantage I can think of is that these being available as normal webextension themes would make it possible to hack on them using about:debugging without having to do your own build, which is nice. It seems unlikely we'd use the system add-on update mechanism or AMO to override these although those are both possibilities.

Comment 13

[Tim Nguyen :ntim]

The only main blocker to this is bug 1395105.

Comment 14

[Axel Hecht [:Pike]]

I filed bug 1457865 as an entry point to some of the l10n-related blockers. That's just about using a different code path to get from localizable resources to show the localized name and description of web extensions.

There's another part that requires the resources to come from language packs, and to work on localized builds.

Not sure right now how to best solve for that. The challenges I see are around exposing data from web-extensions (langpacks) such that we can load the data only for system addons, and for that to work in localized builds. And to not expose that data to the world at the same time.