Closed Bug 966053 Opened 10 years ago Closed 10 years ago

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

Categories

(MailNews Core :: Composition, defect)

defect
Not set
major

Tracking

(thunderbird31+ fixed, thunderbird32 fixed)

RESOLVED FIXED
Thunderbird 33.0
Tracking Status
thunderbird31 + fixed
thunderbird32 --- fixed

People

(Reporter: jik, Assigned: mkmelin)

References

Details

(Keywords: dogfood, regression)

Attachments

(1 file, 2 obsolete files)

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.
Fallout from bug 842632?
Keywords: regression
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>
Component: Message Compose Window → Composition
OS: Linux → All
Product: Thunderbird → MailNews Core
Hardware: x86_64 → All
See Also: → 385571
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
Also happening on Mac SM 2.26 in case that makes a difference.
Assignee: nobody → mkmelin+mozilla
If I use Return instead of Tab to accept the address suggestions it doesn't split them.
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)
(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
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
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.
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)
(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.
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."
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.
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.
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 >>".
(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
(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.
Attached 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)
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+
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+
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: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 33.0
Blocks: 1039443
Is the bug966053_address_comma_split.patch attachment to be obsoleted and moved to the other bug with tests?
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
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?
This fix is specific to thunderbird, though porting it to seamonkey should be straight forward. Please file a new bug for that.
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.