The default bug view has changed. See this FAQ.

Use SVG graphics for the toolbar buttons in main window

RESOLVED FIXED in Thunderbird 41.0

Status

Thunderbird
Theme
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Paenglab, Assigned: Paenglab)

Tracking

unspecified
Thunderbird 41.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 7 obsolete attachments)

(Assignee)

Description

2 years ago
With using svg graphics instead of png should make them DPI independent. Also the size is smaller than png.

I'm trying to make a set of graphics which we use for all platforms (Windows Aero, OS X and Linux. But not for XP as I think we should leave this dead system as it is and only fix bugs).
(Assignee)

Comment 1

2 years ago
Created attachment 8587542 [details] [diff] [review]
useSVGonMainToolbars.patch

This patch is now only for Aero and OS X. Linux comes when the feedback is positive.

The positive of using a shared set is OS X (and later Linux) becomes inverted icons for dark LW themes.

I implemented the colors for every platform differently: Win7, Win8+, OS X up to 9, Yosemite and Linux (Windows non-default themes are using the same colors as Linux). And naturally the inverted theme.

A known issue is the print button icon isn't shown in customize window because the composer window isn't transformed to svg.

Josiah, what do you think to this approach?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8587542 - Flags: feedback?(josiah)
For the record, there are twice as many people on Windows XP as on all versions of Mac and Linux combined.
Dead OS or not, they're still 19% of our users, and our second biggest platform…
(Wait, that was for Firefox.  But I strongly suspect Thunderbird's numbers are similar.)
(Assignee)

Comment 4

2 years ago
But they are resistant to or don't like changes (as we see they don't change to a living platform). So it's better to leave it as it is. XP has already a different graphics set because of this.
(Assignee)

Comment 5

2 years ago
Created attachment 8588309 [details] [diff] [review]
useSVGonMainToolbars.patch

This is now a full patch for main window, composer and address book for Linux, OS X and Windows Vista+.

For the graphics I used the colors FX is using for the different platforms.
Attachment #8587542 - Attachment is obsolete: true
Attachment #8587542 - Flags: feedback?(josiah)
Attachment #8588309 - Flags: ui-review?(josiah)
Attachment #8588309 - Flags: review?(josiah)
(Assignee)

Comment 6

2 years ago
Created attachment 8588459 [details] [diff] [review]
useSVGonMainToolbars.patch

Found a wrong class name on two definitions.
Attachment #8588309 - Attachment is obsolete: true
Attachment #8588309 - Flags: ui-review?(josiah)
Attachment #8588309 - Flags: review?(josiah)
Attachment #8588459 - Flags: ui-review?(josiah)
Attachment #8588459 - Flags: review?(josiah)
(Assignee)

Comment 7

2 years ago
Created attachment 8588676 [details] [diff] [review]
useSVGonToolbars.patch

Found a typo. This should be now really the final patch.
Attachment #8588459 - Attachment is obsolete: true
Attachment #8588459 - Flags: ui-review?(josiah)
Attachment #8588459 - Flags: review?(josiah)
Attachment #8588676 - Flags: ui-review?(josiah)
Attachment #8588676 - Flags: review?(josiah)
Comment on attachment 8588676 [details] [diff] [review]
useSVGonToolbars.patch

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

I've only looked at the OS X parts so far, but I decided I would release comments in waves.

In addition to the comments below, I have some "big picture" comments on the concept.

1. Definitely a little more cartoon-y. Maybe not a bad thing, but doesn't appear as consistent as Firefox.
2. The Send icon appears from a distance to have blurry edges, but it really doesn't. An illusion is created probably due to the narrow cutouts in the icon. Maybe extend them?
3. Really concerned about performance here. Although the PNGs are smaller overall, they take a LOT more effort to render, especially on low-spec machines. I know this makes a lot of things easier for us and theme developers, but this problem does concern me.
4. The icons seem larger than they use to be. Maybe they're just a little bolder, not sure. Regardless, I do think they look slightly less slick than they use to.

All that said, I'm very impressed with this patch, and the icons were done very well!

::: mail/themes/linux/mail/addrbook/addressbook.css
@@ +16,5 @@
>    background-repeat: no-repeat;
>    background-position: top right;
>  }
>  
> +#ab-toolbox > toolbar {

This file doesn't apply cleanly for some reason. In fact, quite severely. Those #addressbookWindow:-moz-lwtheme rules don't even exist on tip.

::: mail/themes/osx/mail/primaryToolbar.css
@@ +809,2 @@
>  #button-chat[unreadMessages="true"] {
>    -moz-image-region: rect(36px, 396px, 54px, 378px);

You need to remove this to get the unread svg icon to show.

::: mail/themes/osx/mail/quickFilterBar.css
@@ +10,1 @@
>    #qfb-show-filter-bar {

Hmm, I think we should move these toolbar button styles to primaryToolbar.css. It's kind of confusing that they're here.
Oh, also, the forward button doesn't seem to actually disable itself with this patch. It appears disabled, but I can still select the button and obtain the dropdown.
(Assignee)

Comment 10

