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 <id1.cc@c.com>
auto-BCC: DisplayB <id1.bcc@b.com>

And another one (identity2 = id2):

auto-CC: DisplayC <id2.cc@c.com>
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 <id1.cc@c.com>, ccX@c.com, ccY@c.com, ccZ@c.com
BCC: DisplayB <id1.bcc@b.com>

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: id2.cc@c.com
BCC:

• Boom! You just lost all of your CC/BCC recipients! ccX@c.com, ccY@c.com, ccZ@c.com are gone for good. Regression by changes of bug 440377 to awRemoveRecipients().
• Display name "DisplayC" of |DisplayC <id2.cc@c.com>| 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 <id2.cc@b.com>, ccX@c.com, ccY@c.com, ccZ@c.com

• 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.

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.

Some nitfixes.

Description of the patch: See comment 1.

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

(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.

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.

(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! :-)

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

(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?

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

Pushed by thunderbird@calypsoblue.org:
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