Closed Bug 911090 Opened 7 years ago Closed 7 years ago

Previous form autocomplete result is sometimes used when it shouldn't because its searchString is incorrect

Categories

(Toolkit :: Form Manager, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: MattN, Assigned: neil)

References

Details

Attachments

(2 files)

While reviewing bug 905190 I found the following issue by inspection (although this may be what that bug was also about):

STR:
1) Submit "aab" and "aacc" in the same field so there are now 2 entries in form history for the input name/id.
2) Copy the text "cc" to the clipboard
3) Load the form again and type "aab". "aab" result from #1 should show in dropdown.
4) Highlight "b" in the text "aab" from #3.
5) Paste "cc" from the clipboard so the resulting text is "aacc".

Expected result:
"aacc" shows in the dropdown as it was submitted in #1

Actual result:
No form history results shown

Reproducible: always

The cause is that the searchString property of an nsIAutoCompleteResult is not getting updated when we reuse it and filter it's entries. In step 5, the previousResult passed to _autoCompleteSearchShared has the searchString set to "aa" (from it's original creation) even though it has since been filtered to only include "aab" matches in step 3. The searchString property on the result should have been modified to be "aab" after step 3. 

The code in _autoCompleteSearchShared attempts to do this with |result.wrappedJSObject.searchString = aUntrimmedSearchString;| but the previousResult being passed in is implemented in nsFormAutoCompleteResult.jsm now rather than the nsIAutoCompleteResult implementation at the bottom of nsFormAutoComplete.js. Setting result.wrappedJSObject.searchString has no effect. 

Setting result.wrappedJSObject._searchString works but that wouldn't work with the nsFormAutoComplete.js implementation and I don't know whether those can be passed as the previousResult.
Duplicate of this bug: 905190
I created attachment 790460 [details] [diff] [review] to the above bug to address the issue by making the replacement results expose a compatible API to the original results.
Attached patch Possible patchSplinter Review
This is the "quick fix", it allows the form history component to set the search string without having to worry about which set of results it's been given.

A nontrivial fix might be to rewrite inputlist-autocomplete to be a separate autocomplete component. If I ever get around to it I'll file a separate bug.
Attachment #799774 - Flags: review?(mnoorenberghe+bmo)
Blocks: 58986
Comment on attachment 799774 [details] [diff] [review]
Possible patch

Changing reviewer because apparently MattN is away.
Attachment #799774 - Flags: review?(mnoorenberghe+bmo) → review?(dolske)
Comment on attachment 799774 [details] [diff] [review]
Possible patch

I'd rather Matt look at this, bouncing back since this isn't an urgent bugfix.
Attachment #799774 - Flags: review?(dolske) → review?(mnoorenberghe+bmo)
Comment on attachment 799774 [details] [diff] [review]
Possible patch

Review of attachment 799774 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a testcase. I'll try to make one this weekend.
Attachment #799774 - Flags: review?(mnoorenberghe+bmo) → review+
This code snippet passes/fails as appropriate, it just needs to be wrapped up.

var search = Cc['@mozilla.org/autocomplete/search;1?name=form-history'].
             getService(Components.interfaces.nsIAutoCompleteSearch);
search.startSearch("aa", "", null, {
  onSearchResult: function(search, result) {
    is(result.searchString, "aa", "Search string with no previous result");
    search.startSearch("aaa", "", result, {
      onSearchResult: function(search, result) {
        is(result.searchString, "aaa", "Search string with previous result");
      }
    });
   }
});
Attached patch XPCshell testSplinter Review
Attachment #815361 - Flags: review?(mnoorenberghe+bmo)
Comment on attachment 815361 [details] [diff] [review]
XPCshell test

Review of attachment 815361 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks!

::: toolkit/components/satchel/test/unit/test_previous_result.js
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +var aaaListener = {

Use "let" instead of "var" for new code please.
Attachment #815361 - Flags: review?(mnoorenberghe+bmo) → review+
Comment on attachment 815361 [details] [diff] [review]
XPCshell test

Review of attachment 815361 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/satchel/test/unit/test_previous_result.js
@@ +1,3 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

Also, tests can be put in the public domain: See http://www.mozilla.org/MPL/headers/
Assignee: mnoorenberghe+bmo → neil
https://hg.mozilla.org/mozilla-central/rev/9ec685d9f769
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.