562608 - Thread summary buttons are way too big

Status: RESOLVED FIXED

(Thunderbird :: Message Reader UI, defect)

Description

[Jonathan Protzenko [:protz]]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.3a4) Gecko/20100413 Minefield/3.7a4
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.2.5pre) Gecko/20100429 Lanikai/3.1b2

See attached screenshot. The buttons for the multimessageview are too big by default. This did not happen with 3.0.x. Maybe someone changed the icons?

Reproducible: Always

Comment 1

[Jonathan Protzenko [:protz]]

Attachment: Screenshot of the issue.

Comment 2

[Andreas Nilsson (:andreasn)]

these buttons didn't have icons in 3.0.x, so that's why they grew a bit. Should be a easy fix to specify them to be 16x16.

Comment 3

[Jonathan Protzenko [:protz]]

I'm pretty sure they had. It probably depends on the Gnome system theme (I'm using Linux). I'm using tango currently.

Comment 4

[Jonathan Protzenko [:protz]]

Andreas, I just fired up a 3.0.x build of Thunderbird (just to make sure). Actually, only the "delete" button has an icon, the "archive" button doesn't have any. The icon is the right size in 3.0.x.

I'm using the Tango icon theme, and the DOM Inspector tells me that chrome://messenger/skin/messageHeader.css:305 has a rule for .hdrTrashButton that says:

list-style-image: url(moz-icon://stock/gtk-delete?size=menu);

(In case any of this information is useful to you :)).

Comment 5

[Andreas Nilsson (:andreasn)]

The internet connection at my hotel is horrible, so I'm pushing the ETA on this a bit until I have access to the source code.

Comment 6

[Andreas Nilsson (:andreasn)]

Figured out how to trigger this. The gconf key buttons_have_icons must be set to true. It's false by default in GNOME and it's distros, so this won't show up in most cases.

I think what's happens is that it picks up the regular toolbar icon size (24x24) instead of [iconsize="small"] that would result in 16x16 in the css. Maybe that needs to be specified with the javascript. Will look into it tomorrow.

Comment 7

[Andreas Nilsson (:andreasn)]

Attachment: small icons in thread view

These changes to multimessageview.xhtml makes the button size 16x16 instead of 24x24

Comment 8

[Andreas Nilsson (:andreasn)]

Attachment: screenshot of patch in action

Comment 9

[Andreas Nilsson (:andreasn)]

Attachment: patch to use <toolbarbutton>s

After some discussion with Jonathan on IRC This patch use <toolbarbutton> instead of <button> so that mode="icons" can be set. There is no ui to change that though.

Comment 10

[Jonathan Protzenko [:protz]]

This patch is quite nice because it makes the markup more consistent and it allows someone to set the toolbar mode, possibly attach a palette, and so on... If you want this toolbar to have the same mode as the regular mail header toolbar, you'd probably want to hack around here http://mxr.mozilla.org/comm-central/source/mail/base/content/msgHdrViewOverlay.js#293 to forward those attributes to the toolbar inside multimessageview.xhtml (I think that's what you meant on IRC, correct me if I'm wrong!). However we must be sure that multimessageview.xhtml is loaded only once because this code is executed only once if I'm not mistaken, so reloads wouldn't have the property set properly.

However, I'm not really sure we want the thread summary toolbar to have the same mode as the mail header toolbar. There's only two buttons here so the user might want to keep text + icons in the multimessageview (because there's plenty of space) and still have icons only in the mail header (because there's no space).

What do you think? Maybe setting defaultmode="full" on the toolbar would be enough to keep icons + text by default. That leaves us the possibility to add a full toolbar palette later, if we decide to do something more elaborate with the thread summary.

Comment 11

[Jonathan Protzenko [:protz]]

Oh and for further reference, there's still the issue of having the buttons not aligned vertically (shifted maybe 3px downwards).

Comment 12

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

Comment on attachment 444422 [details] [diff] [review]
small icons in thread view

seems to work and looks reasonable.

Comment 13

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

