1642279 - Email addresses with comma in display name cannot be entered via keyboard

Status: RESOLVED FIXED

(Thunderbird :: Message Compose Window, defect, P1)

Description

[Jorg K (CEST = GMT+2)]

I discovered a few anomalies that make address entry for addresses that contain a comma in their display name impossible.

Assume that you have the following in your address book:
b, a <[e-mail]>
b, c <[e-mail]>
[e-mail]

First, we want to write an e-mail to a new recipient, a, x <example.com>. So we start typing a, - When the comma is entered, the autocomplete will select [e-mail].

Second, we want to write to b, c <[e-mail]>. So we enter b, and and b, a <[e-mail]> is selected.

Third, since typing doesn't work, we open the address book, used "Write" from the context menu for b, c <[e-mail]> and the resulting pill is b <>.

EDIT: Longer example:
Smith, Joe <[e-mail]>
Smith, Jane <[e-mail]>
Smith Family <[e-mail]>
Entering Smith, selects Smith Family <[e-mail]>.

Comment 1

[Jorg K (CEST = GMT+2)]

Maybe the problem is around here:
https://searchfox.org/comm-central/rev/1caf86b49b044958fed0b0d8d5fb6ec974557aaa/mail/components/compose/content/addressingWidgetOverlay.js#558-565

Not sure about the third issue though.

Comment 2

[Magnus Melin [:mkmelin]]

The third issues is different yes.

For the other cases, there is clearly a weigh off between what's reasonable behaviour or not. The examples are too short, so it's causing way more problems than any real world use case. You don't really have single letter people in your address book.

Comment 3

[Jorg K (CEST = GMT+2)]

I don't know why we're here to argue about the severity of the bug. Same issue for "Doe, Joe" and "Doe, Jane" of other common surnames, like "Smith, Joe" or "Smith, James" and "Smith, John". Not being able to enter addresses via the keyboard is simply broken and not subject to "weighing off".

Needless to say that "Last, First <[e-mail]>" are regularly used with larger entities (enterprise, organizations, authorities, schools, you name it). So it becomes an enterprise issue. Even regular users can be affected if they communicate with such entities who send out mails using comma. And also in other languages/cultures like Japanese where last name is more emphasized. It's not an edge case, so please get it fixed instead of arguing about it.

Comment 4

[Thomas D. (:thomas8)]

Cause is simple: Keypress/keydown event of address inputs listens to "," and then prematurely confirms the first autocomplete result if that's inline.
https://searchfox.org/comm-central/rev/e8cc4c6395d4f9e128e04fc8c4163aa09b35d731/mail/components/compose/content/addressingWidgetOverlay.js#558-566

´´´
case ",":
// Don't trigger autocomplete if the typed value is not a valid address.
if (!isValidAddress(input.value)) {
break;
}
event.preventDefault();
input.handleEnter(event);
break;
´´´

So fix is simple: just remove that comma case entirely.

I have real messages in my inbox from our local authorities using following sender structure:

