609926 - 'Quote Message' menuitem activation misbehaves

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Serge Gautherie (:sgautherie)]

I verified this bug in both SM 2.1b2 and TB 3.3a1.

Bug 607583 comment 0:
{
Serge Gautherie (:sgautherie)      2010-10-27 03:17:50 PDT

http://bonsai.mozilla.org/cvsquery.cgi?module=ThunderbirdTinderbox&sortby=Date&hours=2&date=explicit&mindate=2003-08-03+23%3A36&maxdate=2003-08-03+23%3A36
"Add a new quote button for quoting the selected message like Netscape 4.x"
}

Bug 607583 comment 3:
{
[e-mail]      2010-11-05 04:21:52 PDT

If you start by composing a new message, then open the 3pane window and select
a message, the quote message menuitem remains disabled.

If you reply to a message, then close the 3pane window, the quote message
menuitem remains enabled.

In both cases you can work around it by clicking in and out of the message
body.
}

Comment 1

[Serge Gautherie (:sgautherie)]

(In reply to comment #0)
> open the 3pane window and select a message
> 
> close the 3pane window

Note that the key part is to (un)select a message, no need to explicitly open/close the 3-pane window ;-)

Comment 2

[Serge Gautherie (:sgautherie)]

Attachment: (Av1) Restore updateOptionItems(), Use an event listener

This fixes the menuitem,
and updates the toolbarbutton whenever the menupopup is open.

I'm not familiar with toolbarbuttons: I don't know how to better fix this case, if there is a way.

Comment 3

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

Comment on attachment 488767 [details] [diff] [review]
(Av1) Restore updateOptionItems(), Use an event listener

So, I think I like the change, except for the problem you mentioned about the button not updating at the right time, but it does seem like we can test it fairly easily, so I'ld like it if you wrote one of those.  (I'm going to r- this, but mostly because I want to see the test, not because I dislike the code.)

For toolbar buttons, you just need to update the command, and the button will update itself, so I'm thinking we want to leave the call to:
goUpdateCommand("cmd_quoteMessage");
on line 643-ish, (or change it to a call to "updateOptionItems",) because that should make sure the button is in the right state when we can see it.

Thanks,
Blake.

Comment 4

[Serge Gautherie (:sgautherie)]

(In reply to comment #3)

The added event listener is called when the menupopup is to be displayed:
this is the fix for comment 0 cases.

updateComposeItems() is called when going in/out of the body area
and currently it updates cmd_quoteMessage:
though this can be used as a manual workaround to comment 0 cases to update the toolbar button,
it actually makes no sense as the quoting feature is unrelated to where the focus is on the compose window. (Maybe it should??)
Then, an updateOptionItems() call could be "restored" there, but I would add an explicit comment that it's just a workaround...

We can use 1 (or 2) of these workarounds to update the toolbar button,
but what I was referring to in comment 2 is that the toolbar button never updates "by itself" ... which is what would be needed to be consistent when "returning" from the 3-pane window (and having (un)selected a message):
I think the button (or maybe the command) itself should automatically "depend" on the 3-pane window (no-)selection,
but I don't know if/how to do that.

Wrt a test, please point me to an example I could easily adapt, as I've no idea how to convert theses steps into a test. (I'm not familiar with mailnews tests.)
(Preferably xpcshell, as I know nothing about mozmill (yet).)

Comment 5

[Serge Gautherie (:sgautherie)]

Attachment: (Av2) Restore updateOptionItems(), Add an event listener [Checked in: See comment 7]

As what I'm actually interested in is bug 537219,
I would suggest to take this patch (which is an improvement) in for now,
then move the toolbar button (and test) issue to a follow-up bug (or at least patch),
if that's acceptable.

Comment 6

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

Comment on attachment 490445 [details] [diff] [review]
(Av2) Restore updateOptionItems(), Add an event listener
[Checked in: See comment 7]

>+++ b/mail/components/compose/content/MsgComposeCommands.js
>@@ -590,20 +590,24 @@ function GetSelectedMessages()
> function SetupCommandUpdateHandlers()
> {
>   top.controllers.insertControllerAt(0, defaultController);
>+
>+  document.getElementById("optionsMenuPopup").addEventListener("popupshowing", updateOptionItems, true);

If we made this:
  document.getElementById("optionsMenuPopup")
          .addEventListener("popupshowing", updateOptionItems, true);
then it would fit in 80 characters, without loss of readability.  :)

> function UnloadCommandUpdateHandlers()
> {
>+  document.getElementById("optionsMenuPopup").removeEventListener("popupshowing", updateOptionItems, true);

Ditto this.

> As what I'm actually interested in is bug 537219,
> I would suggest to take this patch (which is an improvement) in for now,
> then move the toolbar button (and test) issue to a follow-up bug (or at least
> patch), if that's acceptable.

Sure, that sounds reasonable.  r=me with the nits fixed, and the follow-up bug filed.

Thanks,
Blake.

Comment 7

[Serge Gautherie (:sgautherie)]

Comment on attachment 490445 [details] [diff] [review]
(Av2) Restore updateOptionItems(), Add an event listener
[Checked in: See comment 7]

http://hg.mozilla.org/comm-central/rev/3ff56d10b66d
Av2, with comment 6 suggestion(s),
plus updated/reverted updateComposeItems() part.


"approval-thunderbird3.1.7=?":
"approval-thunderbird3.0.11=?":
Fix this regression on branches too. No risk.

Comment 8

[Serge Gautherie (:sgautherie)]

(In reply to comment #6)
> [...] the follow-up bug filed.

I filed bug 612380.

Comment 9

[Mark Banner (:standard8)]

This appears to have been broken as far back as probably before Thunderbird 2.0.0.x (I tested it with TB 2), I therefore don't think its worthy to call this a regression.

Given that, and the fact that no-one seems to have complained about this before I think this doesn't fit under the general security & stability rules - we can pick it up from the next release.

(and any change has a risk, this would be better characterised as "low" than "none").

Comment 10

[Serge Gautherie (:sgautherie)]

(In reply to comment #9)

> This appears to have been broken as far back as probably before Thunderbird
> 2.0.0.x (I tested it with TB 2), I therefore don't think its worthy to call
> this a regression.

It was broken since TB v0.2, == forever ;-<
regression was from a code point of view.

> Given that, and the fact that no-one seems to have complained about this before
> I think this doesn't fit under the general security & stability rules - we can
> pick it up from the next release.

I suggest, you decide: no problem.

> (and any change has a risk, this would be better characterised as "low" than
> "none").

Sure, but I (fwiw) rated this risk closer to "none" than to "low"...