496970 - can't search address book for contact that includes apostrophe

Status: RESOLVED FIXED

Description

[Wayne Mery (:wsmwk, NI for questions)]

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)]

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)]

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)]

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)]

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)]

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

[[e-mail]]

Comment on attachment 386737 [details] [diff] [review]
The fix (encodes apostrophes and parentheses)

Don't forget nsAbAutoCompleteSearch.js!

Comment 7

[[e-mail]]

Sadly .replace(/['()]/g, escape) fails because of a hidden parameter :-(

Comment 8

[Josh Geenen (:pi)]

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)]

(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

[[e-mail]]

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

[[e-mail]]

Comment on attachment 391894 [details] [diff] [review]
Additional patch

Pushed changeset 3bae43b68010 to comm-central.

Comment 12

[[e-mail]]

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)]

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)]

filed Bug 703493 for apostrophe autocomplete

Comment 15

[:aceman]

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.