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

RESOLVED FIXED in Thunderbird 33.0

Status

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jik, Assigned: mkmelin)

Tracking

(Blocks 1 bug, {dogfood, regression})

Dependency tree / graph

Thunderbird Tracking Flags

(thunderbird31+ fixed, thunderbird32 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

5 years ago
Bug 1:

1. Create a contact with the display name "Last, First" (yes, just like that) and the email address "lastfirst@example.com".
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 <lastfirst@example.com>" and "First <lastfirst@example.com>".

Bug 2:

4. Double-click on the "First <lastfirst@example.com>" 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 <lastfirst@example.com>", 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 <lastfirst@example.com>" in the second "To:" field.
Assignee

Comment 1

5 years ago
Fallout from bug 842632?
Keywords: regression

Comment 2

5 years ago
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 lastfirst@example.com
2. Open new email and in the to: field start typing  last
3. The autofill will show last, First <lastfirst@example.com>
4. Hit tab and you get 2 to addressees:
Last,First <lastfirst@example.com>
First <lastfirst@example.com>

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 lastfirst@example.com
2. Open new email and in the to: field start typing last
3. The autofill will show last, First <lastfirst@example.com>
4. Hit tab and you get 2 to addressees:
Last
First <lastfirst@example.com>

Updated

5 years ago
Component: Message Compose Window → Composition
OS: Linux → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All

Updated

5 years ago
Duplicate of this bug: 1007354

Updated

5 years ago
See Also: → 385571

Comment 4

5 years ago
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.
Severity: normal → major
Keywords: dogfood
Summary: addressee area mishandles display names with commas in them → Recipient area mishandles display names with commas in them (e.g. "LastName, FirstName"), creates disfunctional extra "LastName" recipients without email address

Comment 5

5 years ago
Also happening on Mac SM 2.26 in case that makes a difference.
Assignee

Updated

5 years ago
Assignee: nobody → mkmelin+mozilla

Comment 6

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

Comment 7

5 years ago
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

5 years ago
(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

Updated

5 years ago
Duplicate of this bug: 1026394
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.
Summary: Recipient area mishandles display names with commas in them (e.g. "LastName, FirstName"), creates disfunctional extra "LastName" recipients without email address → Recipient area mishandles display names with commas in them (e.g. "LastName, FirstName"), during autocomplete creates dysfunctional extra "LastName" recipients without email address
Assignee

Comment 11

5 years ago
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

5 years ago
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.
Flags: needinfo?(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?
Flags: needinfo?(standard8) → needinfo?(Pidgeot18)
Assignee

Comment 14

5 years ago
(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.

Updated

5 years ago
Duplicate of this bug: 1026496

Comment 16

5 years ago
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."
Assignee

Comment 17

5 years ago
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...
(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.
Assignee

Comment 19

5 years ago
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

5 years ago
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 lastfirst@example.com
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 <lastfirst@example.com>"

Expected: Address without this "las >>".

Comment 21

5 years ago
(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 <lastfirst@example.com>"

Nomis, thanks. The latter looks like Bug 1009469.
See Also: → 1009469

Comment 22

5 years ago
(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 <lastfirst@example.com>"
> 
> Nomis, thanks. The latter looks like Bug 1009469.

Ah, yes. Thanks.
Posted patch comma-headerSplinter Review
Like this?

This code is screaming for some decent mozmill tests... unfortunately, I'm no good at those :-(
Attachment #8441566 - Attachment is obsolete: true
Attachment #8451192 - Flags: feedback?(mkmelin+mozilla)
Flags: needinfo?(Pidgeot18)
Assignee

Comment 24

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

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

Looks good to me!
Attachment #8451192 - Flags: feedback?(mkmelin+mozilla) → feedback+
Assignee

Updated

5 years ago
Duplicate of this bug: 1034939
Assignee

Updated

5 years ago
Duplicate of this bug: 1036844
Assignee

Comment 27

5 years ago
I'll look into the mozmill test for this.
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.
Attachment #8451192 - Flags: review+
Assignee

Comment 29

5 years ago
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.
Attachment #8451192 - Flags: approval-comm-beta+
Attachment #8451192 - Flags: approval-comm-aurora+
(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.
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.
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.
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Assignee

Updated

5 years ago
Blocks: 1039443

Comment 34

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

Comment 35

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

Yes, this moved to bug 1039443 (sorry, forgot to note it here)
Attachment #8456437 - Attachment is obsolete: true

Comment 36

5 years ago
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?
Assignee

Comment 37

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

Comment 38

5 years ago
For Seamonkey, this is bug 1071476.
I wonder why this is not discussed (and solved!) for both programs in a single bug...
(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.
You need to log in before you can comment on or make changes to this bug.