Closed Bug 1232108 Opened 4 years ago Closed 4 years ago

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

Categories

(Thunderbird :: Theme, defect)

All
Linux
defect
Not set

Tracking

(thunderbird45 fixed, thunderbird46 fixed)

RESOLVED FIXED
Thunderbird 46.0
Tracking Status
thunderbird45 --- fixed
thunderbird46 --- fixed

People

(Reporter: Paenglab, Assigned: Paenglab)

Details

Attachments

(3 files, 2 obsolete files)

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.
Attached patch LinuxClose.patch (obsolete) — Splinter Review
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?
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8697772 - Flags: review?(aleth)
Could you add a before/after screenshot please? I'm trying to figure out if IB needs the same change or not.
Attached image 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.
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?
Yes, it was a icon theme which doesn't fit well with Kubuntu. With default theme it looks like a normal close button.
Attached patch LinuxClose.patch (obsolete) — Splinter Review
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.
Attachment #8697772 - Attachment is obsolete: true
Attachment #8697772 - Flags: review?(aleth)
Attachment #8697785 - Flags: review?(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?
(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.
I built IB on Linux. The image shows the close button before and after the patch.
Aleth, gentle review ping.
(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).
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.
(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.
(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.
(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?
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/?
This is only a icon in the close menu item and makes sense to use the system wide symbol.
(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.)
(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 :-)
(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.
Now blist.css does no more use the gtk close icon. Built IB on Linux.
Attachment #8697785 - Attachment is obsolete: true
Attachment #8697785 - Flags: review?(aleth)
Attachment #8708044 - Flags: review?(aleth)
Comment on attachment 8708044 [details] [diff] [review]
LinuxClose.patch v2

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

Great, thanks! Sorry for the delay.
Attachment #8708044 - Flags: review?(aleth) → review+
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
Attachment #8708044 - Flags: approval-comm-aurora?
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 ;-)
Attachment #8708044 - Flags: approval-comm-aurora? → approval-comm-aurora+
You need to log in before you can comment on or make changes to this bug.