1404697 - Use context-fill and context-fill-opacity for the toolbar icons

Status: RESOLVED FIXED

(Calendar :: General, enhancement)

Description

[Richard Marti (:Paenglab)]

Until now we use SVG icons with platform dependent filling and need a special set for old OSX versions. We also need special icons for the inverted set.

With this patch we use the button text color to fill the icons and use for every icon his own file like FX is doing and had a performance win with this.

With using a file per icon it's also easier to update the files when we use ntim's Photon icons.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: CalendarIcons.patch

Philipp, MakeMyDay, I put both in the r? as I don't know how your time is for the review.

With this patch we use the button text color to fill the icons and use for every icon his own file like FX is doing and had a performance win with this. I'm still using the original icons except the print and attach icons I'm using from FX Photon set.

To test this patch you need bug 1404696 applied first.

Comment 2

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

Comment on attachment 8914051 [details] [diff] [review]
CalendarIcons.patch

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

Thanks for the patch! I thought putting all icons in one file was a performance win as well? If this makes a future update easier I'm ok with it though.

Comment 3

[Richard Marti (:Paenglab)]

I thought it too. But FX isn't doing it this way and they made a lot of investigations speed wise.

Comment 4

[[:MakeMyDay]]

Having more small files will degrade the startup performance if the profile is located on a network device or slow hard disks, but since the binary component has moved to omni.ja, we could change to a compressed extension if that impact is too bad. Iirc, you already filed a bug for that anyway, right?

Comment 5

[Richard Marti (:Paenglab)]

Filed bug 1406499 for comment 4.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/d20464aedc17
Calendar: Use context-fill and context-fill-opacity for the toolbar icons. r=philipp

Comment 7

[Jorg K (CEST = GMT+2)]

Backout:
https://hg.mozilla.org/comm-central/rev/ef5549e9a1ea35ecccc28f2bdb78f026d9cef818

Comment 8

[Richard Marti (:Paenglab)]

Attachment: CalendarIcons.patch

Sorry, forgot the allowed-dupes.

Added the needed files.
Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=692146bd51379b178648cb1dc433ee093e0796ca

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8916223 [details] [diff] [review]
CalendarIcons.patch

Since the only file changed here is allowed-dupes.mn, I'm happy to do the review here.

Comment 10

[Jorg K (CEST = GMT+2)]

I'm looking at the new duplicates here. Note that the file says:
PLEASE DO NOT ADD MORE EXCEPTIONS TO THIS LIST

You added address.svg, attach.svg, delete.svg, print.svg, save.svg, security.svg.
I guess Calendar uses identical files.

But why spelling.svg and tag.svg in mail and category.svg in Calendar?

I assume there in not way to share lightning-toolbar.css and calendar-event-dialog.css?

Also, at the end of the duplicate file I think you forgot to prepend "distribution/":
https://bugzilla.mozilla.org/attachment.cgi?oldid=8914051&action=interdiff&newid=8916223&headers=1

Comment 11

[Richard Marti (:Paenglab)]

Attachment: calendarIcons.patch

(In reply to Jorg K (GMT+2) from comment #10)
> I'm looking at the new duplicates here. Note that the file says:
> PLEASE DO NOT ADD MORE EXCEPTIONS TO THIS LIST
> 
> You added address.svg, attach.svg, delete.svg, print.svg, save.svg,
> security.svg.
> I guess Calendar uses identical files.

I could use the TB icons but they are needed to add to Lightning because SM (or 3dr party themes) doesn't have this icons and so the icons need to be shipped with the extension.

> But why spelling.svg and tag.svg in mail and category.svg in Calendar?

It's the same icon. But the different name shows better for what they are used.

> I assume there in not way to share lightning-toolbar.css and
> calendar-event-dialog.css?

There are already common files but the rules in this files differ only in the third platform.
I reordered the rules in calendar-event-dialog.css to be not dupes. For lightning-toolbar.css I cant do this as they are almost empty and there is nothing to shuffle.

> Also, at the end of the duplicate file I think you forgot to prepend
> "distribution/":
> https://bugzilla.mozilla.org/attachment.
> cgi?oldid=8914051&action=interdiff&newid=8916223&headers=1

Fixed

Comment 12

[Richard Marti (:Paenglab)]

Attachment: calendarIcons.patch

Sorry for the bug spam. This is the correct patch.

Comment 13

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #11)
> I reordered the rules in calendar-event-dialog.css to be not dupes. For
> lightning-toolbar.css I cant do this as they are almost empty and there is
> nothing to shuffle.
Sorry to be picky here but I don't like that hack. Either
- make the files 100% equal and list them in the duplicates or
- put a comment to make them unequal, like "Mac version", or something.

Comment 14

[Richard Marti (:Paenglab)]

Comment on attachment 8916241 [details] [diff] [review]
calendarIcons.patch

Okay, patch without shuffling.

Comment 15

[Jorg K (CEST = GMT+2)]

Comment on attachment 8916241 [details] [diff] [review]
calendarIcons.patch

Thanks for the patience. We've also have a try run for this, fixing the missing "distribution/" from the tried version doesn't make a difference.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/611f64379d91
Calendar: Use context-fill and context-fill-opacity for the toolbar icons. r=philipp,jorgk