731673 - Shift modifier for starting new message in non-default format is ignored in message menu (Plaintext vs. HTML, Shift+Click, Shift+Enter on Reply, Reply to All, Reply to List, Forward)

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Thomas D. (:thomas8)]

We have Shift as a modifier key to toggle non-default format for starting a new message (Plaintext vs. HTML) on message header buttons, and on msg context menu commands like Reply, Forward etc. Surprisingly, and violating ux-consistency, holding Shift is ignored for the same commands from the main message menu.

STR

1) Hold Shift while choosing any of the following commands from message menu:
Reply, Reply to All, Reply to List, Forward;
observe message format in composition window

2) For comparison:
HOld Shift while choosing any of the following commands from message *context* menu (or message header buttons):
Reply to Sender Only, Reply to All, Reply to List, Forward;
observe message format in composition window

Actual Result

1) Shift+commands from msg menu: message format for composition is still the default format (usually HTML), with or without Shift (this bug)

2) Shift+commands from msg *context* menu: message format is toggled to *non*-default format (usually plaintext) - ok

Expected Result

1) Shift+commands from msg menu: message format for composition should be toggled to *non*-default format (usually plaintext)

There are some other instances of this bug in other places of the UI, esp. "Edit as new" and "Compose message to" (bug 687324) which I'll handle separately for clarifying the expected UI, but all of them could/should be fixed here (see dependent bugs) unless stated otherwise in subsequent comments.

Comment 1

[Thomas D. (:thomas8)]

Richard, is this something you could fix for your new AppMenu (and the respective main menus)?

Comment 2

[Richard Marti (:Paenglab)]

Sorry Thomas, I'm a noob in JS. All I can do is copy/paste. I'm doing mostly CSS on the themes. But if somebody would point me to the needed code and how the shift modifier is read, then maybe I could try to do a patch.

Aceman is a really good programmer. Maybe he would know how to fix.

Comment 3

[Thomas D. (:thomas8)]

Jim, pls skip 1-8 below, we need your input for (9)/10/11 below.

(In reply to Richard Marti [:Paenglab] from comment #2)
> Sorry Thomas, I'm a noob in JS. All I can do is copy/paste. I'm doing mostly
> CSS on the themes. But if somebody would point me to the needed code and how
> the shift modifier is read, then maybe I could try to do a patch.

Richard, you did a very good job on the appMenu. It would be really cool if we could take it from there and push you a bit further into the code ;)
Copy & paste is a legitimate approach if done well... ;)

OK, here's how I found a starting point in the code (and if there are better strategies, pls let me know):

1) Install Addon: DOM Inspector for TB
https://addons.mozilla.org/en-US/thunderbird/addon/dom-inspector-6622/

2) Install Addon: Inspector Widget (to have that practical Inspector Button on your toolbar) 
https://addons.mozilla.org/de/thunderbird/addon/inspectorwidget/?src=ss

3) From comment 0, STR 2) we know that shift+clicking "Reply" etc. works correctly in the message *context* menu, so let's find the commands used there using DOM Inspector:

3a) select a single msg
3b) click on Inspect button on mail toolbar (the next click will inspect)
3c) use context menu key (from your keyboard!) to invoke msg context menu without triggering inspect-click yet
3d) left-click on "Reply to All" (or any other command for your purposes)
3e) Dom Inspector should now show the following document:
        chrome://messenger/content/messenger.xul
with the following DOM Node selected (in DomI's "Document > Dom Nodes" Mode):
nodename: menuitem  ID:mailContext-replyAll
3f) in DomI's object viewer (Object - Dom Node), you should see the attributes, among these:

oncommand="MsgReplyToAllMessage(event);"

So that's the command that works for context menu.

4) So why doesn't it work from AppMenu?
Do something similar to Step 3 above to inspect the command used in appMenu (or use DomI's Edit > Find nodes (Ctrl+F) and search for "Reply", then Ctrl+G to repeat the search until you've found what you're looking for:

<menuitem id="appmenu_replyToAll" commmand="cmd_replyAll">

So that's the command that doesn't work from AppMenu.
Let's check why not, and look at both commands in the code:

5) Open http://mxr.mozilla.org/comm-central/
Search for: cmd_replyAll
From the results list, skip all things "suite" because that's SeaMonkey
Look for things "mail", that's Thunderbird
Look for a <command...>, use Firefox Text search: Ctrl+F, then "<command"
Find the code:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#239
<command id="cmd_replyall" oncommand="goDoCommand('cmd_replyall')"/>