2 years ago
(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #8)
> Comment on attachment 8588676 [details] [diff] [review]
> useSVGonToolbars.patch
> 
> Review of attachment 8588676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I've only looked at the OS X parts so far, but I decided I would release
> comments in waves.
> 
> In addition to the comments below, I have some "big picture" comments on the
> concept.
> 
> 1. Definitely a little more cartoon-y. Maybe not a bad thing, but doesn't
> appear as consistent as Firefox.

They are based on the icons we are using now and it should be no big difference in appearance. On Yosemite the difference is I use now only one color like FX instead of gradients. Maybe this makes it looking cartoonish. With OS X <10.10 you should still see the gradients.

> 2. The Send icon appears from a distance to have blurry edges, but it really
> doesn't. An illusion is created probably due to the narrow cutouts in the
> icon. Maybe extend them?

I can try it, but I need to look at LoDPI to make them not blurry using half pixels. On HiDPI this would be calculated to even pixels and then looking better.

> 3. Really concerned about performance here. Although the PNGs are smaller
> overall, they take a LOT more effort to render, especially on low-spec
> machines. I know this makes a lot of things easier for us and theme
> developers, but this problem does concern me.

Yes, Robert Longson pointed this out in bug 1153615 and I'm now on implementing his proposals.

> 4. The icons seem larger than they use to be. Maybe they're just a little
> bolder, not sure. Regardless, I do think they look slightly less slick than
> they use to.

This could be again because of the LoDPI needs.

> All that said, I'm very impressed with this patch, and the icons were done
> very well!
> 
> ::: mail/themes/linux/mail/addrbook/addressbook.css
> @@ +16,5 @@
> >    background-repeat: no-repeat;
> >    background-position: top right;
> >  }
> >  
> > +#ab-toolbox > toolbar {
> 
> This file doesn't apply cleanly for some reason. In fact, quite severely.
> Those #addressbookWindow:-moz-lwtheme rules don't even exist on tip.

This is probably because I haven't updated the patch after the check-in of bug 1145974.
I'll update the patch after I made the changes Robert Longson proposed.
(Assignee)

Comment 11

2 years ago
Created attachment 8594387 [details] [diff] [review]
useSVGonToolbars.patch v2

Now with the new approach Robert Longson proposed which should use less memory.

I changed also the icon for "Get Mail" to the Windows version. The one from OS X war too big.
I also changed the "Write Mail" icon to a simpler one (but not so simple as Windows uses). What do you think about it?

(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #8)
> Comment on attachment 8588676 [details] [diff] [review]
> useSVGonToolbars.patch
> 
> Review of attachment 8588676 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> 2. The Send icon appears from a distance to have blurry edges, but it really
> doesn't. An illusion is created probably due to the narrow cutouts in the
> icon. Maybe extend them?

I made the cutouts deeper. Is this better?

> 3. Really concerned about performance here. Although the PNGs are smaller
> overall, they take a LOT more effort to render, especially on low-spec
> machines. I know this makes a lot of things easier for us and theme
> developers, but this problem does concern me.

SVG rendering was optimized for the Australis tabs with caching etc. so I think this should be no problem. 

> ::: mail/themes/osx/mail/primaryToolbar.css
> @@ +809,2 @@
> >  #button-chat[unreadMessages="true"] {
> >    -moz-image-region: rect(36px, 396px, 54px, 378px);
> 
> You need to remove this to get the unread svg icon to show.

Done

> ::: mail/themes/osx/mail/quickFilterBar.css
> @@ +10,1 @@
> >    #qfb-show-filter-bar {
> 
> Hmm, I think we should move these toolbar button styles to
> primaryToolbar.css. It's kind of confusing that they're here.

Yes, this makes sense -> done.
Attachment #8588676 - Attachment is obsolete: true
Attachment #8588676 - Flags: ui-review?(josiah)
Attachment #8588676 - Flags: review?(josiah)
Attachment #8594387 - Flags: ui-review?(josiah)
Attachment #8594387 - Flags: review?(josiah)
(Assignee)

Comment 12

2 years ago
(In reply to Josiah Bruner [:JosiahOne] (needinfo! - Won't respond to CC)) from comment #9)
> Oh, also, the forward button doesn't seem to actually disable itself with
> this patch. It appears disabled, but I can still select the button and
> obtain the dropdown.

This wasn't from this patch, it happens also without. But I fixed it with this patch.
(Assignee)

Comment 13

2 years ago
Created attachment 8595523 [details] [diff] [review]
useSVGonToolbars.patch v3

This patch uses now smaller icons like the original ones from png.
Attachment #8594387 - Attachment is obsolete: true
Attachment #8594387 - Flags: ui-review?(josiah)
Attachment #8594387 - Flags: review?(josiah)
Attachment #8595523 - Flags: ui-review?(josiah)
Attachment #8595523 - Flags: review?(josiah)
(Assignee)

Comment 14

2 years ago
Created attachment 8596147 [details] [diff] [review]
useSVGonToolbars.patch v4

Unbitrot after bug 1153553 landing.
Attachment #8595523 - Attachment is obsolete: true
Attachment #8595523 - Flags: ui-review?(josiah)
Attachment #8595523 - Flags: review?(josiah)
Attachment #8596147 - Flags: ui-review?(josiah)
Attachment #8596147 - Flags: review?(josiah)
Duplicate of this bug: 1099290
Blocks: 1099290
Comment on attachment 8596147 [details] [diff] [review]
useSVGonToolbars.patch v4

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

Assuming you tested this thoroughly on Linux and Windows, looks pretty good to me. Only thing I noticed is that the AB icon doesn't switch to white in brighttext mode.

r+ and ui-r+ with that fixed.
Attachment #8596147 - Flags: ui-review?(josiah)
Attachment #8596147 - Flags: ui-review+
Attachment #8596147 - Flags: review?(josiah)
Attachment #8596147 - Flags: review+
(Assignee)

Comment 17

2 years ago
Created attachment 8616732 [details] [diff] [review]
useSVGonToolbars.patch

Fixed the inverted AB icon on OS X. It was a typo.

I've added also Win10 colors to the buttons. This can change as FX hasn't decided which color is used. I'm using now the original Microsoft Edge button colors (#616161).
Attachment #8596147 - Attachment is obsolete: true
Attachment #8616732 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 18

2 years ago
https://hg.mozilla.org/comm-central/rev/ab0a4209f0b5 -> FIXED
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 41.0
You need to log in before you can comment on or make changes to this bug.