Thread summary buttons are way too big

RESOLVED FIXED in Thunderbird 3.3a1

Status

RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: protz, Assigned: andreasn)

Tracking

({polish})

unspecified
Thunderbird 3.3a1
x86
Linux
polish

Firefox Tracking Flags

(blocking-thunderbird3.1 rc1+, thunderbird3.1 rc1-fixed)

Details

Attachments

(5 attachments, 5 obsolete attachments)

(Reporter)

Description

9 years ago
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
(Reporter)

Comment 1

9 years ago
Created attachment 442348 [details]
Screenshot of the issue.
Assignee: nobody → nisses.mail
Status: UNCONFIRMED → NEW
blocking-thunderbird3.1: --- → rc1+
Ever confirmed: true
Keywords: polish
(Assignee)

Comment 2

9 years ago
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.
(Reporter)

Comment 3

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

Comment 4

9 years ago
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 :)).
(Assignee)

Updated

9 years ago
Whiteboard: ETA this Friday
(Assignee)

Comment 5

9 years ago
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.
Whiteboard: ETA this Friday → ETA mid next week
(Assignee)

Comment 6

9 years ago
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.
(Assignee)

Comment 7

9 years ago
Created attachment 444422 [details] [diff] [review]
small icons in thread view

These changes to multimessageview.xhtml makes the button size 16x16 instead of 24x24
Attachment #444422 - Flags: ui-review?(clarkbw)
Attachment #444422 - Flags: review?(dmose)
(Assignee)

Comment 8

9 years ago
Created attachment 444423 [details]
screenshot of patch in action
(Assignee)

Comment 9

9 years ago
Created attachment 444434 [details] [diff] [review]
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.
(Reporter)

Comment 10

9 years ago
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.
(Reporter)

Comment 11

9 years ago
Oh and for further reference, there's still the issue of having the buttons not aligned vertically (shifted maybe 3px downwards).
Comment on attachment 444422 [details] [diff] [review]
small icons in thread view

seems to work and looks reasonable.
Attachment #444422 - Flags: ui-review?(clarkbw) → ui-review+
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.
Whiteboard: ETA mid next week → [has patch; needs r 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!
Attachment #444422 - Flags: review?(dmose) → review+
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?
Whiteboard: [has patch; needs r dmose] → [has reviewed patch ready to land; slight improvements under discussion]
(Assignee)

Comment 16

9 years ago
Created attachment 445691 [details] [diff] [review]
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.
Attachment #444434 - Attachment is obsolete: true
(Assignee)

Comment 17

9 years ago
Created attachment 445714 [details] [diff] [review]
small update for OS X

small update to look all right on OS X.
Attachment #445691 - Attachment is obsolete: true
Whiteboard: [has reviewed patch ready to land; slight improvements under discussion] → [needs re-review dmose]
(Assignee)

Comment 18

9 years ago
Just for the record, this seems to look all right on Windows as well.
(Assignee)

Updated

9 years ago
Attachment #445714 - Flags: review?(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.
Whiteboard: [needs re-review dmose]
Attachment #444422 - Flags: review+ → review-
Attachment #445714 - Flags: review?(dmose) → review-
<http://skitch.com/dmose/dd64n/inbox> is what things look like on Mac.
Whiteboard: [needs updated patch andreasn, review, landing]
(Assignee)

Comment 21

9 years ago
Created attachment 445893 [details] [diff] [review]
more gnomestripe specific css patch

Hopefully this should work better.
Attachment #445893 - Flags: review?(dmose)
(Assignee)

Comment 22

9 years ago
Created attachment 445947 [details] [diff] [review]
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.
Attachment #445893 - Attachment is obsolete: true
Attachment #445947 - Flags: review?(dmose)
Attachment #445893 - Flags: review?(dmose)
(Assignee)

Updated

9 years ago
Whiteboard: [needs updated patch andreasn, review, landing] → [needs review, landing]
(Assignee)

Comment 23

9 years ago
Created attachment 445952 [details] [diff] [review]
and works in Windows too v2

and I should remember to do qrefresh first of course... :/
Attachment #445947 - Attachment is obsolete: true
Attachment #445952 - Flags: review?
Attachment #445947 - Flags: review?(dmose)
(Assignee)

Updated

9 years ago
Attachment #445952 - Flags: review? → review?(dmose)
(Assignee)

Updated

9 years ago
Attachment #445952 - Flags: ui-review?(clarkbw)
(Assignee)

Comment 24

9 years ago
Created attachment 446015 [details] [diff] [review]
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?
Attachment #446015 - Flags: ui-review?(clarkbw)
Attachment #446015 - Flags: review?(dmose)
(Assignee)

Updated

9 years ago
Attachment #445952 - Attachment is obsolete: true
Attachment #445952 - Flags: ui-review?(clarkbw)
Attachment #445952 - Flags: review?(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.
Attachment #446015 - Flags: review?(dmose) → review+
Whiteboard: [needs review, landing] → [has reviewed patch; needs ui-review & landing]
Comment on attachment 446015 [details] [diff] [review]
slightly modified patch

looks good to me
Attachment #446015 - Flags: ui-review?(clarkbw) → ui-review+
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.
status-thunderbird3.1: --- → rc1-fixed
Whiteboard: [has reviewed patch; needs ui-review & landing]
Whiteboard: [landed on branch; waiting for tinderbox greenery to hit trunk]
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.
Whiteboard: [landed on branch; waiting for tinderbox greenery to hit trunk]
Target Milestone: --- → Thunderbird 3.2a1
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Updated

9 years ago
Blocks: 570962
You need to log in before you can comment on or make changes to this bug.