545930 - reply button should not be a menu-button

Status: RESOLVED DUPLICATE of bug 526303

(Thunderbird :: Message Reader UI, defect)

Description

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

In a previous bug we had ambitions to create a reply button that allowed you to choose from multiple possible reply targets like sender or reply-to.  However since we haven't done that we simply have a reply button-menu that looks quite odd as it doesn't do anything more than a button would.  We should change the type back to a button.

Comment 1

[Andreas Nilsson (:andreasn)]

Attachment: screenshot of the issue

Since I kept coming back to this bug and thinking "what does he mean exactly" several times, I added a screenshot showing the issue. Maybe it helps someone else as well. :)

Comment 2

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

Attachment: changes the reply button to a regular button

Andreas, maybe you want to check this out.  I didn't check that we removed all hanging CSS or translations related.  However this makes the change happen and seems to look fine on the Mac.

Comment 3

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

Comment on attachment 431413 [details] [diff] [review]
changes the reply button to a regular button

Dan, can you review this one for me?

Comment 4

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 431413 [details] [diff] [review]
changes the reply button to a regular button

Won't this change make the reply-all menuitem go away in cases where it's needed?

Note that to land, we also need an automated test, though that could be reviewed as a separate patch.

Comment 6

[Daniel Kabs, reporting bugs since 2002]

Na... this bug is a duplicate of bug 525029  but as you are working in this one it might as well be the other way round :-)

Don't be mislead by the summary of bug 525029. It's about the reply button, not the reply-all button. But while you are at it, please fix the reply button as well as both buttons need this fix (bug 551251).

Comment 7

[Daniel Kabs, reporting bugs since 2002]

sorry...that should be "please fix the reply-all button as well" ... argh, tonight I will be dreaming of electric reply buttons.

Comment 9

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

(In reply to comment #4)
> (From update of attachment 431413 [details] [diff] [review])
> Won't this change make the reply-all menuitem go away in cases where it's
> needed?

Ah, right.  Currently we're using toggling the reply-all sub menu options if this is a newsgroup message where we could just be using the reply all button.  I'm not really sure why we are using the drop down instead of showing the reply-all button; maybe that's my fault.

Blake: any idea here?

New patch coming that removes the sub-menu from the reply button and instead uses the Reply All button in a newsgroup situation.

> Note that to land, we also need an automated test, though that could be
> reviewed as a separate patch.

Likely a separate patch is best as I've never been able to get the mozmill environment up and running so it could take me a while.

Comment 10

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

Attachment: updated patch removes the popup and uses just the reply all button

Comment 11

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

Attachment: control the reply-all menu-button nature

So here's an additional patch which controls the reply all button / menu-button nature.  This patch needs to be applied on top of attachment 431455 [details] [diff] [review]

If the user has set to always show the reply to sender button (current default) then the reply all button is not a menu-button.

If the user does not have the reply to sender button shown then the reply all button becomes a menu-button so there is still access to the reply to sender menuitem.

Comment 12

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

Attachment: screenshot depicting the latest patch

Here's a screenshot that better helps describe what attachment 431458 [details] [diff] [review] does.  As you can see with the reply to sender button showing you only get a reply all button without the drop-down.  And without the reply to sender you get a reply all with the regular options.

Comment 13

[Blake Winton (:bwinton) (:☕️)]

(In reply to comment #9)
> > Won't this change make the reply-all menuitem go away in cases where it's
> > needed?
> Ah, right.  Currently we're using toggling the reply-all sub menu options if
> this is a newsgroup message where we could just be using the reply all button. 
> I'm not really sure why we are using the drop down instead of showing the
> reply-all button; maybe that's my fault.
> 
> Blake: any idea here?

Cause we wanted the default to be "Reply", but also to offer "Reply to All" (instead of having the default be "Reply to All", and also offering "Reply").

See https://bugzilla.mozilla.org/show_bug.cgi?id=45715#c196

However, given that we now have the "Always show Reply to Sender" option, I'm okay with going back on that decision.

> > Note that to land, we also need an automated test, though that could be
> > reviewed as a separate patch.
> Likely a separate patch is best as I've never been able to get the mozmill
> environment up and running so it could take me a while.

I've got it up and running here, so if you wanted to write some tests, I'ld run them, and let you know how they work out.  (And probably tweak them a little.)

Later,
Blake.

Comment 14

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

Attachment: current tests, requires previous patches

(In reply to comment #13)
> (In reply to comment #9)
> > > Won't this change make the reply-all menuitem go away in cases where it's
> > > needed?
> > Ah, right.  Currently we're using toggling the reply-all sub menu options if
> > this is a newsgroup message where we could just be using the reply all button. 
> > I'm not really sure why we are using the drop down instead of showing the
> > reply-all button; maybe that's my fault.
> > 
> > Blake: any idea here?
> 
> Cause we wanted the default to be "Reply", but also to offer "Reply to All"
> (instead of having the default be "Reply to All", and also offering "Reply").
> 
> See https://bugzilla.mozilla.org/show_bug.cgi?id=45715#c196
> 
> However, given that we now have the "Always show Reply to Sender" option, I'm
> okay with going back on that decision.

Currently if you view a newsgroup message and turn off "Always show Reply to Sender" you'll just get the reply button with the reply all options.  With the change I have you'd actually always get a reply all button with the reply as a popup option; which seems wrong.  

If instead we continue using the menu-button as is for the newsgroup messages and change the button type on the fly so that it isn't a useless menu-button for other messages.

> > > Note that to land, we also need an automated test, though that could be
> > > reviewed as a separate patch.
> > Likely a separate patch is best as I've never been able to get the mozmill
> > environment up and running so it could take me a while.
> 
> I've got it up and running here, so if you wanted to write some tests, I'ld run
> them, and let you know how they work out.  (And probably tweak them a little.)

Ok, I stayed up last night and got mozmill running on my machine.  

I'd love some tips on what kind of tests I should write.

First, do we write tests that check the defaults?

Here are the tests I've written so far:

* Test that we default to offering the reply button and it is showing
* Test that the reply all button is not showing when there is only one sender
* Test that the reply all button is showing when there is more than one recipient
* Test that newsgroup messages have $THE_CORRECT_BUTTONS showing :)

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

We don't have detailed criteria for what tests to write, the basic answer is "basic testing for the most important bits of functionality".  So the tests you written so far sound great!

Comment 17

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

I shouldn't be the assignee for these bugs.  Filter against clarkbfilter to delete all these from your emails.