975816 - Shift+Click to switch between HTML and Plain text doesn't work on Account Central "Write a new message" link and File > New > Message menu

Status: VERIFIED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[Onno Ekker [:nONoNonO UTC+1]]

When you click on an account in the folder pane and then Shift+Click in the Account Central pane om the Write a new message link, the Shift modifier to switch to the alternate composing format is ignored.

Comment 1

[Thomas D. (:thomas8)]

Good point, Onno! :)

Perhaps not used that way very often, and hence unnoticed so far, but I'm always in favor of more consistency in TB UX (and wondering why it wasn't designed that way to begin with)...

Comment 2

[Thomas D. (:thomas8)]

Same bug in other corners: bug 731688 and bug 687324.
And if we try hard enough, we might find more corners e.g. in AB or contact side bar where this fails.

Comment 3

[:aceman]

Maybe the shift+Click version is only a shortcut on the "Write" button and is not intended to be working at other places. This needs UX decision if we need to persuade this goal to make it work on more places. There could be strange interactions of the shift modifier and the object where the "compose" item is.

Ever saw a context menu (right click) where you then could press ctrl, alt or shift to modify the actions in the list?

Comment 4

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #3)
> Maybe the shift+Click version is only a shortcut on the "Write" button and
> is not intended to be working at other places.

Well, I think in terms of ux-consistency the choices are to generally allow Shift modifier for Write command and its flavors (everywhere) or not (everywhere). The UI element which triggers the write command (button, menu, anything else?) is quite irrelevant for that. We don't want to confuse initiated users with "modifier key works for write command controls in some places, but not in others".

> This needs UX decision if we
> need to persuade this goal to make it work on more places.

Nothing to decide at this time, because there's no alternative in the UI.
Suggested alternatives (like Bug 140800, and its twin 216132 - now dupe) have been around for more than a decade, and there's no sign of that being fixed any time soon, although it's one of TB's most popular bugs, with 25 dupes (including those from bug 216132 which haven't been moved over yet), and around 100 votes (again including an unknown number from bug 216132).

Even with that bug fixed, I don't see an immediate need to remove the legacy "shift modifier" behavior and spoil it for respective users, but if anyone wants that removed, please fix bug 140800 first and then file a new bug spelling out how having shift modifier hurts for write command control elements.
It goes without saying that the current shift-modifier behaviour is pretty much undiscoverable, but that doesn't justify violating ux-consistency for those who know.

> There could be
> strange interactions of the shift modifier and the object where the
> "compose" item is.

Without evidence of such "strange interaction", I won't believe that.
Afasics write commands (+ flavors) are only found on menus, buttons, or links that act as buttons - no strange interactions possible on those - you only hold down shift shortly before clicking them (not while you're still on message lists or such).

> Ever saw a context menu (right click) where you then could press ctrl, alt
> or shift to modify the actions in the list?

Yes, and quite prominently indeed: Hold Shift while clicking "Delete" in Windows Explorer file context menu to purge file bypassing trash (and TB also fails on that, Bug 956446).

We're not talking about introducing any other modifiers like ctrl or alt for any other commands here, and I wouldn't advise that unless there are good reasons.

Comment 5

[Thomas D. (:thomas8)]

TL;DR version of comment 4:

1) From a user's point of view, what's the functional difference between using Write button on mail toolbar and Write button-link in Account Central?
2) What alternatives for toggling are there in the UI except using the Shift modifier on Write command controls (and other write flavors like Reply, Forward, etc.)?

Answer to both questions is: NONE. Hence this bug.

Comment 6

[WADA:World Anti-bad-Duping Agency]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #0)
> Shift+Click in the Account Central pane om the Write a new message link,
> the Shift modifier to switch to the alternate composing format is ignored.

Reason is pretty simple : shift modifiers is lost by;
  "not passing event by onclick" && "passing null as event, even if event is passed".

(1) onclick of "Write a new message link" invokes ComposeAMessage() without passing event,
    even though ComposeAMessage accepts event parameter.
> http://mxr.mozilla.org/comm-central/source/mailnews/base/content/msgAccountCentral.xul#79
>                  onclick="ComposeAMessage();"/>

(2) function ComposeAMessage passes null to MsgNewMessage, even though MsgNewMessage and subsequent
    functions can accept event parameter and final function can prosess shift modifier correctly.
> http://mxr.mozilla.org/comm-central/source/mailnews/base/content/msgAccountCentral.js#257 
>  function ComposeAMessage(event)
>    window.parent.MsgNewMessage(null)

