Closed Bug 1347543 Opened 7 years ago Closed 7 years ago

Change Toolbar Icons from PNG to SVG

Categories

(Firefox :: Theme, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.5 - May 15
Tracking Status
firefox55 --- verified

People

(Reporter: shorlander, Assigned: nhnt11)

References

(Depends on 1 open bug, Blocks 3 open bugs, )

Details

(Whiteboard: [photon-visual][p1])

Attachments

(4 files, 5 obsolete files)

+++ This bug was initially created as a clone of Bug #1054016 +++

Based on the testing in bug 1054016 and the work for color context-fill in bug 1058040 we can start exploring how to switch icons from PNG to SVG.

This work will create the system for how we will use SVG assets, including:
- Naming convention(s)
- File location(s)
- Document structure
- Styling
Whiteboard: [Photon]
This is an initial proof of concept patch that adds some SVG icons and uses context-fill (see bug 1058040) for styling.

I think the first step is testing for this approach for any perf impact.
As file structure goes, I believe the "glyphs" approach worked pretty well for the Control Center permission icons and the Downloads Panel. This approach has one SVG file containing multiple "path" elements. Adding a new icon only means adding one line to the file, and then the icon can be referenced by its "#name".

We may have to fix bug 1027106, which is a performance issue with this approach. Splitting the icons in multiple files is likely to have much worse performance anyways. As Stephen said, we need testing.
(In reply to :Paolo Amadini from comment #3)
> As file structure goes, I believe the "glyphs" approach worked pretty well
> for the Control Center permission icons and the Downloads Panel. This
> approach has one SVG file containing multiple "path" elements. Adding a new
> icon only means adding one line to the file, and then the icon can be
> referenced by its "#name".
> 
> We may have to fix bug 1027106, which is a performance issue with this
> approach. Splitting the icons in multiple files is likely to have much worse
> performance anyways. As Stephen said, we need testing.

Based on the testing Jonathan did in bug 1054016, single SVGs with limited filters performed the best.

The goal of this is approach is to only use one file per icon and change the color via external CSS. This moves us away from the awkward nature of SVG Sprites or using hash based SVGs with multiple glyphs per file. Also it is (hopefully) a perf win ;)
(In reply to Stephen Horlander [:shorlander] from comment #4)
> Based on the testing Jonathan did in bug 1054016, single SVGs with limited
> filters performed the best.

I'm saying that with bug 1027106 fixed, the performance of a single file may be much better.

This was never tested because it requires fixing that bug ;-)

> The goal of this is approach is to only use one file per icon and change the
> color via external CSS. This moves us away from the awkward nature of SVG
> Sprites or using hash based SVGs with multiple glyphs per file. Also it is
> (hopefully) a perf win ;)

Anyways, I'm not opposed to having one file per icon if you've determined it has better trade-offs for other reasons than just performance.
(In reply to :Paolo Amadini from comment #5)
> (In reply to Stephen Horlander [:shorlander] from comment #4)
> > Based on the testing Jonathan did in bug 1054016, single SVGs with limited
> > filters performed the best.
> 
> I'm saying that with bug 1027106 fixed, the performance of a single file may
> be much better.
> 
> This was never tested because it requires fixing that bug ;-)
> 
> > The goal of this is approach is to only use one file per icon and change the
> > color via external CSS. This moves us away from the awkward nature of SVG
> > Sprites or using hash based SVGs with multiple glyphs per file. Also it is
> > (hopefully) a perf win ;)
> 
> Anyways, I'm not opposed to having one file per icon if you've determined it
> has better trade-offs for other reasons than just performance.

From a design and asset maintenance perspective just having a single file makes everything much (*much*) easier and saves a lot of time.

Also from a code maintenance and usage perspective targeting a single file is less complicated than targeting a mega SVG file and having to know which icons it contains.

In the end though performance has been the traditional blocker on doing this. The asset maintenance could be handled through an automated process if it turns out there is a more performant way to handle this :)
Comment on attachment 8847601 [details] [diff] [review]
POC for Changing Toolbar Icons from PNG to SVG — WIP

