482080 - new message created from compose window should use the same initial identity as its parent compose window

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Wayne Mery (:wsmwk)]

new message created from compose is forced to default account (B) when another account (A) is selected

1. click on non-default account A
2. create a new message 1 - it uses identity associated with account A
3. create new message 2 from compose window

expected results:
message 2 is addressed from account A

actual results: 
message 2 is addressed from default account B

Comment 1

[WADA:World Anti-bad-Duping Agency]

Behaviour was observed with Tb trunk 2009/3/02 build on MS Win-XP SP3.
Which should be set as "pre-selected From:" do you think?
 (a) Main identity of default account (current behaviour)
 (b) Main identity of selected account at folder pane (you say your expectation)
 (c) Identity currently selected in compose window
If (b), which one is the "selected account" when multiple mail windows are opened and different accounts are selected?

Comment 2

[Wayne Mery (:wsmwk)]

I would argue using context, so (c) in the case of compose.

It muddies and perhaps hurts my argument for changing compose behavior as suggested by comment 0, but I suppose we should consider the larger question - i.e. should behavior deviate (or not) for other cases. For example:
* when default account B is not even logged in
* when account A is selected and some other window is open, like Address book

Comment 3

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

I think the choice order is supposed to be:

default account OR
  account of folder pane selection OR
     account of message focus selection

The last one is because we have cross account folders now.  I think the real expectation from users is to give the account of whatever you are looking at / focus is on.

Comment 4

[Wayne Mery (:wsmwk)]

