519688 - (Message header) When option "Show icons and text" is ON, "Archive" and "Junk" buttons are too big of other buttons

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect, P3)

Description

[[:Aureliano Buendía]]

Attachment: screenshot

After landing of bug #465138 there is a regression in message header pane: "archive" and "junk" buttons don't have icon and are too big of other buttons.

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.4pre) Gecko/20090930 Lightning/1.0pre Shredder/3.0pre ID:20090930031731

Comment 1

[Ludovic Hirlimann [:Usul]]

seems to be windows only.

Comment 2

[rsx11m]

They don't have an icon in the text+icon mode, I think this was intentional.
Switching to icon-only mode shows all icons for me with the same build.

Comment 3

[Mark Banner (:standard8)]

(In reply to comment #0)
> Created an attachment (id=403757) [details]
> screenshot
> 
> After landing of bug #465138 there is a regression in message header pane:
> "archive" and "junk" buttons don't have icon and are too big of other buttons.

It was bug 465138 that implemented this. Bryan intentionally didn't have icons
on those buttons - I can't see a comment on that bug, but I know there was
definitely one on irc.

Comment 4

[[:Aureliano Buendía]]

Confirmed comment #2: changed title to reflect this.

Comment 5

[[:Aureliano Buendía]]

Sorry for the spam: changed title to reflect comment #3

>Bryan intentionally didn't have icons on those buttons

Comment 6

[Andreas Nilsson (:andreasn)]

on gnomestripe with text set smaller than default, I see the exact opposite of this issue, ie. junk and archive are smaller than the others.

Comment 7

[rsx11m]

To add some more observations to the mix (WinXP SP2, Windows Classic theme):

 - all buttons have same height for me, default font size, non-smoothed fonts;
 - for text-only, the text is aligned in the center of the buttons;
 - for text+icon, the text is aligned to the bottom, icon is centered left;
 - no issues with icon-only display, all same height and width, well aligned.

Another - possibly related - observation: When hovering over the "other action" menu, and the border appears, the header pane shrinks vertically by about 2px, dragging the message text 2px upwards. Leaving the hover area returns it to the previous alignment (three-row default view, not with additional headers).

Thus, looks like a few alignment issues are still left to be polished.

Comment 8

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

Definitely want to get this fixed

Comment 9

[Jim Porter (:squib)]

Attachment: Possible fix

It looks like the issue from bug 495478 has, more or less, reared its ugly head again. This patch fixes the issue on Linux, and I mirrored the change over to Windows since that solved it last time. I haven't tested the Windows part though. I also didn't touch the Mac stuff since I don't know if there's a problem with it or if there is, how I'd fix it.

This also resolves the same basic issue with the multi-message summary.

Comment 10

[Jim Porter (:squib)]

Attachment: Fix for Windows/Linux

This should work for Windows and Linux now. I manually patched the files after downloading binaries instead of building from source though, so there's always the chance that I made a mistake somewhere.

Stuff this fixes:
* All buttons are the right height in Windows
* With tiny fonts in Linux, all buttons are the right height
* Text on archive/junk buttons is in line with other text in full mode
* Button padding in multi-message summary is the same as the message header in Windows
* min-width rule fixed for message header buttons (was being overridden by generic toolbar declaration); has the benefit of shrinking the buttons in full mode a bit

Comment 11

[Andreas Nilsson (:andreasn)]

Jim: do you want UI and code review on this one?
Looks good on Linux for me, trying on Windows next.

Comment 12

[Andreas Nilsson (:andreasn)]

The buttons on Vista still looked odd, but that could be the cache problems playing tricks on me, that have happened before. Anyone else who have a Windows system to test this on?

Comment 13

[Jim Porter (:squib)]

(In reply to comment #11)
> Jim: do you want UI and code review on this one?
> Looks good on Linux for me, trying on Windows next.

Oh yeah, I knew I forgot something! :)

Comment 14

[Andreas Nilsson (:andreasn)]

Attachment: modified patch to make qute buttons have the correct height

I managed to get rid of the tall buttons I noticed in #12 by removing line 316 and 317 in qute's messageHeder.css, but the comment above seems to suggest this is needed to not make the buttons too slim.
Perhaps the lines:

+.msgHeaderView-button > .toolbarbutton-icon,
+.msgHeaderView-button[type="menu-button"] > .toolbarbutton-menubutton-button
+> .toolbarbutton-icon {
+  /* Needed to make the buttons w/o icons the same height as those with icons */
   min-height: 16px;


takes care of this instead?

Comment 15

[Jim Porter (:squib)]

(In reply to comment #14)
> Created an attachment (id=404832) [details]
> modified patch to make qute buttons have the correct height

The stuff you deleted probably wasn't necessary after the other changes I made, but it looked right with it on my end without those changes (in fact, I couldn't reproduce the exact bug you had at all). I think it's best to remove that bit, assuming all looks well without it.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 404832 [details] [diff] [review]
modified patch to make qute buttons have the correct height

Tested and reviewed on both Linux and Windows XP; looks good.  Thanks for the patch, guys!  Note to anyone who wants to test, in order to see the bugs that this fixes, the system fonts must be set to a non-standard size.

Comment 18

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

Comment on attachment 404832 [details] [diff] [review]
modified patch to make qute buttons have the correct height

looks good on Vista, will try it out on Linux in a bit but from looking at the patch I expect it will work well.

Comment 19

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

setting checkin-needed as this is blocking

Comment 20

[Mark Banner (:standard8)]

Checked in:
http://hg.mozilla.org/comm-central/rev/3c5a73356db0
http://hg.mozilla.org/releases/comm-1.9.1/rev/4c2c83edcce4