497541 - Autocomplete dropdown should update when text inserted before end

Status: VERIFIED FIXED

(Toolkit :: Autocomplete, defect)

Description

[Matthew N. [:MattN]]

Attachment: Screenshot showing actual result

If I type text into an text field and then make changes to the text, the list of form history entries in the dropdown is only updated if there were no matching entries for the original text.

Steps to reproduce:
1) Go to mozilla.org
2) In the search box, type "aaabbb" and submit the form.
3) Go back and repeat with "aabb" and submit.
4) Go back and clear the field.  Now type "aabb" into the field and you should see "aabb" in the dropdown.
5) Now insert an additional "a" into the field so that it has the value "aaabb"

Actual result:
The dropdown menu will still show "aabb" even though the text in the field is not a prefix of that.  

Expected result:
The dropdown menu should show "aaabbb".

Comment 1

[Matthew N. [:MattN]]

Attachment: v.1 (also fixes bug 496466)

Comment 2

[Matthew N. [:MattN]]

Attachment: v.2 added tests for caching previous results

Comment 3

[Matthew N. [:MattN]]

Attachment: v.3 fixed passwordmgr and added tests for it

Comment 4

[Shawn Wilsher :sdwilsh]

Could you add a test in toolkit/components/autocomplete/tests/unit/ for this too?

Comment 5

[:Gavin Sharp [email: [e-mail]]]

Comment on attachment 385261 [details] [diff] [review]
v.3 fixed passwordmgr and added tests for it

>diff --git a/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html b/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html

>+var autocompleteMenu;

nit: autocompletePopup?

> function startTest() {

>+    autocompleteMenu = chromeWin.document.getElementById("PopupAutoComplete");

Strictly speaking this test shouldn't reach into browser internals like this ("PopupAutoComplete" is Firefox's ID for the popup it uses, since this code is app-agnostic these tests should be too). I guess there isn't a way to get the popup out of the formfillcontroller, though, and in practice SeaMonkey appears to use the same ID, so this is probably OK. Worth adding a comment and keeping an eye out for SeaMonkey unit test bustage when this lands, though.

>diff --git a/toolkit/components/satchel/src/nsFormFillController.cpp b/toolkit/components/satchel/src/nsFormFillController.cpp

These changes get rid of all the HandleText(PR_FALSE) callers but one, and I don't think it needs to pass that (might be relevant to bug 446247, actually). Worth filing a followup on investigating the removal of that parameter and associated code?

Comment 6

[Matthew N. [:MattN]]

Attachment: v.4 address review comments (incl. HandleText changes)

Made the HandleText changes as the old code does not seem necessary.

Comment 7

[Dão Gottwald [:dao]]

http://hg.mozilla.org/mozilla-central/rev/47bfcd275ede

Comment 8

[Shawn Wilsher :sdwilsh]

no test in toolkit/components/autocomplete/tests/unit/?  It's kinda crappy to have to run tests in other modules to check if some module is not broken by a change...

Comment 10

[Tracy Walker [:tracy]]

verified with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2b1pre) Gecko/20090925 Namoroka/3.6b1pre

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I have a question about the test #700.

It sends string "form9userAB" but the test expects autocomplete popup won't show up.

I'm working on bug 698949 which will kill the untrusted key event support on editor. Therefore, I need to set uname before calling sendString(). If you test the unfocused editor case at #700, it should be dropped by my patch.

What is your purpose of #700?

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

> I need to set uname before calling sendString().

I meant "set focus to uname".

Comment 14

[Matthew N. [:MattN]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #12)
> What is your purpose of #700?

It should be fine to set the focus to uname before calling sendString().

Test 700 is to get the autocomplete results to show for username "form9userAB" (without selecting a result) so that we can test the fix of this bug (inserting text before the end) in test 701. It inserts the letter A before the letter B and makes sure that the autocomplete results are updated. Prior to the fix, the old autocomplete results would still show despite the update to the username field.

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Oh, so, is current test result wrong?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/test/test_basic_form_autocomplete.html?force=1#725

> 720     case 603:
> 721         checkACForm("", "");
> 722         pwmgr.removeLogin(login7);
> 723 
> 724         testNum = 699;
> 725         gNextTestWillOpenPopup = false;
> 726         break;
> 727 
> 728     case 700:
> 729         // Turn our attention to form9 to test the dropdown - bug 497541
> 730         uname = $_(9, "uname");
> 731         pword = $_(9, "pword");
> 732         sendString("form9userAB", uname);
> 733         gNextTestWillOpenPopup = true;
> 734         break;

Currently, the test expects autocomplete popup won't show up by test 700. Can I change gNextTestWillOpenPopup to true at the end of previous test (line 725)? (I need to insert |uname.focus();| between line 731 and 732.) Then, all tests passed.

Comment 16

[Matthew N. [:MattN]]

(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #15)
We discussed this on IRC.

The autocomplete popup is supposed to open if the user is typing in the username field so setting gNextTestWillOpenPopup = true; should be fine.

Comment 17

[Tracy Walker [:tracy]]

removing in-litmus flag, it no longer exists