Closed Bug 1410821 Opened 3 years ago Closed 3 years ago
[Form Autofill] Consider calling start
Search again instead of returning cached m Results when press arrow-down key after the focused input has just been auto-filled
Similarly happens in form history. STR: 1. go to 'data:text/html, <form><input name="test"><input type="submit"></form>' 2. enter "test" and submit 3. enter "test2" and submit 4. clear the input field 5. enter "test" and you'll see two suggestion items: "test" and "test2" 6. use down-key navigate to option "test2" and press enter 7. press down-key again Expected result: The popup only suggest "test2" as current search string has changed to "test2" Actual result: Still get two suggestion: "test" & "test2". In the meanwhile, dbclick get "test2" only. There should has no distinction between dbclick and press down-key. I would expect the mResults be invalidated at the same time while doing the autofilling, therefore, we'll be able to get new results for clear button after auto-filled.  http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/toolkit/components/autocomplete/nsAutoCompleteController.cpp#565-591
Hey Matt, Two approaches I can think of for this problem: 1. Expose a new API for nsAutoCompleteController, i.e. invalidateResults(). And call the function manually in http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/browser/extensions/formautofill/FormAutofillHandler.jsm#410 2. If this is also a bug for form history, maybe we can set mResults to null at the end of somewhere like nsAutoCompleteController::EnterMatch(). Not sure which way is more proper to fix the issue, could you give me some feedback? Thanks
Summary: [Form Autofill] Consider calling startSearch again instead of return mResults when press keydown once the focused input has just been auto-filled → [Form Autofill] Consider calling startSearch again instead of returning cached mResults when press arrow-down key after the focused input has just been auto-filled
Marco, is it a bug or feature that the results are cached as described in comment 0. Do you have opinions on comment 1?
Flags: needinfo?(MattN+bmo) → needinfo?(mak77)
(In reply to Matthew N. [:MattN] (PM if requests are blocking you) from comment #3) > Marco, is it a bug or feature that the results are cached as described in > comment 0. Do you have opinions on comment 1? I think it's a bug, the results should always be consistent with the input value. Regarding the approach, I think the bug should be fixed in the controller, not in form history. I'm not sure if it'd be correct to clear mResults on EnterMatch, because the pointed at code looks like an optimization that would get lost that way. My gut feeling is that we should check if the current mSearchString equals mResults->GetSearchString() before deciding we can just reopen the popup. Something like: if (!mResults.IsEmpty() && NS_SUCCEDED(mResults->GetSearchString(oldSearchString)) && oldSearchString.Equals(mSearchString, nsCaseInsensitiveStringComparator())) Though, I didn't verify it.
Thanks Marco! That's a great timing for the check and also makes sense to the optimization, I'll try it tomorrow. :D
I tested the patch on local and ran the it on TH: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5b1cfb464b5dcc7a49537818a1417c7780064630, the problem is fixed and I haven't observed anything broken so far. A test case for this might needed as well I guess, I'll work on it.
Hi Marco, Could you help me to review the patch? Sorry I didn't find a proper place for the mochitest in autocomplete component. But if you think it worth having a test, would you mind give me some suggestion where to put the test? Thanks.
(In reply to Ray Lin[:ralin] from comment #8) > Hi Marco, > > Could you help me to review the patch? Sorry I didn't find a proper place > for the mochitest in autocomplete component. But if you think it worth > having a test, would you mind give me some suggestion where to put the test? > Thanks. Depending on which kind of test you have in mind, you can add tests for autocomplete into toolkit/content/tests/ I assume for this case a chrome mochitest could be enough, you can already find some test_autocomplete_* tests there. It's not critical to have a test immediately, but we strongly suggest it, so maybe you could play with that for some hours and see if it's easily feasible, by taking inspiration to the other autocomplete tests.
You should also start a Try run to check your change doesn't break some other tests. I' suggest to run mochitests and browser-chrome tests.
Ray, please let me know if you're working on a test, otherwise it may be better to land the fix before we merge to 59, and file a follow-up for the test.
Ah, so sorry that no update anything here. I didn't have cycle working on it in these two days, but I've had a quick look to those test files. Would you mind wait me one more day to try on it? I'll let you know by tomorrow EOD.
Sorry again, Marco. I was thinking to write a test case accumulating startSearch count, and assert the count should increase by 1 when press VK_DOWN right after autocompleted. However, it turned out startSearch count always increase no matter this patch is applied or not. I had a hard time figuring out why and I don't want to block this fix, so let's file a follow-up as you suggested. Could you help me to review this patch? Thanks.
Comment on attachment 8925412 [details] Bug 1410821 - Call startSearch again when the new search string could not hit the cached result. https://reviewboard.mozilla.org/r/196536/#review203384
Attachment #8925412 - Flags: review?(mak77) → review+
Fair. Try looks ok, I'm landing.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/c58e3a20c6f1 Call startSearch again when the new search string could not hit the cached result. r=mak
Thank you Marco :D
You need to log in before you can comment on or make changes to this bug.