1153615 - Use SVG graphics for the Lightning toolbar buttons

Status: RESOLVED FIXED

(Calendar :: Lightning Only, defect)

Description

[Richard Marti (:Paenglab)]

Like bug 1150627 for TB is this bug for Lightning to use SVG graphics instead PNG graphics for Linux, OS X and Windows Vista+.

The plus side is a smaller XPI as the different png graphics aren't packe d and only one file with the SVG graphics is needed. Also are the icons inverted on Linux and OS X when using dark LW-themes or dark system themes.

The minus side could be that Linux is now using the same monochrome icons and no more colored ones. But Firefox has done the same and I heard no howl in the Linux community about this.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: SVGforLightning.patch

This is a full patch which changes the calendar- and task toolbars, the task header toolbar and also the imip buttons and the event dialog toolbar.

Additionally I began to remove the need of the special files for XP and Win Vista+ (win-classic and win-aero) through differing them with media queries in the normal files. I'll completely remove them after this bug in a follow-up bug.

Comment 2

[Richard Marti (:Paenglab)]

I'm waiting for review until I have a feedback from TB bug. But feedback to this bug is also welcome.

Comment 3

[[:MakeMyDay]]

Attachment: comparison-svg-png-winaero.png

I've given this a test on Win7 based try build from [1]. Compared to the current Aero icons, most of the icons look less well defined and a little bit noisy - see the attached screenshots.

Referencing Richard's try builds for 4.2:

[1] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=a113818e2f82 (Win + Linux)
[2] https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d378ce077386 (Mac)

Comment 4

[Richard Marti (:Paenglab)]

Attachment: SVG-icons.png

Screenshot with almost every icon.

(In reply to MakeMyDay from comment #3)
> Created attachment 8591341 [details]
> comparison-svg-png-winaero.png
> 
> I've given this a test on Win7 based try build from [1]. Compared to the
> current Aero icons, most of the icons look less well defined and a little
> bit noisy - see the attached screenshots.

This is because you haven't used the TB patch. It uses 18px icons. With 16px icons they are scaled and blurry.

Comment 5

[[:MakeMyDay]]

Sorry, I didn't realize that you have to apply this combined. On your screenshot, it looks good in general.

I noticed some other icons have slightly changed: delete (strike through is in different direction), add attendee (the head seems to be a little bigger compared to the shoulders), new event/task (the + is much bolder now). The last one is looks odd, but this is just my personal preference.

I assume you will consider the icons for the imip bar as well?

Comment 6

[Richard Marti (:Paenglab)]

Yes, they are a bit different. First it's now a mixture from OS X and Windows. And for the different dimension of details, I had to go to even dimensions to let it draw not blurry. On PNG we had the possibility to make some part sharp manually, but with SVG the image is calculated from the description and this needs to be observed at the beginning.

But for example the bigger + makes it also better remarkable.

Comment 7

[Robert Longson [:longsonr]]

Do you really have to use the <use> tag? Wouldn't just giving the <g> tags some class names do just as well. There's a lot of overhead in <use> tags tracking changes.

Comment 8

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Do class names work with anchor tags? I thought the <use> tags was needed so you can do chrome://calendar/skin/filename.svg#print-icon and have it show the print icon.

Comment 9

[Robert Longson [:longsonr]]

If you do it that way, yes. But you might be able to position things in different places and use an svg view fragment with a viewBox parameter to select each icon (http://www.w3.org/TR/SVG/linking.html#SVGFragmentIdentifiers)

At the moment use clones each item so your memory cost is going to be high, that's why you want to avoid it.

Comment 10

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Interesting. I've been using <use> because I saw it this way in Firefox, e.g. http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/loop/content/shared/img/icons-14x14.svg?raw=1

I kind of liked not having to specify dimensions like we did before with -moz-image-region. Is there a Core bug about the <use> memory usage and/or a Firefox bug to replace use of <use> ?

Comment 11

[Richard Marti (:Paenglab)]

And is somewhere a example svg to look how it is done? I failed with my experiments to display the desired image.

Comment 12

[Robert Longson [:longsonr]]

(In reply to Philipp Kewisch [:Fallen] from comment #10)
> Interesting. I've been using <use> because I saw it this way in Firefox,
> e.g.
> http://mxr.mozilla.org/comm-central/source/mozilla/browser/components/loop/
> content/shared/img/icons-14x14.svg?raw=1

oh dear.

> 
> I kind of liked not having to specify dimensions like we did before with
> -moz-image-region. Is there a Core bug about the <use> memory usage and/or a
> Firefox bug to replace use of <use> ?

Nothing ideal really. There's bug 1103397 and bug 964815 which are perf bugs where the content is mainly use elements.

Ideally we'd fix <use> not to clone.

Comment 13

[Robert Longson [:longsonr]]

image.svg#svgView(viewBox(0,0,100,100))
image.svg#svgView(viewBox(0,100,100,100))

etc.

draw the separate icons within each viewBox. You should use an <svg> element rather than a <g> element for each icon so you get overflow:hidden and each view doesn't spill over into the next.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: test.svg

I'm sorry, but I don't get it working. Please could you add the needed elements in this test file? The viewbox should be 18x18.

Comment 15

[Robert Longson [:longsonr]]

Attachment: without use elements

Comment 16

[Robert Longson [:longsonr]]

https://bugzilla.mozilla.org/attachment.cgi?id=8591690#aa
https://bugzilla.mozilla.org/attachment.cgi?id=8591690#bb

Comment 17

[Robert Longson [:longsonr]]

(In reply to Richard Marti (:Paenglab) from comment #14)
> Created attachment 8591644 [details]
> test.svg
> 
> I'm sorry, but I don't get it working. Please could you add the needed
> elements in this test file? The viewbox should be 18x18.

I couldn't get viewBox working either. I think that's a dead end, sorry. This may be what you need though, one file, two different displays.

Comment 18

[Richard Marti (:Paenglab)]

Thanks a lot Robert! Now I need to look how I can apply the different colors for the platforms. Probably with classes.

Comment 19

[Richard Marti (:Paenglab)]

Attachment: SVGforLightning.patch v2

Now using the proposal from Robert for the SVG. Needs the patch from bug 1150627 to apply and work cleanly.

Robert, is this now okay with the SVG file? Or do you know what could be improved?

Comment 20

[Robert Longson [:longsonr]]

Comment on attachment 8594389 [details] [diff] [review]
SVGforLightning.patch v2

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

Seems fine to me now.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: SVGforLightning.patch v3

Patch using smaller icons.

Comment 22

[Richard Marti (:Paenglab)]

Attachment: SVGforLightning.patch v4

Bug 1150627 has now c-i, and this patch is ready for reviewing. Made the images 16px (in a 18px area) and added Win10 support.

Comment 23

[Richard Marti (:Paenglab)]

Attachment: SVGforLightning.patch v5

Updated the toolbar icon color for Win10 to the color of bug 1166930.

Comment 24

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8627010 [details] [diff] [review]
SVGforLightning.patch v5

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

I like it, r=philipp. I haven't checked all the icons, but I assume they will look nice :-)

Comment 25

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Pushed to comm-central changeset 89e2a4ab8cd9