1286703 - On reply to a own sent email the original sender identity is not chosen if other than the account default identity (port bug 903390 to SeaMonkey)

Status: RESOLVED FIXED

(SeaMonkey :: MailNews: Composition, defect)

Description

[rsx11m]

+++ This bug was initially created as a clone of Bug #903390 +++

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


(Quoting Jorg K from bug 903390 comment #56)
> 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 1

[rsx11m]

Attachment: Proposed fix

This is a fairly direct port of bug 903390 attachment 8767280 [details] [diff] [review] with a couple of adaptions to account for a different handling of the editor object introduced in bug 1265534. I've also retained the explicit brElement for reasons of consistency and slightly edited the introductory comment in the first hunk.

When testing this, remember that on top of at least two identities, you may also need to set mailnews.reply_to_self_check_all_ident "true" (I'm wondering why this defaults to "false" anyway). For the signature, select "Paragraph" mode in the Composition preferences.

Comment 2

[Philip Chee]

Comment on attachment 8771573 [details] [diff] [review]
Proposed fix

r=me a=me

> +      let identityList = document.getElementById("msgIdentity");
let identityList = GetMsgIdentityElement();

>        if (currentNode.nodeName == "BR") {
>          currentNode.remove();
> +        currentNode = mailBody.childNodes[start];
> +        if (currentNode && currentNode.nodeName == "BR") {
> +          mailBody.removeChild(currentNode);

currentNode.remove() just like a few lines above.

> +          let bccAddrs = new Set(msgCompFields.splitRecipients(msgCompFields.bcc, true, {}));
This is used where?

> -            if (newCc != "")
> +            if (newCc != "") {
newCC is a string so you can simplify with 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])) {
> +                  cc2.splice(i, 1);
> +                }
JavaScript arrays have this nifty built in method Array.filter().

> +              }
> +              newCc = cc2.join(", ");

newCC = cc2.filter(x => !toAddrs.has(x)).join(", ");

... or more verbosely:

cc2 = cc2.filter(x => !toAddrs.has(x));
newCC = cc2.join(", ");

> +            if (newBcc != "") {
ditto

> +              // 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])) {
> +                  bcc2.splice(i, 1);
> +                }
> +              }
> +              newBcc = bcc2.join(", ");

let toCcAddrs = new Set([...toAddrs, ...ccAddrs]); // ES6 ...spread operator
newBcc = bcc2.filter(x => !toCcAddrs.has(x)).join(", ");

Comment 3

[Jorg K (CEST = GMT+2)]

Ratty always comes late with his brilliant ideas ;-((
Rsx11m, would you be able to submit a patch for TB as well that includes this clean-up. Thanking you in advance already ;-)

Comment 4

[rsx11m]

Comment on attachment 8771573 [details] [diff] [review]
Proposed fix

Filed bug 1298696 for Thunderbird.

(In reply to Philip Chee from comment #2)
> > -            if (newCc != "")
> > +            if (newCc != "") {
> newCC is a string so you can simplify with if(newCC)

The same applies two lines up each with prev{Cc,Bcc} in the same context:

>             if (prevCc != "")
>               awRemoveRecipients(msgCompFields, "addr_cc", prevCc);
>-            if (newCc != "")
>+            if (newCc != "") {

>             if (prevBcc != "")
>               awRemoveRecipients(msgCompFields, "addr_bcc", prevBcc);
>-            if (newBcc != "")
>+            if (newBcc != "") {

I'll remove the 'if (... != "")' condition for those as well in the final patch.

Comment 5

[rsx11m]

Attachment: Patch as checked in (v2)

Pushed as http://hg.mozilla.org/comm-central/rev/d4de7f49366b

[Approval Request Comment]
Regression caused by (bug #): Bug 903390 backend landed for Thunderbird 50.0
User impact if declined: wrong identity and/or to/cc lists may be chosen
Testing completed (on m-c, etc.): works on c-c
Risk to taking this patch (and alternatives if risky): low
String changes made by this patch: none

Comment 6

[Ian Neal]

Comment on attachment 8786138 [details] [diff] [review]
Patch as checked in (v2)

a=me

Comment 7

[rsx11m]

Landed for 2.47 as http://hg.mozilla.org/releases/comm-aurora/rev/7c375071c615