Closed Bug 1385846 Opened 3 years ago Closed 3 years ago

Fix our close buttons after landing of bug 1385702

Categories

(Thunderbird :: Theme, enhancement)

enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 57.0

People

(Reporter: Paenglab, Assigned: Paenglab)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
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.
Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #8891972 - Flags: review?(jorgk)
Attachment #8891972 - Flags: review?(acelists)
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?
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).
(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.
Bug 1385702 is on autoland. So this need review until tomorrow.
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 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?
Attachment #8891972 - Flags: review?(acelists) → review+
It's tweakable, they use:

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

.close-icon:hover:active {
  fill-opacity: 0.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.
Attachment #8891972 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/13e8fa67e60a
Fix our close buttons after landing of bug 1385702. r=aceman,jorgk
Attached patch Bug1385846followup.patch (obsolete) — Splinter Review
This fixes the conversation close button.
Attachment #8893777 - Flags: review?(jorgk)
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?
(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.
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?
Keywords: leave-open
I'm doing everything intentional ;) but this looks now better.
Attachment #8893777 - Attachment is obsolete: true
Attachment #8893777 - Flags: review?(jorgk)
Attachment #8893829 - Flags: review?(jorgk)
Comment on attachment 8893829 [details] [diff] [review]
Bug1385846followup.patch

Now it looks good.
Attachment #8893829 - Flags: review?(jorgk) → review+
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 57.0
You need to log in before you can comment on or make changes to this bug.