Closed
Bug 609926
Opened 14 years ago
Closed 14 years ago
'Quote Message' menuitem activation misbehaves
Categories
(Thunderbird :: Message Compose Window, defect)
Thunderbird
Message Compose Window
Tracking
(thunderbird3.1 wontfix, thunderbird3.0 wontfix)
RESOLVED
FIXED
Thunderbird 3.3a1
People
(Reporter: sgautherie, Assigned: sgautherie)
References
(Blocks 1 open bug)
Details
(Keywords: ux-userfeedback)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
bwinton
:
review+
standard8
:
approval-thunderbird3.0.11-
standard8
:
approval-thunderbird3.1.7-
|
Details | Diff | Splinter Review |
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:
{
neil@parkwaycc.co.uk 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.
}
Assignee | ||
Comment 1•14 years ago
|
||
(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 ;-)
Assignee | ||
Comment 2•14 years ago
|
||
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.
Attachment #488767 -
Flags: review?(bwinton)
Assignee | ||
Updated•14 years ago
|
Target Milestone: --- → Thunderbird 3.3a1
Comment 3•14 years ago
|
||
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.
Attachment #488767 -
Flags: review?(bwinton) → review-
Assignee | ||
Comment 4•14 years ago
|
||
(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).)
Flags: in-testsuite?
Assignee | ||
Comment 5•14 years ago
|
||
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.
Attachment #488767 -
Attachment is obsolete: true
Attachment #490445 -
Flags: review?(bwinton)
Comment 6•14 years ago
|
||
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.
Attachment #490445 -
Flags: review?(bwinton) → review+
Assignee | ||
Comment 7•14 years ago
|
||
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.
Attachment #490445 -
Attachment description: (Av2) Restore updateOptionItems(), Add an event listener → (Av2) Restore updateOptionItems(), Add an event listener
[Checked in: See comment 7]
Attachment #490445 -
Flags: approval-thunderbird3.1.7?
Attachment #490445 -
Flags: approval-thunderbird3.0.11?
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → sgautherie.bz
No longer blocks: TB2SM
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite? → in-testsuite-
Keywords: ux-feedback
Resolution: --- → FIXED
Assignee | ||
Comment 8•14 years ago
|
||
Comment 9•14 years ago
|
||
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").
Keywords: regression
Updated•14 years ago
|
Attachment #490445 -
Flags: approval-thunderbird3.2a1?
Attachment #490445 -
Flags: approval-thunderbird3.1.7?
Attachment #490445 -
Flags: approval-thunderbird3.1.7-
Attachment #490445 -
Flags: approval-thunderbird3.0.11?
Attachment #490445 -
Flags: approval-thunderbird3.0.11-
Assignee | ||
Comment 10•14 years ago
|
||
(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"...
status-thunderbird3.0:
--- → wontfix
status-thunderbird3.1:
--- → wontfix
Updated•14 years ago
|
Attachment #490445 -
Flags: approval-thunderbird3.2a1?
You need to log in
before you can comment on or make changes to this bug.
Description
•