Closed Bug 230577 Opened 21 years ago Closed 21 years ago

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


(MailNews Core :: Composition, defect)

Windows XP
Not set


(Not tracked)



(Reporter: burleigh, Assigned: mscott)




(2 files, 5 obsolete files)

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
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:
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 

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 

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.
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.
Assignee: sspitzer → mscott
Ever confirmed: true
Target Milestone: --- → mozilla1.7alpha
See bug 182192, and also bug 168204.
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" <>, Scott, David, "Smith, Jane" <>

and have that properly get parsed as:

"Smith, John" <>
"Smith, Jane" <>

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
Attached patch Rough draft of a patch (obsolete) — Splinter Review
This isn't perfect, but it deals with the obvious cases.
BTW, I think the address list dialog could do with something similar.
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.
Is there a reason that the header parser has that unusual effect?
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:

So how would
"Smith, John", Scott MacGregor, David Bienvenu, "Smith, Jane"
as in how would the header parser interpret that?

"Smith, John"
"Smith, Jane"
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?
Attached patch start of a possible fix (obsolete) — Splinter Review
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
Attachment #139417 - Attachment is obsolete: true
Blocks: 230700
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
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....
*** Bug 232848 has been marked as a duplicate of this bug. ***
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.
Attachment #139473 - Attachment is obsolete: true
Attachment #139503 - Attachment is obsolete: true
Attached patch attaching the correct patch (obsolete) — Splinter Review
Attachment #140532 - Attachment is obsolete: true
Comment on attachment 140533 [details] [diff] [review]
attaching the correct patch

most of this came from what Neil already wrote :)
Attachment #140533 - Flags: review?(
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(/.* >> /, "");
Attachment #140533 - Flags: review?( → review+
Attachment #140533 - Attachment is obsolete: true
Comment on attachment 140687 [details] [diff] [review]
updated patch with neil's comments

carrying forward Neil's r, seeking sr.
Attachment #140687 - Flags: superreview?(bienvenu)
Attachment #140687 - Flags: review+
Attachment #140687 - Flags: superreview?(bienvenu) → superreview+
fixed on the trunk and the M4 branch.
Closed: 21 years ago
Resolution: --- → FIXED
Thanks fellas--it's perfect!
Product: MailNews → Core
Product: Core → MailNews Core
You need to log in before you can comment on or make changes to this bug.