(In reply to Bryan Clark [:clarkbw] from comment #3)
> I think the choice order is supposed to be:
> 
> default account OR
>   account of folder pane selection OR
>      account of message focus selection
> 
> The last one is because we have cross account folders now.  I think the real
> expectation from users is to give the account of whatever you are looking at
> / focus is on.

In that case, current behavior is incorrect

Comment 5

[Philip Prindeville]

This should be fairly straightforward to implement, right?

Comment 6

[Magnus Melin [:mkmelin]]

Since this is about creation of a new message from the compose window, shouldn't the default actually be to use the same identity that is currently selected in that window?

Comment 7

[Philip Prindeville]

I'd say this isn't strictly limited to doing a "New Message" from the compose window, but in general.

That is, from the Mailbox view window a "New Message" should use the identity from the currently selected tab.

Comment 8

[David :Bienvenu]

yeah, I agree that we should do this. I'm surprised that we don't, actually.

Comment 9

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Only the goOpenNewMessage changes are really changes here besides the onliner xxx fix to escape & in html. I got rid of two unnecessary if-blocks - we already used the foo variables of if(foo) later regardless, so obviously there were no need for the if blocks.

Comment 10

[Andrew Sutherland [:asuth] (he/him)]

Comment on attachment 606522 [details] [diff] [review]
proposed fix

mconley has indicated a willingness to review in the compose sub-module, so I am deferring review to him to keep this all legit.  I'll poke binevenu/standard8 to try and codify an explicit module owner/peers.

Comment 11

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 606522 [details] [diff] [review]
proposed fix

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

This looks good Magnus!

Just a few questions / comments / suggestions.

All the best,

-Mike

::: mail/components/compose/content/MsgComposeCommands.js
@@ +38,5 @@
>   *
>   * ***** END LICENSE BLOCK ***** */
>  
> +/**
> + * Commands for the message composition win

A nit - I don't see much advantage to shortening "window" to "win".  Might as well use the whole word here, since I was initially confused about what we were winning wrt composition.  :)

@@ +49,5 @@
>  
>  Components.utils.import("resource:///modules/MailUtils.js");
>  Components.utils.import("resource:///modules/errUtils.js");
>  Components.utils.import("resource:///modules/iteratorUtils.jsm");
>  Components.utils.import("resource://gre/modules/InlineSpellChecker.jsm");

This might be a lovely opportunity to import mailServices.js, since we can use it in goOpenNewMessage...

@@ +783,1 @@
>     msgComposeService.OpenComposeWindow(null, null, null,

If we import mailServices.js, we can just use MailServices.compose.OpenComposeWindow(... here.

@@ +832,2 @@
>  
>    gLastWindowToHaveFocus = focusedWindow;

Thanks for cleaning this stuff up. :)

@@ +2207,5 @@
>  
>    // Get the <editor> element to startup an editor
>    var editorElement = GetCurrentEditorElement();
>    gMsgCompose = composeSvc.initCompose(params, window, editorElement.docShell);
> +

You've removed the check to ensure gMsgCompose exists, and we didn't fail out of the initCompose function.  Was this on purpose?  If so, why?

@@ +2227,2 @@
>    {
> +    var editortype = gMsgCompose.composeHTML ? "htmlmail" : "textmail";

While you're here, if you wouldn't mind replacing this "var" with "let", that'd be awesome. :)

@@ -2259,5 @@
> -      InitLanguageMenu();
> -    }
> -
> -    var msgCompFields = gMsgCompose.compFields;
> -    if (msgCompFields)

A check for gMsgCompose.compFields was also removed.  On purpose?

@@ +2260,5 @@
> +      } catch(e) { cleanBody = body;}
> +
> +      body = body.replace("&", "&amp;", "g");
> +      gMsgCompose.compFields.body =
> +        "<br /><a href=\"" + body + "\">" + cleanBody + "</a><br />";

I'm no security expert, but are we not opening up ourselves here to a potential security issue for the sender if a specially crafted, malicious link gets accidentally inserted into the body?  Would it / could it execute chrome-level JS?

@@ +2273,5 @@
> +
> +  AddAttachments(gMsgCompose.compFields.attachments);
> +
> +  var event = document.createEvent("Events");
> +  event.initEvent("compose-window-init", false, true);

We've moved the compose-window-init Event to fire *before* the recycled check, instead of after now.  If at all possible, we should probably maintain the event order for our add-on developers.  If it's necessary to change the order of events here, we might need to update our developer documentation for the compose-window-init event (assuming we have some...)

@@ +2289,5 @@
> +      onFontColorChange();
> +      onBackgroundColorChange();
> +    }
> +
> +    // Reset the priorty field for recycled windows.

typo: priority

::: mail/test/mozmill/composition/test-newmsg-compose-identity.js
@@ +2,5 @@
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/**
> + * Tests that compose new message chooses the correct initial identity.

We should mention that we're testing this from the context of an open composer.

@@ +11,5 @@
> +const RELATIVE_ROOT = "../shared-modules";
> +const MODULE_REQUIRES = ["folder-display-helpers",
> +                         "window-helpers", "compose-helpers"];
> +
> +var folderHelper = null;

We can cut these variables - see my comment in setupModule.

@@ +16,5 @@
> +var windowindowHelperelper = null;
> +var composeHelper = null;
> +
> +var testFolder1 = null;
> +var testFolder2 = null;

Neither of these vars seem to be used, and can be axed.

@@ +21,5 @@
> +
> +const identity1Email = "[e-mail]";
> +const identity2Email = "[e-mail]";
> +
> +var setupModule = function (module) {

I think we can just have this be:

function setupModule(module) {
  // ...
}

...unless there's some hidden advantage to setting it to a var like this that I'm not seeing.

@@ +22,5 @@
> +const identity1Email = "[e-mail]";
> +const identity2Email = "[e-mail]";
> +
> +var setupModule = function (module) {
> +  folderHelper = collector.getModule("folder-display-helpers");

We can do away with folderHelper, windowHelper and composeHelper using:

collector.getModule("folder-display-helpers").installInto(module);

where "folder-display-helpers" can also be "window-helpers" and "compose-helpers".

@@ +32,5 @@
> +
> +  setupAccounts();
> +}
> +
> +var setupAccounts = function() {

Again, not seeing much advantage to setting this function to a var like this... I think I'd prefer

function setupAccounts() {
  // ...
}

@@ +33,5 @@
> +  setupAccounts();
> +}
> +
> +var setupAccounts = function() {
> +  let acctMgr = Cc["@mozilla.org/messenger/account-manager;1"]

mailServices.js is the new hotness - if you import that in the header of the file, you can access the account manager via

MailServices.accounts

@@ +53,5 @@
> +}
> +
> +var checkCompIdentity = function(composeWin, expectedFromEmail) {
> +  let identityList = composeWin.e("msgIdentity");
> +  if (identityList.selectedItem.label.indexOf(expectedFromEmail) == -1)

Can we not use assert_equals, or at the very least, assert_true here?

@@ +61,5 @@
> +}
> +
> +function test_account_comp() {
> +  function test_account(aAccount) {
> +    be_in_folder(aAccount.incomingServer.rootFolder

Nit - I think I'd prefer:

be_in_folder(aAccount.incomingServer
                     .rootFolder
                     .getFolderWithFlags(Ci.nsIMsgFolderFlags.Inbox));

(If Bugzilla / Splinter has botched the formatting, I've moved rootFolder to the next line and lined up the periods.)

@@ +64,5 @@
> +  function test_account(aAccount) {
> +    be_in_folder(aAccount.incomingServer.rootFolder
> +                         .getFolderWithFlags(Ci.nsMsgFolderFlags.Inbox));
> +
> +    let mainCompWin = composeHelper.open_compose_new_mail();

you can just use open_compose_new_mail without composeHelper, since we installed it into this module.  Applies elsewhere in this function.

@@ +68,5 @@
> +    let mainCompWin = composeHelper.open_compose_new_mail();
> +    checkCompIdentity(mainCompWin, aAccount.defaultIdentity.email);
> +
> +    // Compose a new message from the compose window.
> +    windowHelper.plan_for_new_window("msgcompose");

Just use plan_for_new_window, instead of windowHelper.plan_for_new_window

@@ +70,5 @@
> +
> +    // Compose a new message from the compose window.
> +    windowHelper.plan_for_new_window("msgcompose");
> +    mainCompWin.keypress(null, "n", {shiftKey: false, accelKey: true});
> +    let newCompWin = composeHelper.wait_for_compose_window();

Just use wait_for_compose_window().

Comment 12

[Magnus Melin [:mkmelin]]

(In reply to Mike Conley (:mconley) from comment #11)
> Comment on attachment 606522 [details] [diff] [review]

Thx for the review!

> You've removed the check to ensure gMsgCompose exists, and we didn't fail
> out of the initCompose function.  Was this on purpose?  If so, why?

Yes, it's not needed. 
http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2233 then not created inside the if block, and directly after the if-block we'd still blow up since we access it at http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337

> > -    var msgCompFields = gMsgCompose.compFields;
> > -    if (msgCompFields)
> 
> A check for gMsgCompose.compFields was also removed.  On purpose?

Yes,  gMsgCompose.compFields http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337, then accessed (also) in http://mxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#2337 where we'd blow up.

I think in both cases there is some possibility it's null, likely OOM situations and maybe if you happen to hit the code during a suitable time during shutdown - but those would be situations where it doesn't really matter as things won't work anyway.

> > +      body = body.replace("&", "&", "g");
> > +      gMsgCompose.compFields.body =
> > +        "<br /><a href=\"" + body + "\">" + cleanBody + "</a><br />";
> 
> I'm no security expert, but are we not opening up ourselves here to a
> potential security issue for the sender if a specially crafted, malicious
> link gets accidentally inserted into the body?  Would it / could it execute
> chrome-level JS?
 
No, it's just inserting text in an editor, the potential code is never run. When that page is viewed by someone it has whatever privileges its context gives it (what the recipients client allows it to do).

> We've moved the compose-window-init Event to fire *before* the recycled
> check, instead of after now.  If at all possible, we should probably
> maintain the event order for our add-on developers.  If it's necessary to
> change the order of events here, we might need to update our developer
> documentation for the compose-window-init event (assuming we have some...)

I didn't change the order here, what do you mean?

> > +var setupAccounts = function() {
> 
> Again, not seeing much advantage to setting this function to a var like
> this... I think I'd prefer

I don't know, though it seems to be fairly common. I always thought it had something to do with getting mozmill to not count executing that function as a passed test, but it doesn't seem to have much effect. Anyway, changed it.

Comment 13

[Magnus Melin [:mkmelin]]

Attachment: proposed fix v2

Comment 14

[Jb Piacentino]

Does this patch address the Unified view case ? I can see the situation where, in this view, the 'Inbox' folder is selected with no particular account. TB should then use the default account.

Comment 15

[David :Bienvenu]

(In reply to Jb Piacentino from comment #14)
> Does this patch address the Unified view case ? I can see the situation
> where, in this view, the 'Inbox' folder is selected with no particular
> account. TB should then use the default account.

that depends on whether there is a message selected at the time - if there is, it should use the identity best associated with that message.

Comment 16

[Mike Conley (:mconley) (:⚙️)]

(In reply to David :Bienvenu from comment #15)
> (In reply to Jb Piacentino from comment #14)
> > Does this patch address the Unified view case ? I can see the situation
> > where, in this view, the 'Inbox' folder is selected with no particular
> > account. TB should then use the default account.
> 
> that depends on whether there is a message selected at the time - if there
> is, it should use the identity best associated with that message.

Hm, that's definitely not what this patch is doing.

This patch only seems to consider the selection in the folderPane.

Magnus - is it possible to take into consideration the selected message?

Comment 17

[Jb Piacentino]

(In reply to David :Bienvenu from comment #15)
> that depends on whether there is a message selected at the time - if there
> is, it should use the identity best associated with that message.
Not sure I agree here. I would have found more natural to ha

Comment 18

[Jb Piacentino]

(...fingers slipped to tab/enter... arggh)
Not sure I agree here. I would have found more natural to have the default account selected if you _create_ a new message. Replying should obviously use the recipient's account.

Comment 19

[Mike Conley (:mconley) (:⚙️)]

CC'ing bwinton.

Comment 20

[David :Bienvenu]

(In reply to Jb Piacentino from comment #17)
> (In reply to David :Bienvenu from comment #15)
> > that depends on whether there is a message selected at the time - if there
> > is, it should use the identity best associated with that message.
> Not sure I agree here. I would have found more natural to ha

You probably have a much stronger default account. For me, if I'm reading a work message, and click new message (or forward), I probably want it to come from my work account. I don't get why the folder that's selected is more meaningful than the message I'm reading, which is the case you're making.

Comment 21

[Magnus Melin [:mkmelin]]

The 3pane doesn't currently look at the selected msg when choosing identity, only the folder. I agree it might be better to do that there though, to better accommodate unified folders and the global inbox.

For this bug it's more fuzzy and i bet creating a new message from the compose window is a lot less used. What if you went to read something in between while composing? Maybe we should do comment 6 instead? (clone the currently selected identity of the composer window opening a new msg)

Comment 22

[David :Bienvenu]

oh, new message from an already open compose window? duh, how embarrassing, I didn't notice that part of this. I think we should clone the currently selected identity, like you say.

Comment 23

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 608315 [details] [diff] [review]
proposed fix v2

I like the plan from comment #6 - using the currently selected identity in the compose window that spawns the new composer.

If we're going with that, r-, since this patch doesn't do that.  :)

Comment 24

[Magnus Melin [:mkmelin]]

Attachment: proposed fix v3

Comment 25

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 609024 [details] [diff] [review]
proposed fix v3

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

Just two small nits.  Other than that, this looks great, and works as advertised.  +1 for a nice test case.  :)

Thanks for your work, Magnus!

-Mike

::: mail/test/mozmill/composition/test-newmsg-compose-identity.js
@@ +55,5 @@
> + * expected initial identity.
> + */
> +function test_compose_from_composer() {
> +  be_in_folder(account.incomingServer
> +                       .rootFolder

Lines 59 and 60 are misaligned by a single space.

@@ +71,5 @@
> +
> +  // Switch to identity2 in the main compose window, new compose windows
> +  // starting from here should use the same identiy as it's "parent".
> +  let identityList = mainCompWin.e("msgIdentity");
> +  identityList.selectedIndex++;

I think this is a little too generic.  For testing, I'd prefer a "harder" value, like:

identityList.selectedIndex = 1; // The identity at index 1 is the second identity

Comment 26

[Magnus Melin [:mkmelin]]

(In reply to Mike Conley (:mconley) from comment #25)
> > +  // Switch to identity2 in the main compose window, new compose windows
> > +  // starting from here should use the same identiy as it's "parent".
> > +  let identityList = mainCompWin.e("msgIdentity");
> > +  identityList.selectedIndex++;
> 
> I think this is a little too generic.  For testing, I'd prefer a "harder"
> value, like:
> 
> identityList.selectedIndex = 1; // The identity at index 1 is the second
> identity

IIRC, it may not be 1 since we have one other account set up too. It's likely identity 2, but I think it's safer to just ++

Comment 27

[Mike Conley (:mconley) (:⚙️)]

(In reply to Magnus Melin from comment #26)
> IIRC, it may not be 1 since we have one other account set up too. It's
> likely identity 2, but I think it's safer to just ++

Suit yourself. It's not a big deal.

Comment 28

[Magnus Melin [:mkmelin]]

bwinton: ui-r ping

Comment 29

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

Comment on attachment 609024 [details] [diff] [review]
proposed fix v3

Yeah, that makes sense to me.  I agree that we might want to take a look at the currently selected message when determining the identity to use in unified (or search) folders, but that can totally be a separate bug.  :)

ui-r=me!

Later,
Blake.

Comment 30

[Magnus Melin [:mkmelin]]

http://hg.mozilla.org/comm-central/rev/bc7c9ecd1292 ->FIXED