495478 - Buttons on summary page for collapsed threads don't have same height

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[[:Aureliano Buendía]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; it; rv:1.9.1b4) Gecko/20090423 Firefox/3.5b4 (.NET CLR 3.5.30729)
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090529 Lightning/1.0pre Shredder/3.0b3pre ID:20090529032206

Archive and Delete buttons not have the same height.

using

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090529
Lightning/1.0pre Shredder/3.0b3pre ID:20090529032206

attached file for details

Reproducible: Always

Comment 1

[[:Aureliano Buendía]]

Attachment: screenshot

Comment 2

[David Ascher (:davida)]

Bryan, giving to you as we were talking about it. We should at least figure out what we want those two buttons to look like on XP & vista.

Comment 3

[Bryan Clark (DevTools PM) [:clarkbw]]

Those are some ugly looking buttons.  I suppose it's the old XP theme?  We should at least match height with each button.

Comment 4

[Phil Ringnalda (:philor)]

It's not just Classic, it's Luna too, and it's not just the collapsed thread summary, it's the message reader too, since the Tango icons landed. I thought about filing on it then, then thought about claiming that it was just bug 456832 now hitting default font size too, then thought about how little fun I had with Lightning's similar issue, and then a bird flew by the window, and...

Comment 5

[Bryan Clark (DevTools PM) [:clarkbw]]

maybe blake can take a look at this

Comment 6

[Mark Banner (:standard8)]

This seems like needed polish blocking+ and initially setting at b3.

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

It would be great to get this for b3, but we wouldn't actually hold b3 for this.

Comment 8

[Blake Winton (:bwinton) (:☕️)]

Attachment: WIP patch that makes all the buttons the same height.

I haven't quite gotten the Reply/Reply-All/Reply-List buttons to line up with the rest, so I'm not going to ask for a review quite yet.

Comment 9

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first cut at the patch.

A few images will be coming up, if you wanted to wait to see what it looked like, instead of patching and building your own copy, Bryan.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: Some examples of the new similarly-heighted buttons.

Comment 11

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 390326 [details] [diff] [review]
The first cut at the patch.

The buttons on the multimessage view are now incorrectly sized on OSX, so let's hold off on this review until I get that working.

Comment 12

[Blake Winton (:bwinton) (:☕️)]

Attachment: The previous patch, with an OS X fix.

Okay, it's all lining up on OS X now, too.

I don't have a linux box/VM to test on, so if it looks wonky there, please let me know, and I'll try to set one up/ask someone for help.

Thanks,
Blake.

Comment 13

[Bryan Clark (DevTools PM) [:clarkbw]]

I'll try this on linux later tonight and get a review off

Comment 14

[David :Bienvenu]

Comment on attachment 390512 [details] [diff] [review]
The previous patch, with an OS X fix.

looks better on Windows.

Comment 15

[Jim Porter (:squib)]

Comment on attachment 390512 [details] [diff] [review]
The previous patch, with an OS X fix.

> .msgHeaderView-button > .button-box,
>-.msgHeaderView-button[type="menu-button"] > .button-menubutton-button {
>+.msgHeaderView-button[type="menu-button"] .button-box {
>   /* Needed to make the buttons the same height as the trash icon */
>-  min-height: 16px;
>+  min-height: 21px;
> }

Another way to do this that relies less on magic numbers would be as follows (which works in Windows, anyway):

.msgHeaderView-button > .button-box > .button-icon,
.msgHeaderView-button[type="menu-button"] .button-icon {
  /* Needed to make the buttons the same height as the trash icon */
  min-height: 16px;
}

Comment 16

[Jim Porter (:squib)]

Well... I suppose "16px" is still a magic number, but at least it's the same magic number as the one used for the dimensions of the trash icon! :)

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Attachment: A picture of the slightly-too-small button.

(In reply to comment #15)
> Another way to do this that relies less on magic numbers would be as follows
> (which works in Windows, anyway):
> .msgHeaderView-button > .button-box > .button-icon,
> .msgHeaderView-button[type="menu-button"] .button-icon {
>   /* Needed to make the buttons the same height as the trash icon */
>   min-height: 16px;
> }

I just tried that, but the button still seems a pixel too small.  So since 17 doesn't seem much better than 21, I think I'll leave the patch the way it is.  (Unless there's something else I can change.  :)

Thanks,
Blake.

Comment 18

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 390512 [details] [diff] [review]
The previous patch, with an OS X fix.

tried it on linux earlier and it looked fine.  also checked it on os x today and it looks good.

Comment 19

[Jim Porter (:squib)]

Attachment: Remove superstitious CSS

(In reply to comment #17)
> I just tried that, but the button still seems a pixel too small.  So since 17
> doesn't seem much better than 21, I think I'll leave the patch the way it is. 
> (Unless there's something else I can change.  :)

Now that I've looked more carefully, I see a one-pixel difference in the heights with your patch, too. Comparing them side-by-side, it appears that the Trash icon's height in the multi-message summary is one pixel taller than the one in the single-message header. The culprit seems to be some superstitious code in "mail/themes/qute/mail/multimessageview.css".

The attached patch seems to resolve it for me, though there are probably some improvements that could be made to it still, such as eliminating the .trashButton class from multimessageview.css and using .hdrTrashButton from messageHeader.css.

Comment 20

[Jim Porter (:squib)]

Attachment: Use messageHeader.css rules where applicable

This updated patch removes some of the duplicate CSS in <theme>/mail/multimessageview.css and defers instead to <theme>/mail/messageHeader.css which is included in multimessageview.xhtml anyway.

Note that I didn't touch the pinstripe theme since I don't have a Mac and have no idea what's going on there with some of the styling. It works in Windows and Linux (GTK at least) though.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 392626 [details] [diff] [review]
Use messageHeader.css rules where applicable

Looks good on OS X, too.

One nit, the patch to mail/themes/qute/mail/messageHeader.css didn't apply in my version of the source.  It looks like a line-ending thing.  We'll see if bienvenu or clarkbw run into it as well, or if it's something on my end.  :)

Thanks,
Blake.

Comment 22

[Jim Porter (:squib)]

Yeah, when I was editing (In reply to comment #21)
> One nit, the patch to mail/themes/qute/mail/messageHeader.css didn't apply in
> my version of the source.  It looks like a line-ending thing.  We'll see if
> bienvenu or clarkbw run into it as well, or if it's something on my end.  :)

Yeah, when editing that file, I noticed that it had Windows-style carriage returns even though none of the other Qute stuff did. I tried to maintain the line-ending scheme in it, but it's possible that the diff got angry about it.

Comment 23

[Bryan Clark (DevTools PM) [:clarkbw]]

Comment on attachment 392626 [details] [diff] [review]
Use messageHeader.css rules where applicable

didn't get any issues applying the patch and it looks good.  Thanks!!

Comment 24

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 390512 [details] [diff] [review]
The previous patch, with an OS X fix.

I like the newer patch more.  Thanks, Jim!

Comment 25

[Mark Banner (:standard8)]

Checked in: http://hg.mozilla.org/comm-central/rev/b9f3451243e9

Note: I had to fix the line endings in mail/themes/qute/mail/messageHeader.css for some reason they were a mixture of unix and non-unix.