6) From http://mxr.mozilla.org/comm-central/
search for "function goDoCommand"
Find this from results:
http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/content/globalOverlay.js#86
(I think that's what TB uses)
Well, 6 doesn't help much, let's just look at the command:

7) From http://mxr.mozilla.org/comm-central/
as in 5, search for: cmd_replyall
look for switches:
> case "cmd_replyall":
Find this:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#603

603 case "cmd_replyall":
604         MsgReplyToAllMessage(null);
605         break;

8) Compare:

Working (from step 3):
> MsgReplyToAllMessage(event);

Not working (from step 7):
> MsgReplyToAllMessage(null)

So passing the event object makes the difference.
Let's see how:

9) From http://mxr.mozilla.org/comm-central/
search for: MsgReplyToAllMessage
Find this:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1563 

> composeMsgByType(Components.interfaces.nsIMsgCompType.ReplyAll, event)

mxr-search for: function composeMsgByType

-> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1523

> 1529   if (aEvent && aEvent.shiftKey) {

There we go, that's where the shiftKey gets caught from the event object.

10) Still, the solution is not necessarily trivial, because of this:

http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#575

> 573   doCommand: function(command, aTab)
> 574   {
> 575     // if the user invoked a *key short cut* then it is possible that we
>            got here...
> 579     switch ( command ) ...
> 603       case "cmd_replyall":
> 604         MsgReplyToAllMessage(null);
> 605         break;

Iow, it's possible that we end up in the main handler for "cmd_replyall" from a keyboard shortcut. The keyboard shortcut for "cmd_replyall" is Ctrl+*Shift*+R, so Shift will *always* have been pressed when the command is fired from keyboard shortcut, but that's *NOT* the same *Shift* that we're listening for to toggle to Non-Default Format.
(It is not possible to trigger the non-default format with keyboard shortcut, but it's possible with Shift+Mouseclick or Shift+Enter on the menuitem from context menu).

So I am not sure if just changing (null) to (event) in the main command handler will work, because if we arrive there from keyboard shortcut, I think we might always catch "Shift" from Ctrl+Shift+R but's that's the wrong one.

http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#604
> MsgReplyToAllMessage(null)
-> MsgReplyToAllMessage(event) ?

11) We could consider the way of least resistance and just copy from where it works:

Replace current
> <menuitem id="appmenu_replyToAll" commmand="cmd_replyAll">
with
> <menuitem id="appmenu_replyToAll" oncommmand="MsgReplyToAllMessage(event);">

But iirc, we are trying to avoid using
> oncommand="somefunction()"
whereever possible in favor of
> command="cmd_something"
to have cleaner code and also better for addons.

I'm sure Jim will know more about this (Bug 526998 Comment 13; bug 690631)...

Comment 4

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #3)

> 11) We could consider the way of least resistance and just copy from where
> it works:
> 
> Replace current
> > <menuitem id="appmenu_replyToAll" commmand="cmd_replyAll">
> with
> > <menuitem id="appmenu_replyToAll" oncommmand="MsgReplyToAllMessage(event);">

And I'm also irritated why we have "MsgReplyToAll_*Message*_", so I'm not sure if this might fail if used from AppMenu on the wrong selected items (News?). Whereas from the message context menu, this function will probably only occur on messages.

Comment 5

[Richard Marti (:Paenglab)]

Before I replied in comment 2 I already tried the change of MsgReplyMessage(null) to MsgReplyMessage(event) but this didn't work. And I know mconley has a bug where he is removing the oncommand entries for Test Pilots click count. This is also why I didn't tried the oncommand way.

I hope squib has a easy solution how this could be done.

Comment 6

[Onno Ekker [:nONoNonO UTC+1]]

It works when you change the command="cmd_..." to the respective oncommand="Msg...(event)", both for the message menu as the appmenu.
It feels very unnatural though, to go to the appmenu, scroll to the Message sub menu, and then hold the Shift key before selecting the option you want...

The Shift modifier should also work with the Compose Message To function on a right-click on an address in the header toolbar and on a mailto: link in an e-mail or news message.

Comment 7

[Jim Porter (:squib)]

