Change Toolbar Icons from PNG to SVG

ASSIGNED
Assigned to
(Needinfo from 2 people)

Status

()

Firefox
Theme
P1
normal
ASSIGNED
2 months ago
2 days ago

People

(Reporter: shorlander, Assigned: nhnt11, NeedInfo)

Tracking

(Blocks: 4 bugs)

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 4 obsolete attachments)

(Reporter)

Description

2 months ago
+++ 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
(Reporter)

Updated

2 months ago
Whiteboard: [Photon]
(Reporter)

Comment 1

2 months ago
Created attachment 8847601 [details] [diff] [review]
POC for Changing Toolbar Icons from PNG to SVG — WIP

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.

Updated

2 months ago
with patch:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=244ea3bad79b32693190246571dcaf02744eae0b

baseline:

remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=f53e35cb028f0e987c18387e12de8276b00cf955

compare (when both are finished)

https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=f53e35cb028f0e987c18387e12de8276b00cf955&newProject=try&newRevision=244ea3bad79b32693190246571dcaf02744eae0b&framework=1&showOnlyImportant=0

Comment 3

2 months ago
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.
(Reporter)

Comment 4

2 months ago
(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 ;)

Comment 5

2 months ago
(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.
(Reporter)

Comment 6

2 months ago
(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 7

2 months ago
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?
(Reporter)

Comment 8

2 months ago
(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)

Comment 9

2 months ago
(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.
(Reporter)

Comment 10

2 months ago
(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)?

Updated

2 months ago
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.

Updated

a month ago
Blocks: 1325171
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.

Updated

a month ago
See Also: bug 1054016, bug 1058040
Whiteboard: waiting for bug 1058040 (SVG context-fill)

Updated

a month ago
status-firefox55: affected → ---

Updated

a month ago
See Also: → bug 1348039

Updated

a month ago
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)

Updated

a month ago
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED

Updated

a month ago
Whiteboard: [photon]

Updated

a month ago
Priority: -- → P1

Updated

a month ago
Flags: needinfo?(shorlander)
(Assignee)

Comment 19

19 days ago
Created attachment 8856939 [details] [diff] [review]
Patch
Attachment #8847601 - Attachment is obsolete: true

Updated

19 days ago
Blocks: 1355455

Updated

19 days ago
Whiteboard: [photon] → [photon-visual]

Updated

18 days ago
No longer blocks: 1325171
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.)

Updated

18 days ago
Flags: qe-verify-

Updated

15 days ago
Whiteboard: [photon-visual] → [photon-visual][p1]
(Assignee)

Comment 21

14 days ago
Created attachment 8858749 [details] [diff] [review]
Patch v2

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
(Assignee)

Comment 22

13 days ago
Created attachment 8858759 [details] [diff] [review]
Patch v3

Support inverted icons
Attachment #8858749 - Attachment is obsolete: true

Updated

12 days ago
Iteration: --- → 55.4 - May 1
(Assignee)

Comment 23

10 days ago
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 hidden (mozreview-request)
(Assignee)

Comment 26

5 days ago
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)
(Assignee)

Comment 27

5 days ago
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.)
(Reporter)

Comment 30

5 days ago
Created attachment 8861381 [details]
containers-16.svg
Flags: needinfo?(shorlander) → needinfo?(nhnt11)

Comment 31

5 days ago
mozreview-review
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.
(Assignee)

Comment 33

5 days ago
(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 34

5 days ago
mozreview-review
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)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 38

3 days ago
(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 hidden (mozreview-request)

Comment 41

3 days ago
mozreview-review
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 hidden (mozreview-request)

Comment 43

3 days ago
mozreview-review
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 hidden (mozreview-request)

Comment 45

3 days ago
mozreview-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.
(Assignee)

Updated

3 days ago
Blocks: 1360199
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 49

2 days ago
mozreview-review
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@.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 52

2 days ago
(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 53

2 days ago
mozreview-review
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 hidden (mozreview-request)

Comment 56

2 days ago
mozreview-review
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]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 days ago
Attachment #8861360 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 61

2 days ago
mozreview-review
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+

Comment 62

2 days ago
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/0e3f9e184e6b
Use SVGs instead of PNGs for toolbar button icons. r=dao

Comment 63

2 days ago
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

Comment 64

2 days ago
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)
You need to log in before you can comment on or make changes to this bug.