1345958 - Port bug 1343921 to TB [Implement support for custom icons through the Theming API]

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1343921 implemented an API to add icons to webExtensions. This is like LW-Themes with the ability to theme the button icons.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: webExtIcons-c-c.patch

Magnus, I'm asking now only for feedback awaiting an answer from Mike if it would be okay to add our entities to the toolkit theme scheme.

To work it needs both patches for c-c and m-c.

Comment 2

[Richard Marti (:Paenglab)]

Attachment: webExtIcons-c-c.patch

Mike, TB needs to add entities to the theme scheme. Would this be okay? There a lot more than FX has because TB has also a lot more buttons.

Or does a possibility exist to add them during runtime? Or could it be implemented that these schemes are placed in browser and mail and toolkit loads them during build from there?

Comment 3

[Richard Marti (:Paenglab)]

Attachment: nuvola-webext-theme.xpi

This quickly made webExtension works on FX and TB. Thanks to bug 1339131 it does no more fail when unknown entities are found.

To test load about:debugging in TB's home page and with "Load Temporary Add-on" the XPI can be loaded temporary.

Before, the prefs extensions.webextensions.themes.enabled and extensions.webextensions.themes.icons.enabled need to be set to true.

Comment 4

[Mike de Boer [:mikedeboer]]

Hi Richard, cool to see you working on this! I do think, however it might be a bit too soon, because we're still working on the framework itself.
For example, we don't think that having the list of icons live in a pref is ideal; we'll probably change it to be in a .js file called 'ThemeData.js', which'll contain icon names and many other properties in the near future.

What do you think of:
```js
let themeData = null;
try {
  themeData = Cu.import("resource:///modules/ThemeData.js", {}).ThemeData;
} catch (ex) {
  Cu.reportError("No Theme details could be found, which will severely limit your theming options.");
}

console.dir(themeData.icons); // '["icon1", "icon2", ...]'
```

?

Comment 5

[Mike de Boer [:mikedeboer]]

Also, please be aware that we'll put in a hard requirement for SVG icons to be used as icons to be able to support different DPI settings.

Comment 6

[Richard Marti (:Paenglab)]

Mike, thank you for checking this. Then I'm waiting until it's in a final stage.

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> What do you think of:
> ```js
> let themeData = null;
> try {
>   themeData = Cu.import("resource:///modules/ThemeData.js", {}).ThemeData;
> } catch (ex) {
>   Cu.reportError("No Theme details could be found, which will severely limit
> your theming options.");
> }
> 
> console.dir(themeData.icons); // '["icon1", "icon2", ...]'
> ```
> 
> ?

This sounds good.

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Also, please be aware that we'll put in a hard requirement for SVG icons to
> be used as icons to be able to support different DPI settings.

Then I embed the bitmap in a SVG. ;)
Joke aside, the extension was only quickly made to see a first result. My bitmaps don't look really good in the FX AppMenu panel.

Comment 7

[Richard Marti (:Paenglab)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> Hi Richard, cool to see you working on this! I do think, however it might be
> a bit too soon, because we're still working on the framework itself.
> For example, we don't think that having the list of icons live in a pref is
> ideal; we'll probably change it to be in a .js file called 'ThemeData.js',
> which'll contain icon names and many other properties in the near future.
> 
> What do you think of:
> ```js
> let themeData = null;
> try {
>   themeData = Cu.import("resource:///modules/ThemeData.js", {}).ThemeData;
> } catch (ex) {
>   Cu.reportError("No Theme details could be found, which will severely limit
> your theming options.");
> }
> 
> console.dir(themeData.icons); // '["icon1", "icon2", ...]'
> ```
> 
> ?

Mike, are you still planning to do above?

Comment 8

[Mike de Boer [:mikedeboer]]

Hi Richard, thanks for the ping!

We've opted for defining the icons statically in theme.json, in the icons section. Please feel free to add icon identifiers there that are TB specific (jaws or I can review them just fine).
In browser/base/content/theme-vars.inc.css you'll find how we activate the icons using CSS variables with same name on the root element. If you follow that example for TB windows, it *should* work. The activating mechanism (JS) is in LightweightThemeConsumer.jsm.

Also remember to set the pref 'extensions.webextensions.themes.icons.enabled' to `true`.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: webExtIcons-m-c.patch, checked-in comment 17

This is the part for M-C which defines the elements to style.

Comment 10

[Richard Marti (:Paenglab)]

Attachment: webExtIcons-c-c.patch

C-C part updated to tip.

Comment 11

[Mike de Boer [:mikedeboer]]

Comment on attachment 8871377 [details] [diff] [review]
webExtIcons-m-c.patch, checked-in comment 17

Review of attachment 8871377 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM!

Comment 12

[Richard Marti (:Paenglab)]

Aryx, please can you land the webExtIcons-m-c.patch in M-I and leave the bug open for the TB patch?

Comment 13

[Richard Marti (:Paenglab)]

Not sure if Aryx around.
Carsten, could you land the webExtIcons-m-c.patch and leave the bug open?

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0b91f39cf7
Add TB items to theme.json shema. r=mikedeboer

Comment 15

[Carsten Book [:Tomcat]]

landed on m-i so it get merged to m-c at some point today

Comment 16

[Richard Marti (:Paenglab)]

Thank you, Carsten.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6f0b91f39cf7

Comment 18

[Richard Marti (:Paenglab)]

Attachment: webExtIcons-c-c.patch

This patch adds the webextension themes on Daily like bug 1341722 does.

To test the patch you can use the webExt theme I attached in this bug. The pref extensions.webextensions.themes.icons.enabled needs to be set to true. Then the best is, you load about:debugging in TB's home page and with "Load Temporary Add-on" the XPI can be loaded temporary.

Comment 19

[Richard Marti (:Paenglab)]

Bug 1341722 enabled the pref for all channels. I'll follow this change with the next patch or before landing.

Comment 20

[Richard Marti (:Paenglab)]

Attachment: nuvola-webext-theme.xpi

This extension is now normally installable. You need only to enable extensions.webextensions.themes.icons.enabled to see the icons.

Some icons are already SVG ones to test also this.

Comment 21

[Magnus Melin [:mkmelin]]

Comment on attachment 8873160 [details] [diff] [review]
webExtIcons-c-c.patch

Review of attachment 8873160 [details] [diff] [review]:
-----------------------------------------------------------------

Pretty sweet, thx! r=mkmelin

Comment 22

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/comm-central/rev/167a8565ea427bfdd897a1f3835b075a7091a150

https://hg.mozilla.org/comm-central/rev/b6669e74cafa0fba300b034dcbb7f7b832a812a7 - backout
https://hg.mozilla.org/comm-central/rev/4f6639dc0572bc3dc0f08c60f355d1ea2448355b - landed again with bug number.