Don't change command="..." to oncommand="...". That just makes life harder for everyone. If you really need something more advanced than the usual command controllers, do something like the attach to cloud feature[1]:

  <command id="cmd_attachCloud" oncommand="attachToCloud(event.target.cloudProvider); event.stopPropagation();"/>

Note: the stopPropagation call isn't necessary in general.

[1] http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/messengercompose.xul#107

Comment 8

[Onno Ekker [:nONoNonO UTC+1]]

I see the cmd_* functions also in mailnews files. I suppose this should also be fixed for suite/SeaMonkey?

Comment 9

[Richard Marti (:Paenglab)]

(In reply to Jim Porter (:squib) from comment #7)
> Don't change command="..." to oncommand="...". That just makes life harder
> for everyone. If you really need something more advanced than the usual
> command controllers, do something like the attach to cloud feature[1]:
> 
>   <command id="cmd_attachCloud"
> oncommand="attachToCloud(event.target.cloudProvider);
> event.stopPropagation();"/>

Thanks Jim, great help. I have it locally working.

Comment 10

[Onno Ekker [:nONoNonO UTC+1]]

Can someone please explain why changing command to oncommand makes it harder for everyone? Or point me at the bug mconley has? I use oncommand in my extension too, so if necessary I have to change it there too

Comment 11

[Richard Marti (:Paenglab)]

Attachment: proposed fix

I removed some case commands and it still works. Is this okay?

Comment 12

[Onno Ekker [:nONoNonO UTC+1]]

You forgot to change the oncommands for cmd_forwardInline to MsgForwardAsInline(event) and cmd_forwardAttachment to MsgForwardAsAttachment(event)

Comment 13

[Richard Marti (:Paenglab)]

(In reply to Onno Ekker from comment #12)
> You forgot to change the oncommands for cmd_forwardInline to
> MsgForwardAsInline(event) and cmd_forwardAttachment to
> MsgForwardAsAttachment(event)

I wasn't sure if this is really needed and left them unchanged.
Thomas do you have a opinion?

I wait for Jim's review and can add this in the next version.

Comment 14

[Thomas D. (:thomas8)]

(In reply to Richard Marti [:Paenglab] from comment #13)
> (In reply to Onno Ekker from comment #12)
> > You forgot to change the oncommands for cmd_forwardInline to
> > MsgForwardAsInline(event) and cmd_forwardAttachment to
> > MsgForwardAsAttachment(event)
> 
> I wasn't sure if this is really needed and left them unchanged.
> Thomas do you have a opinion?

Clearly yes. Both of them are needed for ux-consistency. Holding Shift while clicking Message > Forward As > Inline/Attachment should also toggle to non-default composition format.

Furthermore, we should also try to fix cmd_editAsNew as the last piece of the puzzle along the way (dependent bug 731688). It's slightly different because the underlying function does not yet use the event (but I wonder if it would work if we just change the function to pass the event anyway...):

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1859
1859 function MsgEditMessageAsNew()
1860 {
1861   composeMsgByType(Components.interfaces.nsIMsgCompType.Template);
1862 }

What if we just change the call from   case "cmd_editAsNew"   to
> MsgEditMessageAsNew(event);
and then change the function to be

1859 function MsgEditMessageAsNew(event)
1860 {
1861   composeMsgByType(Components.interfaces.nsIMsgCompType.Template, event);
1862 }

Would that work?

Comment 15

[Richard Marti (:Paenglab)]

That could work. The function at http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1523 checks the shift key.

Comment 16

[Richard Marti (:Paenglab)]

Attachment: patch v2

I've added the Forward as attachment and - Inline. The Edit as new doesn't work, it looks this needs more work. So let's do this in your referenced bug.

Comment 17

[Jim Porter (:squib)]

Comment on attachment 690934 [details] [diff] [review]
patch v2

Review of attachment 690934 [details] [diff] [review]:
-----------------------------------------------------------------

This works for menu items, but unfortunately, it breaks the keyboard shortcuts. It would also be nice to change some of the toolbar buttons to use the command="cmd_..." syntax instead of oncommand="...", but that seems to not work very well in the message header toolbar.

::: mail/base/content/mail3PaneWindowCommands.js
@@ -613,5 @@
> -        MsgForwardAsInline(null);
> -        break;
> -      case "cmd_forwardAttachment":
> -        MsgForwardAsAttachment(null);
> -        break;

Please don't remove these. They're necessary for making the keyboard shortcuts work. (I think cmd_forwardInline and cmd_forwardAttachment can be removed, though.)

::: mail/base/content/messageWindow.js
@@ -1057,5 @@
> -        MsgForwardAsInline(null);
> -        break;
> -      case "cmd_forwardAttachment":
> -        MsgForwardAsAttachment(null);
> -        break;

Same here.

Comment 18

[Thomas D. (:thomas8)]

(In reply to Jim Porter (:squib) from comment #17)
> Comment on attachment 690934 [details] [diff] [review]
> patch v2
> 
> Review of attachment 690934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This works for menu items, but unfortunately, it breaks the keyboard
> shortcuts.

Richard, respective keyboard shortcuts are defined here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#411

411   <key id="key_reply" key="&replyMsgCmd.key;"                        oncommand="goDoCommand('cmd_reply')" modifiers="accel"/>
412   <key id="key_replyall" key="&replyToAllMsgCmd.key;"                oncommand="goDoCommand('cmd_replyall')" modifiers="accel, shift"/>

So the keys use goDoCommand, and that ends up in mail3PaneWindowCommands.js's doCommand: function where you removed the switches so they are no longer handled at all.

For the new patch, pls also verify that Ctrl+Shift+R (as defined in line 412 above) still triggers the *default* format (because we are now listening for the Shift key which might wrongly trigger the *non-default* format always for keyboard shortcuts that contain Shift as part of the shortcut key combination).

> It would also be nice to change some of the toolbar buttons to
> use the command="cmd_..." syntax instead of oncommand="...", but that seems
> to not work very well in the message header toolbar.
> 
> ::: mail/base/content/mail3PaneWindowCommands.js
> @@ -613,5 @@
> > -        MsgForwardAsInline(null);
> > -        break;
> > -      case "cmd_forwardAttachment":
> > -        MsgForwardAsAttachment(null);
> > -        break;
> 
> Please don't remove these. They're necessary for making the keyboard
> shortcuts work. (I think cmd_forwardInline and cmd_forwardAttachment can be
> removed, though.)

No, even cmd_forwardInline and cmd_forwardAttachment cannot be removed because they are still in use in the [Appmenu] > Message > Forward as > Inline/Attachment popups here:
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#1881
http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2673

> 
> ::: mail/base/content/messageWindow.js
> @@ -1057,5 @@
> > -        MsgForwardAsInline(null);
> > -        break;
> > -      case "cmd_forwardAttachment":
> > -        MsgForwardAsAttachment(null);
> > -        break;
> 
> Same here.

dito, not removable.

Comment 19

[Jim Porter (:squib)]

Wouldn't the AppMenu use the new definition for cmd_forwardInline, instead of going through that switch statement?

Comment 20

[Onno Ekker [:nONoNonO UTC+1]]

(In reply to Thomas D. from comment #14)
> Furthermore, we should also try to fix cmd_editAsNew as the last piece of
> the puzzle along the way (dependent bug 731688). It's slightly different
> because the underlying function does not yet use the event (but I wonder if
> it would work if we just change the function to pass the event anyway...):
> 
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mailWindowOverlay.js#1859
> 1859 function MsgEditMessageAsNew()
> 1860 {
> 1861   composeMsgByType(Components.interfaces.nsIMsgCompType.Template);
> 1862 }
> 
> What if we just change the call from   case "cmd_editAsNew"   to
> > MsgEditMessageAsNew(event);
> and then change the function to be
> 
> 1859 function MsgEditMessageAsNew(event)
> 1860 {
> 1861   composeMsgByType(Components.interfaces.nsIMsgCompType.Template,
> event);
> 1862 }
> 
> Would that work?

I've tested this locally, but it doesn't seem to work. The "template" seems to determine the message format: If the message you edit was written in HTML, you get the HTML Editor, otherwise plain text. Adding the event to the call doesn't seem to make a change. Also Shift key for Edit as new is a different bug: bug 731688 or bug 78794.

Comment 21

[Thomas D. (:thomas8)]

(In reply to Jim Porter (:squib) from comment #19)
> Wouldn't the AppMenu use the new definition for cmd_forwardInline, instead
> of going through that switch statement?

That's right. I didn't express myself well, just wanted to point out that those two commands are still in use, though we're probably currently not going through that particular doCommand:function switch list to use them. Still, I don't think it's wise to remove the switches for just 2 random live commands from that switch list in http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#573.
Those switches seem to cover *all* commands (commands with and without keyboard shortcuts), so I don't see the benefit and I think it's inconsistent and risky to remove just 2 random live commands. E.g. it might break addons that rely on that universal doCommand switch statement when they change keyboard shortcuts for any command using the current syntax of goDoCommand...

Comment 22

[Thomas D. (:thomas8)]

@Richard & everyone: Related work was once done by Magnus in Bug 228562, perhaps he can assist especially when we proceed to Bug 731688 about Shift to toggle format of edit-as-new.

(In reply to Onno Ekker from comment #20)
> (In reply to Thomas D. from comment #14)
> > Furthermore, we should also try to fix cmd_editAsNew as the last piece of
> > the puzzle along the way (dependent bug 731688). [...]
> I've tested this locally, but it doesn't seem to work. The "template" seems
> to determine the message format: If the message you edit was written in
> HTML, you get the HTML Editor, otherwise plain text. Adding the event to the
> call doesn't seem to make a change.

Onno, thanks for testing. Cited bug 78794 covers the pre-determined edit-as-new format based on original msg format, which imo violates ux-consistency with per-account setting of compose-in-HTML (but I'm not 100% sure).

Did you add *event* to both the command call and the actual function call, like this? :
1) Add event to the function call (whereever you call it):
> MsgEditMessageAsNew(*event*)
2) Add event inside the MsgEditMessageAsNew function [1]:
1861   composeMsgByType(Components.interfaces.nsIMsgCompType.Template,*event*);

> Also Shift key for Edit as new is a
> different bug: bug 731688 or bug 78794.

Yes, bug 731688 (as mentioned in comment 14 you are responding to, & comment 16).
Only bug 731688 is explicitly about Shift key to toggle format for Edit as new.
Bug 731688 is one possible workaround (or even minimal solution) for different problem of Bug 78794, which is about the unexpected pre-determination of format derived from original msg, where users get stuck in plaintext composition if the original was plaintext and they do "edit as new" (which might also be a variant of Bug 216132).

[1] http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1859

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to Onno Ekker from comment #10)
> Can someone please explain why changing command to oncommand makes it harder
> for everyone? Or point me at the bug mconley has? I use oncommand in my
> extension too, so if necessary I have to change it there too

Bug 767118.

Comment 24

[Richard Marti (:Paenglab)]

Attachment: patch v3

(In reply to Jim Porter (:squib) from comment #17)
> Review of attachment 690934 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This works for menu items, but unfortunately, it breaks the keyboard
> shortcuts. It would also be nice to change some of the toolbar buttons to
> use the command="cmd_..." syntax instead of oncommand="...", but that seems
> to not work very well in the message header toolbar.

I've changed the toolbar buttons but didn't touch the message header buttons. I tried it and saw no big difference with the focus (I think with command="..." it's better, because if the focus is in message body it stays there after closing the reply and doesn't change to the thread tree) but when I also saw the event.stopPropagation(); I decided to leave it untouched.

> ::: mail/base/content/mail3PaneWindowCommands.js
> @@ -613,5 @@
> > -        MsgForwardAsInline(null);
> > -        break;
> > -      case "cmd_forwardAttachment":
> > -        MsgForwardAsAttachment(null);
> > -        break;
> 
> Please don't remove these. They're necessary for making the keyboard
> shortcuts work. (I think cmd_forwardInline and cmd_forwardAttachment can be
> removed, though.)
> 
> ::: mail/base/content/messageWindow.js
> @@ -1057,5 @@
> > -        MsgForwardAsInline(null);
> > -        break;
> > -      case "cmd_forwardAttachment":
> > -        MsgForwardAsAttachment(null);
> > -        break;
> 
> Same here.

I leaved both unchanged to be sure everything and also Add-ons are still working.

Comment 25

[Jim Porter (:squib)]

Apologies for taking so long on this review. I promise I'll look at this after the 15th, when my plate isn't quite so full.

Comment 26

[Jim Porter (:squib)]

Comment on attachment 691845 [details] [diff] [review]
patch v3

Review of attachment 691845 [details] [diff] [review]:
-----------------------------------------------------------------

Ok, I finally got to this. Everything looks good!

Comment 27

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/0651aef2db9b