1658157 - Email addresses with comma in display name cannot be entered via keyboard if they match an auto-complete result

Status: NEW

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

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1642279 +++

Have John Smith <[e-mail]> in your address book.

Try to send an e-mail to Smith, James <[e-mail]>.

It doesn't work, entering Smith, will auto-complete John.

Comment 1

[Jorg K (CEST = GMT+2)]

Looks like this code
https://searchfox.org/comm-central/rev/085526b22865ea90738d779fa14bf711d46f69d3/mail/components/compose/content/addressingWidgetOverlay.js#564
isn't smart enough since the selection we test is >> John Smith <[e-mail]> and that is a valid address, only that the user never entered it.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: 1658157.patch

What do you think of this? I started coding the four cases separately, like so:

        !isValidAddress(input.value.substring(0, input.value.selectionEnd)) ||
        selection[0] == "," ||
        selection.startsWith(" >> ") ||
        // how to detect case 4??

and then it occurred to me that input.selectionStart > 0 covers it all. Am I missing something?

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1658157.patch (v1b)

This is better.

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1658157.patch (v2)

Actually, this is a whole let easier. If invalid or any autocomplete in progress, don't terminate with comma.

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 9169014 [details] [diff] [review]
1658157.patch (v2)

Hmm, Alex is on PTO?

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1658157.patch (v2)

Rebased, sorry about the noise.

Comment 7

[Magnus Melin [:mkmelin]]

Comment on attachment 9169028 [details] [diff] [review]
1658157.patch (v2)

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

But then you can't autocomplete with comma, which gives inconsistent behaviour.
As bug 1138033 comment 53 points out, to achieve the wanted result, one can remove the autocomplete suggestion using Delete, and then go on typing in this unusual case. That may be enough.

Comment 8

[Jorg K (CEST = GMT+2)]

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

But then you can't autocomplete with comma, which gives inconsistent behaviour.

Right, the patch stops auto-complete when typing a comma. That's the desired behaviour. The comma should only trigger pill creation when you've completely entered the address. I don't know what's inconsistent about that. Doing auto-complete on a comma, that is inconsistent and surprising.

Let me list the cases I had spelled out in the original version of the patch and you tell me why auto-completion would make sense here:

+      // Don't trigger autocomplete in the following cases:
+      // 1) `Doe,` was entered quickly, no autocomplete, that's not a valid address.
+      // 2) `Doe,` was entered, triggering autocomplete with `Doe, John <[e-mail]`,
+      //    selection is `, John <[e-mail]`.
+      // 3) `Doe,` was entered, triggering autocomplete with ` >> Jane Doe <[e-mail]>`.
+      // 4) `Do,` was entered, triggering autocomplete with `Doe, John <[e-mail]>`,
+      //    selection is `e, John <[e-mail]`.
+      // Cases 2) to 4) can be summarised as `input.selectionStart > 0`.

I think deleting the auto-complete suggestion/selection is not very intuitive and should only be promoted as the last resort. What do Alex and Thomas think?

Comment 9

[Jorg K (CEST = GMT+2)]

Actually, this is worse, since no one will know when to delete the auto-complete selection.

If you have Jane Doe <[e-mail]> in the AB, typing Doe will come up with that suggestion. If you continue typing the comma, Jane will already be accepted. That's utterly wrong. You need to know before typing the comma that you need to delete the auto-complete selection. That's not obvious at all.

It's a different case to wanting to write to [e-mail] when the suggestion is [e-mail] with the m being selected. Then you just hit delete and be done. That's bug 1138033.

Comment 10

[Magnus Melin [:mkmelin]]

It's inconsistent within(In reply to Jorg K (CEST = GMT+2) from comment #8)

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

But then you can't autocomplete with comma, which gives inconsistent behaviour.

Right, the patch stops auto-complete when typing a comma. That's the desired behaviour. The comma should only trigger pill creation when you've completely entered the address. I don't know what's inconsistent about that. Doing auto-complete on a comma, that is inconsistent and surprising.

It's not desired behaviour. Auto-complete on comma is how one enumerates items on one line, which is what this is about.
Gmail does it. Outlook (at least online) does it.

If we "sometimes" do it, that's inconsistent within the application.

Comment 11

[Jorg K (CEST = GMT+2)]

Comment on attachment 9169028 [details] [diff] [review]
1658157.patch (v2)

And further to the inconsistency issue: After bug 1642279, the behaviour is now totally inconsistent. Depending on the auto-complete selection, entering a comma now sometimes triggers auto-complete, and sometimes it doesn't, in case the selection starts with a comma. Why is that case exempt but not the others?

Comment 12

[Magnus Melin [:mkmelin]]

Comment on attachment 9169028 [details] [diff] [review]
1658157.patch (v2)

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

Well, I think it makes sense to exempt the case where the comma is in the autocomplete suggestion exactly because the comma is next char is the suggestion.

Comment 13

[Jorg K (CEST = GMT+2)]

