Closed Bug 1345958 Opened 3 years ago Closed 2 years ago

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

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 56.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(3 files, 4 obsolete files)

Bug 1343921 implemented an API to add icons to webExtensions. This is like LW-Themes with the ability to theme the button icons.
Depends on: 1343921
Attached patch webExtIcons-c-c.patch (obsolete) — Splinter Review
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8845545 - Flags: feedback?(mkmelin+mozilla)
Attached patch webExtIcons-c-c.patch (obsolete) — Splinter Review
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?
Attachment #8845549 - Flags: review?(mdeboer)
Attached file nuvola-webext-theme.xpi (obsolete) —
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.
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", ...]'
```

?
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.
Attachment #8845549 - Flags: review?(mdeboer)
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.
(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?
Flags: needinfo?(mdeboer)
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`.
Flags: needinfo?(mdeboer)
This is the part for M-C which defines the elements to style.
Attachment #8871377 - Flags: review?(mdeboer)
Attachment #8845549 - Attachment is obsolete: true
Attached patch webExtIcons-c-c.patch (obsolete) — Splinter Review
C-C part updated to tip.
Attachment #8845545 - Attachment is obsolete: true
Attachment #8845545 - Flags: feedback?(mkmelin+mozilla)
Comment on attachment 8871377 [details] [diff] [review]
webExtIcons-m-c.patch, checked-in comment 17

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

LGTM!
Attachment #8871377 - Flags: review?(mdeboer) → review+
Aryx, please can you land the webExtIcons-m-c.patch in M-I and leave the bug open for the TB patch?
Flags: needinfo?(aryx.bugmail)
Not sure if Aryx around.
Carsten, could you land the webExtIcons-m-c.patch and leave the bug open?
Flags: needinfo?(cbook)
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f0b91f39cf7
Add TB items to theme.json shema. r=mikedeboer
landed on m-i so it get merged to m-c at some point today
Flags: needinfo?(cbook)
Keywords: leave-open
Thank you, Carsten.
Flags: needinfo?(aryx.bugmail)
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.
Attachment #8871379 - Attachment is obsolete: true
Attachment #8873160 - Flags: review?(mkmelin+mozilla)
Bug 1341722 enabled the pref for all channels. I'll follow this change with the next patch or before landing.
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.
Attachment #8845554 - Attachment is obsolete: true
Comment on attachment 8873160 [details] [diff] [review]
webExtIcons-c-c.patch

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

Pretty sweet, thx! r=mkmelin
Attachment #8873160 - Flags: review?(mkmelin+mozilla) → review+
Attachment #8871377 - Attachment description: webExtIcons-m-c.patch → webExtIcons-m-c.patch, checked-in comment 17
You need to log in before you can comment on or make changes to this bug.