966053 - Recipient area mishandles display names with commas in them (e.g. "LastName, FirstName"), during autocomplete creates dysfunctional extra "LastName" recipients without email address

Status: RESOLVED FIXED

(MailNews Core :: Composition, defect)

Description

[Jonathan Kamens]

Bug 1:

1. Create a contact with the display name "Last, First" (yes, just like that) and the email address "[e-mail]".
2. Type "lastfirst" into the To: field of a compose window, wait until Thunderbird finds the match, and then hit Tab.
3. You'll end up with two addressees, "Last, First <[e-mail]>" and "First <[e-mail]>".

Bug 2:

4. Double-click on the "First <[e-mail]>" and hit backspace or delete to erase it.
5. Hit Shift-Tab three times. The first time should take you to the "To:" drop-down next to the now-empty field, the second should take you to "Last, First <[e-mail]>", and the third should take you to teh "To:" drop-down before it.
6. As you hit Shift-Tab the third time, the address will split into "Last" in the first "To:" field and "First <[e-mail]>" in the second "To:" field.

Comment 1

[Magnus Melin [:mkmelin]]

Fallout from bug 842632?

Comment 2

[Jeremy Bascom]

Bug 1 is still present in 30.0b1 running on a mac.
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Thunderbird/30.0
Build ID 20140509030228

extra info: the autofill pop-up does not display the quotes.

Steps to reproduce

1. Create contact with display name "Last, First" and the email address [e-mail]
2. Open new email and in the to: field start typing  last
3. The autofill will show last, First <[e-mail]>
4. Hit tab and you get 2 to addressees:
Last,First <[e-mail]>
First <[e-mail]>

If you create a contact without the quotes around the comma separated name you get different results.

1. Create contact with display name Last, First and the email address [e-mail]
2. Open new email and in the to: field start typing last
3. The autofill will show last, First <[e-mail]>
4. Hit tab and you get 2 to addressees:
Last
First <[e-mail]>

Comment 4

[Thomas D. (:thomas8)]

For users or locales that have "LastName, FirstName" as their preferred display name format (with or without quotes in AB), this will mess up the recipient area functionality big time by creating useless extra recipient rows just having "Lastname" for any contact from their AB. Dogfood.

Not seen in TB24. Seen on Trunk -> Regression. Needs fix for TB31.

Comment 5

[long]

Also happening on Mac SM 2.26 in case that makes a difference.

Comment 6

[Jeremy Bascom]

If I use Return instead of Tab to accept the address suggestions it doesn't split them.

Comment 7

[long]

On Linux SM 2.26 it doesn't seem to matter if you use Enter or Tab, either way the addressing gets screwed up (although slightly differently depending on which you use)

Comment 8

[Jeremy Bascom]