>--- a/browser/themes/shared/jar.inc.mn
>+++ b/browser/themes/shared/jar.inc.mn
>@@ -85,6 +85,53 @@
>   skin/classic/browser/fxa/android@2x.png                      (../shared/fxa/android@2x.png)
>   skin/classic/browser/fxa/ios.png                             (../shared/fxa/ios.png)
>   skin/classic/browser/fxa/ios@2x.png                          (../shared/fxa/ios@2x.png)
>+
>+
>+  skin/classic/browser/glyph-addons-16.svg                     (../shared/glyph-addons-16.svg)
>+  skin/classic/browser/glyph-app-16.svg                        (../shared/glyph-app-16.svg)
>+  skin/classic/browser/glyph-back-16.svg                       (../shared/glyph-back-16.svg)
>+  skin/classic/browser/glyph-back-large-16.svg                 (../shared/glyph-back-large-16.svg)
>+  skin/classic/browser/glyph-bookmark-16.svg                   (../shared/glyph-bookmark-16.svg)
[...]

nit: We should group these files in a folder, e.g. toolbar/. Also, do we really need the "glyph-" prefix and "-16" suffix?
(In reply to Dão Gottwald [::dao] from comment #7)
> nit: We should group these files in a folder, e.g. toolbar/. 

The end goal is that these assets are not tied to a specific part of the UI. Instead they are general reusable assets that represent a metaphor. 

E.g. the 16px Add-ons metaphor is always /path/glyph-addons-16.svg and you would reference that anywhere you needed a 16px representation for Add-ons; whether that's in the toolbar or incontent or where ever.

> Also, do we really need the "glyph-" prefix and "-16" suffix?

Probably? We may have different sized glyphs (16px and 32px for example) and "glyph" has a semantic meaning vs. say "icon".

Glyph = flat shape that represents a thing (e.g. current toolbar icons)

Icon = could be a full color representational image  (e.g. current sidebar icons for Bookmarks and History)
(In reply to Stephen Horlander [:shorlander] from comment #8)
> (In reply to Dão Gottwald [::dao] from comment #7)
> > nit: We should group these files in a folder, e.g. toolbar/. 
> 
> The end goal is that these assets are not tied to a specific part of the UI.
> Instead they are general reusable assets that represent a metaphor. 

I still think we should group them rather than dumping them in the root folder. If not toolbar/, then something else that makes sense (maybe glyphs/).

> E.g. the 16px Add-ons metaphor is always /path/glyph-addons-16.svg and you
> would reference that anywhere you needed a 16px representation for Add-ons;
> whether that's in the toolbar or incontent or where ever.
> 
> > Also, do we really need the "glyph-" prefix and "-16" suffix?
> 
> Probably? We may have different sized glyphs (16px and 32px for example) and

But they're scaleable and don't need to be used precisely at these sizes, right? We've previously used a "-detailed" suffix to differentiate the larger variants from the rest.

> "glyph" has a semantic meaning vs. say "icon".

Right, I'm just not sure this needs to be part of the name. We usually don't put "icon" in the name either.
(In reply to Dão Gottwald [::dao] from comment #9)
> I still think we should group them rather than dumping them in the root
> folder. If not toolbar/, then something else that makes sense (maybe
> glyphs/).

Yeah, makes sense to me. I guess the question is how, or if, we want to group things. We could just create an "assets" folder and put everything there. The expectation being that these are supposed to be reusable and it makes it easier to find an asset. Currently they are scattered, and often duplicated, all over the place.

This is kind of related the name convention question below: the "glyph" prefix is a way to distinguish between other types of graphical assets. This is more of a book keeping thing than a strict naming requirement, but if I want to find specific type of asset it's hard to search for it if we don't tag it in some way.

> But they're scaleable and don't need to be used precisely at these sizes,
> right? We've previously used a "-detailed" suffix to differentiate the
> larger variants from the rest.

That's true. I wasn't aware of the "-detailed" suffix, but that makes sense. 

Although there are sizes of icons that won't scale cleanly from 16px. The Control Panel icons for example are 24 x 24 I think. SVG only scales nicely in multiples of two in my experience.

> > "glyph" has a semantic meaning vs. say "icon".
> 
> Right, I'm just not sure this needs to be part of the name. We usually don't
> put "icon" in the name either.

Right, currently we don't, but I am suggesting that maybe we should (see above WRT to asset management and finding things).
(In reply to Stephen Horlander [:shorlander] from comment #8)
> "glyph" has a semantic meaning vs. say "icon".
> 
> Glyph = flat shape that represents a thing (e.g. current toolbar icons)
> 
> Icon = could be a full color representational image  (e.g. current sidebar
> icons for Bookmarks and History)

OpenType glyphs can contain SVG and be "a full color representational image", so I'm not sure I agree with the semantics you're assigning here. To me a "glyph" is a shape used to represent a character, so the use of the word in the file names seems confusing. If you specifically want to denote that these files contain outlines, how about using the word "outline" instead (maybe still under an 'icons' directory)?
Blocks: 1348039
I looked at a perf comparison:
https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f53e35cb028f0e987c18387e12de8276b00cf955&newProject=try&newRevision=244ea3bad79b32693190246571dcaf02744eae0b&framework=1

there is a lot of retriggers, very high confidence!  Thanks for doing the leg work here.  One item stands out as sessionrestore e10s for osx- but looking at the graph in detail you see overlap of almost all the data points- then looking at related data points on autoland/inbound/try, these seem in the same range.  While there is a slight chance of regression for one test, I am pretty confident this is just lack of overlap in the noise and would be fine assuming this isn't a regression!
The performance for the final patch may be better than the current WIP patch since the WIP only removes the setting of Toolbar.png in one places, but there are many other places in CSS that reference the PNG:

https://dxr.mozilla.org/mozilla-central/search?q=browser%2Fskin%2FToolbar.png

If Toolbar.png is loaded during startup by even *one* of these places then we are paying the cost to both render Toolbar.png and the cost to render the new SVG icons. If this is happening then the Talos numbers we have currently  will be worse than they should be. We'd really need Talos numbers for a patch that removes the Toolbar.png file from the tree.
Well, unless everyone is happy that the Talos results are already good enough. Gijs?
We'll definitely get rid of Toolbar.png, if not here then in a followup.
I just spun a build with some logging code in imgRequest::Init() to log all image requests and Toolbar.png does NOT load when the browser starts with the WIP patch applied. So I think the Talos numbers should be reasonably accurate.
Seems Joel and :jwatt are both happy, so then I think we should push ahead and try to convert the WIP patch into a 'real' patch.

I'll note that per https://mail.mozilla.org/pipermail/photon-dev/2017-March/000000.html Dão is probably the best person to help guide this bug further. Not sure if Stephen wants to finish the patch himself or someone else needs to pick it up, but if not Dão can either take it or find an assignee, AIUI.
See Also: 1054016, 1058040
Whiteboard: waiting for bug 1058040 (SVG context-fill)
See Also: → 1348039
Depends on: 1350015
Stephen, are you going to make a patch ready for review or should somebody take over?
Flags: needinfo?(shorlander)
Whiteboard: waiting for bug 1058040 (SVG context-fill)
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Whiteboard: [photon]
Priority: -- → P1
Flags: needinfo?(shorlander)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8847601 - Attachment is obsolete: true
Blocks: 1355455
Whiteboard: [photon] → [photon-visual]
No longer blocks: photon-visual
Comment on attachment 8856939 [details] [diff] [review]
Patch

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

::: browser/themes/shared/icons/addons.svg
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +<svg width="18" height="18" viewBox="0 0 18 18" xmlns="http://www.w3.org/2000/svg">
> +  <path style="fill: context-fill;" id="icon-addons" d="M12,17c0.5,0,1-0.5,1-1v-4c0,0,0.2-0.8,0.8-0.8c0.6,0,0.6,0.8,1.8,0.8 c0.6,0,1.5-0.2,1.5-2c0-1.8-0.9-2-1.5-2c-1.1,0-1.2,0.8-1.8,0.8C13.2,8.8,13,8,13,8V6c0-0.6-0.4-1-1-1H9c0,0-0.8-0.1-0.8-0.8 S9,3.6,9,2.5C9,1.9,8.8,1,7,1S5,1.9,5,2.5c0,1.1,0.8,1.2,0.8,1.8S5,5,5,5H2C1.4,5,1,5.4,1,6l0,2.5c0,0-0.1,1.5,1.1,1.5 c0.8,0,0.9-1,1.9-1c0.5,0,1,0.5,1,1.6c0,1-0.5,1.6-1,1.6c-1,0-1.1-1-1.9-1C0.9,11,1,12.5,1,12.5L1,16c0,0.6,0.4,1,1,1h3.9 c0,0,1.5,0.1,1.5-1.1c0-0.8-1-0.9-1-1.9c0-0.5,0.7-1.2,1.8-1.2s1.9,0.7,1.9,1.2c0,1-1,1.1-1,1.9c0,1.2,1.5,1.1,1.5,1.1H12z" />

Please avoid the 'style' attribute, here and in all the other files. That will make it easier to switch these icons to "LSVG" (see bug https://bugzilla.mozilla.org/show_bug.cgi?id=LSVG ).

(There's also no point in specifying an 'id' attribute. It just wastes cycles creating an ID table and adding this element to it.)
Flags: qe-verify-
Whiteboard: [photon-visual] → [photon-visual][p1]
Attached patch Patch v2 (obsolete) — Splinter Review
I've fixed up the SVGs, and the bookmark animation on Windows.
I'll request review after a try push.
Attachment #8856939 - Attachment is obsolete: true
Attached patch Patch v3Splinter Review
Support inverted icons
Attachment #8858749 - Attachment is obsolete: true
Iteration: --- → 55.4 - May 1
So the downloads indicator sets the icon as a background-image (https://dxr.mozilla.org/mozilla-central/rev/20dff607fb88ee69135a280bbb7f32df75a86237/browser/themes/osx/downloads/indicator.css#46)

Replacing this with
>  background: <svg>;
>  fill: <some color>;

doesn't seem to work - the fill value seems to be ignored.

Jonathan, is this because context-fill isn't implemented for background images?
I don't think it is, because setting `fill` in CSS after changing the SVG image to fill="red" doesn't work either when using the SVG as a background-image. I thought you might be able to provide insight either way.
Flags: needinfo?(jwatt)
Correct, currently context paint is not supported with background images. I've put up patches in bug 1358690 to support that for you.
Flags: needinfo?(jwatt)
Depends on: 1358690
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

Jonathan, requesting f? from you since you expressed interest in testing this.

(In reply to Nihanth Subramanya [:nhnt11] from comment #23)
> I don't think it is, because setting `fill` in CSS after changing the SVG
> image to fill="red" doesn't work either when using the SVG as a
> background-image. I thought you might be able to provide insight either way.

Heh, seems like I was quite confused about context-fill when I wrote this comment.
Attachment #8861360 - Flags: feedback?(jwatt)
I forgot to mention, we still need an SVG version of the containers icon. There was an "XXXjwatt" comment in the original patch, from which I inferred that you are the person I should needinfo for this. :)
Flags: needinfo?(jwatt)
(In reply to Nihanth Subramanya [:nhnt11] from comment #27)
> I forgot to mention, we still need an SVG version of the containers icon.
> There was an "XXXjwatt" comment in the original patch, from which I inferred
> that you are the person I should needinfo for this. :)

That XXXjwatt comment came from one of the patches that I attached to bug 1054016:

https://bugzilla.mozilla.org/attachment.cgi?id=8758711&action=diff

I added it because I noticed this icon is missing, that's all. I'm definitely not the person to provide that icon. Maybe Stephen can help there.
Flags: needinfo?(jwatt) → needinfo?(shorlander)
To be clearer, I used sync.svg in the patch on that bug simply because we were testing perf over in that bug and I needed some placeholder SVG that would give us representative performance numbers for a switch to SVG. I didn't care so much about rendering the correct icon in that one instance.

(If it were to turn out to take a long time to get an appropriate SVG icon created you could clip out the icon from the PNG file (Toolbar.png) and just leave this one icon as PNG for now. Obviously this patch can't land without us using the correct icon in some form there. If you do that obviously file a follow-up bug though.)
Attached image containers-16.svg
Flags: needinfo?(shorlander) → needinfo?(nhnt11)
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review136214

What I can provide you review on is the *structure* of the SVG files. They look good to me so I'm happy to give you r+ for that. Obviously you still need Dão's full review.

That said I did look at the rest of the patch and have a couple of comments:

::: browser/base/content/browser-places.js:1799
(Diff revision 1)
>        this.dropmarkerNotifier.style.transform = dropmarkerTransform;
>  
>        let dropmarkerAnimationNode = this.dropmarkerNotifier.firstChild;
> -      dropmarkerAnimationNode.style.MozImageRegion = dropmarkerStyle.MozImageRegion;
>        dropmarkerAnimationNode.style.listStyleImage = dropmarkerStyle.listStyleImage;
> +      dropmarkerAnimationNode.style.fill = dropmarkerStyle.fill;

It's not clear to me if we'll need to set 'context-properties' here too, or if that is set in a style sheet.

::: browser/themes/shared/toolbarbutton-icons.inc.css:175
(Diff revision 1)
>  #panic-button[cui-areatype="toolbar"] {
> -  -moz-image-region: rect(0, 702px, 18px, 684px);
> +  list-style-image: url("chrome://browser/skin/forget.svg");
>  }
>  
>  #panic-button[cui-areatype="toolbar"][open] {
> -%ifdef XP_MACOSX
> +  list-style-image: url("chrome://browser/skin/forget.svg");

It looks like we only have one variant of forget.svg being committed, but the code that's being replaced here implies that there should be two different variants.
Attachment #8861360 - Flags: review+
Attachment #8861360 - Flags: feedback?(jwatt)
Comment on attachment 8861381 [details]
containers-16.svg

Nihanth, please remove the 'xmlns:xlink' attribute before integrating this into the patch.
(In reply to Stephen Horlander [:shorlander] from comment #30)
> Created attachment 8861381 [details]
> containers-16.svg

Thanks! I'm tweaking the patch some more and will include this in the next version.
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review136202

::: browser/themes/shared/toolbarbutton-icons.inc.css:38
(Diff revision 1)
>  
>  #bookmarks-menu-button[cui-areatype="toolbar"][starred] {
> -  -moz-image-region: rect(0, 162px, 18px, 144px);
> +  list-style-image: url("chrome://browser/skin/bookmark.svg");
> +}
> +
> +#bookmarks-menu-button[cui-areatype="toolbar"][starred] .toolbarbutton-icon

just one drive-by nit: Please use the child selector here.

Waiting for the next version to do a full review.
Attachment #8861360 - Flags: review?(dao+bmo)
(In reply to Jonathan Watt [:jwatt] from comment #31)
> ::: browser/themes/shared/toolbarbutton-icons.inc.css:175
> (Diff revision 1)
> >  #panic-button[cui-areatype="toolbar"] {
> > -  -moz-image-region: rect(0, 702px, 18px, 684px);
> > +  list-style-image: url("chrome://browser/skin/forget.svg");
> >  }
> >  
> >  #panic-button[cui-areatype="toolbar"][open] {
> > -%ifdef XP_MACOSX
> > +  list-style-image: url("chrome://browser/skin/forget.svg");
> 
> It looks like we only have one variant of forget.svg being committed, but
> the code that's being replaced here implies that there should be two
> different variants.

Stephen, I need a color code for the fill for the red version of the "forget" icon.
Flags: needinfo?(nhnt11) → needinfo?(shorlander)
(In reply to Nihanth Subramanya [:nhnt11] from comment #38)
> (In reply to Jonathan Watt [:jwatt] from comment #31)
> > ::: browser/themes/shared/toolbarbutton-icons.inc.css:175
> > (Diff revision 1)
> > >  #panic-button[cui-areatype="toolbar"] {
> > > -  -moz-image-region: rect(0, 702px, 18px, 684px);
> > > +  list-style-image: url("chrome://browser/skin/forget.svg");
> > >  }
> > >  
> > >  #panic-button[cui-areatype="toolbar"][open] {
> > > -%ifdef XP_MACOSX
> > > +  list-style-image: url("chrome://browser/skin/forget.svg");
> > 
> > It looks like we only have one variant of forget.svg being committed, but
> > the code that's being replaced here implies that there should be two
> > different variants.
> 
> Stephen, I need a color code for the fill for the red version of the
> "forget" icon.

You can use this for now: rgb(213,32,20)
I extracted this from Toolbar.png for Windows in the middle of the icon.
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137258

::: browser/themes/shared/toolbarbutton-icons.inc.css:187
(Diff revision 4)
> -  #new-window-button[cui-areatype="toolbar"] {
> -    -moz-image-region: rect(0, 684px, 36px, 648px);
> -  }
> +}
>  
> -  #e10s-button[cui-areatype="toolbar"] {
> -    -moz-image-region: rect(0, 684px, 36px, 648px);
> +#pocket-button {
> +  list-style-image: url("chrome://browser/skin/pocket.svg") !important;

doesn't pocket.css handle this already?
Attachment #8861360 - Flags: review?(dao+bmo)
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137264

::: browser/themes/windows/places/organizer.css:29
(Diff revision 6)
>  #back-button {
> -  -moz-image-region: rect(0, 54px, 18px, 36px);
> +  list-style-image: url("chrome://browser/skin/back.png");
>  }
>  
>  #forward-button {
> -  -moz-image-region: rect(0, 72px, 18px, 54px);
> +  list-style-image: url("chrome://browser/skin/forward.png");

should be svg, not png
Attachment #8861360 - Flags: review?(dao+bmo) → review+
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137266

::: browser/themes/windows/places/organizer.css:21
(Diff revision 7)
>  }
>  
>  #back-button,
>  #forward-button {
> -  list-style-image: url("chrome://browser/skin/Toolbar.png");
> +  /* uncomment after bug 1350010 lands: context-properties: fill; */
> +  fill: var(--toolbarbutton-icon-fill);

Oh wait, I don't think --toolbarbutton-icon-fill is available here.
Flags: qe-verify- → qe-verify+
QA Contact: brindusa.tot
(In reply to Dão Gottwald [::dao] from comment #45)
> Comment on attachment 8861360 [details]
> Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.
> 
> https://reviewboard.mozilla.org/r/133348/#review137266
> 
> ::: browser/themes/windows/places/organizer.css:21
> (Diff revision 7)
> >  }
> >  
> >  #back-button,
> >  #forward-button {
> > -  list-style-image: url("chrome://browser/skin/Toolbar.png");
> > +  /* uncomment after bug 1350010 lands: context-properties: fill; */
> > +  fill: var(--toolbarbutton-icon-fill);
> 
> Oh wait, I don't think --toolbarbutton-icon-fill is available here.

I think you can just use currentColor instead of the variable. Maybe reduce the opacity of .toobarbutton-icon if currentColor is too dark.
It seems like it might be a good policy to require that 'context-properties' is set just after the line that sets the image. That way it is clearer which SVG images depend on context paint, and make it less likely people will land changes that break context paint (and all the wasted time that will go with that). 'fill'/'stroke' could be set elsewhere if appropriate (say, for selectors for hover or active state). Any thoughts, Dão?
Flags: needinfo?(dao+bmo)
(In reply to Jonathan Watt [:jwatt] from comment #47)
> It seems like it might be a good policy to require that 'context-properties'
> is set just after the line that sets the image. That way it is clearer which
> SVG images depend on context paint, and make it less likely people will land
> changes that break context paint (and all the wasted time that will go with
> that). 'fill'/'stroke' could be set elsewhere if appropriate (say, for
> selectors for hover or active state). Any thoughts, Dão?

Sounds good in theory, but we'll want to make exceptions. In toolbarbutton-icons.inc.css, it makes sense to set this once in a shared rule rather than in the dozens of individual icon rules.
Flags: needinfo?(dao+bmo)
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137606

::: browser/themes/shared/toolbarbutton-icons.inc.css:8
(Diff revision 7)
> +  --toolbarbutton-icon-fill-inverted: #fff;
> +  --toolbarbutton-icon-fill-attention: #177ee5;
> +}
> +
>  :-moz-any(@primaryToolbarButtons@),
>  #bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon {

I think you can remove #bookmarks-menu-button > .toolbarbutton-menubutton-dropmarker > .dropmarker-icon from this selector. #bookmarks-menu-button is part of @primaryToolbarButtons@.
(In reply to Dão Gottwald [::dao] from comment #46)
> (In reply to Dão Gottwald [::dao] from comment #45)
> > Comment on attachment 8861360 [details]
> > Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.
> > 
> > https://reviewboard.mozilla.org/r/133348/#review137266
> > 
> > ::: browser/themes/windows/places/organizer.css:21
> > (Diff revision 7)
> > >  }
> > >  
> > >  #back-button,
> > >  #forward-button {
> > > -  list-style-image: url("chrome://browser/skin/Toolbar.png");
> > > +  /* uncomment after bug 1350010 lands: context-properties: fill; */
> > > +  fill: var(--toolbarbutton-icon-fill);
> > 
> > Oh wait, I don't think --toolbarbutton-icon-fill is available here.

D'oh

> 
> I think you can just use currentColor instead of the variable. Maybe reduce
> the opacity of .toobarbutton-icon if currentColor is too dark.

currentcolor is too dark, yeah; I just copied over the value of --toolbarbutton-icon-fill.
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137646

::: browser/themes/windows/places/organizer.css:21
(Diff revision 9)
>  }
>  
>  #back-button,
>  #forward-button {
> -  list-style-image: url("chrome://browser/skin/Toolbar.png");
> +  /* uncomment after bug 1350010 lands: context-properties: fill; */
> +  fill: #4c4c4c;

(In reply to Nihanth Subramanya [:nhnt11] from comment #52)
> > I think you can just use currentColor instead of the variable. Maybe reduce
> > the opacity of .toobarbutton-icon if currentColor is too dark.
> 
> currentcolor is too dark, yeah; I just copied over the value of
> --toolbarbutton-icon-fill.

Please use currentColor + opacity instead, as the value of --toolbarbutton-icon-fill is going to be wrong with dark themes and this window doesn't have the [brighttext] switch.
(In reply to Dão Gottwald [::dao] from comment #53)
> Please use currentColor + opacity instead, as the value of
> --toolbarbutton-icon-fill is going to be wrong with dark themes and this
> window doesn't have the [brighttext] switch.

Could also be a followup bug, though. I don't want to block landing this if the above is the last remaining issue.
Comment on attachment 8861360 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/133348/#review137656

::: browser/themes/shared/toolbarbutton-icons.inc.css:36
(Diff revision 10)
>  
>  #bookmarks-menu-button[cui-areatype="toolbar"][starred] {
> -  -moz-image-region: rect(0, 162px, 18px, 144px);
> +  list-style-image: url("chrome://browser/skin/bookmark.svg");
> +}
> +
> +toolbar:not([brighttext]) #bookmarks-menu-button[cui-areatype="toolbar"][starred] > [anonid=button] {

Please use .toolbarbutton-menubutton-button rather than [anonid=button]
Attachment #8861360 - Attachment is obsolete: true
Comment on attachment 8862917 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/134790/#review137758
Attachment #8862917 - Flags: review?(dao+bmo) → review+
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0e3f9e184e6b
Use SVGs instead of PNGs for toolbar button icons. r=dao
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9542b21d8f46
Use SVGs instead of PNGs for toolbar button icons. r=dao
Backout by kwierso@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/a966d5d61194
Backed out changeset 0e3f9e184e6b for landing on the wrong tree a=backout
Backed out for failing browser_all_files_referenced.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/e804405fc25611d7b633c14fd1b47e3507df5b66

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=9542b21d8f462f31727c82bbeb214935085f763c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=95307964&repo=mozilla-inbound

[task 2017-04-28T20:08:56.922504Z] 20:08:56     INFO - TEST-START | browser/base/content/test/static/browser_all_files_referenced.js
[task 2017-04-28T20:09:17.784157Z] 20:09:17     INFO - TEST-INFO | started process screentopng
[task 2017-04-28T20:09:19.075360Z] 20:09:19     INFO - TEST-INFO | screentopng: exit 0
[task 2017-04-28T20:09:19.075783Z] 20:09:19     INFO - Buffered messages logged at 20:08:56
[task 2017-04-28T20:09:19.076061Z] 20:09:19     INFO - Entering test bound checkAllTheFiles
[task 2017-04-28T20:09:19.076319Z] 20:09:19     INFO - Buffered messages finished
[task 2017-04-28T20:09:19.078520Z] 20:09:19     INFO - TEST-UNEXPECTED-FAIL | browser/base/content/test/static/browser_all_files_referenced.js | there should be no unreferenced files - Got 18, expected 0
[task 2017-04-28T20:09:19.078785Z] 20:09:19     INFO - Stack trace:
[task 2017-04-28T20:09:19.079093Z] 20:09:19     INFO -     chrome://mochikit/content/browser-test.js:test_is:928
[task 2017-04-28T20:09:19.080503Z] 20:09:19     INFO -     chrome://mochitests/content/browser/browser/base/content/test/static/browser_all_files_referenced.js:checkAllTheFiles:659
[task 2017-04-28T20:09:19.080881Z] 20:09:19     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:752:9
[task 2017-04-28T20:09:19.081223Z] 20:09:19     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:672:7
[task 2017-04-28T20:09:19.085475Z] 20:09:19     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59
Flags: needinfo?(nhnt11)
Iteration: 55.4 - May 1 → 55.5 - May 15
Attachment #8862917 - Attachment is obsolete: true
Comment on attachment 8863696 [details]
Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons.

https://reviewboard.mozilla.org/r/135478/#review138482

::: browser/themes/shared/icons/app.svg:1
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

Let's not land this file if it's not used. Can be added later if needed.

::: browser/themes/shared/icons/refresh.svg:1
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

Please rename to reload.svg. (This is fine to land since we have concrete plans for using it.)

::: browser/themes/shared/icons/syncedTabs.svg:1
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

Let's not land this either.

::: browser/themes/shared/icons/tabGroups.svg:1
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

ditto

::: browser/themes/shared/icons/webRTC-camera.svg:1
(Diff revision 2)
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public

ditto
Attachment #8863696 - Flags: review?(dao+bmo) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s f2ef2dafa740 -d f97d552d92a0: rebasing 393348:f2ef2dafa740 "Bug 1347543 - Use SVGs instead of PNGs for toolbar button icons. r=dao" (tip)
merging browser/extensions/pocket/skin/osx/pocket.css
merging browser/themes/linux/browser.css
merging browser/themes/linux/jar.mn
merging browser/themes/osx/browser.css
merging browser/themes/osx/jar.mn
merging browser/themes/shared/compacttheme.inc.css
merging browser/themes/shared/jar.inc.mn
merging browser/themes/shared/toolbarbuttons.inc.css and browser/themes/windows/browser.css to browser/themes/shared/toolbarbuttons.inc.css
merging browser/themes/windows/browser.css
merging browser/themes/windows/jar.mn
warning: conflicts while merging browser/themes/linux/browser.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/themes/osx/browser.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/themes/shared/toolbarbuttons.inc.css! (edit, then use 'hg resolve --mark')
warning: conflicts while merging browser/themes/windows/browser.css! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4d6cbe2ebbd1
Use SVGs instead of PNGs for toolbar button icons. r=dao
Blocks: 1043864
Blocks: 1215480
Blocks: 1236213
Blocks: 1236214
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f00d97d896ed
Backed out changeset 4d6cbe2ebbd1 for causing bc6 issues
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4577d0417879
Use SVGs instead of PNGs for toolbar button icons. r=dao
https://hg.mozilla.org/mozilla-central/rev/4577d0417879
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Depends on: 1362011
Blocks: 1362083
I see an improvement in talos:
== Change summary for alert #6359 (as of May 03 2017 12:15 UTC) ==

Improvements:

  2%  tresize windows8-64 opt e10s     11.01 -> 10.76

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=6359
Depends on: 1362100
Niiiice! \o/

(And bonus perf _win_?!)
Depends on: 1362120
Blocks: 1363477
Depends on: 1363731
Depends on: 1364329
No longer depends on: 1364329
Flags: needinfo?(shorlander)
Flags: needinfo?(nhnt11)
I tested this issue on Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.12 with the latest Nightly(55.0a1-20170516122050) and I can confirm the fix.
 
After checking the following preferences "Enable browser chrome and add-on debugging toolboxes", "Enable remote debugging", "Enable worker debugging(in development)" in Toolbox Options and pressing CTRL+SHIFT+ALT+I, to open the inspector, when inspecting all the different Toolbar Icons, the image source for them are pointing to the corresponding .SVG file.
Blocks: 1301355
Based on comment 86, mark bug verified-fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Flags: qe-verify-
Depends on: 1394314
You need to log in before you can comment on or make changes to this bug.