MsgNewMessage(evet) -> composeMsgByType( ... , event)
       if shiftKey    :   -> ComposeMessage( ... , nsIMsgCompFormat.OppositeOfDefault, ... )
       if no shiftKey :   -> ComposeMessage( ... , nsIMsgCompFormat.Default,           ... ) 
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1531
>  function MsgNewMessage(event)
>    composeMsgByType(Components.interfaces.nsIMsgCompType.New, event);
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.js#1514
>  function composeMsgByType(aCompType, aEvent) {
>     if (aEvent && aEvent.shiftKey) {
>       ComposeMessage(aCompType,
>                      Components.interfaces.nsIMsgCompFormat.OppositeOfDefault,
>                      msgFolder, msgUris);
>     }
>     else {
>       ComposeMessage(aCompType, Components.interfaces.nsIMsgCompFormat.Default,
>                      msgFolder, msgUris);
>     }

"2 stage protection from shift modifier" is carefully done.
It looks for me that "Write a new message" link of "Account Central" pane intentionally ignores event.

Comment 7

[WADA:World Anti-bad-Duping Agency]

Actually problem like "inconsistency in UX"?
As seen in function composeMsgByType(aCompType, aEvent), modules who supports modifier key can process the modifier key if event is correctly passed.
Merely problem of "UX fails to pass event correctly to subsequent function", isn't it?

Comment 8

[Onno Ekker [:nONoNonO UTC+1]]

The Shift modifier even works when you use it on the menu Message -> Reply or Forward...
It is ignored again however on the menu File -> New -> Message.
Quite inconsistent.
You think I should file a new bug for the File -> New -> Message case?

Comment 9

[Thomas D. (:thomas8)]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #8)
> The Shift modifier even works when you use it on the menu Message -> Reply
> or Forward...
> It is ignored again however on the menu File -> New -> Message.
> Quite inconsistent.

+1. And obviously never "intended", just an omission.

> You think I should file a new bug for the File -> New -> Message case?

I think we can handle that here.

Comment 10

[Jim Porter (:squib)]

As a UX person now, I'm going to make a command decision: shift-clicking to open a compose window should *always* toggle between HTML and plain text. (Note that this does not apply to using keyboard shortcuts, which wouldn't work for obvious reasons!)

Comment 11

[Onno Ekker [:nONoNonO UTC+1]]

For the MsgNewMessage case it is also sufficient to change the oncommand call from MsgNewMessage(null); to MsgNewMessage(event);

There are a lot of calls to MsgNewMessage that pass null instead of event in mxr...
http://mxr.mozilla.org/comm-central/search?string=MsgNewMessage

Comment 12

[WADA:World Anti-bad-Duping Agency]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #8)
> It is ignored again however on the menu File -> New -> Message.

This is also pretty simple : 
  oncommand="MsgNewMessage(null);" is used for "New Message" at anywhere in menu,
  even though MsgNewMessage and subsequent modules correctly supports event(shift modifier key),
  except ToolBarButon for "New Message" which uses oncommand="MsgNewMessage(event);"
> http://mxr.mozilla.org/comm-central/search?string=MsgNewMessage%28&find=mail&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

> Quite inconsistent.

All "New Message" doesn't support any modifier key except ToolBar button. So, as for modifier key support in "New Message" of menu/menu button, inconsistent one == "New Message" of ToolBar button :-)

> The Shift modifier even works when you use it on the menu, Message -> Reply or Forward...

This is also pretty simple.
- Message -> New Message : still uses oncommand=oncommand="MsgNewMessage(null);"
- Message -> Reply or Forward :
             Recently, Reply/Forward related menu was sorted out,
             and command was utilized, instead of direct script call by oncommand.
             And, in new command use, modifier key is supported as we expect.
                       command="cmd_forwardAttachment"/>
                       command="cmd_forwardInline"/>
                       command="cmd_forward"/>
                       command="cmd_reply"/>
                       command="cmd_replyGroup"/>
                       command="cmd_replySender"/>
                       command="cmd_replyall"/>
                       command="cmd_replylist"/>
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#2703

FYI.
Bug 956446 is another "shift modifer loss" in "Shift+Delete of message", which is caused by inappropriate command definition/use.

Comment 13

[WADA:World Anti-bad-Duping Agency]

