1232108 - Override the use of the toolkit close.png with its close.svg

Status: RESOLVED FIXED

(Thunderbird :: Theme, defect)

Description

[Richard Marti (:Paenglab)]

On Linux the close.png icon is now the one from Win8/10 and this looks ugly on Linux. Also because it's 20px * 20px.

This must be happen, when they simplified the jar.mn in toolkit.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: LinuxClose.patch

Aleth, I know you are normally not on Linux but there is also chat affected.

The override is a simple change. But in mail/components/im/themes/chat.css the ifdef UNIX_BUT_NOT_MAC doesn't work because this variable wasn't defined before. I removed this now to use the toolkit close icon because on my Kubuntu the GTK close icon looks really strange (totally not like a normal close button) and the toolkit close button fits better to the tab close button.

Is it okay to remove this or do you want I add the variable define and leave the GTK icon?

Comment 2

[aleth [:aleth]]

Could you add a before/after screenshot please? I'm trying to figure out if IB needs the same change or not.

Comment 3

[Richard Marti (:Paenglab)]

Attachment: Close button comparison

Comparison with before patch, after patch and how it would look with my GTK close button.

The only issue I found in IB could be this: https://dxr.mozilla.org/comm-central/source/im/themes/blist.css#487 The other are using the GTK icon. If you want I can add this override also to IB or add a GTK icon rule like the others have in IB.

Comment 4

[aleth [:aleth]]

Wow, something is very wrong with that GTK icon. Looks like a generic placeholder (missing icon). Is that a KDE issue? I thought KDE had a special oxygen-gtk theme for GTK apps. Or has there been a gecko regression/change here due to the GTK3 changes?

Comment 5

[Richard Marti (:Paenglab)]

Yes, it was a icon theme which doesn't fit well with Kubuntu. With default theme it looks like a normal close button.

Comment 6

[Richard Marti (:Paenglab)]

Attachment: LinuxClose.patch

I added on blist.css the GTK icon. IB is using eveerywhere the GTK close icon and it makes sense to use it also here.

Comment 7

[aleth [:aleth]]

Comment on attachment 8697785 [details] [diff] [review]
LinuxClose.patch

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

Thanks. I'll need to test this on Linux somehow.

What's your opinion on https://dxr.mozilla.org/comm-central/source/im/themes/blist.css#194 ?

For reference, could you link to the toolkit jar.mn change that broke this?

Comment 8

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #7)
> Comment on attachment 8697785 [details] [diff] [review]
> LinuxClose.patch
> 
> Review of attachment 8697785 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. I'll need to test this on Linux somehow.
> 
> What's your opinion on
> https://dxr.mozilla.org/comm-central/source/im/themes/blist.css#194 ?

This should be okay as the UNIX_BUT_NOT_MAC variable is set some lines above. I copied this code to #userIconPanelImageRemove to have the same logic.

> For reference, could you link to the toolkit jar.mn change that broke this?

This is https://hg.mozilla.org/mozilla-central/rev/98befa2ee036 from bug 1210703.

Comment 9

[Richard Marti (:Paenglab)]

Attachment: userIconPanelImageRemove before/after

I built IB on Linux. The image shows the close button before and after the patch.

Comment 10

[Richard Marti (:Paenglab)]

Aleth, gentle review ping.

Comment 11

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #10)
> Aleth, gentle review ping.

I've been meaning to find a way to try what this change (gtk -> toolkit) looks like when using Gnome or Unity (which is what most users will be on).

Comment 12

[aleth [:aleth]]

Could it be that the Linux equivalent of https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/icons/close.png is https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/icons/close.svg ? 

Looks like this change happened in 2014 in bug 879921. Not sure whether we should drop gtk close everywhere and use it or not. I suppose the advantage is it has hover/disabled states and is OS theme independent.

Comment 13

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #12)
> Looks like this change happened in 2014 in bug 879921. Not sure whether we
> should drop gtk close everywhere and use it or not. I suppose the advantage
> is it has hover/disabled states and is OS theme independent.

Ah, sorry, I missed the override in jar.mn, which is why I was so confused about what exactly your patch was proposing! 

Which is to use close.svg everywhere in TB. I think we should make the same change in IB and drop the gtk close to be consistent.

Comment 14

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #12)
> Could it be that the Linux equivalent of
> https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/osx/global/
> icons/close.png is
> https://mxr.mozilla.org/mozilla-central/source/toolkit/themes/linux/global/
> icons/close.svg ?

 Yes, this is the equivalent. TB uses it now for all close buttons (with this patch also in chat the correct icon). The close-icon class is a toolkit class which uses this icons.

You can compare TB with IB on Linux to see the difference, for example the tab close button.

Comment 15

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #13)
> Ah, sorry, I missed the override in jar.mn, which is why I was so confused
> about what exactly your patch was proposing! 
> 
> Which is to use close.svg everywhere in TB. I think we should make the same
> change in IB and drop the gtk close to be consistent.

Only drop now the gtk close in blist.css and in a new bug everywhere in IB?

Comment 16

[aleth [:aleth]]

Here's another reference to gtk-close in TB: https://dxr.mozilla.org/comm-central/source/mail/themes/linux/mail/messenger.css#282

I assume that has survived as there is a similar line in browser/?

Comment 17

[Richard Marti (:Paenglab)]

This is only a icon in the close menu item and makes sense to use the system wide symbol.

Comment 18

[aleth [:aleth]]

(In reply to Richard Marti (:Paenglab) from comment #15)
> > Which is to use close.svg everywhere in TB. I think we should make the same
> > change in IB and drop the gtk close to be consistent.
> 
> Only drop now the gtk close in blist.css and in a new bug everywhere in IB?

Yes, sounds good. (Outside blist.css I think the only other references are in tabbrowser.css for tabs/menus, not sure what TB does for those. Sadly IB still hasn't made the switch to Australis.)

Comment 19

[aleth [:aleth]]

(In reply to aleth [:aleth] from comment #11)
> I've been meaning to find a way to try what this change (gtk -> toolkit)
> looks like when using Gnome or Unity (which is what most users will be on).

Dropping gtk-close in IB's blist.css also has the advantage that this check becomes unnecessary, as your KDE experience will match everything Linux :-)

Comment 20

[Richard Marti (:Paenglab)]

(In reply to aleth [:aleth] from comment #18)
> Yes, sounds good. (Outside blist.css I think the only other references are
> in tabbrowser.css for tabs/menus, not sure what TB does for those. Sadly IB
> still hasn't made the switch to Australis.)

TB uses the .close-icon. Especially on Win8/10 this icon would look weird in IB with the actual tabs as it's 20px and grey only.

Comment 21

[Richard Marti (:Paenglab)]

Attachment: LinuxClose.patch v2

Now blist.css does no more use the gtk close icon. Built IB on Linux.

Comment 22

[aleth [:aleth]]

Comment on attachment 8708044 [details] [diff] [review]
LinuxClose.patch v2

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

Great, thanks! Sorry for the delay.

Comment 23

[aleth [:aleth]]

https://hg.mozilla.org/comm-central/rev/d12a69c2456b09b3740d6bb428d003d62ca90489
Bug 1232108 - Override toolkit's close.png to close.svg. r=aleth

Comment 24

[Jorg K (CEST = GMT+2)]

Comment on attachment 8708044 [details] [diff] [review]
LinuxClose.patch v2

Hmm, I'm approving this, but usually the person requesting uplift needs to fill in a little questionnaire ;-)

Comment 25

[Jorg K (CEST = GMT+2)]

Aurora:
https://hg.mozilla.org/releases/comm-aurora/rev/f302f6e8a0b2