1623285 - Changing sender identity removes all CC and BCC recipients if auto-CC or auto-BCC changes, loses auto-* display names; awRemoveRecipients() disfunctional and wrongly documented

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[Thomas D. (:thomas8)]

Seen on Trunk 76.0a1 (2020-03-11) (64-bit); regressed by bug 440377; minor flaws and behaviour inconsistencies pre-existing.

STR

1. Account Settings: Set up auto-CC/BCC on any sender account (identity1 = id1):

auto-CC: DisplayA <[e-mail]>
auto-BCC: DisplayB <[e-mail]>

And another one (identity2 = id2):

auto-CC: DisplayC <[e-mail]>
auto-BCC: [unchecked]

2. Compose with identity1
   (--> auto-CC/BCC correctly added from C++ code)
3. add more CCs / BCCs manually (or compose in reply to msg with many existing CCs, or use template with many CCs/BCCs), so it now looks like this:

CC: DisplayA <[e-mail]>, [e-mail], [e-mail], [e-mail]
BCC: DisplayB <[e-mail]>

5. Change sender identity (From) to another account/identity (id2) with different or no auto-CC/BCC, and observe your CC/BCC recipient fields

Actual Result

CC: [e-mail]
BCC:

• Boom! You just lost all of your CC/BCC recipients! [e-mail], [e-mail], [e-mail] are gone for good. Regression by changes of bug 440377 to awRemoveRecipients().
• Display name "DisplayC" of |DisplayC <[e-mail]>| also gone (same for any identity you change to, including original identity1, because it's now controlled by JS code instead of C++ code).
• Empty BCC field resulting from removing auto-BCC of previous identity (id1) is still shown, but shouldn't according to design

Expected Result

CC: DisplayC <[e-mail]>, [e-mail], [e-mail], [e-mail]

• Do not remove unrelated CCs/BCCs when changing identities
• Do not remove auto-CC/BCC display names when changing identities
• Remove empty CC/BCC fields after changing identity if the previous identity had Auto-CC/BBC

Code review of awRemoveRecipients() after bug 440377

https://searchfox.org/comm-central/rev/9f02dd21f41f406480da1b0b3fa1d38626ed0492/mail/components/compose/content/addressingWidgetOverlay.js#317-354

/**
 * Clear a specific recipient row if is visible and pills are present. This is
 * commonly used when loading a new identity.
 *
 * @param {Array} msgCompFields - The array containing all the recipient fields.
 * @param {string} recipientType - Which recipient needs to be cleared.
 * @param {Array} recipientsList - The array containing the old recipients.
 */
function awRemoveRecipients(msgCompFields, recipientType, recipientsList) {

• msgCompFields is an object, not an array.
• recipientType only specifies where to look, not which recipients to delete.
• recipientsList is a string, not an array, containing CSVs of individual recipients to be deleted.
• both msgCompFields and recipientsList parameters are unused except checking for their existence (red flag??)

  let container = element.closest(".address-container");
  for (let pill of container.querySelectorAll("mail-address-pill")) {
    pill.remove();
  }

• the correct/original purpose of this function is to remove individual auto-CC/BCC/Reply-To recipients from previous identity upon identity change. The fact that recipients used to live in one row each does not warrant the deletion of an entire "row" aka recipient field and all of its contained pills in the new order of things.

  // Reset the original input.
  let input = container.querySelector(`input[is="autocomplete-input"]`);
  input.value = "";

• If user has started another unfinished recipient input (for which autocomplete didn't trigger), who are we to just delete that?

  if (recipientType != "addr_to") {
    container.classList.add("hidden");
    document.getElementById(recipientType).removeAttribute("collapsed");
  }

• this attempts to remove the entire empty row (after the unwarranted removal of recipients), but has no effect because it's on the wrong element: .address-container cannot be hidden this way, only .address-row. What we want here is to hide rows if they were added by auto-CC/BCC of the previous identity AND they do not have any other pills nor user input.

I'm not sure how this could escape review, QA, and tests, which is worrisome.

Comment 1

[Thomas D. (:thomas8)]

Attachment: 1623285_tameIdentityChange.diff

Let's fix it :-)

• This fixes all of the above-mentioned issues from comment 0, especially to prevent the unwarranted removal of recipients when changing identity.
• For a more consistent and predictable UX, I have also improved the filter algorithms which prevent adding auto-CC/BCC as duplicates of existing recipients. E.g., when adding auto-bcc, it made no sense to prevent duplication of existing To and CC while accepting duplication of existing BCC.
• Removing alleged auto-CC/BCC from previous identity is now less greedy as it matches only full addresses. We must only remove recipients which have actually been added by auto-CC/BCC, not random recipients of the message which accidentally match some identity's auto-CC/BCC. Ultimately, that would require having an auto-CC/BCC flag on each recipient pill added by auto-CC/BCC.
• Disclaimer: This does not fix the pre-existing bug that we do not de-duplicate when adding auto-CCs/BCCs at composition startup. Removing that from C++ code would be easy enough, but it would also require more changes to LoadIdentity() in the JS code to handle the startup case. At least with this patch, when you change back to the original identity, it will correctly de-duplicate.

Comment 2

[Thomas D. (:thomas8)]

Attachment: 1623285_tameIdentityChange.diff

Some nitfixes.

Description of the patch: See comment 1.

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9134366 [details] [diff] [review]
1623285_tameIdentityChange.diff

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

Thanks for taking care of this, this works pretty well.
I noted some small nits and a necessary fix.

Also, can you update the commit message to:
"Bug 1623285 - Prevent removing recipient pills from Cc and Bcc addressing fields when changing identity. r=aleca"

::: mail/components/compose/content/MsgComposeCommands.js
@@ +6859,4 @@
>            awRemoveRecipients(msgCompFields, "addr_bcc", prevBcc);
>          }
>          if (newBcc) {
> +          // Add only Auto-Bcc recipients whose email isn't already in To, Cc,

Super tiny thing, but maybe we can keep the comment consistent, so if you wrote "is not" before, let's use that here as well and no contraction.

@@ +6860,5 @@
>          }
>          if (newBcc) {
> +          // Add only Auto-Bcc recipients whose email isn't already in To, Cc,
> +          // or Bcc, nor in just added Auto-Ccs (we check newCc from above;
> +          // updating msgCompFields.Cc would be too costly).

let's update this comment with "..is not already in To, Cc, Bcc, or added in the newCc declared above."

@@ +6862,5 @@
> +          // Add only Auto-Bcc recipients whose email isn't already in To, Cc,
> +          // or Bcc, nor in just added Auto-Ccs (we check newCc from above;
> +          // updating msgCompFields.Cc would be too costly).
> +
> +          let newCcAddrs = new Set(msgCompFields.splitRecipients(newCc, true));

Would be possible to add this to the `toCcBccAddrs` in order to avoid the double filter()?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +312,1 @@
>   *

A bit verbose, we could update this with "Remove a recipient pill based on full address matching. This is commonly used when loading a new identity."

@@ +324,5 @@
>  
>    let element;
>    switch (recipientType) {
>      case "addr_cc":
> +      container = document.getElementById("ccAddrContainer");

I think you forgot to update the `let element` declaration.

@@ +352,5 @@
>    }
>  
> +  let inputText = container.querySelector(`input[is="autocomplete-input"]`)
> +    .value;
> +  let pill = container.querySelector(`mail-address-pill`);

We can avoid declaring pill and input text since we're not reusing these variables and have these querySelector inline the condition instead.

Comment 4

[Alessandro Castellani [:aleca]]

I also launched a try run to see if we're affecting any test: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=4210db1e74199a0ac0132a23f64bfec9c4f88764

Comment 5

[Thomas D. (:thomas8)]

Attachment: 1623285_tameIdentityChange.diff

(In reply to Alessandro Castellani (:aleca) from comment #3)

Review of attachment 9134366 [details] [diff] [review]:

Thanks Alex for rapid review!
I have fixed all the nits.

Also, can you update the commit message to:
"Bug 1623285 - Prevent removing recipient pills from Cc and Bcc addressing
fields when changing identity. r=aleca"

Yes, I have sanitized the commit message. I think both for future reference and recognition it's important that we have a terse but sufficiently precise record of what we actually did.

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +312,1 @@

A bit verbose, we could update this with "Remove a recipient pill based on
full address matching. This is commonly used when loading a new identity."

Adjusted. Same here; let's be sufficiently precise to avoid misunderstandings. There's a difference between removing "a pill" vs. a list of pills, and removing them from anywhere in the recipient area or just from a given addressing field. Let's assist ourselves and others to avoid re-using such dedicated (non-generic) helper functions in wrong ways.

Comment 6

[Alessandro Castellani [:aleca]]

Comment on attachment 9134706 [details] [diff] [review]
1623285_tameIdentityChange.diff

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

This looks good, thanks for taking care of it.
I launched another try-run as the previous one had some failing tests, but they pass when I run them locally.
Let's wait and see before marking this for landing: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=dfc9e231d30d7a7bf16d190a12b1b678de0b38dc

Comment 7

[Alessandro Castellani [:aleca]]

Some linting errors and test failures for the send buttons.
If that's OK with you Thomas, I'll take care of those and update your patch.

Comment 8

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #7)

Some linting errors and test failures for the send buttons.
If that's OK with you Thomas, I'll take care of those and update your patch.

I would much appreciate that Alex, and thanks for asking! :-)

Comment 9

[Alessandro Castellani [:aleca]]

Attachment: 1623285_tameIdentityChange.diff

Patch updated and clean try-run completed: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=caab61e1ec9022ed52a06e823b016103a5c4ea02

Comment 10

[Thomas D. (:thomas8)]

(In reply to Alessandro Castellani (:aleca) from comment #9)

Created attachment 9135179 [details] [diff] [review]

Ok, I am seeing two commas added after last items in arrays for the linter, and one semicolon after let. But how does this fix the "test failures for the send buttons" mentioned in comment 7?

Comment 11

[Alessandro Castellani [:aleca]]

Those failure were related to another bug and have been fixed since.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/5384fda06708
Prevent removing recipient pills from Cc and Bcc addressing fields when changing identity, and polish auto-CC/BCC behaviour. r=aleca