1348039 - Enable pref for custom (SVG) icons support via Theming API

Status: NEW

(WebExtensions :: Themes, enhancement, P5)

Description

[:shell escalante]

the pref name is extensions.webextensions.themes.icons.enabled.  changing it to true will allow icons to be specified by webextension themes.  the bug that introduced the theming:icon code and pref is bug 1343921

right now bug 1343921 will accept any image format, but we should restrict it to SVG before preffing on (since it's new for lwt) - unless it is decided that SVG icons are not being used in firefox toolbar.  The bugs this depends upon are the ones needed to make sure SVG icons are performant enough.

not using SVG leaves scaling issues between themes - shorlander is the UX person on this and explains best in https://bugzilla.mozilla.org/show_bug.cgi?id=1054016#c40

Comment 1

[Jonathan Watt [:jwatt]]

(In reply to :shell escalante from comment #0)
> right now bug 1343921 will accept any image format, but we should restrict
> it to SVG before preffing on (since it's new for lwt) - unless it is decided
> that SVG icons are not being used in firefox toolbar.

Even if the default icons use SVG, I'm not sure requiring SVG for icons provided via the theming API is a good idea. SVG can be quite close to raster formats in terms of performance _IF_ the SVG doesn't contain expensive effects. On the other hand, if the SVG needs to contain CPU intensive effects such as gradients or filters then raster image equivalents can perform much better (since all the expensive work of computing the gradients, filters, etc. is done while the raster images are being created instead of when they're loaded).

Comment 2

[:shell escalante]

removing depends on bug 1347543 and bug 1054016.  Follow bug 1347543 for results of patch and perf.

The SVG toolbar icons perf tests (uses different SVGs) looks good enough to try experiment with our icon SVG pref to true based on https://bugzilla.mozilla.org/show_bug.cgi?id=1347543#c16 ... 

there is a test SVG icon theme included with jareds' patch in bug 1343921

Comment 3

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

(In reply to Jonathan Watt [:jwatt] from comment #1)
> (In reply to :shell escalante from comment #0)
> > right now bug 1343921 will accept any image format, but we should restrict
> > it to SVG before preffing on (since it's new for lwt) - unless it is decided
> > that SVG icons are not being used in firefox toolbar.
> 
> Even if the default icons use SVG, I'm not sure requiring SVG for icons
> provided via the theming API is a good idea. SVG can be quite close to
> raster formats in terms of performance _IF_ the SVG doesn't contain
> expensive effects. On the other hand, if the SVG needs to contain CPU
> intensive effects such as gradients or filters then raster image equivalents
> can perform much better (since all the expensive work of computing the
> gradients, filters, etc. is done while the raster images are being created
> instead of when they're loaded).

Yes, while this is true we also get many benefits from using scalable graphics in that we will not be locked in to one size for the future of our UI. We could implement some rules that block SVGs that use expensive functions if that is our only worry.

Comment 4

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

Attachment: Bug 1348039 - Enable the support for icons in webextension themes.

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

Comment 5

[Mike de Boer [:mikedeboer]]

Comment on attachment 8862158 [details]
Bug 1348039 - Enable the support for icons in webextension themes.

https://reviewboard.mozilla.org/r/134134/#review137036

Thanks, Jared!

Comment 6

[Stephen Horlander [:shorlander]]

(In reply to Mike de Boer [:mikedeboer] from comment #5)
> Comment on attachment 8862158 [details]
> Bug 1348039 - Enable the support for icons in webextension themes.
> 
> https://reviewboard.mozilla.org/r/134134/#review137036
> 
> Thanks, Jared!

What does this do?

Comment 7

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

(In reply to Stephen Horlander [:shorlander] from comment #6)
> (In reply to Mike de Boer [:mikedeboer] from comment #5)
> > Comment on attachment 8862158 [details]
> > Bug 1348039 - Enable the support for icons in webextension themes.
> > 
> > https://reviewboard.mozilla.org/r/134134/#review137036
> > 
> > Thanks, Jared!
> 
> What does this do?

This allows themes written through the new theming API to replace icons with their own version. Themes would have to use the following in their manifest:

> {
>   "manifest_version": 2,
>   "name": "webext-lwt-icons",
>   "version": "1.0",
> 
>   "description": "Loads a lwt and icons using the webext format",
>   "icons": {
>     "96": "face.png"
>   },
>   "theme": {
>     "colors": {
>       "frame": [71, 105, 91],
>       "tab_text": [207, 221, 192, 0.9]
>     },
>     "images": {
>       "theme_frame": "face.png"
>     },
>     "icons": {
>       "back": "fox.svg",
>       "forward": "fox.svg",
>       "reload": "fox.svg",
>       "stop": "fox.svg",
>     }
>   }
> }

Creating a theme like this, with a 'fox.svg' file, would replace the back, forward, reload, and stop icons with the fox SVG icon.

Comment 8

[Stephen Horlander [:shorlander]]

Sorry, I mean does this specifically turn that on with the intent to ship? I discussed with Kev that we had never talked about this functionality and no one from UX had input here that I am aware of.

Comment 9

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

(In reply to Stephen Horlander [:shorlander] from comment #8)
> Sorry, I mean does this specifically turn that on with the intent to ship? I
> discussed with Kev that we had never talked about this functionality and no
> one from UX had input here that I am aware of.

Oh, I was out of the loop on that. I thought we were just trying to tie this together with switching our icons from PNG to SVG. I think we should do this, it is a nice way for theme devs to customize the browser and create a rich/full theme with little work and low risk of side-effects. What are the issues that you or Kev see with this?

Comment 10

[:shell escalante]

(In reply to (Away 3 May - 5 May) Jared Wein [:jaws] (please needinfo? me) from comment #9)
> (In reply to Stephen Horlander [:shorlander] from comment #8)
> > Sorry, I mean does this specifically turn that on with the intent to ship? I
> > discussed with Kev that we had never talked about this functionality and no
> > one from UX had input here that I am aware of.
> 
> Oh, I was out of the loop on that. I thought we were just trying to tie this
> together with switching our icons from PNG to SVG. I think we should do
> this, it is a nice way for theme devs to customize the browser and create a
> rich/full theme with little work and low risk of side-effects. What are the
> issues that you or Kev see with this?

Sorry this bug was filed to track the work to do, but not to land pre-57.  As this isn't a blocker area for 57, and the risk/time isn't something that is planned pre-57.  The issues Kev brought up were around defining and constraining the SVGs accepted to avoid any security risk of including more than intended with the SVG file and keep the perf on par with what was tested with toolbar icons. In addition to getting further UX input.

Comment 11

[Stephen Horlander [:shorlander]]

(In reply to :shell escalante from comment #10)
> Sorry this bug was filed to track the work to do, but not to land pre-57. 
> As this isn't a blocker area for 57, and the risk/time isn't something that
> is planned pre-57.  The issues Kev brought up were around defining and
> constraining the SVGs accepted to avoid any security risk of including more
> than intended with the SVG file and keep the perf on par with what was
> tested with toolbar icons. In addition to getting further UX input.

Yes. We need to have a bigger discussion around the constraints and also the design risks this introduces. Can we please make sure this does not get enabled until we have those discussions? Thanks!

Comment 12

[:shell escalante]

Hi JAWS,  Can you not land that patch to enable?  I'll check with Shorlander/Kev on timing, I imagine to avoid introducing any risk - this will be post 57.

Comment 13

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

Yeah, I'm holding off from landing and watching the conversation here. Thanks for double-checking.

Comment 14

[Kev Needham [:kev]]

Bringing back up, adding shorlander on needinfo and scheduling a conversation so we get resolution.

Comment 15

[Kev Needham [:kev]]

Clearing my needinfo, pinging shorlander for comment by email, because we do need to rekindle this for 58/59 if this is going to be a thing.