Closed
Bug 1347543
Opened 8 years ago
Closed 8 years ago
Change Toolbar Icons from PNG to SVG
Categories
(Firefox :: Theme, enhancement, P1)
Firefox
Theme
Tracking
()
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
Reporter | ||
Updated•8 years ago
|
Whiteboard: [Photon]
Reporter | ||
Comment 1•8 years ago
|
||
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•8 years ago
|
Comment 2•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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•8 years 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).
Comment 11•8 years ago
|
||
(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)?
Comment 12•8 years ago
|
||
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!
Comment 13•8 years ago
|
||
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.
Comment 14•8 years ago
|
||
Well, unless everyone is happy that the Talos results are already good enough. Gijs?
Comment 15•8 years ago
|
||
We'll definitely get rid of Toolbar.png, if not here then in a followup.
Comment 16•8 years ago
|
||
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•8 years ago
|
Blocks: photon-visual
Comment 17•8 years ago
|
||
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•8 years ago
|
See Also: https://bugzilla.mozilla.org/show_bug.cgi?id=1054016,
https://bugzilla.mozilla.org/show_bug.cgi?id=1058040 →
Whiteboard: waiting for bug 1058040 (SVG context-fill)
Updated•8 years ago
|
status-firefox55:
affected → ---
Updated•8 years ago
|
Comment 18•8 years ago
|
||
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•8 years ago
|
Assignee: nobody → nhnt11
Status: NEW → ASSIGNED
Updated•8 years ago
|
Whiteboard: [photon]
Updated•8 years ago
|
Priority: -- → P1
Updated•8 years ago
|
Flags: needinfo?(shorlander)
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8847601 -
Attachment is obsolete: true
Updated•8 years ago
|
Whiteboard: [photon] → [photon-visual]
Updated•8 years ago
|
No longer blocks: photon-visual
Comment 20•8 years ago
|
||
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•8 years ago
|
Flags: qe-verify-
Updated•8 years ago
|
Whiteboard: [photon-visual] → [photon-visual][p1]
Assignee | ||
Comment 21•8 years ago
|
||
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•8 years ago
|
||
Support inverted icons
Attachment #8858749 -
Attachment is obsolete: true
Updated•8 years ago
|
Iteration: --- → 55.4 - May 1
Assignee | ||
Comment 23•8 years 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)
Comment 24•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 26•8 years 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•8 years 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)
Comment 28•8 years ago
|
||
(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)
Comment 29•8 years ago
|
||
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•8 years ago
|
||
Flags: needinfo?(shorlander) → needinfo?(nhnt11)
Comment 31•8 years 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+
Updated•8 years ago
|
Attachment #8861360 -
Flags: feedback?(jwatt)
Comment 32•8 years ago
|
||
Comment on attachment 8861381 [details]
containers-16.svg
Nihanth, please remove the 'xmlns:xlink' attribute before integrating this into the patch.
Assignee | ||
Comment 33•8 years 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•8 years 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•8 years 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)
Comment 39•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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.
Updated•8 years ago
|
Flags: qe-verify- → qe-verify+
QA Contact: brindusa.tot
Comment 46•8 years ago
|
||
(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.
Comment 47•8 years ago
|
||
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)
Comment 48•8 years ago
|
||
(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•8 years 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•8 years 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•8 years 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.
Comment 54•8 years ago
|
||
(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•8 years 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•8 years ago
|
Attachment #8861360 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 61•8 years 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•8 years 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•8 years 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•8 years 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
Comment 65•8 years ago
|
||
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)
Updated•8 years ago
|
Iteration: 55.4 - May 1 → 55.5 - May 15
Comment hidden (mozreview-request) |
Assignee | ||
Updated•8 years ago
|
Attachment #8862917 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Comment 68•8 years ago
|
||
mozreview-review |
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+
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Comment 73•8 years ago
|
||
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)
Comment hidden (mozreview-request) |
Comment 75•8 years ago
|
||
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4d6cbe2ebbd1
Use SVGs instead of PNGs for toolbar button icons. r=dao
Comment 77•8 years ago
|
||
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f00d97d896ed
Backed out changeset 4d6cbe2ebbd1 for causing bc6 issues
Comment hidden (mozreview-request) |
Comment 79•8 years ago
|
||
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/4577d0417879
Use SVGs instead of PNGs for toolbar button icons. r=dao
Comment 80•8 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment 82•8 years ago
|
||
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
Comment 83•8 years ago
|
||
Niiiice! \o/
(And bonus perf _win_?!)
Comment hidden (off-topic) |
Comment hidden (off-topic) |
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(shorlander)
Flags: needinfo?(nhnt11)
Comment 86•8 years ago
|
||
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.
Comment 88•7 years ago
|
||
Based on comment 86, mark bug verified-fixed.
Status: RESOLVED → VERIFIED
Flags: qe-verify+ → qe-verify-
Updated•7 years ago
|
Flags: qe-verify-
You need to log in
before you can comment on or make changes to this bug.
Description
•