(Since pEp also have an Outlook plugin, I actually have an up-to-date OL client on my machine, I'll try it.)

Comment 14

[Thomas D. (:thomas8)]

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

I think deleting the auto-complete suggestion/selection is not very intuitive and should only be promoted as the last resort. What do Alex and Thomas think?

+1. Afaics Jörg's patch attachment 9169028 [details] [diff] [review] does exactly the right thing, prevent premature autocompletion while preserving the convenience of pill creation with comma.

• We all agree that we want to make entering addresses convenient by allowing comma to finish a pill. Thanks Magnus for keeping that on the agenda.
• Please note the difference between comma for autocompletion and comma for pill creation!
• As Alex and others on this bug have pointed out, we cannot prescribe/control user's input, and Doe, John <[e-mail]> is a perfectly valid recipient input, as is Alessandro Castellani, TB UX <[e-mail]>, "Miller, Jane, dep. 45, Anytown City Administration" <[e-mail]> and Koizumi, Jun'ichirō <[e-mail]> for our friends from the East who prefer Last name first. No doubt, Thunderbird must respect the standards and provide for these everyday use cases. This isn't a matter of personal preference.
• It follows, as Alex and others here have all agreed, that autocompletion on comma is not possible (like it or not) because it interferes with legitimate everyday input prematurely. We can't expect users to guess in advance that typing comma will suddenly hijack their input, and when it happens, it's too late.
• Also, autocompletion on comma is not very intuitive - I am yet to see a single application which does that, so my theory is that users would not even try to autocomplete with comma. Everybody knows autocomplete suggestions must be confirmed with enter or tab to apply. Why would anyone try to type a comma when there's a pending autocomplete selection, given that every known application in this world overwrites text selections when you type another character?
• Having to interrupt typing legitimate input before typing comma, delete an autocomplete suggestion which you haven't asked for, and then continue typing, is clearly not discoverable, highly interruptive and error-prone, so we don't want to go there.
• That said, nothing stops us to allow pill creation on comma for user's convenience, which is what Jörg's patch does, combining convenience with standards.

In a followup bug, we'll want to ensure that isValidAddress() actually checks accurately for variants of valid recipients, and does not consider an incomplete address complete; which involves considering double quotes around display name: "Alessandro Castellani, [e-mail], Canada" <[e-mail]>.

• isValidAddress() is currently not really validating anything:
  return address.includes("@", 1) && !address.endsWith("@");
  https://searchfox.org/comm-central/rev/4ea623bcaa0bc74d148ed3560f7e89ffd01de8d9/mail/base/content/mailWidgets.js#1937-1939
• full email validation regex for email addresses without display name is something like 20 lines. We might get away with less if we apply some fuzzy logics/heuristics, but surely it must cover quoted display names which can contain anything, including what looks like a valid email address, but isn't.

Comment 15

[Thomas D. (:thomas8)]

I have tested gmail webmail. It just stupidly creates a pill upon typing comma, even for "Jo, you'll get "Jo (with quotation mark) as a pill - I don't think they have put any effort into allowing and ensuring valid addresses. Thunderbird must do better than that.

Whilst it's true that gmail will autocomplete the topmost autocomplete proposal when you type comma, that stops being useful behaviour when it prevents valid recipients for large real world use cases.

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: OL-auto-complete.png

The desktop application Outlook does a far less aggressive auto-complete. It doesn't fill anything into the input area, and the user needs to explicitly click or otherwise select the suggestion. There are no suggestions based on surname or any other parts of the AB card, it's only matched by e-mail address (which is not helpful, but that's OL for you).

So Magnus, how can we resolve this issue? Thomas and I think that the proposed patch is right, we believe that Alex will share that opinion, but we'll have to see when he returns from PTO.

IMHO, auto-competing Smith, with anything with Smith in the address books is a HUGE fault of the product which will cause quite a storm when it hits the masses, especially since auto-complete used to stop when a comma was entered.

Comment 17

[Magnus Melin [:mkmelin]]

(In reply to Thomas D. from comment #15)
The UI must not allow quotes to be anything except quotes. It just doesn't work out and makes a huge mess of the area.

Like I wrote, comma for confirm is very common elsewhere too. Not supporting it would surely get way more complaints than not.

I understand you want to be able to enter "first, last" addresses. We allow that now. I don't think we need to optimize for it - it's a one time operation per recipient. And from what I can tell, not a very common operation at all: I think I've never got any email from anyone where the displayname is filled on first correspondence.

Comment 18

[Alessandro Castellani [:aleca]]

Comment on attachment 9169028 [details] [diff] [review]
1658157.patch (v2)

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

I see the intention here, and I agree that we should probably better optimize the generation of pills on comma, but this change breaks what we achieved in the previous patch.

Now if I type `Doe` and I get the only available autocomplete I have, which is `Doe, Jane <email>`, it doesn't matter how many times I hit comma, the pill is not created.

As reported, other email clients like Outlook and Gmail have comma autocomplete/pill generation, so we need to support and respect that.

Comment 19

[Wayne Mery (:wsmwk)]

Doesn't seem worthy of high priority.