1612369 - Compfields2Recipients leaves old entries intact

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect)

Description

[musiquegraeme]

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:72.0) Gecko/20100101 Firefox/72.0

Steps to reproduce:

I tried to use Compfields2Recipients to expand the mailing list items in the to, cc and Bcc fields of the email.

Actual results:

All the addresses were added to what was already there.

Expected results:

They should have replaced the items that were there. This appears to be because CompFields2Recipients is different now between comm-release/mail/components/compose/content/addressingWidgetOverlay.js
and
comm-release/suite/mailnews/components/compose/content/addressingWidgetOverlay.js
Whereas it was the same in previous versions.

Comment 1

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Taking this because it's the same problem I need to solve for other reasons.

Comment 2

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1612369-compfields2recipients-1.diff

Comment 3

[Alessandro Castellani [:aleca]]

Comment on attachment 9123983 [details] [diff] [review]
1612369-compfields2recipients-1.diff

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

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +125,5 @@
>      // Populate all the recipients with the proper values.
>      // We need to force the focus() on each input to trigger the attachment
>      // of the autocomplete mController.
>      if (msgReplyTo) {
> +      if (msgReplyTo.length) {

Why this double condition?
If the first condition is not null it means that we have addresses to add.
There's no need for this.

@@ +566,5 @@
>  function recipientAddPill(element, automatic = false) {
> +  let container = element.closest(".address-container");
> +  for (let pill of container.getElementsByTagName("mail-address-pill")) {
> +    pill.remove();
> +  }

This shouldn't be here as the recipientAddPill method is called whenever a new pill is created.
Adding this method here removes all the pills whenever a new is typed/created/pasted, which makes the entire area unusable.
Also, calling this method for every added pill is very taxing.

Since this issue is related to the CompFields2Recipients method not clearing previous pills, we could add this at the beginning of that method:

for (let pill of document.querySelectorAll("mail-address-pill")) {
    pill.remove();
}

Comment 4

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

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

Why this double condition?
If the first condition is not null it means that we have addresses to add.
There's no need for this.

That's not quite true. If msgCompFields.replyTo is an empty string (plausible, and wanted in my use case), msgReplyTo would be an empty array.

This shouldn't be here as the recipientAddPill method is called whenever a
new pill is created.
Adding this method here removes all the pills whenever a new is
typed/created/pasted, which makes the entire area unusable.
Also, calling this method for every added pill is very taxing.

Since this issue is related to the CompFields2Recipients method not clearing
previous pills, we could add this at the beginning of that method:

for (let pill of document.querySelectorAll("mail-address-pill")) {
pill.remove();
}

Yeah, that makes more sense.

Comment 5

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

… although I'd want it on a per-field basis, since not specifying a value for a field should not change it.

Comment 6

[Alessandro Castellani [:aleca]]

(In reply to Geoff Lankow (:darktrojan) from comment #4)

That's not quite true. If msgCompFields.replyTo is an empty string (plausible, and wanted in my use case), msgReplyTo would be an empty array.

In that case, what's the point of triggering the recipientAddPill() method if the array is empty?
Since we won't actually generate any pill, we update the first condition as:

if (msgReplyTo && msgReplyTo.lenght) {

What do you think?

… although I'd want it on a per-field basis, since not specifying a value for a field should not change it.

Then maybe creating an helper function like clearRecipientPills(input) to call for each recipient type might be a good solution.
That function can then do something like you did originally:

for (let pill of input.closest(".address-container").getElementsByTagName("mail-address-pill")) {
  pill.remove();
}

Comment 7

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1612369-compfields2recipients-2.diff

That's basically what I had in mind. I've called the function recipientClearPills to match recipientAddPill (which is a bit of a misnomer IMO, never mind). If you can think of a better wording for the comment, be my guest.

Comment 8

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1612369-compfields2recipients-alternative.diff

Here's a tidier way of doing CompFields2Recipients. It seems that the only reason it was in two parts (all the variables worked out first, then the if blocks) was for the call to addRecipientsToIgnoreList at the bottom, but the variables aren't needed for that because recipientAddPill also calls addRecipientsToIgnoreList.

I didn't combine this with the first patch because you may disagree that it is better, and also reading the changes in two parts should be easier.

Comment 9

[Alessandro Castellani [:aleca]]

Comment on attachment 9124200 [details] [diff] [review]
1612369-compfields2recipients-2.diff

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

This looks good, other than the question I wrote.
r=aleca

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +130,2 @@
>        let input = document.getElementById("replyAddrInput");
> +      recipientClearPills(input);

Are we sure we want to clear the pills before checking if we have any address?

Comment 10

[Alessandro Castellani [:aleca]]

Comment on attachment 9124203 [details] [diff] [review]
1612369-compfields2recipients-alternative.diff

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

Indeed, this is cleaner and removes some repetition.
You can go ahead and merge both diffs into one.

Comment 11

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

If we passed a value, then yes we want to clear the pills. There has to be a way to say we don't want any addresses in a field, and this is it.

Comment 12

[Geoff Lankow (:darktrojan) [PTO, back 8 Dec]]

Attachment: 1612369-compfields2recipients-3.diff

This differs only in the documentation and the test for a value. With this, some code can do

CompFields2Recipients({
  to: "foo@invalid",
  cc: "",
  bcc: null,
});

which will set To, clear CC and completely ignore BCC (also Reply To and the others not specified). Since in my WebExtensions use-case any property not specified is set to null, it prevents a bit of marshalling beforehand.

Comment 13

[Alessandro Castellani [:aleca]]

Comment on attachment 9124394 [details] [diff] [review]
1612369-compfields2recipients-3.diff

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

Looks good, this should cover all the possible data coming from that object.
r=aleca

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/26bc58b44952
Remove old addressing entries when Compfields2Recipients is called. r=aleca