641806 - Only half of the icons in the message header on OS X are glyphs

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Andreas Nilsson (:andreasn)]

Reply, Reply All and Forward are glyphs, while Archive, Junk and Delete are full color icons.

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: new icons (not a patch)

My qseries is behaving oddish on the Mac, so want to make sure I get the graphics too :)

Comment 2

[Andreas Nilsson (:andreasn)]

Attachment: patch with the icons

Lets see if we get any binaries with this :)

Comment 3

[Andreas Nilsson (:andreasn)]

Attachment: and this time with the binary attached

No idea why this happens really, but now it should work :)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 519393 [details] [diff] [review]
and this time with the binary attached

>+++ b/mail/themes/pinstripe/jar.mn
>@@ -221,16 +221,17 @@ classic.jar:
>   skin/classic/messenger/icons/symbolic-reply.png                (mail/icons/symbolic-reply.png)
>   skin/classic/messenger/icons/symbolic-replyall.png             (mail/icons/symbolic-replyall.png)
>   skin/classic/messenger/icons/symbolic-replylist.png            (mail/icons/symbolic-replylist.png)

Are we still using these files, or can we remove them?

> .hdrReplyToSenderButton {
>   /* large icons are only needed for customization dialog, so use here also small icons */
>-  list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");
>+  list-style-image: url("chrome://messenger/skin/icons/message-header-toolbar.png");
>+  -moz-image-region: rect(0px, 16px, 16px, 0px);
>+  /*list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");*/
> }
> 
> toolbar[iconsize="small"] .hdrReplyToSenderButton {
>-  list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");
>+  /*list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");*/
>+  list-style-image: url("chrome://messenger/skin/icons/message-header-toolbar.png");
>+  -moz-image-region: rect(0px, 16px, 16px, 0px);
> }

If those two blocks are the same, then why are we bothering to specify the second one?

>+++ b/mail/themes/pinstripe/mail/messageHeader.css
>@@ -330,48 +330,65 @@ toolbar[mode="text"] .msgHeaderView-butt
> .hdrReplyToSenderButton {
>   /* large icons are only needed for customization dialog, so use here also small icons */
>-  list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");
>+  list-style-image: url("chrome://messenger/skin/icons/message-header-toolbar.png");
>+  -moz-image-region: rect(0px, 16px, 16px, 0px);
>+  /*list-style-image: url("chrome://messenger/skin/icons/symbolic-reply.png");*/

Don't comment these lines out, just remove them.  If we want them back, that's what hg diff is for.  ;)

> .hdrReplyListButton {
> toolbar[iconsize="small"] .hdrReplyListButton {
> .hdrForwardButton {
> toolbar[iconsize="small"] .hdrForwardButton {
> .hdrArchiveButton {
> toolbar[iconsize="small"] .hdrArchiveButton {
[…]
>+  /*list-style-image: url("chrome://messenger/skin/icons/symbolic-replylist.png");*/
>+    list-style-image: url("chrome://messenger/skin/icons/message-header-toolbar.png");

For these styles, this line is too far indented.

> /* XXX taken from gnomestripe, has to be checked */
> #header-view-toolbar toolbarpaletteitem toolbarseparator,
> #header-view-toolbar toolbarseparator {

I checked it, and it looks okay to me.

Finally, the Reply icon seems to be the same as the ReplyAll icon, as you can see in http://dl.dropbox.com/u/2301433/Screenshots/ReplyButtons.png

So, I think I want to see the next version of this patch, with these things fixed, so I'm going to call this one r-.

(But I really do like the way it looks, so I'm leaving the ui-r? for Bryan, in the hopes that you'll be able to carry it forward if he likes it as much as I do.)

Thanks,
Blake.

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 519393 [details] [diff] [review]
and this time with the binary attached

no comments beyond blake's other than I like the overall direction.  very simple and clean icons.

Comment 6

[Andreas Nilsson (:andreasn)]

Attachment: new try (lets see if I get any binaries now)

Comment 7

[Andreas Nilsson (:andreasn)]

Comment on attachment 521162 [details] [diff] [review]
new try (lets see if I get any binaries now)

All remarks should be fixed now. Let me know if I missed something.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 521162 [details] [diff] [review]
new try (lets see if I get any binaries now)

>+++ b/mail/themes/pinstripe/mail/messageHeader.css
>@@ -415,20 +397,14 @@
> /* XXX taken from gnomestripe, has to be checked */

You could remove this comment.  ;)

But other than that, I like it!  r=me!

Later,
Blake.

Comment 9

[Andreas Nilsson (:andreasn)]

Attachment: addressing last comment in the review as well

Ok, this removes the line:
/* XXX taken from gnomestripe, has to be checked */

too.

Comment 10

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/ce2e0ddd206c