Comment on attachment 444434 [details] [diff] [review]
patch to use <toolbarbutton>s

This looks like it could be useful for future extensions who want to try offering a palette so it  might make sense to make this happen as well.

Comment 14

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 444422 [details] [diff] [review]
small icons in thread view

In an ideal world, this would have an automated test, but I think the cost outweighs the benefit here.  r=dmose; thanks for the patch!

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

After looking around a bit more, I actually like the markup in the second patch better, because I recall having had problems with mismatched markup of just this sort in the past.

I'd say that the second patch is probably fine as is, and we could spin off any custom palletry or a separate default mode to another bug.  I haven't actually tried it myself, but I can do that tomorrow.

Andreas, any thoughts about the vertical alignment mentioned in comment 11?

Comment 16

[Andreas Nilsson (:andreasn)]

Attachment: update to the patch

removing two lines from #buttonbox in multimessageview.css seems to take care of the top padding issue. Not sure if that have other implications though.
If people can take it for a test spin before I ask for code and ui-review on it, that would be great.

Comment 17

[Andreas Nilsson (:andreasn)]

Attachment: small update for OS X

small update to look all right on OS X.

Comment 18

[Andreas Nilsson (:andreasn)]

Just for the record, this seems to look all right on Windows as well.

Comment 19

[Dan Mosedale (:dmosedale, :dmose)]

Unfortunately, it doesn't look right on the Mac: the words are off of vertical center, and the gradient looks weird.

Looking more closely at the patch, I see something that I should have caught previously: in additional to changing the XUL structure (and XBL-tag behind it), which has a non-trivial amount of behavioral risk to it, this also changes the ID of the element.  Together (and even individually, to a reasonable degree) this has the effect of making it an API-breaking change for people that are working on updating their existing add-ons.

The direction of this patch is clearly the right forward-looking one, but my inclination is that we'd do better to not accept this risk on the branch this late in the game.  So I'd suggest moving this patch forward for the trunk, and doing the most minor-possible band-aid for the branch, probably just changing a couple of very specific size units on the existing elements.

Comment 20

[Dan Mosedale (:dmosedale, :dmose)]

<http://skitch.com/dmose/dd64n/inbox> is what things look like on Mac.

Comment 21

[Andreas Nilsson (:andreasn)]

Attachment: more gnomestripe specific css patch

Hopefully this should work better.

Comment 22

[Andreas Nilsson (:andreasn)]

Attachment: and works in Windows too

Hello bug mail lovers :)
Apparently this issue affects Windows too, but not Mac OS X.
This patch takes care of that too.

Comment 23

[Andreas Nilsson (:andreasn)]

Attachment: and works in Windows too v2

and I should remember to do qrefresh first of course... :/

Comment 24

[Andreas Nilsson (:andreasn)]

Attachment: slightly modified patch

After speaking with Bryan and Dan on the phone, here is a slightly modified patch making use of child selectors.

Tried to specify #headingwrapper, but that did not work at all, I was able to select the ones below (#buttonbox and #buttonhbox) but it totally refused to act upon that one. No idea what's going on really. xul vs. html?

Comment 25

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 446015 [details] [diff] [review]
slightly modified patch

r=dmose; thanks for the patch.  To be honest, I think the smaller buttons on Windows look a little curious since they're now out of line with and differently sized from the number of conversations.  But I only noticed that because I was specifically looking for it.

Comment 26

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

Comment on attachment 446015 [details] [diff] [review]
slightly modified patch

looks good to me

Comment 27

[Dan Mosedale (:dmosedale, :dmose)]

Pushed to the branch in order to facilitate hitting code freeze:

http://hg.mozilla.org/releases/comm-1.9.2/rev/a6e8a5d55d18

The trunk tinderboxen are something of a mess right now, so I think pushing this to the trunk can wait.

Comment 28

[Mark Banner (:standard8)]

Landed on trunk as well:

http://hg.mozilla.org/comm-central/rev/f414faed8c57

If we want to do any follow-ups for trunk, I'd suggest doing them in separate bugs.