User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.0.3) Gecko/2008092414 Firefox/3.0.3 Build Identifier: 3.0a3 When previewing mail, the header section displays "Reply", "Forward", "Mark as Junk" and the trash can. To be consistent, TB should also add "Reply All" button if not as a button then at least under "other actions" drop down. Reproducible: Always Steps to Reproduce: 1.Click on a mail and see the header in the preview pane. 2. 3. Actual Results: Missing "Reply All" Expected Results: To be consistent, TB should also add "Reply All" button if not as a button then at least under "other actions" drop down.

Not sure how to manage these bugs, perhaps this will be the tracker for Reply All. But in Bug 45715 Comment #125 I added explained our path for the Reply to List button. The Reply All button is intended to work in a similar manner as Reply to List, replacing the Reply button when there are more than one recipients of the message. Replying directly to the sender will be available off the senders email address.

A few additional thoughts : From a practical perspective I get many emails a month (~5000+) and many, if not all have multiple contacts on the To/CC list, I predominantly need to reply "all" rather than reply to the person who it is From. A single click reply all button would be great (PS: so would an "Edit Message As New" button too :-) ). If available panel real estate is an issue, ie. too much button clutter, maybe it would be possible for a user to "Customize" the buttons they want in the panel or the actions of a button, if clicked with a keyboard modifier... Thanks

Here's my current work in progress patch. I changed a bit of the padding and spacing in the surrounding areas. I ruined the flat buttons and haven't been able to fix them quite yet. However I've changed the top row of buttons to be actual buttons (in linux). I think in OS X we'll need to use an image background for the buttons to come out looking good, however in Linux this is the better bet. The reply button has been converted to be a 'dualbutton' such that it's vertically flat and offers a drop-down popup. Currently I have a couple of stub options in place, reply all and others. What I believe needs to be done is a set of buttons for each scenario; reply, reply all, and reply list (bug 45715). I wasn't really sure how to attack this problem since I could have used a deck, but my natural HTML instincts wanted to use some display:none magic. dmose: if you can take a look at this and perhaps decide the right technical direction. I can continue toying with it once you've worked your magic. I should be getting a mac that I can use fairly soon so I can start some work on the mac side of things as well; using a border-image button for these buttons.

for those visual people like myself. here's a screenshot of the changes in linux w/ the stub menu popup showing

For the record -- for pinstripe, we could use a non-native-but-common border like: .msgHeaderView-button { -moz-appearance: none; -moz-margin-start: 0px; -moz-margin-end: 5px; -moz-appearance: none; border: 1px solid #5F5F5F; -moz-border-radius: 4px; background: url(chrome://global/skin/icons/white-gray-gradient.gif) #A09E9D repeat-x top center; min-height: 0; min-width: 0; padding: 2px; margin: 0 6px; min-height: 1ex; } .msgHeaderView-button:hover:active:not([disabled="true"]) { background: url(chrome://global/skin/icons/white-gray-gradient-active.gif); } (ripped off from activity manager, which stole it from download manager -- see the "Clear list" button).

Dan, perhaps you can take a look at this patch. This is essentially the minimal patch I was talking about before. It adds a drop down for the reply all function to the reply button. This patch, unlike before, doesn't change the other "flat" buttons in the header; i.e. other actions, hide detail, show detail.

Forgot to include the msgHdrViewOverlay.xul file in my previous patch. Try this again. Note, this is only a gnomestripe, aka Linux theme. So dmose if you don't have Linux handy I could ask magnus for a review since I believe he runs the L word; or someone else.

Comment on attachment 359655 [details] [diff] [review] minimal reply all message header w/ msgHdrViewOverlay.xul included My VMs are in a weird state at the moment, so I think it's better if someone else can poke at this.

Pinstripe version of the theme part.

curse thee, invisible patch queue.

for the curious

Comment on attachment 359655 [details] [diff] [review] minimal reply all message header w/ msgHdrViewOverlay.xul included Now that there's a version I can easily test attached, I'm happy to do the whole review; stealing r? back from mkmelin. :-)

I was looking at it, so I'll put down my comments so far. I'll let you finish it off.

