101498 - Mail auto complete preselects the default domain OVER the first ldap match

Status: VERIFIED FIXED

(MailNews Core :: LDAP Integration, defect)

Description

[Thierry Carrez]

Filed as a new bug from bug 88315 :

The current behaviour (BuildID 2001092308) is different from Netscape 4.7X
behaviour, when there is a match on a LDAP directory :

Suppose the following situation : you have address book search and LDAP search
enabled. "[e-mail]" is in our LDAP directory (but not in our address
books). Local domain is scort.com.

I type "de" : address completion proposes two possibilities : "[e-mail]" and
"(LDAP) John Delly <[e-mail]>". But if you use RETURN or TAB,
autocomplete uses [e-mail].

Please note that if you DISABLE address book search, autocomplete correctly uses
the LDAP match rather than the localhost autocomplete !!

Something must be wrong in the search priorities, but I don't think anyone would
prefer a localhost autocomplete over an LDAP match autocomplete.

Comment 1

[Sheela Ravindran]

nbaca,
I am pretty sure this is a dup 

Comment 2

[Ninoschka Baca]

Assigning to yulian since this is specific to LDAP/autocomplete. 

Comment 3

[yulian chang]

Should reassign to mscott since this is related to recent bug 99222 fix.

Comment 4

[Jussi-Pekka Mantere]

Adding keywords.

Scott, your thoughts?

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

mscott: I think this should be straightforward to fix with my proposed semantics
clarifications for how to treat multiple default indexes in composite
autocomplete items in autocomplete.xml, as well as the tweak to
nsAbAutoCompleteSession to never consider point the default index to a "default
domain" match.

Comment 6

[Scott MacGregor]

this is a usability issue I made reference to in 99222 that I thought we should
fix. Unfortunately I don't believe it qualifies under the "stop ship" terms PDT
is mandating right now. 

Comment 7

[Jaime Rodriguez, Jr.]

greg/jeff - is this still a nsenterprise+? if we can minus it, let's get ti off
the radar.

Comment 8

[Scott MacGregor]

I thought i had a minus in there but I guess I didn't. Putting it back, read up
for my justifications.

Comment 9

[grega]

This is still a plus and we want to fix it for 0.9.4.  Thanks!  
Mscott, why do you think we should not fix this?  Does it only happen on NT? 
Does it rarely happen?  Unless you can convince me otherwise I think we should
fix this.

Comment 10

[Hong Kwon]

QA assessment is that this is NOT a showstopper and in the interest of
stability, we should not introduce this fix at this time.

Comment 11

[Scott MacGregor]

Great now you guys tell me =). I just created the fix. Patch coming.

Comment 12

[Scott MacGregor]

what a minute the subject is all wrong for this bug to. Clarifying...

when there are no local matches, the auto complete widget pre fills the default
domain entry the address book added instead of pre-selecting the first LDAP
search result.

Comment 13

[Scott MacGregor]

Attachment: proposed fix for better ldap auto complete usability

Comment 14

[Scott MacGregor]

This patch to the auto complete widget does several things. They all revolve
around the same idea of allowing subsequent search sessions from the default to
set a default index value and have it be honored.

1) when focus leaves the widget or the user hits enter, we no longer use the
default session's default index as the default value to return. Instead, we now
look for the first session with a VALID default index (i.e. != -1). If none
exists, then just take the first session that has a match. 

2) For each call to processSearchResults (called once per completed search
session), we call autoFillInput. We need to do this each time because it's
possible that a subsequent search will be the first one to set a valid default
index. I also added a variable: 
mDefaultMatchFilled to make autoFillInput smart so it kicks out right away if a
previous search session already caused us to prefill something in.

3) In the case where no one sets a default index, at the end of all the
searching we need to grab the default session (the first session with one or
more matches) and force an autoFillInput on the first match for this session. I
added an extra boolean to autoFillInput to explicitly state when we want to
force the first match to be selected as opposed to just using the default index
if it isn't -1. This handles the scenario where I type something that doesn't
match anything. i.e. (ladjlfkafjklasjldfj) But the address book automatically
inserts a match against the default domain. We still want to force the first
match to be the pre fill default if no one gave us a default index. 

Comment 15

[Joe Hewitt (gone)]

A few nitpicks on the patch:

1. can you change the comment above the changed line in
nsAbAutoCompleteSession.cpp to reflect the change
2. use the new xbl tab <field> instead of <property> for mDefaultMatchFilled. 
This is for xbl data members which don't have getters or setters.  I'll fix the
other properties at some point in the future...
3. can you rename the parameter useFirstMatchIfNoDefault to
aUseFirstMatchIfNoDefault

Fix those and you have [s]r=hewitt

Comment 16

[Scott MacGregor]

Attachment: updated patch with hewitt's suggestions

Comment 17

[Jean-Francois Ducarroz]

I have one concern: In the case of URL autocompletion, we are currently never
preselecting a result (no inline autocompletion). With this patch, if I
correclty understand it, you will alway finish by forcing a selection even if
the search engin hasn't specified one by returning -1 as defaultItem! Right? in
that case. you will cause a regression! we need to have a way for the search
engin to say no inline autocompletion.

Comment 18

[Scott MacGregor]

JF, autoFillInput already has safeguards to kick out if the widget has not been
configured for inline auto completion. So that won't be an issue. (I already
tested on the browser to confirm that).

Comment 19

[Scott MacGregor]

Comment on attachment 51264 [details] [diff] [review]
updated patch with hewitt's suggestions

this was sr=hewitt

Comment 20

[(not reading, please use [e-mail] instead)]

Comment on attachment 51264 [details] [diff] [review]
updated patch with hewitt's suggestions

r=sspitzer

Comment 21

[Scott MacGregor]

This is checked into the trunk. it'd be great if some of the ldap guys (if they
are around this weekend could test it out) b4 I email PDT. 

Test scenarios: (search local + ldap directory). Scenarios include tests of the
new behavior and regression tests.

1) Search for a name with no matches in your local address book but one exact
match in an ldap directory. Verify that the popup shows up showing the default
domain + the ldap match. the ldap match should be the entry pre filled into the
text field.

2) Same scenario as 1. Hit tab or enter. Make sure the ldap match is the default
value actually placed into the widget instead of the default domain match. (b4
the fix, the default domain would have been used)

2) Search for a name with no matches in your local address book but multiple
matches in an ldap directory. Verify that the default domain is not pre filled
nor pre selected when you hit enter or tab. 

3) Search for a name with no matches in your address book and LDAP directory.
Verify that the default domain is properly appended and pre-filled into the text
field. 

4) Search for a name with at least one valid address book match and at least one
ldap match. Verify that the prefilled in entry is the first address book match.
Enter or tab should also choose this first address book match (not the default
domain). 


5) Search for a name with one exact match in the address book and no ldap
matches. You shouldn't see a popup at all. You should see your exact match get
pre filled in automatically and enter or tab automatically accepts that. 

Comment 22

[yulian chang]

Using 20010929 trunk builds on Linux to test.


There are duplicate 2)s from above. 

1), first 2), 3) and 4) PASSED!

The second 2) Search for a name with no matches in your local address book but
multiple matches in an ldap directory. Verify that the default domain is not pre
filled nor pre selected when you hit enter or tab. 

Actual result: 

the default domain IS pre filled and pre selected when you hit enter or tab. 


5) Search for a name with one exact match in the address book and no ldap
matches. You shouldn't see a popup at all. You should see your exact match get
pre filled in automatically and enter or tab automatically accepts that. 


Actual result:
The popup shows up the default domain + the address book match. the exact match
is pre filled in automatically and enter or tab automatically accepts that.


Conclusion:

The problem addressed at the beginning has been fixed in case 1) and first 2).
trunk (20010929) and branch builds behave the same for second 2), 3), 4) and 5).
There's no regression.
There's no more improvement.

Comment 23

[yulian chang]

Verified with 20010929 trunk build on NT.
No 20010929 trunk build on Macs yet.

Comment 24

[Scott MacGregor]

thanks for the testing yulian! I just got the plus from PDT over email just now.
This is now in the branch and should show up in Monday's branch builds. 

Comment 25

[Thierry Carrez]

Test on nightly 2001093008 / Win32 : OK for me.
Thanks for correcting this one !

Comment 26

[Thierry Carrez]

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

Comment 27

[yulian chang]

Verified with 20011001 branch builds on various platforms. 

How about the second 2) and 5)? Will be fixed in the future?

Comment 28

[Scott MacGregor]

Yeah you should file a bug on the ldap auto complete owners to implement
selection of the first match when multiple matches are present for those 2 cases.

Comment 29

[yulian chang]

bug 102693 and 102698 are filed.