Last Comment Bug 496970 - can't search address book for contact that includes apostrophe
: can't search address book for contact that includes apostrophe
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Address Book (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Josh Geenen (:pi)
:
Mentors:
Depends on:
Blocks: 585483
  Show dependency treegraph
 
Reported: 2009-06-08 15:02 PDT by Wayne Mery (:wsmwk, NI for questions)
Modified: 2016-05-31 09:40 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Possible fix (991 bytes, patch)
2009-06-28 22:32 PDT, Josh Geenen (:pi)
no flags Details | Diff | Splinter Review
The fix (encodes apostrophes and parentheses) (4.02 KB, patch)
2009-07-03 10:02 PDT, Josh Geenen (:pi)
standard8: review+
neil: superreview+
Details | Diff | Splinter Review
Fixes bitrot (4.00 KB, patch)
2009-07-25 09:59 PDT, Josh Geenen (:pi)
joshgeenen+bugzilla: review+
joshgeenen+bugzilla: superreview+
Details | Diff | Splinter Review
Additional patch (1.92 KB, patch)
2009-07-31 09:30 PDT, neil@parkwaycc.co.uk
standard8: review+
standard8: superreview+
Details | Diff | Splinter Review

Description Wayne Mery (:wsmwk, NI for questions) 2009-06-08 15:02:50 PDT
can't search (quick or advanced) address book for contact that includes apostrophe.  quote and accent works.

1. create AB entry for O'Dork
2. in quick search field type O'Dork - O, ', D, ...

results: 
as soon as you type ' the search results end

expected results:
finds the dork :)
Comment 1 Josh Geenen (:pi) 2009-06-28 18:39:21 PDT
I get the same result in Linux x86_64 comm-central trunk.

The JavaScript function encodeURIComponent is called on the text entered but apostrophes are not encoded--along with letters, numbers, and - _ . ! ~ * ' ( and ).  I'm not sure if that is related to the problem.

abCommon.js calls nsIRDFService.GetResource with the following search URI:
"moz-abmdbdirectory://abook.mab?(or(PrimaryEmail,c,O'D)(DisplayName,c,O'D)(FirstName,c,O'D)(LastName,c,O'D))"

Usually this would reach nsAbDirectoryQuery::matchCardCondition several times but that doesn't seem to happen with a ' in the search string.  Next time I have a chance to compile w/ debugging enabled I'll take a look at why this happens.
Comment 2 Josh Geenen (:pi) 2009-06-28 22:32:33 PDT
Created attachment 385690 [details] [diff] [review]
Possible fix

nsAbDirectoryRDFResource::Init usually only gets called once with the full query URI.  If there is an apostrophe in the query URI, then it gets called twice (once for the full query URI and once for an incorrect URI) because the apostrophe isn't being encoded (encodeURIComponent does not escape it).

I'm not sure if the fix should go in the search function onEnterInSearchBar (where encodeURIComponent is called) or in the lower-level 
GetDirectoryFromURI function.  This patch does the latter and works for me.

Normal:
Breakpoint 1, nsAbDirectoryRDFResource::Init (this=0xaf77a300, 
    aURI=0xadaee198 "moz-abmdbdirectory://abook.mab?(or(PrimaryEmail,c,O)(DisplayName,c,O)(FirstName,c,O)(LastName,c,O))")
    at /home/josh/comm-central/mailnews/addrbook/src/nsAbDirectoryRDFResource.cpp:61

Bad:
Breakpoint 1, nsAbDirectoryRDFResource::Init (this=0xaf77ce20, 
    aURI=0xab871508 "moz-abmdbdirectory://abook.mab?(or(PrimaryEmail,c,O'D)(DisplayName,c,O'D)(FirstName,c,O'D)(LastName,c,O'D))")
    at /home/josh/comm-central/mailnews/addrbook/src/nsAbDirectoryRDFResource.cpp:61

Breakpoint 1, nsAbDirectoryRDFResource::Init (this=0xaf77a300, 
    aURI=0xaf5b8488 "moz-abmdbdirectory://a")
    at /home/josh/comm-central/mailnews/addrbook/src/nsAbDirectoryRDFResource.cpp:61

