461098 - "Reply All" button missing when in message preview

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[DP]

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.

Comment 1

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

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.

Comment 2

[Stephen]

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

Comment 3

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

Attachment: WIP patch for reply dual button w/ drop down

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.

Comment 4

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

Attachment: header-reply-all-fixes.png

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

Comment 5

[David Ascher (:davida)]

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).

Comment 6

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

Attachment: minimal reply-all functionality patch

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.

Comment 7

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

Attachment: minimal reply all message header w/ msgHdrViewOverlay.xul included

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 8

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 9

[David Ascher (:davida)]

Attachment: pinstripe patch

Pinstripe version of the theme part.

Comment 10

[David Ascher (:davida)]

Attachment: real pinstripe patch

curse thee, invisible patch queue.

Comment 11

[David Ascher (:davida)]

Attachment: os x screenshot

for the curious

Comment 12

[Dan Mosedale (:dmosedale, :dmose)]

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.  :-)

Comment 13

[Magnus Melin [:mkmelin]]

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

Comment 14

[Magnus Melin [:mkmelin]]

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.

Comment 15

[Magnus Melin [:mkmelin]]

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.

Comment 16

[Günter]

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??

Comment 17

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

Attachment: updated patch with colors

(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 18

[Magnus Melin [:mkmelin]]

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.

Comment 19

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

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.

Comment 20

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

Attachment: reply all gnomestripe theme fixes w/ a11y css colors

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.

Comment 21

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

Attachment: comparison of fedora vs. high contrast inverse themes with summary and detailed view shown

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.

Comment 22

[Magnus Melin [:mkmelin]]

(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 23

[Magnus Melin [:mkmelin]]

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

Comment 24

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

Attachment: [checked in] reply all gnomestripe theme fixes w/ a11y css colors - padding declaration dropped

(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 25

[Magnus Melin [:mkmelin]]

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 :)

Comment 26

[Mark Banner (:standard8)]

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

Comment 27

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

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

Comment 28

[Magnus Melin [:mkmelin]]

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

Comment 29

[Magnus Melin [:mkmelin]]

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

Comment 30

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

thanks again!  landed and looks good

Comment 31

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

Attachment: updated patch with a better padding [Checkin: Comment 33]

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 32

[Dan Mosedale (:dmosedale, :dmose)]

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 33

[Serge Gautherie (:sgautherie)]

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 34

[Simon Paquet [:sipaq]]

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.

Comment 35

[Phil Ringnalda (:philor)]

(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."

Comment 36

[Dan Mosedale (:dmosedale, :dmose)]

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.

Comment 37

[Dan Mosedale (:dmosedale, :dmose)]

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.