903390 - On reply to a own sent email the original sender identity is not chosen if other than the account default identity

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[lk]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_3) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.95 Safari/537.36

Steps to reproduce:

(1) Send an email
(2) Navigate to the Sent folder
(3) Reply to your own email


Actual results:

A message compose window appears where the recipients are the same as in the original email and the sender identity is the default identity


Expected results:

A message compose window should appear where the recipients are the same as in the original email and the sender identity is the same identity that was used to send the previous email

Comment 1

[Matt]

Just got struck by this bug today with latest version (38.4.0) on Windows x64. The problem occurs when either replying or forwarding a message from the sent box. Now my client knows my personal email address. Thanks for leaving this bug unfixed for 2 years. How many more years before we get a fix?

Comment 2

[Greg Colyer]

Notice that the original sender address is correctly omitted from the new recipient list, so in that sense Thunderbird does already "know" which identity was involved. The correct behaviour is to use the omitted identity as the sender, which does indeed happen for messages where this identity appears as a recipient (i.e. typically messages in your Inbox rather than Sent folder).

Can we specify, simply, a completely general algorithm for the correct behaviour of Reply? I haven't tested what the current behaviour is when more than one identity appears in the relevant headers, but excepting that case I would say the algorithm should be: if the sender address is an identity, then don't change anything; otherwise, move the sender address to the To list, and then if an identity appears in the recipient (To, failing that Cc, failing that maybe BCc) list, move it to the sender address. The default identity would then be used only as the fallback if no identity appears in the relevant headers.

Comment 3

[Masashi FUJITA]

I'm also suffered by same problem as Matt's for long time since mozilla had equipped multi-accounts management. Is this such difficult to implement? Can somebody please help?

Comment 4

[Magnus Melin [:mkmelin]]

Stange. I see this how, but I'm pretty sure it used to work.

Comment 5

[Masashi FUJITA]

I reported another bug related to account management on reply.
May be may not be related. Bug1240748

Comment 6

[Masashi FUJITA]

Hi Magnus, 

Would you please give update to those who are longing this fix?

Thanks.

Comment 7

[Magnus Melin [:mkmelin]]

I was wrong. This never worked.

Comment 8

[Magnus Melin [:mkmelin]]

Attachment: bug903390_reply_to_self_broken.patch

Comment 9

[Jorg K (CEST = GMT+2)]

I have a bit of trouble with this. First, I don't really understand what the problem is. And then, the patch doesn't do anything for me. Here is what I did:

I have many accounts, for the sake of the argument, say [e-mail] and [e-mail].

If I send e-mail from a1, it will go to a Sent box belonging to the folders of a1.
If I send e-mail from a2, it will go to a Sent box belonging to the folders of a2.

If I reply to an e-mail in the a1 Sent box, the new e-mail will be sent to the original recipient coming from a1. And if I do it from a2, it will be sent from a2 again. So that works and has always worked.

So what is the case that doesn't work? OK, I now copied an e-mail originally sent from a1 to a2's Sent box and replied from a2's sent box. If I understood the bug correctly, this should now be sent from a1, since it originally came from a1 and go to the original recipient.

But that doesn't happen, neither with nor without the patch. What I get is an e-mail from a2 (since I'm in a2's Sent box when replying) going to a1.

I did a tiny bit of debugging in nsCompose.cpp and I didn't go through the isReplyToSelf part.

<off topic>
What I really hate in TB is that when I reply to an e-mail I sent in a local folder, the reply goes to myself, and the sender is also me. That ought to be fixed one day
</off topic>

Comment 10

[Jorg K (CEST = GMT+2)]

Forget what I said. But why doesn't anyone tell me that I need to set mailnews.reply_to_self_check_all_ident :-((

Will review again now.

Comment 11

[Magnus Melin [:mkmelin]]

Not sure if you meant you already understood the problem, but anyway: the problem is when you've composed something using a second identity of an account. Account was chosen correctly, but not the correct identity of the account.

Comment 12

