Closed Bug 325842 Opened 14 years ago Closed 11 years ago

Make setting the autocomplete attribute 'completeDefaultIndex' do something sane when the search string does not match the beginning of the result string

Categories

(Toolkit :: Autocomplete, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.1b1

People

(Reporter: ajschult784, Assigned: standard8)

References

()

Details

Attachments

(2 files, 2 obsolete files)

Toolkit autocomplete should implement xpfe's autoFillAfterMatch.  TBird and mail in SM both use this to display the best match inline when the result string doesn't start with the search string (which is common when autocompleting last names and such).
Attached patch implement autoFillAfterMatch (obsolete) — Splinter Review
Attachment #210817 - Flags: first-review?(mconnor)
Blocks: 360648
No longer blocks: 360648
Attachment #210817 - Flags: first-review?(mconnor) → review?(mconnor)
I guess this patch has bitrotted :/
Blocks: 360648
I initially was going to implement autoFillAfterMatch for toolkit autocomplete so that there is feature parity for Thunderbird to switch over to it.

However having looked at the current implementation of toolkit autocomplete with respect to the auto fill / complete default index, the implementation of cases where the search string doesn't match the result string (at all) doesn't work correctly.

Thunderbird needs this search string != result string case for cases during its autocomplete lookup where it matches against first/last/nick names but only displays the Display Name and Email address.

In the current toolkit implementation, if the string doesn't match the beginning of the result string then one of two things happens:

1) If the search string is "zilla" and the result string is "bugzilla@host" then "zilla@host" will be displayed.
2) If the search string is "z1" and the result string is "bugzilla", then the cursor will be put at the start of the search string, and autocomplete pretty much aborted.

This is a different case to where a url is middle-completed.

I'm proposing with this patch that we just replace the broken cases by the way xpfe autocomplete does it:

If the search string doesn't match the beginning of the result string then make the autocomplete contents:

searchstring >> resultstring

and highlight from the end of the searchstring to the end of the string.

This will indicate to the user that their search string will be replaced by the result string.

Note: this functionality is OFF by default in Firefox. Therefore I won't be affecting default functionality.

Hi-jacking this bug, as if we implement it this way then this bug would become obsolete anyway.

No idea if I need ui-review for this, but I would really like to get feedback/reviews before Tuesday (feature-freeze) as if I need to implement an extra attribute then I would assume that would count as a feature.
Assignee: nobody → bugzilla
Attachment #210817 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #333549 - Flags: ui-review?
Attachment #333549 - Flags: review?(enndeakin)
Attachment #210817 - Flags: review?(mconnor)
Comment on attachment 333549 [details] [diff] [review]
Improve implementation when search string doesn't match start of result string.

Correct email address, please see comment 3.
Attachment #333549 - Flags: ui-review? → ui-review?(beltzner)
Comment on attachment 333549 [details] [diff] [review]
Improve implementation when search string doesn't match start of result string.

>diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
>--- a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
>+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
>@@ -1399,8 +1399,6 @@ nsAutoCompleteController::CompleteValue(

CompleteValue is also called from within HandleDelete without checking the completedefaultvalue property from what I can see. Is this a current bug?

>+  else {
>+  autocomplete.removeAttribute("onsearchcomplete");
>+    setTimeout(function() {
>+      // Unregister the factory so that we don't get in the way of other tests
>+ //     componentManager.unregisterFactory(autoCompleteSimpleID, autoCompleteSimple);
>+      SimpleTest.finish();
>+    }, 0);

Why is this line here commented out? Also, why are you removing the attribute and using a timeout?
(In reply to comment #5)
> (From update of attachment 333549 [details] [diff] [review])
> >diff --git a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
> >--- a/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
> >+++ b/toolkit/components/autocomplete/src/nsAutoCompleteController.cpp
> >@@ -1399,8 +1399,6 @@ nsAutoCompleteController::CompleteValue(
> 
> CompleteValue is also called from within HandleDelete without checking the
> completedefaultvalue property from what I can see. Is this a current bug?

I think that's a current bug. Although it doesn't seem to have any adverse effect - both xpfe & toolkit (with this patch) implementations when you press delete don't autofill until you enter a new character.

> >+  else {
> >+  autocomplete.removeAttribute("onsearchcomplete");
> >+    setTimeout(function() {
> >+      // Unregister the factory so that we don't get in the way of other tests
> >+ //     componentManager.unregisterFactory(autoCompleteSimpleID, autoCompleteSimple);
> >+      SimpleTest.finish();
> >+    }, 0);
> 
> Why is this line here commented out? Also, why are you removing the attribute
> and using a timeout?

Sorry, that was so I could do some manual testing in the test harness. I'll revert that to its proper state and attach a new patch.
updated patch.
Attachment #333549 - Attachment is obsolete: true
Attachment #333563 - Flags: ui-review?(beltzner)
Attachment #333563 - Flags: review?(enndeakin)
Attachment #333549 - Flags: ui-review?(beltzner)
Attachment #333549 - Flags: review?(enndeakin)
Attachment #333563 - Flags: review?(enndeakin) → review+
Attachment #333563 - Flags: ui-review?(beltzner) → ui-review?(mconnor)
Comment on attachment 333563 [details] [diff] [review]
[checked in] Improve implementation when search string doesn't match start of result string v2

beltzner still hasn't got back to me on this. So trying mconnor in the hope that this could actually be checked in before Thunderbird's beta.

Mike (Connor), I'm not totally sure that ui-review is required, however please see comment 3 for a discussion of what this patch is doing and fixing.
Attachment #333563 - Flags: ui-review?(mconnor) → ui-review+
Summary: implement autoFillAfterMatch → Make setting the autocomplete attribute 'completeDefaultIndex' do something sane when the search string does not match the beginning of the result string
I checked this in and backed it out, unfortunately it failed in tests, probably due to the bug mentioned in comment 5.

Failures as follows:

*** 65075 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Starting test #50
*** 65076 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Correct number of logins before deleting one
*** 65077 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 username - got "zzzuser4 >> testuser2", expected "zzzuser4"
*** 65078 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 password
*** 65079 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Correct number of logins after deleting one
*** 65080 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 username
*** 65081 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 password
*** 65082 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Starting test #51
*** 65083 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 username
*** 65084 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 password
*** 65085 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Starting test #52
*** 65086 ERROR TEST-UNEXPECTED-FAIL | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 username - got "testuser2 >> zzzuser4", expected "testuser2"
*** 65087 INFO TEST-PASS | /tests/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html | Checking form1 password
Blocks: 408133
Blocks: 333722
This is a supplemental patch to the first one.

As it turns out CompleteValue in HandleDelete should have been protected by a check for completeDefaultIndex.

This was highlighted in the test_basic_form_autocomplete.html tests. There was an XXX why does a value get set here question, and when debugging it, it was then obvious that not only did a value get put in the field when you deleted one, but it was also typically the wrong value.

Now that HandleDelete will only call CompleteValue if completeDefaultIndex is on, I've changed the tests in test_basic_form_autocomplete.html to look for the proper expected result - blank.

Lastly, the PR_FALSE turning off highlighting of the rest of the string was wrong, because if we're completing a new value, we still want the rest of the string to be highlighted so we can make it easy to cancel that completion. Therefore changing it to PR_TRUE means all callers are true, and we can drop that parameter.
Attachment #339443 - Flags: review?(enndeakin)
Attachment #339443 - Flags: review?(enndeakin) → review+
Comment on attachment 333563 [details] [diff] [review]
[checked in] Improve implementation when search string doesn't match start of result string v2

Both patches checked in under changeset 19422:afe1069742c2
Attachment #333563 - Attachment description: Improve implementation when search string doesn't match start of result string v2 → [checked in] Improve implementation when search string doesn't match start of result string v2
Attachment #339443 - Attachment description: Fix mochitest problems. → [checked in] Fix mochitest problems.
(In reply to comment #11)
> (From update of attachment 333563 [details] [diff] [review])
> Both patches checked in under changeset 19422:afe1069742c2

I also checked in changeset id 19440:142686d55743 which included changes for xpfe autocomplete to implement completeDefaultIndex there (which fixed mochitest bustage).

http://hg.mozilla.org/mozilla-central/rev/142686d55743

I raised bug 456256 to follow up on the xpfe autocomplete issues.
Blocks: 456256
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1b1
Version: unspecified → Trunk
Flags: in-testsuite+
What happens for RTL languages with the ">>"?
(In reply to comment #13)
> What happens for RTL languages with the ">>"?

It probably doesn't work right thinking about it - the existing xpfe code also doesn't manage RTL languages. Please file a separate bug on that issue.
Blocks: 494809
Blocks: 639293
Pushed by frgrahl@gmx.net:
https://hg.mozilla.org/comm-central/rev/b65f73fe8eab
Fix SeaMonkey bustage from bug 325842. looks reasonable from Neil over irc.
You need to log in before you can comment on or make changes to this bug.