230577 - mail address autocompletion chooses three addresses from address book entries of a certain form

Status: VERIFIED FIXED

(MailNews Core :: Composition, defect)

Description

[Frank Burleigh]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040110
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a) Gecko/20040110

Address completion inserts three addresses when the addressee's Name in my
address book looks like

"Burleigh, Frank"

but selects one when the Name looks like

Frank Burleigh

Reproducible: Always

Steps to Reproduce:
1. Identity an addressee whose Name is in the form "Last, First" in your address
book.
2. Type enough into Compose's To line to allow autocomplete to find this
address, then press tab or enter.

Actual Results:  
Three addresses, a few of which will be correct.

Expected Results:  
One address that's correct.

These addresses are from Exchange correspondents, their names are within quotes
with the last name first, then a comma, then the first name.  This behavior
began within the last week, I *believe* after a checkin here:
http://bugzilla.mozilla.org/show_bug.cgi?id=227206.

Comment 1

[Mike Cowperthwaite]

Frank Burleigh, please provide more information about the address-book entries, 
both of the address you *want* to match and the addresses that are inadvertantly 
matching:

Is the *only* name field in the correct 'address card' the First, with the 
quoted name (e.g. "Burleigh, Frank") in it?

You said that of the addresses filled in, "a few... will be correct."  Are there 
multiple email addresses on that card, or are there multiple cards for these 
individuals?

Is there any commonality between what you've typed in the To: field and the 
incorrect address(es) that end up inserted?


CC'ing mscott due to the possible tie-in to 227206.

Comment 2

[Frank Burleigh]

Attachment: Results of tabbing after typing a few characters of an address

Attachment showing results of address completion.

All of autocompletion's guesses derive from the right AB entry--it gets the
right card but seems to parse it in multiple ways and then yield a number of
results to me (also: only one address per these cards).  Further testing shows
that <tab> after I type (in this case) "kl" gives me three addresses, whereas
<enter> gives me two addresses.  I see by looking at the AB entries that the
fields include

First: "Frye, Kristy
Last: L"
Display: "Frye, Kristy L"

NB: these addresses have been captured by Moz, I didn't create these.  All of
the AB entries captured by Moz from my local Exchange correspondents seem to
have this form.

Comment 3

[Mike Cowperthwaite]

See bug 182192, and also bug 168204.

Comment 4

[Scott MacGregor]

There has to be an easy way to craft a reg expressionin JS to tokenize names by
commas not contained in quotes so we can get right:

"Smith, John" <foo.com>, Scott, David, "Smith, Jane" <bar.com>

and have that properly get parsed as:

"Smith, John" <foo.com>
Scott
David
"Smith, Jane" <bar.com>

I just don't know what the reg expression magic is to do that yet. It would
replace the following two lines in abAddressingWidgetOverlay.js:

fullNames = addressText.split(',');
numAddresses = fullNames.length;

cc'ing Neil as he was asking me about this bug

Comment 5

[[e-mail]]

Attachment: Rough draft of a patch

This isn't perfect, but it deals with the obvious cases.
BTW, I think the address list dialog could do with something similar.

Comment 6

[Scott MacGregor]

Neil, I initially wrote this code using the header parser but it doesn't work
for the case we are trying to fix. It doesn't cope with names (first and last)
separated by commas:

Scott MacGregor, David Bienvenu, John Smith

which was the case the original bug was fixing.

That's why i needed to use reg expressions to tokenize off of commas that aren't
contained within quotes.

Comment 7

[[e-mail]]

Is there a reason that the header parser has that unusual effect?

Comment 8

[Scott MacGregor]

because it thinks spaces which aren't contained in quotes are valid delimters
for names I believe. 

Another way to fix this. This code really should not be executing at all unless
the  text box contains a list of addresses delimited by comma. Instead, we
should be doing exactly what we used to do to autocomplete (or not) the line. 

But because the address has a comma in the quoted part, we think we have a list
and call into the new code which parses down a comma delimited list of
addresses. We could fix this by making our test for a comma delimited list smarter.

That would mean modifying the if clause here:

http://lxr.mozilla.org/seamonkey/source/mailnews/compose/resources/content/addressingWidgetOverlay.js#857

Comment 9

[[e-mail]]

So how would
"Smith, John", Scott MacGregor, David Bienvenu, "Smith, Jane"
work?

Comment 10

[Scott MacGregor]

as in how would the header parser interpret that?

"Smith, John"
Scott 
MacGregor
David
Bienvenu
"Smith, Jane"

Comment 11

[[e-mail]]

But you'd still need some code that would parse my test case into this list:
"Smith, John"
Scott MacGregor
David Bienvenu
"Smith, Jane"
Would it be unreasonable to change the header parser to do this?

Comment 12

[Scott MacGregor]

Attachment: start of a possible fix

this leverages regular expressions to fix this problem. I'm not sure whether to
fix it this way or by modifying the behavior of the msg header parser. This is
less riskier but I'm still open.

In addition to this change, we should also modify our search for a comma in the
text field to search for a comma outside of quotes before calling into
parseAndAddAddresses.

Comment 13

[[e-mail]]

Attachment: Remove space-splitting code from the header parser

This also goes some way to fixing a bugbear of mine - when you get a message
whose header contains "To: Undisclosed Recipients" mail changes it to *To:*
_Undisclosed_ , _Recipients_ but this patch changes it to display *To:*
_Undisclosed Recipients_ although I would prefer *To:* Undisclosed Recipients

Comment 14

[Scott MacGregor]

I only wonder why that code was added (probably back in the 4.x days), at some
point we seemed to think we had split based on spaces. HOw can we prove to
ourselves we can safely remove that check....

Comment 15

[Scott MacGregor]

*** Bug 232848 has been marked as a duplicate of this bug. ***

Comment 16

[Scott MacGregor]

Attachment: updated patch using Neil's approach

I've been running a build with the modified header parser behavior that Neil
has suggested and:

1) I get the most consistent results going through the modified header parser
rather than any reg expressions logic I can come up with.

2) Granted I am a small sample set, but I have not run into any email messages
where the MUA used spaces instead of commas to delimit addresses anymore.

I'd like to go with this approach if Neil is on board.

Comment 17

[Scott MacGregor]

Attachment: attaching the correct patch

Comment 18

[Scott MacGregor]

Comment on attachment 140533 [details] [diff] [review]
attaching the correct patch

most of this came from what Neil already wrote :)

Comment 19

[[e-mail]]

Comment on attachment 140533 [details] [diff] [review]
attaching the correct patch

>+  var strippedAddresses = addressText.replace(/.* >> (.*)/, "$1"); // strip any leading >> characters inserted by the autocomplete widget

I suggest you put the comment on the previous line, and you may want to
simplifiy the regexp to addressText.replace(/.* >> /, "");

Comment 20

[Scott MacGregor]

Attachment: updated patch with neil's comments

Comment 21

[Scott MacGregor]

Comment on attachment 140687 [details] [diff] [review]
updated patch with neil's comments

carrying forward Neil's r, seeking sr.

Comment 22

[Scott MacGregor]

fixed on the trunk and the M4 branch.

Comment 23

[Frank Burleigh]

Thanks fellas--it's perfect!