[Jorg K (CEST = GMT+2)]

Now I set mailnews.reply_to_self_check_all_ident to true.
When replying to an e-mail sent from a1 which was moved to a2's Sent box ...
without the patch I get an e-mail from a2 to the original recipient,
with    the patch I get an e-mail from a1 to the original recipient. So that's an improvement.

> Not sure if you meant you already understood the problem ...
Perhaps I shouldn't be reviewing this since I only learned just now that an account can have multiple identities ;-(

I'll look at it further. The C++ part looks good, the test looks good, now I just need to understand the JS bit.

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8735972 [details] [diff] [review]
bug903390_reply_to_self_broken.patch

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

Looks good. I've tested the intended case with more than one identity in the account and also some unintended cases with mailnews.reply_to_self_check_all_ident set to true where this patch also gives an improvement, see comment #12.

I've added debug to the C++ and the JS to make sure that it behaves as expected.

I've even run the test. Would you mind adding this comment to the start of that file:
// mozmake SOLO_TEST=message-header/test-reply-identity.js mozmill-one

::: mail/test/mozmill/message-header/test-reply-identity.js
@@ +81,5 @@
> +  add_message_to_folder(testFolder, create_message({
> +    from: identity2Email + " <" + identity2Email+ ">",
> +    to: "Marge <[e-mail]>",
> +    subject: "from second self",
> +    body: {body: "All my life I've had one dream, to achieve my many goals."}

https://en.wikiquote.org/wiki/The_Simpsons/Season_14#C.E._D.27oh ;-)

Comment 14

[Masashi FUJITA]

Wow! Seems the fix is coming soon! Can't wait to have it. 

Thanks very much!!!

Comment 15

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/e022ba0d3f29 -> FIXED

Comment 16

[Jorg K (CEST = GMT+2)]

Backed out due to test failures at Magnus' request:
https://hg.mozilla.org/comm-central/rev/610287517070

Comment 17

[Jorg K (CEST = GMT+2)]

I'm running a Daily of 2016-04-04 which was compiled before the backout. I'm noticing that when I reply, I get two signatures.

I've just switched to the Daily of 2016-04-05 after this was backed out. Now I don't get two signatures any more.

Please consider this problem when working on the patch.

Comment 18

[Magnus Melin [:mkmelin]]

With what settings to you get the double signature? Haven't been able to reproduce that.

Comment 19

[Jorg K (CEST = GMT+2)]

Not easy to reproduce, I struggled to find it again.

This is what I've done:
Latest trunk plus this patch here.
Signature is simple, just some plain text "JK sig".
mailnews.reply_to_self_check_all_ident set to true.
Now I reply to a message I have written to my mother from a local folder.
Without the preference set, the message goes from myself to myself. Grrrr. So silly this behaviour.
With the preference set, the message goes from me to my mother (as desired, I love this preference), but it has two signatures. One below (as configured, answer below, signature below) and one right above. Looks very funny.

Would you like to see a screen shot?

Comment 20

[Magnus Melin [:mkmelin]]

Attachment: bug903390_reply_2nd_identity.v2.patch

SetIdentity() did more than one would have expected. That "more" should really be split out somewhere separate, but then again, we already have a processSignature...

Comment 21

[Jorg K (CEST = GMT+2)]

Comment on attachment 8741099 [details] [diff] [review]
bug903390_reply_2nd_identity.v2.patch

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

Do you have a try run so we don't have to deal with test failures later? Or should I just run the test-reply-addresses.js test manually?

::: mail/test/mozmill/composition/test-reply-addresses.js
@@ +798,2 @@
>        "addr_bcc": ["[e-mail]"],
> +      // xxxmagnus [e-mail],[e-mail] not cleared due to id change

Oops, xxxmagnus ;-)

Comment 22

[Jorg K (CEST = GMT+2)]

Comment on attachment 8741099 [details] [diff] [review]
bug903390_reply_2nd_identity.v2.patch

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +2647,5 @@
>          {
> +          // Cast to concrete class. We *only* what to change m_identity, not
> +          // all the things compose->SetIdentity would do.
> +          nsMsgCompose* _compose = static_cast<nsMsgCompose*>(compose.get());
> +          _compose->m_identity = selfIdentity;

I'm not an expert on the stuff that's generated via the IDL (nsIMsgComposeParams.idl), so just asking: This really looks a bit hacky. And you do it twice, here an below. Is there not a cleaner way to do this?

Comment 23

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2) from comment #21) 
> Do you have a try run so we don't have to deal with test failures later? Or
> should I just run the test-reply-addresses.js test manually?

Sent one off now, https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=66e15a049edf
I don't expect trouble as that was the only thing that broke earlier.

> Oops, xxxmagnus ;-)

Whops. Removed locally. Just a reminder to my self what the problem was while developing the fix :)