Miller, 123, City of Sometown <[e-mail]> ⭐
Last, section/department number, name of authority <...> (sender's name is Jane Miller in this example).

So they are actually using multiple commas in their display names for all members of the entire municipal administration of this large city. We certainly want to continue supporting such use cases for our enterprise users, given that display names with commas are perfectly valid syntax.
When receiving citizens/clients/customers click on star of TB header, address with display name gets taken over into their AB, too. So even if I don't regularly use Last, First display names, I might have some in my AB from others, and due to this bug, I cannot retrieve them via typing, as TB will prematurely confirm a random unrelated autocompleted address upon typing comma, which violates ux-error-prevention.

I am also yet to see any application which autocompletes anything on typing comma, so apart from breaking the behaviour, it's also a bit hard to discover as no-one might ever try. And it would only really work for unique matches, unlikely especially for enterprise which needs reliable efficiency most. Firefox location bar does not even autocomplete on tab...

Comment 5

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #4)

Cause is simple: Keypress/keydown event of address inputs listens to "," and then prematurely confirms the first autocomplete result if that's inline. [snip] So fix is simple: just remove that comma case entirely.

That technical analysis may not be accurate enough, and we might be able to find another fix which would still allow pill creation with comma after typing full valid address like "[e-mail],". That would still prevent having @-character and comma in the display name, not sure if the @ is valid syntax without quotes.

Comment 6

[Jorg K (CEST = GMT+2)]

You can have comma and @ in the display name, like enter a, c @ BMN <[e-mail]>, but if you enter BMO @ Bugzilla, 1st attempt <[e-mail]>, you get that split. So yes, a comma after what looks like a complete address could trigger pill creation.

Comment 7

[Thomas D. (:thomas8)]

(In reply to Jorg K (CEST = GMT+2) from comment #6)

You can have comma and @ in the display name, like enter a, c @ BMN <[e-mail]>, but if you enter BMO @ Bugzilla, 1st attempt <[e-mail]>, you get that split. So yes, a comma after what looks like a complete address could trigger pill creation.

So that's another part of this problem, isValidAddress() function needs a better algorithm so that obviously invalid addresses like BMO @ Bugzilla are correctly recognized, to avoid creating recipient pills without a valid email address prematurely. I'm sure we're not the first software to do email validation, so maybe we could just find a suitable regex for that. Caution with potential special cases like own domains and auto-complete-to-domain (not sure if that still exists).

Comment 8

[Thomas D. (:thomas8)]

Attachment: Screenshot 1: AB search for 'Doe,' succeeds, autocompletion about to fail with premature pill creation upon comma (Jane and John's twin not reachable)

The problem of accepting the first (random) autocomplete suggestion prematurely upon comma will be even more exposed for users with AB setting "View > Show Name As > Last, First": In spite of contacts side bar showing all contacts as Last, First, entering any known "Lastname," into address input will trigger premature pill creation from a random first autocomplete match (see screenshot: typing the comma now will autocomplete first result). But we explicitly allow searching for last names followed by comma since bug 1630330 as an intuitive and efficient way of narrowing down your AB search / autocompletion, especially for large entities/enterprise.

Note: the difference in matches between contacts side bar and autocomplete is bug 1644026, and LDAP autocomplete was off.

Comment 9

[Thomas D. (:thomas8)]

For enterprise and other users with Last, First scenarios, this bug will make Thunderbird virtually unusable, because autocompletion prematurely enters random and unwanted recipients, which is very risky and error-prone in terms of privacy. Also, existing Last, First recipients from address book are now impossible to enter via keyboard, whereas enterprise have legal obligation to ensure accessibility.

Comment 10

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #9)

For enterprise and other users with Last, First scenarios, this bug will make Thunderbird virtually unusable, because autocompletion prematurely enters random and unwanted recipients, which is very risky and error-prone in terms of privacy. Also, existing Last, First recipients from address book are now impossible to enter via keyboard, whereas enterprise have legal obligation to ensure accessibility.

Comment 11

[Alessandro Castellani [:aleca]]

Thanks for highlighting this issue.

Upon further thinking and testing, I do believe we shouldn't listen for the comma to trigger the autocomplete.
Here's a little analysis.

Why the comma implementation
The original idea was to treat addresses like a regular text, so multiple addresses separated by a comma in a single field.
That is not possible due to the various limitations of the autocomplete fields.
Also, showing a comma in between addresses causes so many edge cases for reordering and editing that it was going to be a nightmare.
We left the comma listener to enable a sort of more natural typing experience when adding multiple recipients.

Why we should remove it

• We can't control how users save their AB contacts, as highlighted in this bug.
• We offer an AB option to "View > Show Name As > Last, First" as pointed in comment 8.
• We don't currently listen to the comma in 68, so it's not something we have to maintain but it's an addition that users will need to learn, and by not implementing it we're not breaking any previously established usability paradigm.
• We want to emphasize the fact that recipient pills are standalone elements that the user needs to willingly create, so triggering that creation primarily with ENTER or TAB is more inline with what pills are.

Comment 12

[Thomas D. (:thomas8)]

Mass-changing bugs around the new recipient area (pills) from product/component MailNews Core/Composition to Thunderbird/Message Compose Window, because composition frontend code is not shared with SM. Mostly cloned from Bug 440377 which started out in MailNews Core long back.

20200614001RecipientPillsProductChangeTypeBug

Comment 13

[Thomas D. (:thomas8)]

Add relevant search words to summary, and link with similar symptoms (causes may differ).

Comment 14

[Thomas D. (:thomas8)]

I have moved the address book issue of comment 0 into bug 1656278 (Write fails for AB contact with comma in display name).

(In reply to Magnus Melin [:mkmelin] from comment #2)

The third issues is different yes.

(In reply to Jorg K (CEST = GMT+2) from comment #0)

Third, since typing doesn't work, we open the address book, used "Write" from the context menu for b, c <[e-mail]> and the resulting pill is b <>.

Comment 15

[Jorg K (CEST = GMT+2)]

Uff, this is still not fixed? That won't make TB 78 very popular when people get auto-updated. Anyone tracking issues like this?

Comment 16

[Thomas D. (:thomas8)]

(In reply to Thomas D. from comment #4)

https://searchfox.org/comm-central/rev/e8cc4c6395d4f9e128e04fc8c4163aa09b35d731/mail/components/compose/content/addressingWidgetOverlay.js#558-566

    case ",":

So fix is simple: just remove that comma case entirely.

Not that simple: After removing our own comma handler, autocomplete itself will just stop autocompleting when it sees a comma.
autocomplete fail: Doe, John (no results inspite of existing matches)
success: Doe. John
So maybe we need to figure out where autocomplete itself listens for comma and if we can adjust that.

Comment 17

[Jorg K (CEST = GMT+2)]

But before introducing the pills, auto-complete worked as expected, no?

Comment 18

[Thomas D. (:thomas8)]

(In reply to Jorg K (CEST = GMT+2) from comment #17)

But before introducing the pills, auto-complete worked as expected, no?

No, TB 68.10.0 also stops autocompleting on comma entered in input string (whereas any other characters like full stop work).

Comment 19

[Jorg K (CEST = GMT+2)]

Well, we should at least restore the abilities users had in TB 68.x. As I recall, entering a comma also was the workaround for bug 1138033.

Comment 20

[Jorg K (CEST = GMT+2)]

Thomas, thanks for splitting out bug 1656278. I've come back here and compared the TB 68 and the TB 78 behaviour.
With Doh, James <[e-mail]> and Doh, John <[e-mail]> it is possible to enter Doh, Mary <[e-mail]> in TB 68, but not TB 78. That's the bug I reported.

It is true, that in TB 68 auto-complete stops when entering a comma, but this is not subject of this bug. So in the interest of getting the regression fixed, please let's not fix another issue into this bug.

So maybe we need to figure out where autocomplete itself listens for comma and if we can adjust that.

Yes, but please not here. I still think that removing the case ",": will stop the unwanted pill creation and restore the previous TB 68 behaviour. So can we do that, Alex?

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1642279-dont-complete-pill-on-comma.patch

[Approval Request Comment]
Regression caused by (bug #): 440377
User impact if declined: Can't enter certain e-mail addresses with a comma in the display name.
Risk: Low, just removing a case that wasn't considered properly and shouldn't have been there in the first place.

Comment 22

[Magnus Melin [:mkmelin]]

If comma doesn't break to new pill, it would be much harder to get to entering the next address, so I don't think just dropping that option is a good idea. We'll have to do something smarter.

Comment 23

[Jorg K (CEST = GMT+2)]

Hmm, personally I'd be happy with tab and enter. If you handle comma, then you should also handle a semicolon. That said, I don't understand the code added in bug 1609894: https://hg.mozilla.org/comm-central/rev/1d6c19070bff

If you type Doh, how is that a valid address? Or is element.value the auto-complete supplied value at the time, so for example Doh, John <[e-mail]> although that wasn't typed completely?

Comment 24

[Jorg K (CEST = GMT+2)]

Looking at comment #11, just removing the comma case is what Alex suggested. I played with this a little more, but essentially you don't know what the user typed and what was delivered by auto-complete. So I'm running out of ideas what the "smarter" approach would be. I tried this code:

    case ",":
    case ";":
      // Don't trigger autocomplete if the typed value is not a valid address.
      // Note that we don't strickly have access to the user-typed value since the value
      // may be auto-completed to more than the user typed.
      // Only check to "," or ";" so when the user typed `Doh,` that's not automatically
      // auto-completed to `Doh, Joe <[e-mail]>` if such value was in the address book.
      let last = input.value.lastIndexOf(event.key);
      if (last < 0) {
        last = input.value.length;
      }
      if (!isValidAddress(input.value.substring(0, last))) {
        break;
      }
      event.preventDefault();
      input.handleEnter(event);
      break;

so now you can enter Doh, Marry <[e-mail]> but you need to type two commas after that to complete. Anyone with a better idea, please take over.

Comment 25

[Alessandro Castellani [:aleca]]

If comma doesn't break to new pill, it would be much harder to get to entering the next address.

That's a good point, since the comma stops the autocomplete users won't get any suggestion and no chance of quickly picking a result it they type it.
With that said, we shouldn't create a pill with the first available result when the user types a comma IF there are contacts saved with commas.

We'll have to do something smarter.

A better implementation would be to only trigger the creation of the pill on comma if:

• It's a valid email address
• AND only 1 result is available in the autocomplete popup

If more results are available, we shouldn't do anything.
The problem is that we can't rely on the autocomplete since it can't handle commas properly, so I think we should first try to fix the autocomplete issue, and then once that works properly, we can improve the comma handling listener.

Comment 26

[Jorg K (CEST = GMT+2)]

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

A better implementation would be to only trigger the creation of the pill on comma if:

• It's a valid email address
• AND only 1 result is available in the autocomplete popup

Certainly not. The bug is about not being able to enter Doh, Mary <[e-mail]> when Doh, John <[e-mail]> is already in the address book. Your suggestion doesn't fix this. How about fixing the regression now that makes it impossible to enter certain addresses, which won't be appreciated by our users, and think about the grand scheme later? My best "smart" solution is in comment #24, fine if you can improve on that. As I said there, it's impossible to distinguish what the user typed from the auto-complete result, so we have a fundamental issue here (which I don't think we can fix in a hurry).

EDIT:

I think we should first try to fix the autocomplete issue, and then once that works properly ...

That's in M-C code and likely not fixable at all, or if it is, it will take months, during which users won't be able to write e-mail to certain people :-(

Comment 27

[Alessandro Castellani [:aleca]]

I guess I fail to understand why a user would want to write Doh, Mary <[e-mail]> as a recipient that it's not in the address book, that seems so cumbersome and not usual when writing to a new recipient for the first time, but I'm aware that we can't control the way users write things.

I'm okay with dropping the comma entirely as I wrote in comment 11.
I'll check the solution on comment 24 and see if there's anything else we could do.

Comment 28

[Jorg K (CEST = GMT+2)]

Oops, spoke too early. The Firefox "awsome bar" (or whatever it's called these days) can perfectly handle a comma, tried with: file:///D:/Desktop/Bugzilla,down.html.

The problem is home made:
https://searchfox.org/comm-central/rev/5ed7de91a637b0b52125e6f40a8ee79e0396192b/mailnews/addrbook/src/AbAutoCompleteSearch.jsm#396

    // If the search string is empty, or contains a comma, or the user
    // hasn't enabled autocomplete, then just return no matches or the
    // result ignored.
    // The comma check is so that we don't autocomplete against the user
    // entering multiple addresses.
    if (!fullString || aSearchString.includes(",")) {
      result.searchResult = ACR.RESULT_IGNORED;
      aListener.onSearchResult(this, result);
      return;
    }

So we explicitly stop on a ",". That should be "enhanced" to keep checking as long as no valid e-mail address was entered yet. Alex, do you want to give that a go?

Comment 29

[Alessandro Castellani [:aleca]]

So we explicitly stop on a ",". That should be "enhanced" to keep checking as long as no valid e-mail address was entered yet. Alex, do you want to give that a go?

Ah! Amazing, thanks for finding this.
Yes, I'll give it a stab, thanks

Comment 30

[Alessandro Castellani [:aleca]]

Attachment: 1642279-comma-autocomplete.diff

This is a test more than the final patch.
I talked with Mark Banner, the author of bug 370306 where that comma handling was originally implemented.
This seems to be the reason behind it:

so back when we had multiple lines for the compose window recipients, we did allow entering a bunch of comma-separated email addresses and pressing enter, and it would split those onto multiple lines.
we probably didn't think too much of the last name, first name case.

So they removed the autocomplete after a comma to avoid conflicts in returning strange results.

I think, and correct me if I'm wrong, we could safely drop that since our pills creation removes these concerns, since after a pill is created, the autocomplete input field won't be affected by the previously entered address.

With this change, we can remove the pill-creation-on-comma thingy since it prevents users from typing the full name of what they're searching, or even typing a name that doesn't currently exist in the list of saved contacts (not sure why they'd do that, but it might happen).

In addition, users can type multiple addresses separated by commas and the on blur the pills will be automatically created, kinda matching the way we handled things in 68, and relying only on ENTER and TAB to quickly create a pill with the autocompleted suggestion.

What do you think?

Comment 31

[Jorg K (CEST = GMT+2)]

Comment on attachment 9167770 [details] [diff] [review]
1642279-comma-autocomplete.diff

I'm OK with this since it fixes the bug as reported (can't enter `Doh, Mary <[e-mail]>` when `Doh, John <[e-mail]>` is already in the address book) (*) and it improves the comma situation overall.

However, it doesn't provide the smart fix Magnus wanted, that is, to maintain the behaviour that comma entry closes the pill when necessary. If we do a smart fix, which, as I said, isn't easy since we can't distinguish user input from auto-complete provided string, please consider adding `;` as pill-closing character, simply since both comma and semicolon already break up the user entry when hitting enter.

And further off-topic: Please address bug 1656278 which is a real regression and doesn't need further discussion.

(*): Re. comment #27: Why a user enter `Doh, Mary <[e-mail]>`. Well, say they received a written paper communication from their local town administration where the e-mail was printed like this. Or they come from a culture where the surname goes first. You can't tell why, but sadly with 25 million users and 10.000 (weird) functions n TB, there will always be someone using any function, so it's not a good idea to break things. Especially address entry is very sensitive since the standards allow almost anything in the display part, so TB shouldn't reject entries in an unexpected way.

Comment 32

[Thomas D. (:thomas8)]

Comment on attachment 9167770 [details] [diff] [review]
1642279-comma-autocomplete.diff

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

Thanks Alex! I think this looks like our best way forward for now:
- fixes the significant regression of this bug, AND the TB68 bug that comma stops autocompletion - nice! :-)
- resulting behaviour is neither harder than nor different from TB68
- avoids risks/regressions of fast-tracking possibly "smarter" solutions now which might be unsustainable

Thanks Jörg for finding the spot where autocomplete was blocked on comma!

Comment 33

[Thomas D. (:thomas8)]

Comment on attachment 9167770 [details] [diff] [review]
1642279-comma-autocomplete.diff

I think that this will actually enable my comma-handling improvements of Bug 1630330 for recpient autocompletion - nice! Sort of reverse case: We ignore comma in search string (but now continue finding matches) to cover cases where there's no comma in the AB data but the names are matching (especially for users with `Show Name as: Last, First`).

Comment 34

[Magnus Melin [:mkmelin]]

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

I guess I fail to understand why a user would want to write Doh, Mary <[e-mail]> as a recipient that it's not in the address book, that seems so cumbersome and not usual when writing to a new recipient for the first time, but I'm aware that we can't control the way users write things.

This!!

I'll note we didn't get any bug reports either, just the theoretical one filed here. For the odd use case, it's of course possible for the user to go back and correct the created pill. It would be a one time thing since the next time it gets autocompleted and there is no problem

It's not logical to press enter to finish a completion when the completions go to the same row. (It was logical before, with separate rows since Enter means change row.) The logical way to enumerate items when they are on the same row is to use comma.

Even Gmail and Outlook web interfaces will finish their pills on comma, so let's once and for all forget that we'd not be supporting that. There are deeper repercussions on a the "soul of the pills UI" that could be bad if make the UI too disconnected from the specs.

Comment 35

[Magnus Melin [:mkmelin]]

(Not an enterprise bug)

Comment 36

[Jorg K (CEST = GMT+2)]

(In reply to Magnus Melin [:mkmelin] from comment #34)

I'll note we didn't get any bug reports either, just the theoretical one filed here.

The "floodgates" to TB 78 haven't been opened yet. I don't think that entering an e-mail address is theoretical. I didn't file the bug to annoy anyone, I found it in real life use. Entering an e-mail address is the most basic feature of an e-mail client. It's like a phone which refuses to call a certain number just because a similar number has already been dialled.

This is the use case: Reply to and e-mail your received from Smith, John <[e-mail]>. This address is now "collected addresses" AB. That guy now blocks the entry of sending to any address Smith,*. Now, is this really so unrealistic? And frankly, this is and enterprise bug, since enterprises use Surname, Firstname <[e-mail]>.

I find it rather disappointing that we're discussing a regression here: It's fine that the new system can many new bright shiny things, but you can't go breaking existing functions in the process. And going back to edit a pill that got messed up is clearly horrible UI, if the user can find the context menu. The natural reaction will be: remove and try again.

Comment 37

[Worcester12345]

I have the equivalent of two "jsmith's" at the same company. They are related. Not only that, I have another pair that one is the boss of the other, so those should not be getting transposed. This basic functionality is important.

Comment 38

[Jorg K (CEST = GMT+2)]

Attachment: 1642279-alternative.patch

Here's your perfect solution. All happy? I took over the hunk which I researched, I hope you don't mind.

Regression caused by (bug #): 440377
User impact if declined: Can't enter certain e-mail addresses with a comma in the display name.
Risk: Low, basically a one liner considering the user entered text instead of the full auto-complete result.

Comment 39

[Alessandro Castellani [:aleca]]

Comment on attachment 9167979 [details] [diff] [review]
1642279-alternative.patch

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

This is an improvement, but it's not quiet there yet.

I have 3 addresses: Doe, John | Doe, Jane | Doe, Sadie
The generation of the pill still fires on comma if the autocomplete fills the field with the first available address, making it impossible to keep typing to write a new name or reach Sadie.

This problem doesn't present if I quickly type `doe,` before the autocomplete triggers, which is good because it prevents the early creation of a pill, but it doesn't fully solve this problem.
If a user is "slow" in typing, it will stumble upon the same problem.

**Proposal**
Since the autocomplete selects the autocompleted value, we can have a condition that detects if the first character of the selected value is a comma.
If it is, it means that the user typed the comma matching the first character of the suggested autocomplete, and we shouldn't generate the pill because it could mean:
- The user wants to keep typing to select another result instead of the first.
- The user might be entering a new name not saved in the contacts.

This might gives us the flexibility we need, and the only "downside" is the fact that users with many contacts with a comma in it will need to get used to it.
But I think this is the best of both worlds, allowing users to keep searching and typing addresses even after a comma, and keeping the comma autocomplete when it makes sense.

What do you think?

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +563,2 @@
>        // Don't trigger autocomplete if the typed value is not a valid address.
> +      if (!isValidAddress(input.value.substring(0, input.value.selectionEnd))) {

We should improve the comment here to reflect the fact that we're stripping the last typed character.
Maybe we could add this to the `isValidAddress` method directly so if an address ends with a comma or semicolo, we strip it for validation.

Comment 40

[Alessandro Castellani [:aleca]]

Attachment: 1642279-alternative.patch

What about this?

Comment 41

[Jorg K (CEST = GMT+2)]

Comment on attachment 9168008 [details] [diff] [review]
1642279-alternative.patch

Hmm, how did I miss the "slow" typing case. Well, glad you found it. Some comments:
How about we make this shared authorship?
# User Jorg K <[e-mail]> and Alessandro Castellani <[e-mail]>
and make it r=mkmelin
Some ideas came from me and some from you.

The patch has `input.selectionEnd` and `input.value.selectionEnd`. Is that the same? If so, use one version.

Comment 42

[Magnus Melin [:mkmelin]]

Comment on attachment 9168008 [details] [diff] [review]
1642279-alternative.patch

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

Looks pretty good, I'll try it out when the below is adjusted

::: mail/components/compose/content/addressingWidgetOverlay.js
@@ +558,5 @@
>        }
>        break;
>  
>      case ",":
> +    case ";":

I wouldn't add the semi-colon. Let's save that for group syntax support later.

@@ +570,5 @@
> +      // with commas in the display name, or if the typed value is not a valid
> +      // address, after the comma or semicolon has been stripped.
> +      if (
> +        selection[0] == "," ||
> +        !isValidAddress(input.value.substring(0, input.value.selectionEnd))

I don't think input.value.selectionEnd is a thing?

Comment 43

[Jorg K (CEST = GMT+2)]

I don't think input.value.selectionEnd is a thing?

It works, but see comment #41, last line.

Comment 44

[Jorg K (CEST = GMT+2)]

Attachment: 1642279-alternative.patch

Comment 45

[Magnus Melin [:mkmelin]]

Comment on attachment 9168077 [details] [diff] [review]
1642279-alternative.patch

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

Seems alright, r=mkmelin

Comment 46

[Jorg K (CEST = GMT+2)]

Comment on attachment 9168077 [details] [diff] [review]
1642279-alternative.patch

[Approval Request Comment]
Regression caused by (bug #): bug 440377, bug 1609894
User impact if declined: Can't enter certain e-mail addresses with a comma in the display name.
Risk: Low, basically only considering the user entered text instead of the full auto-complete result.
Tested: Yes, three peers have tested it, but hey, three peers have also messed up before ;-)

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/e837623109dc
Follow-up to bug 1609894: When finishing pill entry with comma, check valid address of entered text. r=mkmelin

Comment 48

[Geoff Lankow (:darktrojan)]

This patch broke mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js.

Comment 49

[Geoff Lankow (:darktrojan)]

I backed this out. There's enough bustage in the tree right now without adding more.

https://hg.mozilla.org/comm-central/rev/4488a81ea2fe431f822fbf33570d01746e7ac532

Comment 50

[Alessandro Castellani [:aleca]]

Thanks for backing this out.
Definitely this needed a try run before landing.
I'll take a look at it tomorrow and upload a patch with the fixed tests.

Comment 51

[Alessandro Castellani [:aleca]]

Comment on attachment 9168077 [details] [diff] [review]
1642279-alternative.patch

Removing the flags due to test bustages

Comment 52

[Alessandro Castellani [:aleca]]

Attachment: 1642279-alternative.patch

Updated tests to be sure the autocomplete works with commas.

Comment 53

[Jorg K (CEST = GMT+2)]

Comment on attachment 9168553 [details] [diff] [review]
1642279-alternative.patch

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

::: mailnews/addrbook/test/unit/test_nsAbAutoCompleteSearch1.js
@@ +26,5 @@
>    { email: "te <lis>", dirName: kPABData.dirName }, // 7
>    { email: "tes <li>", dirName: kPABData.dirName }, // 8
>    // this contact has a nickname of "abcdef"
>    { email: "test <l>", dirName: kPABData.dirName }, // 9
> +  { email: "zoe, jane <[e-mail]>", dirName: kPABData.dirName }, // 10

We don't use possible live domains in testing. Could you change this to @test.invalid or @example.com (and below). Sorry about the nit and thanks for taking care of this. Also, usually we use sample names, like Joe or James Doh, so I'm not totally convinced by Jane, oops, Zoe ;-)

Comment 54

[Alessandro Castellani [:aleca]]

Sounds good, thanks for checking.

Comment 55

[Alessandro Castellani [:aleca]]

Attachment: 1642279-alternative.patch

Comment 56

[Thomas D. (:thomas8)]

(In reply to Jorg K (CEST = GMT+2) from comment #53)

Also, usually we use sample names, like Joe or James Doh, so I'm not totally convinced by Jane, oops, Zoe ;-)

Fwiw, I thought it's John Doe and Jane Doe (1), and he's got known friends and family, too (2). Never seen Doh, duh! ;-)
(1) https://en.wikipedia.org/wiki/Placeholder_name
(2) https://en.wikipedia.org/wiki/John_Doe

Comment 57

[Jorg K (CEST = GMT+2)]

I think "James Doh" is OK, no need to change it again.

Comment 58

[Thomas D. (:thomas8)]

(In reply to Jorg K (CEST = GMT+2) from comment #57)

I think "James Doh" is OK, no need to change it again.

+1, absolutely.

Also, next time I wouldn't needlessly define test contacts with single-character nicknames ("j"), which isn't feasible in real world. Nicknames are supposed to get preferential treatment on full match, so that can easily have unwanted side effects for future tests of autocomplete searches. But for now, if it all works, no need to change that, either.

Thanks everyone involved for figuring this bug out, "hart aber fair" - hard but fair. The cooperative solution looks promising. I'll test it after landing.

Comment 59

[Geoff Lankow (:darktrojan)]

Comment on attachment 9168571 [details] [diff] [review]
1642279-alternative.patch

Okay, this looks fine to me.

Comment 60

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/12e194897622
Follow-up to bug 1609894: When finishing pill entry with comma, check valid address of entered text. r=mkmelin,darktrojan DONTBUILD

Comment 61

[Jorg K (CEST = GMT+2)]

Comment on attachment 9168571 [details] [diff] [review]
1642279-alternative.patch

[Approval Request Comment]
Regression caused by (bug #): bug 440377, bug 1609894
User impact if declined: Can't enter certain e-mail addresses with a comma in the display name.
Risk: Low, basically only considering the user entered text instead of the full auto-complete result.
Tested: Yes, three peers have tested it, but hey, three peers have also messed up before ;-)

Update from comment #46: Now four peers have looked at it and it has a test.

Comment 62

[Wayne Mery (:wsmwk)]

Comment on attachment 9168571 [details] [diff] [review]
1642279-alternative.patch

[Triage Comment]
Approved for beta

Yay automated tests.

Walt, can you test this in your smoketest?

Comment 63

[Rob Lemley [:rjl]]

Thunderbird 80.0b2:
https://hg.mozilla.org/releases/comm-beta/rev/495af765c9d7

Comment 64

[WaltS48 [:walts48]]

Testing in the 80.0b2 release candidate on Ubuntu 18.04.4.

Added the following in a new Test address book:
b, a <[e-mail]>
b, c <[e-mail]>
[e-mail]

Attempted to write an e-mail to a new recipient, a, x <example.com> by typing a, - When the comma is entered, the autocomplete still selects [e-mail].
Added an a, x address to my address book and that was autocompleted when typed.

Second, we want to write to b, c <[e-mail]>. So we enter b, and and b, a <[e-mail]> is selected.
Well, 'a' does come before 'c' in the alphabet, but I do see 'b, c' in the drop-down choices and it is the first selection if I type 'b, c'.

HTH

Comment 65

[Jorg K (CEST = GMT+2)]

Hmm, I went through the test cases from comment #0 and indeed, entering a, still selects [e-mail]. Too bad we didn't test that :-( - I had no trouble with entering b, a <[e-mail]> and b, c <[e-mail]>.
EDIT: Works if you quickly type a, before the auto-complete kicks in.

Walt, your grammar derailed, can you please correct your last paragraph.

Comment 66

[Jorg K (CEST = GMT+2)]

Sorry, Walt, this sentence is still not clear, from comment #64:

So we enter b, and and b, a <[e-mail]> is selected.

Meanwhile I filed bug 1658157 as a follow-up.

Comment 67

[WaltS48 [:walts48]]

(In reply to Jorg K (CEST = GMT+2) from comment #66)

Sorry, Walt, this sentence is still not clear, from comment #64:

So we enter b, and and b, a <[e-mail]> is selected.

Meanwhile I filed bug 1658157 as a follow-up.

That's one I copied and pasted from your STR in comment 0.

Comment 68

[Jorg K (CEST = GMT+2)]

OK, I see the doubled-up "and" now. But you can enter b, c now without getting b, a (and vice versa), so at least that part got fixed, right?

Comment 69

[Intermittent Failures Robot]

6 failures in 4618 pushes (0.001 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• comm-central: 6

Platform breakdown:

• linux64-shippable: 1
• linux64: 3
• windows7-32: 1
• windows10-64: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1642279&startday=2020-08-03&endday=2020-08-09&tree=all

Comment 70

[Jorg K (CEST = GMT+2)]

Any reason not to approve this for TB 78? It's been on TB 80 beta. It needs more work, see bug 1658157, but this here is a start.

Comment 71

[Wayne Mery (:wsmwk)]

Comment on attachment 9168571 [details] [diff] [review]
1642279-alternative.patch

[Triage Comment]
Approved for esr78

Just some confusion on my part after the arrival of bug 1658157

Comment 72

[Jorg K (CEST = GMT+2)]

Well, a bit more thorough testing discovered more issues, hence the follow-up (which is sadly blocked by Magnus, so we're waiting for Alex to moderate, I guess). In my opinion users will complain if they can't enter certain recipients any more, or worse, email gets sent to the wrong recipient by accident.

Comment 73

[Rob Lemley [:rjl]]

Thunderbird 78.2.0:
https://hg.mozilla.org/releases/comm-esr78/rev/e2a7f7534cca