(In reply to Jeremy Bascom from comment #6)
> If I use Return instead of Tab to accept the address suggestions it doesn't
> split them.

Sorry, same platform as above for my testing
30.0b1 running on a mac 10.9.3.
User Agent Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Thunderbird/30.0
Build ID 20140509030228

Comment 10

[Wayne Mery (:wsmwk)]

We really need this for TB31. 

There is also bug 1012398 (blocked on review), and perhaps others. We will need some serious autocomplete testing during the beta.

Comment 11

[Magnus Melin [:mkmelin]]

Just FTR, I believe this bug is fallout from Joshua's work js mime work. The results we get back in autocomplete doesn't include quotes for the cases they have to.

This (and bug 1012398 which is just another symptom) are fallout from the switch to toolkit autocomplete.

Comment 12

[Thomas D. (:thomas8)]

Hi Mark (Standard8), Wayne & I concur that this is a serious dogfood bug causing major annoyances for entire locales in everyday usage of TB. Can you please set tracking-thunderbird31+ flag accordingly? Tia.

(In reply to Thomas D. from comment #4)
> For users or locales that have "LastName, FirstName" as their preferred
> display name format (with or without quotes in AB), this will mess up the
> recipient area functionality big time by creating useless extra recipient
> rows just having "Lastname" for any contact from their AB. Dogfood.
> 
> Not seen in TB24. Seen on Trunk -> Regression. Needs fix for TB31.

(In reply to Wayne Mery (:wsmwk) from comment #10)
> We really need this for TB31. 
> 
> There is also bug 1012398 (blocked on review), and perhaps others. We will
> need some serious autocomplete testing during the beta.

Comment 13

[Mark Banner (:standard8)]

(In reply to Magnus Melin from comment #11)
> Just FTR, I believe this bug is fallout from Joshua's work js mime work. The
> results we get back in autocomplete doesn't include quotes for the cases
> they have to.
> 
> This (and bug 1012398 which is just another symptom) are fallout from the
> switch to toolkit autocomplete.

Joshua: can you take a look at this please?

Comment 14

[Magnus Melin [:mkmelin]]

(In reply to Mark Banner (:standard8) from comment #13)
> > This (and bug 1012398 which is just another symptom) are fallout from the
> > switch to toolkit autocomplete.

Sorry, This = bug 1009469.

Comment 16

[James Rome]

It did not get fixed for 31 thus far. A side effect of this bug is that if you make the first address a blind copy, the addresses after this split address are just "to."

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: bug966053_address_comma_split.patch

This fixes it, and arguably it is actually how a mailbox should be toStringed. It would add quotes where needed in some other places also. (From what I've seen so far, rightly so though.)

I haven't tested it much yet, and maybe there are tests that need to be fixed...

Comment 18

[Joshua Cranmer [:jcranmer]]

(In reply to Magnus Melin from comment #17)
> Created attachment 8441566 [details] [diff] [review]
> bug966053_address_comma_split.patch
> 
> This fixes it, and arguably it is actually how a mailbox should be
> toStringed. It would add quotes where needed in some other places also.
> (From what I've seen so far, rightly so though.)

No, it's not. Look at the plethora of bugs filed in the past about the desire to unquote mailboxes to see why this is the case. Indeed, the ad-hoc attempts to do this is what motivated me to actually take an axe to the nsIMsgHeaderParser interface.

Comment 19

[Magnus Melin [:mkmelin]]

I meant that in the technical manner, as the RFC's would print a mailbox.

There are of course cases where you'd prefer not to have the quotes, but for the cases where you do have a comma in the display name (yes, it's a silly format) you always end up with strange things otherwise. 

Say you have a list of mailboxes 
"first, last" <foo@invalid>, "second, last2" <second@invalid>

Having that shown as just 
first, last <foo@invalid>, second, last2 <second@invalid>

... is just confusing, especially when we don't show addresses if the contact is in the address book.

But if you don't like this approach, where is the version that does preserve quotes nowadays? There was an UnquotePhraseOrAddr parser method that had the option to preserve quotes, but that has been removed, and i don't see any replacement in nsIMsgHeaderParser.

Comment 20

[Nomis101]

I had this bug now very often since I'm testing TB 31. Nearly everytime I'm composing a new mail. I also see another variant of this bug:
1. Create contact with display name "Last, First" and the email address [e-mail]
2. Open new email, and in the to: field start typing "las"
3. Now click on the subject or the message field
4. You end with "las >> First Last <[e-mail]>"

Expected: Address without this "las >>".

Comment 21

[Thomas D. (:thomas8)]

(In reply to Nomis101 from comment #20)
> I had this bug now very often since I'm testing TB 31. Nearly everytime I'm
> composing a new mail. I also see another variant of this bug:
> 1. Create contact with display name "Last, First" and the email address
> 4. You end with "las >> First Last <[e-mail]>"

Nomis, thanks. The latter looks like Bug 1009469.

Comment 22

[Nomis101]

(In reply to Thomas D. from comment #21)
> (In reply to Nomis101 from comment #20)
> > I had this bug now very often since I'm testing TB 31. Nearly everytime I'm
> > composing a new mail. I also see another variant of this bug:
> > 1. Create contact with display name "Last, First" and the email address
> > 4. You end with "las >> First Last <[e-mail]>"
> 
> Nomis, thanks. The latter looks like Bug 1009469.

Ah, yes. Thanks.

Comment 23

[Joshua Cranmer [:jcranmer]]

Attachment: comma-header

Like this?

This code is screaming for some decent mozmill tests... unfortunately, I'm no good at those :-(

Comment 24

[Magnus Melin [:mkmelin]]

Comment on attachment 8451192 [details] [diff] [review]
comma-header

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

Looks good to me!

Comment 27

[Magnus Melin [:mkmelin]]

I'll look into the mozmill test for this.

Comment 28

[Mark Banner (:standard8)]

Comment on attachment 8451192 [details] [diff] [review]
comma-header

I really think we should take this for 31. Whilst I realise it hasn't got tests, I think we should add those later.

Comment 29

[Magnus Melin [:mkmelin]]

FWIW, tests are on try now.
After trying it some more I do dislike the missing quotes even more, I think they should be there in the composition area. The patch I'm testing does that.

Comment 30

[Magnus Melin [:mkmelin]]

Attachment: bug966053_address_comma_split.patch

Comment 31

[Joshua Cranmer [:jcranmer]]

(In reply to Magnus Melin from comment #29)
> FWIW, tests are on try now.
> After trying it some more I do dislike the missing quotes even more, I think
> they should be there in the composition area. The patch I'm testing does
> that.

The evidence I have seen from bug reports on bugzilla is that quoted addresses in the compose window cause more problems, rather than fewer. I know Josiah has put some work into moving away from intensely string-based composition strings, which makes the quotation marks or lack thereof principally moot.

Comment 32

[Mark Banner (:standard8)]

If we want to fix the missing quotes, I'd be prepared to do that in the first dot release. For now, we're overdue on the final beta and other builds. So lets take this, which fixes most of the issue, and we can fine tune later.

Comment 33

[Mark Banner (:standard8)]

https://hg.mozilla.org/comm-central/rev/2a9c11b62db2
https://hg.mozilla.org/releases/comm-aurora/rev/f1120f0c8259
https://hg.mozilla.org/releases/comm-beta/rev/99db90ff6c68

Please can we move tests/other issues to new bugs for tracking purposes.

Comment 34

[:aceman]

Is the bug966053_address_comma_split.patch attachment to be obsoleted and moved to the other bug with tests?

Comment 35

[Magnus Melin [:mkmelin]]

Comment on attachment 8456437 [details] [diff] [review]
bug966053_address_comma_split.patch

Yes, this moved to bug 1039443 (sorry, forgot to note it here)

Comment 36

[long]

When is this getting fixed in Seamonkey?  Do I need to open a new bug for that or can somebody with the power re-open this one?

Comment 37

[Magnus Melin [:mkmelin]]

This fix is specific to thunderbird, though porting it to seamonkey should be straight forward. Please file a new bug for that.

Comment 38

[Tilmann Reh]

For Seamonkey, this is bug 1071476.
I wonder why this is not discussed (and solved!) for both programs in a single bug...

Comment 39

[Joshua Cranmer [:jcranmer]]

(In reply to Tilmann Reh from comment #38)
> For Seamonkey, this is bug 1071476.
> I wonder why this is not discussed (and solved!) for both programs in a
> single bug...

Much of the front-end code is not shared between the two programs.