(In reply to Jim Porter (:squib) from comment #10)
> (Note that this does not apply to using keyboard shortcuts, which wouldn't work for obvious reasons!)
It's by design? Or due to difficulty in modifier key handling with command & Command Controller/Command Dispatcher(event object passing is pretty simple if oncommand). Or fault in menu&key definition/command design/Command Dispatcher implementation?

For example, CTRL(Command)/Shift + M & CTRL(Command) + M of File/New/Message.
    Note: Similar to Bug 956446
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailOverlay.xul#47
>   <menuitem id="menu_newMessage" label="&newMessageCmd.label;"
>             accesskey="&newMessageCmd.accesskey;" key="key_newMessage"
>             command="cmd_newMessage"/>
>   <keyset id="tasksKeys">
> #ifdef XP_MACOSX
>   <key id="key_newMessage" key="&newMessageCmd.key;" command="cmd_newMessage"
>        modifiers="accel,shift"/>
>   <key id="key_newMessage2" key="&newMessageCmd2.key;" command="cmd_newMessage"
>        modifiers="accel"/>
> #else
>   <key id="key_newMessage"  key="&newMessageCmd.key;" command="cmd_newMessage"
>        modifiers="accel"/>
>   <key id="key_newMessage2" key="&newMessageCmd2.key;" command="cmd_newMessage"
>        modifiers="accel"/>
> #endif
> 
> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/messenger/mailOverlay.dtd#5
> <!ENTITY  newMessageCmd2.key              "N">
> <!ENTITY  newMessageCmd.key               "M">
> 
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailOverlay.xul#42
>    <command id="cmd_newMessage" oncommand="goOpenNewMessage();"/>

event object can't be passed to final script if command is used?

"Removal of shift from modifiers of key_newMessage2(==N) of Mac OSX" is intentional change?
   (See Bug 672475)

By the way, Bug 687324 is sae problem as this bug, and Bug 78794 / Bug 731688 is for "Edit As New"(not problem in menu design/implementation).

Comment 14

[Onno Ekker [:nONoNonO UTC+1]]

(In reply to WADA from comment #13)
> (In reply to Jim Porter (:squib) from comment #10)
> > (Note that this does not apply to using keyboard shortcuts, which wouldn't work for obvious reasons!)
> It's by design?

I think this is by design, because a couple of Shift modifiers for the shortcut keys are used for different functions.

For example Ctrl+R does Reply and Ctrl+Shift+R does Reply All, instead of Reply in alternate format.
Also Ctrl+L does Forward and Ctrl+Shift+L does Relpy To List, instead of Forward in alternate format.

Comment 15

[WADA:World Anti-bad-Duping Agency]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #14)
> > It's by design?
> I think this is by design, because a couple of Shift modifiers for the shortcut keys are used for different functions.

Generally speaking, you are correct, and I agree with you.
However, as for (Command+N) and (Command+N)+Shift on Mac OS X for "New Message", if (Shift+N) is not assigned to other function, (Command+N)+Shift can be correctly processed with current definition by pretty simple/easy following change.
>     <command id="cmd_newMessage" oncommand="goOpenNewMessage();"/>
>  => <command id="cmd_newMessage" oncommand="goOpenNewMessage(event);"/>
>     function goOpenNewMessage() => function goOpenNewMessage(event)
>     In function goOpenNewMessage(event),
>        MsgNewMessage(null); => MsgNewMessage(event);
This(Script is directly called by oncommand in command definition) is biggest difference from Bug 956446. That bug is : Command Controller/Command Dispatcher is utilized, and oncommand="goDoCommand('CommandNameInDispatcher_For_cmd_delete');" is used, and subsequent process after the goDoCommand doesn't support event object passing via parameter.

Comment 16

[WADA:World Anti-bad-Duping Agency]

FYI.

There are three ways to process oncommand event at an XUL element.
(A) <menuitem/toolbarbutton/key oncommand="MenuCommandHandler(event);" />
    MenuCommandHandler can process event directly.
(B) <menuitem/toolbarbutton/key command="CommandID_for_MenuCommandHandler" />
    <command                         id="CommandID_for_MenuCommandHandler" 
             oncommand="MenuCommandHandler(event);" />
    MenuCommandHandler can process event directly.
(C) <menuitem command="CommandID_for_MenuCommandHandler" />
    <command       id="CommandID_for_MenuCommandHandler"
             oncommand="
               if(event.shiftKey) goDoCommand('CommandName_for_WithShift');
               else               goDoCommand('CommandName_for_NoShift')  ;"  />
    In Command Controller/Dispatcher, support CommandName_for_WithShift and CommandName_for_NoShift.
    Final MenuCommandHandler can not process event directly, because event object can't be passed.
    So, final MenuCommandHandler should support both WithShift case and NoShift case.
    Command Controller is applicable to toolbar button and key,
    but control of toolbar button and key is perhaps difficult and/or useless than menuitem.

According to bug 672475, regression happened in Tb 5.
Transfer from (A) to (B) or (C) was done in Tb 5 then "event object passing" was lost?

Comment 17

[Thomas D. (:thomas8)]

Attachment: Patch V1: Enable Shift modifier for most UI locations

Thanks WADA for your excellent analysis, which inspired me to write up a patch for this.

Can you please review and test the patch? (untested patch)
Can someone please complete the patch header (Date, Node ID, Parent)?

This should enable holding Shift to start compositions in non-default format (HTML vs. plaintext) for the following UI locations:

* Account Central: Write a new message link
* Main 3 Pane:
  o Main menu: File > New > Message
  o App menu: New Message
  o App menu: New Message > Message
* Composition Window: File > New > Message

There are still some other locations where it won't work yet even after the patch, usually involving addresses.

Comment 18

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #17)
> Created attachment 8388183 [details] [diff] [review]
> Patch V1: Enable Shift modifier for most UI locations
> 
> Thanks WADA for your excellent analysis, which inspired me to write up a
> patch for this.

...and Onno's analysis, too!

Comment 19

[WADA:World Anti-bad-Duping Agency]

Comment on attachment 8388183 [details] [diff] [review]
Patch V1: Enable Shift modifier for most UI locations

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

> -                   oncommand="MsgNewMessage(event)"/>
> +                   command="cmd_newMessage"/>
> +  <command id="cmd_newMessage" oncommand="MsgNewMessage(event)"/>

After change to "command" use from direct Script call by "oncommand", update of at least mail3PaneWindowCommands.js and messageWindow.js is perhaps needed.
See "cmd_replySender" case(and "cmd_replyGroup", "cmd_replyall" etc.), please.
> http://mxr.mozilla.org/comm-central/search?string=%22cmd_replySender%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

Comment 20

[Thomas D. (:thomas8)]

Attachment: Patch V2

Patch addresses previous comment.

I don't fully understand how these work together, so it's now more of a copy and paste approach.
WADA, can you explain this:

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

594       case "cmd_replySender":
595         MsgReplySender(null);
596         break;

So here event is not passed. But for reply commands, holding shift while clicking always works, so event must be passed. So when does this ever get executed? Only for keyboard shortcuts? And why do we define the same oncommand function in mailWindowOverlay.xul again?

http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#237

237   <command id="cmd_replySender" oncommand="MsgReplySender(event);"/>

So is it correct to pass event only for cmd_newMessage (as I did), or it will break on keyboard shortcuts e.g. Command+Shift+M on Mac?

      case "cmd_newMessage":
        MsgNewMessage(event); // <-- correct to pass event for this cmd only?
        break;
      case "cmd_reply":
        MsgReplyMessage(null);
        break;
      case "cmd_replySender":
        MsgReplySender(null);
        break;

Please test. Tia.

Comment 21

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #20)
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#594
> 594       case "cmd_replySender":
> 595         MsgReplySender(null);
> 596         break;

This part is code in doCommand.
i.e. for <command oncommand="goDoCommand('cmd_replySender');" .../> which is currently not used.
> 570   doCommand: function(command, aTab)
I dislike same name for "ID attribute value of <command> element" and "string passed to goDoCommand".
  ID attribute value of <command> element == for <element command="ID of <command>"
  string passed to goDoCommand            == string in "case switch" of doCommand feature

> And why do we define the same oncommand function in mailWindowOverlay.xul again?
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mailWindowOverlay.xul#237
> 237   <command id="cmd_replySender" oncommand="MsgReplySender(event);"/>

This is actually used <command> definition by Click/Enter of <element command="cmd_replySender"> who points "<command> element what has id=cmd_replySender".

http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#124
> 112 // DefaultController object (handles commands when one of the trees does not have focus)
> 113 var DefaultController =
> 114 {
> 115   supportsCommand: function(command)
> 116   {
> 117     switch ( command )
> 118     {
>(snip)
> 124       case "cmd_replySender":

As known by deinition, this is for "if true==DefaultController.supportsCommand('cmd_replySender')".
I think this is not "id attribute value of <command> element". I think this is for "string passed to goDoCommand". But I'm not sure.

http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#311
This is for DefaultController.isCommandEnabled('cmd_replySender'). IIUC, this is for actvate/disable(gray out) menu item. So, I think 'cmd_replySender' here == "id attribute value of <command> element". But I'm not sure.

If you use same "ID attribute value of <command> element" and "string passed to goDoCommand" always, you don't need to pay attention on difference :-)

Comment 22

[WADA:World Anti-bad-Duping Agency]

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

FYI.
Isolation of CommandID_for_MenuCommandHandler and CommandName_for_goDoCommand is required in (C) of comment #20 (Bug 956446).
 (C) <menuitem command="CommandID_for_MenuCommandHandler" />
     <command       id="CommandID_for_MenuCommandHandler"
              oncommand="
                if(event.shiftKey) goDoCommand('CommandName_for_WithShift');
                else               goDoCommand('CommandName_for_NoShift')  ;"  />
- "Conversion from CommandName_for_goDoCommand to 'actual event handler function held in what object'" is done by doCommand: function registered to CommandControler.
- "Enabling/Disabling menu item" is done via function registered to CommandControler(e.g. isCommandEnabled). In this context, "cmd_replySender" correspnds to CommandID_for_MenuCommandHandler in above (C).
So, for ease of code maintenance, for ease of understanding, naming convention like next is perhaps better.
     <command                               id="CommandID_for_MenuCommandHandler"
              oncommand="
                if(event.shiftKey) goDoCommand('CommandID_for_MenuCommandHandler_WithShift');
                else               goDoCommand('CommandID_for_MenuCommandHandler')  ;"       />
- 'case "CommandID_for_MenuCommandHandler_WithShift"' in dCommand: works as expected.
- 'case "CommandID_for_MenuCommandHandler_WithShift"'
     which is added under 'case "CommandID_for_MenuCommandHandler"' in isCommandEnabled:
  will do nothing, because <command id="CommandID_for_MenuCommandHandler_WithShift"> doesn't exist.

Comment 23

[Thomas D. (:thomas8)]

WADA, if possible, could you review my patch and set feedback+/- accordingly, and answer last question of comment 20, will my code work for potential cases of goDoCommand("cmd_newMessage"); ?

Iow, is the original click event still available in doCommand section?
How does this behave for keyboard shortcuts if they include shift modifier? If we only read event in doCommand section, will it have shiftKey attribute (which it should not, because shift is part of the shortcut key, so it should not toggle the function)?
Sorry if yr comments already answer this...

> (In reply to Thomas D. from comment #20)
> Created attachment 8388408 [details] [diff] [review]
> Patch V2
>
> http://mxr.mozilla.org/comm-central/source/mail/base/content/
> mail3PaneWindowCommands.js#594
> 
> So is it correct to pass event only for cmd_newMessage (as I did), or it
> will break on keyboard shortcuts e.g. Command+Shift+M on Mac?
> 
>       case "cmd_newMessage":
>         MsgNewMessage(event); // <-- correct to pass event for this cmd only?
>         break;
>       case "cmd_reply":
>         MsgReplyMessage(null);
>         break;
>       case "cmd_replySender":
>         MsgReplySender(null);
>         break;
> 
> Please test. Tia.

Comment 24

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #23)
> and answer last question of comment 20, (snip)

(In reply to Thomas D. from comment #20)
> http://mxr.mozilla.org/comm-central/source/mail/base/content/mail3PaneWindowCommands.js#594
> So is it correct to pass event only for cmd_newMessage (as I did), or it
> will break on keyboard shortcuts e.g. Command+Shift+M on Mac?
>       case "cmd_newMessage":
>         MsgNewMessage(event); // <-- correct to pass event for this cmd only?
>         break;
>       case "cmd_reply":
>         MsgReplyMessage(null);
>         break;
>       case "cmd_replySender":
>         MsgReplySender(null);
>         break;

How can JavaScript code there access "event object" after event on an XUL element occurs?
> 570   doCommand: function(command, aTab) 
During CommandName_for_goDoCommand of goDoCommand() is passed to final doCommand handler, "aTab" can be passed from a step to subsequent(lower) step as second parameter. However, topmost(main) goDoCommand of ToolKit doesn't support second parameter. Therfore, there is no way to pass event object as parameter.
Read Bug 956446, please.

Do you want to use <command oncommand="goDoCommand('ID_of_this_command');> instead of current <command oncommand="OurOwnFunctionName(event);"> which is currently utilized for sorted out "cmd_reply", "cmd_replySender", "cmd_replyAll", ...?

For <key> and <command> and "event, modifier".
  <key> has accel key definition.
  i.e. when a key defind by a <key> is pressed and <command id=xxx> is invoked by <key command=xxx>,
       event.shiftKey, event.altKey etc. is already processed by <key> processing code.
       when Command+N is pressed and a <command> is invoked,
       "event.shiftKey==false" is guaranteed, as far as shiftKey is not defined as accel key in <key>.

Anyway, I'm not professional of Command Controler, Command Dispatcher, so I can't review your patch, although I can give you advice based on my knowledge(I learned about Command Controler while analyzing Bug 956446, but I don't know detail of Command Controler.)

Comment 25

[WADA:World Anti-bad-Duping Agency]

(In reply to Thomas D. from comment #20)
> Created attachment 8388408 [details] [diff] [review]
> Patch V2

> diff --git a/mail/base/content/mail3PaneWindowCommands.js b/mail/base/content/mail3PaneWindowCommands.js
> @@ -583,14 +586,17 @@
> +        break;
> +      case "cmd_newMessage":
> +        MsgNewMessage(event);
> diff --git a/mail/base/content/messageWindow.js b/mail/base/content/messageWindow.js
> @@ -1061,14 +1063,17 @@
> +        break;
> +      case "cmd_newMessage":
> +        MsgNewMessage(event);

(a) "event" is not passed to this function(doCommand). 
    "reference to undefined variable" may occur.
(b) Because <command id="cmd_newMessage" oncommand="goDoCommand('cmd_newMessage');" /> is never used,
    there is no need to add code here.
    However, for ease of future use, 'case "cmd_newMessage":' is better added here, as done for
    cmd_replySender.
    I prefer code like next;
      break;
      case "cmd_newMessage":
         // dummy, for future goDoCommand('cmd_newMessage' or 'cmd_newMessage_Shift') use
         var event=null; MsgNewMessage(event);

Comment 26

[Thomas D. (:thomas8)]

Attachment: Patch V2.1

Jim, this should be a quick review, just a few lines to enable Shift modifier more systematically on UI elements which create a new message (per your UX decision of Comment 10, which is exactly what I've always advocated for in this bug). Tia.

(In reply to WADA from comment #25)
> (b) Because <command id="cmd_newMessage"
> oncommand="goDoCommand('cmd_newMessage');" /> is never used,
>     there is no need to add code here.
>     However, for ease of future use, 'case "cmd_newMessage":' is better
> added here, as done for
>     cmd_replySender.

Done. I don't want to spend more time on this than necessary, so I just took the way of least resistance and tried to make cmd_newMessage in analogy to cmd_replySender, passing (null) because (event) can't be passed into goDoCommand function. Anyway, these things are mostly used for keyboard shortcuts or SeaMonkey only, and they are not very consistent. So hopefully it's a bit better than before because cmd_newMessage is now listed. Otherwise let the experts judge.

Comment 27

[Jim Porter (:squib)]

Comment on attachment 8392902 [details] [diff] [review]
Patch V2.1

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

There are a few small issues with this patch, so r- for now. See below. UI-wise, this is good though.

Thanks for working on it!

::: mail/components/compose/content/MsgComposeCommands.js
@@ +884,5 @@
>  /**
>   * Start composing a new message.
>   */
> +function goOpenNewMessage(aEvent)
> +{ // If aEvent was passed, check if Shift key was pressed for composition in non-default format (HTML vs. plaintext).

This comment should be on a new line and wrapped to 80 characters.

::: mail/base/content/mail3PaneWindowCommands.js
@@ +362,5 @@
>          if (GetNumSelectedMessages() == 1)
>            return gFolderDisplay.getCommandStatus(nsMsgViewCommandType.cmdRequiringMsgBody);
>          return false;
> +      case "cmd_newMessage":
> +      // This enables Write button even without any accounts set up, so users might run into Bug 524863

We should fix this before we land this patch. You could just check if there is at least one account.

@@ +591,5 @@
>        case "cmd_archive":
>          MsgArchiveSelectedMessages(null);
> +        break;
> +      case "cmd_newMessage":
> +        MsgNewMessage(event);

I'm fairly sure event is undefined in this function. (But in any case, this should never be called.)

Comment 28

[Thomas D. (:thomas8)]

Attachment: Patch V2.2 (addressing review comment 27)

Jim, thanks for quick review. I've addressed your issues except one which imo doesn't apply and should be fixed in its own bug, not here.

(In reply to Jim Porter (:squib) from comment #27)
> Comment on attachment 8392902 [details] [diff] [review]
> Patch V2.1
> 
> Review of attachment 8392902 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a few small issues with this patch, so r- for now. See below.
> UI-wise, this is good though.
> 
> Thanks for working on it!
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +884,5 @@
> >  /**
> >   * Start composing a new message.
> >   */
> > +function goOpenNewMessage(aEvent)
> > +{ // If aEvent was passed, check if Shift key was pressed for composition in non-default format (HTML vs. plaintext).
> 
> This comment should be on a new line and wrapped to 80 characters.

Done.

> ::: mail/base/content/mail3PaneWindowCommands.js
> @@ +362,5 @@
> >          if (GetNumSelectedMessages() == 1)
> >            return gFolderDisplay.getCommandStatus(nsMsgViewCommandType.cmdRequiringMsgBody);
> >          return false;
> > +      case "cmd_newMessage":
> > +      // This enables Write button even without any accounts set up, so users might run into Bug 524863
> 
> We should fix this before we land this patch. You could just check if there
> is at least one account.

I think it's not necessary to fix this here because afasics this never gets called from current code (see your comment below), and even if it were to get called, this patch doesn't change anything compared to current UX. This is a can of worms which needs to be fixed in its own bug (Bug 524863, Bug 713277) for which there's no consistent design approach yet.

> @@ +591,5 @@
> >        case "cmd_archive":
> >          MsgArchiveSelectedMessages(null);
> > +        break;
> > +      case "cmd_newMessage":
> > +        MsgNewMessage(event);
> 
> I'm fairly sure event is undefined in this function. (But in any case, this
> should never be called.)

Done.

Comment 29

[Thomas D. (:thomas8)]

Jim, I have addressed your review comments which are pertinent to this bug (see comment 28), so if you have a minute to look over this little patch here again and set review+, it would be nice to land this before TB31 branch deadline of April 28th (although it's not urgent or very important as such). Tia.

Comment 30

[Jim Porter (:squib)]

Comment on attachment 8408079 [details] [diff] [review]
Patch V2.2 (addressing review comment 27)

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

Looks good! rs=me

Comment 31

[Thomas D. (:thomas8)]

Ryan, sorry I can't do patch headers because I'm not building. Tia for checkin.
ui-r=squib, r=squib

Jim, thanks for rapid review.

Wada, thanks for valuable research & information.

Comment 32

[:aceman]

Thomas, you can put those headers manually into the patch:
# HG changeset patch
# User Thomas....

The "# Parent" line you can see in other's patches is optional.

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/f169a340dd89

Comment 34

[Ryan VanderMeulen [:RyanVM]]

Backed out for mozmill failures.
https://hg.mozilla.org/comm-central/rev/fa28dedcd009

https://tbpl.mozilla.org/php/getParsedLog.php?id=38327502&tree=Thunderbird-Trunk

Comment 35

[Thomas D. (:thomas8)]

ace, or anybody, can you please look into those two or three mozmill failures this patch has caused. Unfortunately, I know nothing about that.

Comment 36

[Magnus Melin [:mkmelin]]

So how to do this is go to the log - https://tbpl.mozilla.org/php/getParsedLog.php?id=38327502&tree=Thunderbird-Trunk and then go to the full log at 
https://tbpl.mozilla.org/php/getParsedLog.php?id=38327502&full=1&branch=comm-central

Then search down to the ::test_compose_from_composer details

SUMMARY-UNEXPECTED-FAIL | test-newmsg-compose-identity.js | test-newmsg-compose-identity.js::test_compose_from_composer
  EXCEPTION: Timed out waiting for window open!
    at: utils.js line 447
       TimeoutError utils.js:447 1
       waitFor utils.js:485 5
       WindowWatcher_waitForWindowOpen test-window-helpers.js:253 1
       wait_for_new_window test-window-helpers.js:575 1
       wait_for_compose_window test-compose-helpers.js:218 1
       test_compose_from_composer test-newmsg-compose-identity.js:68 1
       Runner.prototype.wrapper frame.js:585 9
       Runner.prototype._runTestModule frame.js:655 9
       Runner.prototype.runTestModule frame.js:701 3
       Runner.prototype.runTestDirectory frame.js:525 7
       runTestDirectory frame.js:707 3
       Bridge.prototype._execFunction server.js:179 3
       Bridge.prototype.execFunction server.js:183 1
       Session.prototype.receive server.js:283 3
       AsyncRead.prototype.onDataAvailable server.js:88 3

 => failure at test-newmsg-compose-identity.js line 68 -> http://mxr.mozilla.org/comm-central/source/mail/test/mozmill/composition/test-newmsg-compose-identity.js#68

The visual state can be checked with arbpl at
http://clicky.visophyte.org/tools/arbpl-mozmill-standalone/?log=http%3A%2F%2Fftp.mozilla.org%2Fpub%2Fmozilla.org%2Fthunderbird%2Ftinderbox-builds%2Fcomm-central-linux%2F1398257644%2Fcomm-central_ubuntu32_vm_test-mozmill-bm06-tests1-linux32-build15.txt.gz

[no time to investigate why]

Comment 37

[:aceman]

Can you check this code in the patch:
+function goOpenNewMessage(aEvent)
+{
+  // If aEvent is passed, check if Shift key was pressed for composition in
+  // non-default format (HTML vs. plaintext).
+  if (aEvent && aEvent.shiftKey) {
+    let msgCompFormat = Components.interfaces.nsIMsgCompFormat.OppositeOfDefault;
+  } else {
+    let msgCompFormat = Components.interfaces.nsIMsgCompFormat.Default;
+  }
   let identity = getCurrentIdentity();
   MailServices.compose.OpenComposeWindow(null, null, null,
     Components.interfaces.nsIMsgCompType.New,
-    Components.interfaces.nsIMsgCompFormat.Default, identity, null);
+    msgCompFormat, identity, null);

It looks like msgCompFormat is defined only inside if() but you then use it outside of it.

Comment 38

[:aceman]

'let' makes the variable local to the enclosing block. 'var' makes it global. But in this case, just shuffle the declaration so it is local to the whole goOpenNewMessage function. I wonder whether this didn't produce a warning in the Error console. Maybe only with some strict javascript settings.

Comment 39

[Thomas D. (:thomas8)]

Attachment: Patch V2.3: Address comment 37 (fix test failure from undefined variable)

Nitfix simple variable scope issue of comment 37, otherwise unchanged, hence forwarding r+ui-r=squib.

This should pass the tests now.

Comment 40

[Thomas D. (:thomas8)]

(In reply to :aceman from comment #37)
> Can you check this code in the patch:
> +function goOpenNewMessage(aEvent)
> +  if (aEvent && aEvent.shiftKey) {
> +    let msgCompFormat =
> It looks like msgCompFormat is defined only inside if() but you then use it
> outside of it.

Thanks, good catch! :)

(In reply to :aceman from comment #38)
> 'let' makes the variable local to the enclosing block. 'var' makes it
> global.

Well yes, but it depends:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/var#Description

> The scope of a variable declared with *var* is _the enclosing function_ *or*, for variables declared
> _outside a function, the global scope_ (which is bound to the global object).

So in this case, I could have safely used var for function scope.

> But in this case, just shuffle the declaration so it is local to the
> whole goOpenNewMessage function.

Shorthand if ("Conditional Operator") solves the problem very elegantly, as it's not only shorter, but also doesn't lock variables in inner if/else blocks... :)

+  let msgCompFormat = (aEvent && aEvent.shiftKey) ? 
+    Components.interfaces.nsIMsgCompFormat.OppositeOfDefault :
+    Components.interfaces.nsIMsgCompFormat.Default;

In fact, you can do amazing things with "Conditional Operator", check out the full documentation here:

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Operators/Conditional_Operator

Comment 41

[Thomas D. (:thomas8)]

Comment on attachment 8411745 [details] [diff] [review]
Patch V2.3: Address comment 37 (fix test failure from undefined variable)

Aceman, could you glance over this 3-line-change quickly if it's correct (per Comment 40), just in case, and then set checkin-needed? Tia.

Comment 42

[:aceman]

Comment on attachment 8411745 [details] [diff] [review]
Patch V2.3: Address comment 37 (fix test failure from undefined variable)

I can't run with this patch right now, but the change to goOpenNewMessage seems correct.

Comment 43

[Thomas D. (:thomas8)]

Ready for checkin again :)

(In reply to Thomas D. from comment #39)
> Created attachment 8411745 [details] [diff] [review]
> Patch V2.3: Address comment 37 (fix test failure from undefined variable)
> 
> Nitfix simple variable scope issue of comment 37 (with f+=aceman), otherwise unchanged, hence
> forwarding r+ui-r=squib.
> 
> This should pass the tests now.

Comment 44

[:aceman]

Aryx, could you push this to try server (mozmill only) before it blows up trunk? :)

Comment 45

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed: https://tbpl.mozilla.org/?tree=Thunderbird-Try&showall=1&rev=0407e30e6c5d

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/comm-central/rev/a129bf9b1057

Comment 47

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Canceled the Thunderbird-Try builds because the patch landed on comm-central.

Comment 48

[:aceman]

Congrats, looks like the tests worked now ;)

Comment 49

[Thomas D. (:thomas8)]

(In reply to Onno Ekker [:nONoNonO UTC+1] from comment #0)
> When you click on an account in the folder pane and then Shift+Click in the
> Account Central pane om the Write a new message link, the Shift modifier to
> switch to the alternate composing format is ignored.

Onno, thanks for reporting. This works now, and will land with TB 31 :)
Verified fixed for all spots covered by the patch, on WinXP, TB Trunk 32.0a1 (2014-05-02).