Comment 24

[Jorg K (CEST = GMT+2)]

Are you going to submit a new version with the complaint from comment #22 fixed before I review it?
Can't you just have a function SetIdentityMember() or so?

As for the comment: I just ignore it and don't strain my brain about its possible meaning.

Comment 25

[Magnus Melin [:mkmelin]]

Right, I forgot about your second comment.
So, like I wrote, changing the signature should *really* be decoupled from SetIdentity - which is the method you want. And to add a cpp (only) function I could call I'd need to change the idl. Which, already has at least 3 methods that handles the signature. 

What I mean is, this is ancient ugly ugly code, so the static cast was the prettiest I could do (and it's used elsewhere there too). I only do it once - since you never hit both cases more than once.

Comment 26

[Jorg K (CEST = GMT+2)]

Comment on attachment 8741099 [details] [diff] [review]
bug903390_reply_2nd_identity.v2.patch

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

I've run the two tests (test-reply-addresses.js, test-reply-identity.js) manually, since due to application crashes we can't be sure that they actually ran.

The new patch addresses the problem mentioned in comment #19 and of course fixes the tests. However, it's still not quite ready for prime time, since I found more problems during my testing.

I'm using two identities on the same account. When I reply to a message sent by the default identity, everything is fine.

If I reply to a message sent by the other identity and don't enter any text and close the window, you're prompted to save/don't save/cancel. That isn't right. Also I noticed extra white-space between the quote and the signature (using reply below and signature below). There should be two lines between the quote and the signature, not three.

I debugged it a bit and you call SetIdentity (C++) from "ComposeBodyReady" (JS) and run through a identity switch which replaces the signature of the default identity. Sadly, on replies with paragraph format, that inserts that ugly third empty line, since signatures are inserted with a <br> before them. And a switch also dirties the document.

Usually on a reply starting off with the correct identity, the one for the given identity will be used straight away.

Can you please look into that?

::: mail/components/compose/content/MsgComposeCommands.js
@@ +275,5 @@
>      updateSendCommands(true);
>    },
>  
>    NotifyComposeBodyReady: function() {
> +    if (gMsgCompose.identity != gCurrentIdentity) {

Why did that get moved here? It was elsewhere before. Here it comes to late, since you run through SetIdentity() again by selecting an item from the list. You need to set the identity before you build the message.

@@ +4184,5 @@
>              needToCleanUp = true;
>              if (prevBcc != "")
>                awRemoveRecipients(msgCompFields, "addr_bcc", prevBcc);
> +            if (newBcc != "") {
> +              // Ensure none of the Cccs are already in To or Cc.

Typo: Ccc

::: mail/test/mozmill/composition/test-reply-addresses.js
@@ +83,5 @@
>      let addrType = (selectedIndex != -1) ?
>        typeMenuitems[selectedIndex].value : typeMenuitems[0].value;
>  
> +    if (!addrTextbox.value)
> +      continue;

Why this change?

Comment 27

[Masashi FUJITA]

Hi Magnus, I'm not intending to rush, but it is great if you can have time to take a look on Jorg's comment. Thanks.

Comment 28

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #26)
> ugly third empty line, since signatures are inserted with a <br> before
> them. 

I do wonder if there's a good reason to do so...

> Usually on a reply starting off with the correct identity, the one for the
> given identity will be used straight away.

Yes but that's not doable here without significant reworking I think, as the replytoself detection happens too late.

> Can you please look into that?
> 
> ::: mail/components/compose/content/MsgComposeCommands.js
> @@ +275,5 @@
> >      updateSendCommands(true);
> >    },
> >  
> >    NotifyComposeBodyReady: function() {
> > +    if (gMsgCompose.identity != gCurrentIdentity) {
> 
> Why did that get moved here? 

Wish I had commented before. I forgot the details but I believe it was necessary, and I don't see a problem with the current path. Something to do with the auto-b/cc fixups.

> > +    if (!addrTextbox.value)
> > +      continue;
> 
> Why this change?

We don't care in case there's no address filled in, only an empty (last) row. Evidently that made some tests fail.

Comment 29

[Magnus Melin [:mkmelin]]

Attachment: bug903390_reply_2nd_identity.v2.patch

Comment 30

[Jorg K (CEST = GMT+2)]

Comment on attachment 8763914 [details] [diff] [review]
bug903390_reply_2nd_identity.v2.patch

This doesn't work (unless I confused myself).

Here is my manual test:
I have one account with two identities:
JK      <[e-mail]>, signature: JK ID 2 (without the spaces, of course)
Jörg <[e-mail]>, signature: JK ID 1

I have two messages:
JK      <[e-mail]> to [e-mail]
Jörg <[e-mail]> to [e-mail]

First test: JK is the default identity.
Observations:
When replying to the JK to jorgk3 message, I get a message JK to JK.
That's wrong. It should be JK to jorgk3.
Note that the body has: <br><p><br></p>
Anyway, that's already wrong on current trunk.

When replying to the Jörg to jorgk3 message, I get a message Jörg to jorgk3.
That's correct.
Note that the body has: <p><br></p><br>. That's wrong, maybe due to your new processing.
The caret is at the end of the signature, that's wrong, it should be above the signature.
Not sure why "Reply All" is enabled since the effect is the same as "Reply".
I believe this case is where the new code kicks in.

Second test: Jörg is the default identity.
Observations:
When replying to the Jörg to jorgk3 message, I get a message Jörg to jorgk3.
That's correct.
Note that the body has: <br><p><br></p>. That's correct.

When replying to the JK to jorgk3 message, I get a message Jörg to JK.
That's wrong. It should be JK to jorgk3.
"Reply All" is also offered here and this time the result is different to "Reply".
Looks like you code should have kicked in here, and I'm surprised that is hasn't.

I'm very surprised that it matters which identity is the default.

Another general comment:
I'd prefer to see this snippet
    if (gMsgCompose.identity != gCurrentIdentity) {
      let identityList = document.getElementById("msgIdentity");
      identityList.selectedItem = identityList.getElementsByAttribute(
        "identitykey", gMsgCompose.identity.key)[0];
      LoadIdentity(false);
    }
to go into NotifyComposeFieldsReady if at all possible since it makes more sense logically. The sender is a function of the compose fields and should be set when they are ready, not when the body is ready. However, perhaps you need to wait until the body is ready to do your identity/signature switch. I was going to try to move the snippet to where I'd prefer it, but given that I don't have a working solution, I can't try it.

Comment 31

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #26)
> ...ugly third empty line, since signatures are inserted with a <br> before
> them. 
I'd like to clarify that. Message structure is:
blockquote
<br>
<br> or <p><br></p>
<div class="moz-signature>

Comment 32

[Jorg K (CEST = GMT+2)]

Some more details of my test:
Jörg is actually id1, since it was set up first. JK is id5 in my testing profile.

I don't think this test: if (gMsgCompose.identity != gCurrentIdentity) { is correct.

When my default identity is Jörg (id1) and I am replying to a message by JK (id5) dumping out
    dump("==== gMsgCompose.identity " + gMsgCompose.identity.key + "\n");
    dump("==== gCurrentIdentity " + gCurrentIdentity.key + "\n");
    if (gMsgCompose.identity != gCurrentIdentity) {
shows id1 twice, to the new code doesn't kick in at all.

BTW, I moved the snipped I talked about to NotifyComposeFieldsReady and then I get the effect I described in comment #19 (quote): One below (as configured, answer below, signature below) and one right above. Looks very funny.

So the reason you moved this to NotifyComposeBodyReady is that you trigger a signature switch on a signature that hasn't been placed yet, so in the end you get two. So your comment would be:

    // Setting the selected item in the identity list will cause an identity/signature switch.
    // This can only be done once the message body has already been assembled with
    // the signature we need to switch. Doing it in NotifyComposeFieldsReady comes to early.
    if (gMsgCompose.identity != gCurrentIdentity) { <=== meed to fix this.
      let identityList = document.getElementById("msgIdentity");
      identityList.selectedItem = identityList.getElementsByAttribute(
        "identitykey", gMsgCompose.identity.key)[0];
      LoadIdentity(false);
    }

Comment 33

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #32)
> When my default identity is Jörg (id1) and I am replying to a message by JK
> (id5) dumping out
>     dump("==== gMsgCompose.identity " + gMsgCompose.identity.key + "\n");
>     dump("==== gCurrentIdentity " + gCurrentIdentity.key + "\n");
>     if (gMsgCompose.identity != gCurrentIdentity) {
> shows id1 twice, to the new code doesn't kick in at all.

Replies between identities is not treated as replytoself, so this is just a normal reply - nothing special to do.

Comment 34

[Magnus Melin [:mkmelin]]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #30)
> I have two messages:
> JK      <[e-mail]> to [e-mail]
> Jörg <[e-mail]> to [e-mail]
> 
> First test: JK is the default identity.
> Observations:
> When replying to the JK to jorgk3 message, I get a message JK to JK.
> That's wrong. It should be JK to jorgk3.
> Note that the body has: <br><p><br></p>
> Anyway, that's already wrong on current trunk.

I wonder how you got this (JK to JK). That would basically replytoself is broken, but I don't see that.


> When replying to the Jörg to jorgk3 message, I get a message Jörg to jorgk3.
> That's correct.
> Note that the body has: <p><br></p><br>. That's wrong, maybe due to your new
> processing.

Isn't that correct though? (If we want to preserve status quo?)

Comment 35

[Jorg K (CEST = GMT+2)]

Sorry for the late reply, I read my bugmail and then got distracted by something else (actually, 1000 MSF files disappeared from my system leading to all sorts of weird behaviour, like filters failing, etc., bug 1279344 comment #7)

(In reply to Magnus Melin from comment #33)
> (In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #32)
> > When my default identity is Jörg (id1) and I am replying to a message by JK
> > (id5) dumping out
> >     dump("==== gMsgCompose.identity " + gMsgCompose.identity.key + "\n");
> >     dump("==== gCurrentIdentity " + gCurrentIdentity.key + "\n");
> >     if (gMsgCompose.identity != gCurrentIdentity) {
> > shows id1 twice, to the new code doesn't kick in at all.
> 
> Replies between identities is not treated as replytoself, so this is just a
> normal reply - nothing special to do.

Sorry, I don't understand this comment. I repeat:
When my default identity is Jörg (id1) and I am replying to a message by JK (id5) to jorgk3, then the new code should kick in and generate a message of JK to jorgk3, should it not? That's the whole idea of the bug.

(In reply to Magnus Melin from comment #34)
> I wonder how you got this (JK to JK). That would basically replytoself is
> broken, but I don't see that.

Well, I described the test case. You can set up identities MM1 and MM2 and reply to messages that MM1 and MM2 sent to MM3. It happens to me that if MM2 is the default identity, then replying to a message from MM2 to MM3 results in a message from MM2 to MM2. I tried TB 38, 45 and trunk and they all do that.

Original message:
To: [e-mail]
From: JK <[e-mail]>

Reply as JK (id5):
To: JK <[e-mail]>
From: JK <[e-mail]>

> > Note that the body has: <p><br></p><br>. That's wrong, maybe due to your new
> > processing.
> Isn't that correct though? (If we want to preserve status quo?)

No, correct is ...</blockquote><br><p>|<br></p><div class="moz-signature" ... where the | indicates the caret. You're processing had the caret at the end of the signature.

Comment 36

[Jorg K (CEST = GMT+2)]

I don't know whether is matters: My test environment has four accounts and for the testing here in this bug I added another identity to the first account, hence the id5.

Comment 37

[Magnus Melin [:mkmelin]]

I set up the MM1 and MM2 accounts. It all works for me. (testing on imap)

What my comment meant is that the jorg3 address can't be an identity (in any account if the check_all pref is set). If it is, then the reply is not treated specially. 

Maybe you have some Reply-To or other things set somewhere? Or what could be different?

Comment 38

[Magnus Melin [:mkmelin]]

I meant the MM1 and MM2 *identitites*

Comment 39

[Jorg K (CEST = GMT+2)]

I'm testing on POP. No reply-to set up and jorgk3 is an external account @gmail with no corresponding identity.

The point is that MM1 and MM2 should not be accounts created consecutively. Please see comment #36: Four accounts with id1 to id4, then id5 added to the first account. I'll e-mail you my prefs.js.

Comment 40

[Jorg K (CEST = GMT+2)]

Damn, I had the message to reply to stored in a local folder. That's considered as belonging to another identity. Now I moved them to the inbox and I'll test it all again. Sorry.

Comment 41

[Jorg K (CEST = GMT+2)]

Repeating tests from comment #30 with the messages now stored in the inbox:

I have one account with two identities:
JK      <[e-mail]>, signature: JK ID 2 (without the spaces, of course)
Jörg <[e-mail]>, signature: JK ID 1

I have two messages:
JK      <[e-mail]> to [e-mail]
Jörg <[e-mail]> to [e-mail]

First test: JK is the default identity.
Observations:
When replying to the JK to jorgk3 message, I get a message JK to jorgk3. Correct.
Note that the body has: <br><p><br></p> which appears suboptimal, see *).

When replying to the Jörg to jorgk3 message, I get a message Jörg to jorgk3. Correct.
This is where the new code kicks in.
Note that the body has: <p><br></p><br>. That's inconsistent but better, due to your new processing.
The caret is at the end of the signature, that's wrong, it should be above the signature.
Not sure why "Reply All" is enabled since the effect is the same as "Reply".

Second test: Jörg is the default identity.
Observations:
When replying to the Jörg to jorgk3 message, I get a message Jörg to jorgk3. Correct.

When replying to the JK to jorgk3 message, I get a message JK to jorgk3. Correct.
New code kicks in here, too.

===

Conclusion:
Your patch mostly works. There are two composition problems:
1) The resulting body has <p><br></p><br> which seems better than the current
   behaviour where we get the <br> first.
2) The caret should be positioned inside the paragraph, not at the end of the signature.

Without paragraph mode, the cursor is still at the end of the signature.

*) From nsMsgCompose.cpp we get
<blockquote>...</blockquote><br><br><div class="moz-signature" ...>

Here I remove the first <br>
https://dxr.mozilla.org/comm-central/source/mail/components/compose/content/MsgComposeCommands.js#369
and insert the <p><br></p> in its place, or so I thought, but it doesn't work. The result should be <p><br></p><br>. But we get:
<br><p><br></p>. I tried for a while to fix that looking at offsets, etc., but the editor refuses steadfastly to insert the paragraph first. Grrrr. It would be good if we could fix that to get a consistent result.

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: Please add this to your patch.

For as much as I tried, I couldn't not convince the editor to place the paragraph *before* the second <br>. So now I'm ripping them both out and putting a second one back if required.

Now replying with either identity gives <p><br></p><br>.

Do you agree we should make it consistent? The <br> should really be just before the signature <div> so a signature switch can find it.

Comment 43

[Jorg K (CEST = GMT+2)]

Comment on attachment 8763914 [details] [diff] [review]
bug903390_reply_2nd_identity.v2.patch

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

::: mailnews/compose/src/nsMsgCompose.cpp
@@ +5637,5 @@
> +            tempNode->GetLocalName(tagLocalName);
> +            if (tagLocalName.EqualsLiteral("br"))
> +              m_editor->DeleteNode(tempNode);
> +          }
> +        }

I think this hunk can go since it catered for the wrong initial format: <br> before <p>. So please use my patch to fix the root cause of this problem.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: bug903390_reply_2nd_identity.v3.patch

I've removed the hunk I mentioned and added my processing.
That solves the cursor problem, too. What do you think?
I'd give this r+.

Comment 45

[Jorg K (CEST = GMT+2)]

Attachment: bug903390_reply_2nd_identity.v3.patch

(added the comment I wanted.)

Comment 46

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2, PTO during summer, NI me) from comment #44)
> That solves the cursor problem, too.
Oh, I've just noticed, it doesn't. Calling the identity switch leaves the cursor at the end of the signature. But it shouldn't be too hard to get the selection before the switch and restore it.

Comment 47

[Jorg K (CEST = GMT+2)]

Attachment: bug903390_reply_2nd_identity.v3.patch (v3b)

Moved the processing into JS code and fixed the cursor issue. Should be good to go now.

Comment 48

[Magnus Melin [:mkmelin]]

Comment on attachment 8766068 [details] [diff] [review]
bug903390_reply_2nd_identity.v3.patch (v3b)

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

I had tested

 let editor = GetCurrentEditor();
 let selContainer = editor.getSelectionContainer();
 LoadIdentity(false);
 editor.setCaretAfterElement(selContainer.previousSibling);

... which seems to work for <p> mode but not for <br>. But I suppose your way is fine too, we should just add a comment that it's about the cursor, as that's not obvious from looking at the code

::: mail/components/compose/content/MsgComposeCommands.js
@@ +315,5 @@
> +    // identity/signature switch. This can only be done once the message
> +    // body has already been assembled with the signature we need to switch.
> +    if (gMsgCompose.identity != gCurrentIdentity) {
> +      let mailDoc = document.getElementById("content-frame").contentDocument;
> +      let mailBody = mailDoc.querySelector("body");

you don't seem to use any of these

Comment 49

[Jorg K (CEST = GMT+2)]

Attachment: bug903390_reply_2nd_identity.v3.patch (v3c)

OK, I removed the unneeded lines and added a comment. r+ for your part, you can review my changes now.

Comment 50

[Magnus Melin [:mkmelin]]

Comment on attachment 8766429 [details] [diff] [review]
bug903390_reply_2nd_identity.v3.patch (v3c)

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

Looks good to me, with some corrections.

I'll let the try run finish, and push it later.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +390,5 @@
>        let currentNode = mailBody.childNodes[start];
>        if (currentNode.nodeName == "BR") {
>          mailBody.removeChild(currentNode);
> +        currentNode = mailBody.childNodes[start];
> +        if (currentNode.nodeName == "BR") {

this check needs to be if (currentNode &&

@@ +4292,5 @@
> +            if (newCc != "") {
> +              // Ensure none of the Ccs are already in To.
> +              let cc2 = msgCompFields.splitRecipients(newCc, true, {});
> +              for (let i = cc2.length - 1; i >= 0; i--) {
> +                if (toAddrs.has(cc2[i]) || ccAddrs.has(cc2[i])) {

this ccAddrs.has was wrong

@@ +4311,5 @@
> +              // Ensure none of the Bccs are already in To or Cc.
> +              let bcc2 = msgCompFields.splitRecipients(newBcc, true, {});
> +              for (let i = bcc2.length - 1; i >= 0; i--) {
> +                if (toAddrs.has(bcc2[i]) || ccAddrs.has(bcc2[i]) ||
> +                    bccAddrs.has(bcc2[i])) {

and here the this bccAddrs.has was wrong

Comment 51

[Magnus Melin [:mkmelin]]

Attachment: bug903390_reply_2nd_identity.final.patch

Final version with adjustments.

Comment 52

[Jorg K (CEST = GMT+2)]

Comment on attachment 8767147 [details] [diff] [review]
bug903390_reply_2nd_identity.final.patch

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

You didn't pick up one of my changes from version v3c.

::: mail/components/compose/content/MsgComposeCommands.js
@@ +315,5 @@
> +    // identity/signature switch. This can only be done once the message
> +    // body has already been assembled with the signature we need to switch.
> +    if (gMsgCompose.identity != gCurrentIdentity) {
> +      let mailDoc = document.getElementById("content-frame").contentDocument;
> +      let mailBody = mailDoc.querySelector("body");

Magnus, I killed these two lines as requested and inserted the comment you had requested.

That change didn't make it into your "final" version.

Please correct before landing!!

Comment 53

[Jorg K (CEST = GMT+2)]

Please check the interdiff:
https://bugzilla.mozilla.org/attachment.cgi?oldid=8766429&action=interdiff&newid=8767147&headers=1

Comment 54

[Magnus Melin [:mkmelin]]

Attachment: bug903390_reply_2nd_identity.reallyfinal.patch

Must have imported the wrong version somehow...

Comment 55

[Magnus Melin [:mkmelin]]

https://hg.mozilla.org/comm-central/rev/cdecd4cf51a2 -> FIXED

Comment 56

[Jorg K (CEST = GMT+2)]

Rsx11m, we noticed that the processing in the NotifyComposeBodyReadyNew function didn't work as expected.

We wanted to change
<blockquote>...</blockquote>
<br>
<br>
<div class="moz-signature" ...>

to

<blockquote>...</blockquote>
<p><br></p>
<br>
<div class="moz-signature" ...>

but the existing code gave us:

<blockquote>...</blockquote>
<br>
<p><br></p>
<div class="moz-signature" ...>

Apparently the editor refuses to insert a <p> before a <br>. I ended up doing a brute-force approach:
https://hg.mozilla.org/comm-central/rev/cdecd4cf51a2#l1.69

The <p> before <br> works better for an identity/signature switch since that expects the <br> directly before the signature.

There's also another change here:
https://hg.mozilla.org/comm-central/rev/cdecd4cf51a2#l1.50

You might want to port one/both to SM.

Comment 57

[rsx11m]

Thanks for the heads-up. I'll have to see if any of the other changes prompting this bug report apply to SeaMonkey as well and thus should be ported.

Comment 58

[rsx11m]

Not quite unexpectedly, SeaMonkey is affected by the identity issue also. I've filed bug 1286703.

Comment 59

[Masashi FUJITA]

Thanks very much guys worked for fixing this bug. 
Really appreciated. 
And sorry for my thanks is too late.

Comment 60

[lk]

Wow guys, I'm impressed that this was actually fixed after 3 years :D Honestly, I didn't expect the bug to ever revive. Just tested in Earlybird! Thanks!

Comment 62

[tubesing]

What does it mean for this to have been fixed? Do I need to install a patch to fix this, or is there some version of TB after which this bug no longer exists? 

Sorry to not follow this well... this thread looks like conversations between programmers about how to fix the problem. If there was a solution for the end user identified, I did not find it.  A little help please? Thanks.

Comment 63

[lk]

It means that Jorg believes that the problem you described as #1329441 as actually the same as this one and this one has already been fixed. However, the fix hasn't made it into the stable version of TB yet. For testing purposes you can obtain a build for the current beta and development version of TB here [1] to check if your problem is solved there, if it is there is nothing more to do that to wait for the fix to go into TB stable.

[1] https://www.mozilla.org/en-US/thunderbird/channel/