Use SVG graphics for the Lightning toolbar buttons

RESOLVED FIXED in 4.4

Status

Calendar
Lightning Only
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified

Details

Attachments

(4 attachments, 5 obsolete attachments)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8591334 [details] [diff] [review]
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
(Assignee)

Comment 2

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

Comment 3

3 years ago
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.

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

Comment 4

3 years ago
Created attachment 8591362 [details]
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

3 years ago
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?
(Assignee)

Comment 6

3 years ago
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.
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.
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.
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.
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> ?
(Assignee)

Comment 11

3 years ago
And is somewhere a example svg to look how it is done? I failed with my experiments to display the desired image.
(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.
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.
(Assignee)

Comment 14

3 years ago
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.
Attachment #8591644 - Flags: feedback?(longsonr)
Created attachment 8591690 [details]
without use elements
Attachment #8591690 - Attachment mime type: text/plain → image/svg+xml
https://bugzilla.mozilla.org/attachment.cgi?id=8591690#aa
https://bugzilla.mozilla.org/attachment.cgi?id=8591690#bb
(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.
Attachment #8591644 - Flags: feedback?(longsonr)
(Assignee)

Comment 18

3 years ago
Thanks a lot Robert! Now I need to look how I can apply the different colors for the platforms. Probably with classes.
(Assignee)

Comment 19

3 years ago
Created attachment 8594389 [details] [diff] [review]
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?
Attachment #8591362 - Attachment is obsolete: true
Attachment #8594389 - Flags: feedback?(longsonr)
Comment on attachment 8594389 [details] [diff] [review]
SVGforLightning.patch v2

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

Seems fine to me now.
Attachment #8594389 - Flags: feedback?(longsonr) → feedback+
(Assignee)

Comment 21

3 years ago
Created attachment 8595525 [details] [diff] [review]
SVGforLightning.patch v3

Patch using smaller icons.
Attachment #8594389 - Attachment is obsolete: true
(Assignee)

Updated

3 years ago
Attachment #8591334 - Attachment is obsolete: true
(Assignee)

Comment 22

3 years ago
Created attachment 8616773 [details] [diff] [review]
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.
Attachment #8595525 - Attachment is obsolete: true
Attachment #8616773 - Flags: review?
(Assignee)

Comment 23

3 years ago
Created attachment 8627010 [details] [diff] [review]
SVGforLightning.patch v5

Updated the toolbar icon color for Win10 to the color of bug 1166930.
Attachment #8616773 - Attachment is obsolete: true
Attachment #8616773 - Flags: review?
Attachment #8627010 - Flags: review?(philipp)
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 :-)
Attachment #8627010 - Flags: review?(philipp) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
Pushed to comm-central changeset 89e2a4ab8cd9
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 4.4
(Assignee)

Updated

2 years ago
Depends on: 1211643
You need to log in before you can comment on or make changes to this bug.