Note the incorrect URI in the second call.
Comment 3 Josh Geenen (:pi) 2009-07-01 08:10:57 PDT
Parentheses do not appear to work, either.  MDC says that encodeURIComponent "escapes all characters except the following: alphabetic, decimal digits, - _ . ! ~ * ' ( )" [0].  All of those characters except ' ( and ) work.

Search URI with '(' entered in the search box:
"moz-abmdbdirectory://abook.mab?(or(PrimaryEmail,c,()(DisplayName,c,()(FirstName,c,()(LastName,c,())"

[0] https://developer.mozilla.org/en/Core_JavaScript_1.5_Reference/Global_Functions/encodeURIComponent
Comment 4 Josh Geenen (:pi) 2009-07-03 10:02:52 PDT
Created attachment 386737 [details] [diff] [review]
The fix (encodes apostrophes and parentheses)

This patch should allow searches with apostrophes and parentheses in TB and Seamonkey.

Let me know if you would like a unit test for the encodeDirectoryQuery function or basic queries in this patch.
Comment 5 Mark Banner (:standard8) 2009-07-20 04:44:00 PDT
Comment on attachment 386737 [details] [diff] [review]
The fix (encodes apostrophes and parentheses)

I guess there's no other easier way around this. Neil would most likely know.

There's a little bit of rot caused by the removal of the resource/ directory.

Otherwise r=Standard8 if Neil is happy.
Comment 6 neil@parkwaycc.co.uk 2009-07-20 05:04:11 PDT
Comment on attachment 386737 [details] [diff] [review]
The fix (encodes apostrophes and parentheses)

Don't forget nsAbAutoCompleteSearch.js!
Comment 7 neil@parkwaycc.co.uk 2009-07-20 05:29:00 PDT
Sadly .replace(/['()]/g, escape) fails because of a hidden parameter :-(
Comment 8 Josh Geenen (:pi) 2009-07-25 09:59:19 PDT
Created attachment 390655 [details] [diff] [review]
Fixes bitrot

This fixes the bitrot on the previous patch.

Auto-complete already seems to work for me with contacts containing apostrophes or parentheses, can anyone confirm this?
Comment 9 Mark Banner (:standard8) 2009-07-31 06:00:48 PDT
(In reply to comment #7)
> Sadly .replace(/['()]/g, escape) fails because of a hidden parameter :-(

(In reply to comment #8)
> Auto-complete already seems to work for me with contacts containing apostrophes
> or parentheses, can anyone confirm this?

Neil, this works for me as well. Can you explain? how/why?
Comment 10 neil@parkwaycc.co.uk 2009-07-31 09:30:13 PDT
Created attachment 391894 [details] [diff] [review]
Additional patch

I couldn't get apostrophes to work reproducibly, but while struggling to come up with an answer to the previous question, I thought it might be worthwhile simplifying the string fu in this method; this happens to make apostrophes work anyway (but we still need to encode brackets of course).
Comment 11 neil@parkwaycc.co.uk 2009-08-03 15:21:56 PDT
Comment on attachment 391894 [details] [diff] [review]
Additional patch

Pushed changeset 3bae43b68010 to comm-central.
Comment 12 neil@parkwaycc.co.uk 2009-08-03 15:25:54 PDT
So the state of play with this patch should be that apostrophes now work in addressbook search and autocomplete, but (a variant of) attachment 390655 [details] [diff] [review] is still needed to fix addressbook search of brackets and autocomplete still doesn't work for brackets.
Comment 13 Wayne Mery (:wsmwk, NI for questions) 2010-08-08 12:36:36 PDT
the main search case works afaict with v3.1. Does anyone think this should not be marked fixed?

autocomplete is a different story. filed bug 585483
Comment 14 Wayne Mery (:wsmwk, NI for questions) 2011-11-17 19:53:55 PST
filed Bug 703493 for apostrophe autocomplete
Comment 15 :aceman 2016-05-31 09:40:35 PDT
It seems attachment 390655 [details] [diff] [review] never landed here. So the bracket case had to be fixed again in bug 749097, fortunately in the same way.

Note You need to log in before you can comment on or make changes to this bug.