1385846 - Fix our close buttons after landing of bug 1385702

Status: RESOLVED FIXED

(Thunderbird :: Theme, enhancement)

Description

[Richard Marti (:Paenglab)]

Bug 1385702 removes all close*.png images and uses a new close.svg.

We need to follow to use this icon instead of copying all the PMG files to ur repository. With the SVG we get automatically the correct stroke color - and it's looking better than the Win10 close icon.

Comment 1

[Richard Marti (:Paenglab)]

Attachment: closeButtons.patch

Bug 1385702 not landed yet. I created the patch to be able to land it directly after the M-C bug. I tested it on all platforms.

Comment 2

[Jorg K (CEST = GMT+2)]

So can this be tested now or do we need bug 1385702 to land first (or apply it's patch for testing)? We need close.svg, right?

What would happen when bug 1385702 lands and this hadn't landed yet?

Comment 3

[Richard Marti (:Paenglab)]

It needs the M-C patch applied first to work. It should work without the patch in the sense that the old icons are used but don't look correct on dark backgrounds (not tested).

Comment 4

[Jorg K (CEST = GMT+2)]

(In reply to Richard Marti (:Paenglab) from comment #3)
> It needs the M-C patch applied first to work.
Well, then the review isn't urgent. I'm following the other bug, so I can take a look when the other bug hits autoland.

Comment 5

[Richard Marti (:Paenglab)]

Bug 1385702 is on autoland. So this need review until tomorrow.

Comment 6

[Jorg K (CEST = GMT+2)]

I will review this after the M-C merge and land it. If Aceman finds a problem, we have to do a follow-up. There are more removals than anything else, so I trust it's working. Swiss quality ;-)

Comment 7

[:aceman]

Comment on attachment 8891972 [details] [diff] [review]
closeButtons.patch

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

OK, I see the new close icon is a bit larger now. Can the background on hover be made stronger? It is quite invisible now (try e.g. Monterail theme and hover an inactive tab with a folder). Or is that a toolkit problem now?

Comment 8

[Richard Marti (:Paenglab)]

It's tweakable, they use:

.close-icon:hover {
  fill-opacity: 0.1;
}

.close-icon:hover:active {
  fill-opacity: 0.2;
}

Comment 9

[Jorg K (CEST = GMT+2)]

Comment on attachment 8891972 [details] [diff] [review]
closeButtons.patch

Hmm, I'm not exactly sure which close buttons to look at, the ones closing a window, which are red on Win 10, or the ones closing a tab, which used to be circular and dark, now they're square (with rounded corners) and lighter.

I lot of changes are in Calendar, for Today Pane and "unifinder", I don't know what the latter one is.

For mail we have the tab close button (already mentioned) and the close button for the contacts sidebar, which used to be red, now it's grey like the others.

The conversation close button in IM is also grey now, and it looks wrong, it goes to far to the right.

I'm landing this now, please attach a follow-up.

I couldn't check the new message alert.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/13e8fa67e60a
Fix our close buttons after landing of bug 1385702. r=aceman,jorgk

Comment 11

[Richard Marti (:Paenglab)]

Attachment: Bug1385846followup.patch

This fixes the conversation close button.

Comment 12

[Jorg K (CEST = GMT+2)]

Before I look at this, can you please give me a comprehensive list of all the close buttons I need to look at. So far I have:

1) Today pane - OK.
2) unifinder, where, what?
3) Compose contacts side panel - OK.
4) Mail tabs - OK. 
5) New e-mail alert - Will check when I get new messages
6) Chat conversations on the left (fixed in follow up).

What else?

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Jorg K (GMT+2) from comment #12)
> Before I look at this, can you please give me a comprehensive list of all
> the close buttons I need to look at. So far I have:
> 
> 1) Today pane - OK.
> 2) unifinder, where, what?

In Calendar tab the searchbox above the event list. If it's hidden: menu Events and Tasks/Find Events.
The button is a bit too low but this is because he has the state checked="true" which should be addressed in a calendar bug.

> 3) Compose contacts side panel - OK.
> 4) Mail tabs - OK. 
> 5) New e-mail alert - Will check when I get new messages
> 6) Chat conversations on the left (fixed in follow up).
> 
> What else?

That's all I know.

Comment 14

[Jorg K (CEST = GMT+2)]

I've checked the close buttons on all items 1)-6) now, they all look good.

The in-content prefs have their own CSS, right? - mail/themes/shared/mail/incontentprefs/aboutPreferences.css

As for the chat conversations close: I think it needs to shift one pixel to the right. Now it has one above and below, and two the right. Or is that intentional?

Comment 15

[Richard Marti (:Paenglab)]

Attachment: Bug1385846followup.patch

I'm doing everything intentional ;) but this looks now better.

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8893829 [details] [diff] [review]
Bug1385846followup.patch

Now it looks good.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/50c68d9d4672
Follow-up for chat conversation close icon after M-C changed the icon size to 20px. r=jorgk