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)

RESOLVED FIXED in seamonkey2.48

Status

SeaMonkey
MailNews: Composition
RESOLVED FIXED
a year ago
a month ago

People

(Reporter: rsx11m, Assigned: rsx11m)

Tracking

Trunk
seamonkey2.48
Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.46 wontfix, seamonkey2.47 fixed, seamonkey2.48 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

5.64 KB, patch
rsx11m
: review+
Details | Diff | Splinter Review
(Assignee)

Description

a year ago
+++ 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.
(Assignee)

Updated

a year ago
See Also: → bug 903390
(Assignee)

Comment 1

a year ago
Created attachment 8771573 [details] [diff] [review]
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.
Attachment #8771573 - Flags: review?(philip.chee)

Comment 2

a year ago
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(", ");
Attachment #8771573 - Flags: review?(philip.chee) → review+
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 ;-)
Flags: needinfo?(rsx11m.pub)
(Assignee)

Updated

a year ago
Blocks: 1298696
(Assignee)

Comment 4

a year ago
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.
(Assignee)

Updated

a year ago
Flags: needinfo?(rsx11m.pub)
(Assignee)

Comment 5

a year ago
Created attachment 8786138 [details] [diff] [review]
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
Attachment #8771573 - Attachment is obsolete: true
Attachment #8786138 - Flags: review+
Attachment #8786138 - Flags: approval-comm-aurora?
(Assignee)

Updated

a year ago
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-seamonkey2.46: --- → wontfix
status-seamonkey2.47: --- → affected
status-seamonkey2.48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.48

Comment 6

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

a=me
Attachment #8786138 - Flags: approval-comm-aurora? → approval-comm-aurora+
(Assignee)

Comment 7

a year ago
Landed for 2.47 as http://hg.mozilla.org/releases/comm-aurora/rev/7c375071c615
status-seamonkey2.47: affected → fixed

Updated

8 months ago
Depends on: 1349021

Updated

a month ago
See Also: → bug 1112784
You need to log in before you can comment on or make changes to this bug.