Comment on attachment 359655 [details] [diff] [review] minimal reply all message header w/ msgHdrViewOverlay.xul included > /* ::::: for the entire area ::::: */ > .main-header-area { >- background-color: #eeeeec; >+ color: rgb(46, 52, 54); /* Aluminium 6 */ >+ background-color: rgb(238, 238, 236); /* Aluminium 1 */ >+ border-bottom: 2px groove rgb(186,189,182); /* Aluminium 3 */ >+ padding: 0.2ex; > } I'm not sure the whole background color setting is a good idea. I don't think the current one for windows melts in well at all, on linux it's a bit better by chance, but if we want it to look native removing the color and background-color rows entirely is way, way better IMO. That would fix bug 456829 at least wrt header background color. > >+.hdrReplyButton { Trailing space. >+ margin: 0px 0px 0px 0.5em; >+ padding: 0px; >+ -moz-padding-start: 0px; >+ -moz-padding-end: 0px; >+ -moz-margin-start: 0px; >+ -moz-margin-end: 0px; Nicer to not set the margins/paddings twice. Doesn't drop-down buttons like that have all the options in them - now it's always only "reply all" which feels a bit weird (add the "reply" too?) Also, the reply button text color is different from the other ones (blacker), and the down arrow isn't "inside" the button for some reason.

I wonder if it shouldn't work more like the "File" button on the toolbar - so you always have to choose which format you want - reply to sender or reply to all.

Good seeing things move .. but still wonder why not using icons instead of labeled buttons. There is much space occupied by those label-buttons .. the icons could do much better here!?? There was an example with "headerUI" .. maybe a contribution this change??

(In reply to comment #14) > I'm not sure the whole background color setting is a good idea. I don't think > the current one for windows melts in well at all, on linux it's a bit better by > chance, but if we want it to look native removing the color and > background-color rows entirely is way, way better IMO. That would fix bug > 456829 at least wrt header background color. Ok, I've removed those tango colors now. I would have liked to have colors that look nicer in general, but I guess we can't make that happen with the way the themes work. > >+.hdrReplyButton { > > Trailing space. fixed > Nicer to not set the margins/paddings twice. fixed, only changing the padding that was a problem. > Doesn't drop-down buttons like that have all the options in them - now it's > always only "reply all" which feels a bit weird (add the "reply" too?) It seemed strange to have both options in there when the ( reply ) button itself performed the reply to sender action. > Also, the reply button text color is different from the other ones (blacker), > and the down arrow isn't "inside" the button for some reason. Right, that should be fixed now as well. I added back in the margin space for the arrow and just made sure it didn't change when opened. I think it looks a lot better now. > I wonder if it shouldn't work more like the "File" button on the toolbar - so > you always have to choose which format you want - reply to sender or reply to > all. This is more like the Get Mail menu-button where it has a default action and then alternates. I'd like to see, after this patch, a patch that switches the button to be the proper default value; i.e. reply all, reply list, reply, etc. and also switching the menu options to appropriate values as well. Throwing this part to magnus as the reviewer so dmose can own the os x version.

Comment on attachment 359808 [details] [diff] [review] updated patch with colors Looks a lot better, yay! I still shows the down arrow outside the button for me, though. >+ padding: 0px; >+ -moz-padding-start: 0px; >+ -moz-padding-end: 0px; There are still a few of these ^^^ (and similar for margin), where we should just set the padding-top and padding-bottom for instead of setting padding and then -start and -end.

ok, working on a new one now. I'll clean up the bad fighting padding declarations, though some of them are intentional. +.hdrReplyButton > .button-menubutton-dropmarker { + -moz-margin-start: 2px; + -moz-margin-end: 2px; +} + +.hdrReplyButton > .button-menubutton-dropmarker[open="true"] { + margin-top: 0px; + -moz-margin-end: 2px; +} + This uses the mix of margin-start/end/top specifically to control what the toolkit button.css is doing to the button. Each theme is going to have to inspect these values for themselves. Otherwise we should be able to play within the lines.

here's the updated patch that fixes the duelling padding and margin problems. I think the arrow outside the button you're seeing is (somewhat) intentional. That's how a dualbutton looks when there's no hover event; I haven't found a way to make it appear like a button without hover because I couldn't locate the rule that created this. I don't really mind it either way. I'm attaching an updated screenshot next to detail the colors changes so you'll be able to see what the button looks like normally for me.

here's an updated screenshot that shows the high contrast inverse theme working with the new colors as well as the default fedora theme inside the header.

(In reply to comment #20) > I think the arrow outside the button you're seeing is (somewhat) intentional. > That's how a dualbutton looks when there's no hover event; I haven't found a > way to make it appear like a button without hover because I couldn't locate the > rule that created this. I don't really mind it either way. Didn't poke around, but I suppose they are supposed to be like that natively. (Since at least for the osx screenshot it's inside.)

Comment on attachment 360178 [details] [diff] [review] reply all gnomestripe theme fixes w/ a11y css colors I get one css error with this applied Warning: Error in parsing value for 'padding'. Declaration dropped. Source File: chrome://messenger/skin/messageHeader.css Line: 247

(In reply to comment #23) > (From update of attachment 360178 [details] [diff] [review]) > I get one css error with this applied > > Warning: Error in parsing value for 'padding'. Declaration dropped. > Source File: chrome://messenger/skin/messageHeader.css > Line: 247 I suppose it's not needed if it's being dropped anyway. :) Updated patch. Removed the padding declaration since it was being dropped anyway and things look in the same place.

Comment on attachment 360627 [details] [diff] [review] [checked in] reply all gnomestripe theme fixes w/ a11y css colors - padding declaration dropped r=mkmelin I guess I have to blame my self for using a theme with somewhat ugly buttons :)

I couldn't get this to apply cleanly and I tried two ways of doing so. Does it rely on another patch?

ah right, the mark as junk button change (bug 476555) got in before and broke this

I'll unbitrot it, change the entity name to hdrReplyAllButton.label to match the others, and check it in shortly.

Comment on attachment 360627 [details] [diff] [review] [checked in] reply all gnomestripe theme fixes w/ a11y css colors - padding declaration dropped http://hg.mozilla.org/comm-central/rev/f501e3b80550

thanks again! landed and looks good

I updated this patch a bit to fix a padding issue I wanted to work on. The date value seems a bit too close to the buttons, but otherwise I believe everything else lines up nicely.

Comment on attachment 361821 [details] [diff] [review] updated patch with a better padding [Checkin: Comment 33] r=dmose on the pinstripe patch; building with it now and will push in the morning (though if someone else wants to do that before tomorrow's builds kick off, they're more than welcome to).

Comment on attachment 361821 [details] [diff] [review] updated patch with a better padding [Checkin: Comment 33] http://hg.mozilla.org/comm-central/rev/85dd0123fc05

Comment on attachment 361821 [details] [diff] [review] updated patch with a better padding [Checkin: Comment 33] Please update the patch and re-commit it as the review seems to have been a little bit lax: >+.hdrReplyButton > .button-menubutton-button { >+ -moz-appearance: none; >+ padding: 0; >+ margin: 0; >+ -moz-padding-start: 0px; >+ -moz-padding-end: 0px; >+ -moz-margin-start: 0px; >+ -moz-margin-end: 0px; The -moz-padding-* and -moz-margin-* statements aren't necessary here, because we already have "padding: 0" and "margin: 0" > .msgHeaderView-button { >+ -moz-appearance: none; >+ border: 1px solid #5F5F5F; >+ -moz-border-radius: 4px; >+ background: url(chrome://global/skin/icons/white-gray-gradient.gif) #A09E9D repeat-x top center; >+ min-height: 0; >+ min-width: 0; >+ padding: 2px; >+ margin: 0px; >+ -moz-margin-start: 0.4em; The last two lines contradict each other. Just removing "margin: 0px" is probably the best option.

(In reply to comment #34) > >+ margin: 0px; > >+ -moz-margin-start: 0.4em; > > The last two lines contradict each other. Just removing "margin: 0px" is > probably the best option. That might be true if you were using NSS, the Noncascading Style Sheet specification. Here, I think you mean "the last two lines confuse me. Specifying all four sides separately wouldn't confuse me as much."

You're right, I should have caught more stuff at review time. That said, reviewers are human and make mistakes. It seems to me that attempting to address that by doing post-facto drive-by reviews and asking people to update and re-land makes the act of contribution unnecessarily frustrating and painful to the people submitting code. I'd suggest it would make for a more humane process to address these situations by filing a new bug in bugzilla and CCing the initial reporter.

Re-resolving, because this bug itself clearly would block, but the changes suggested in comments 34 and 35 clearly would not. Please file another bug for them.