[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

RESOLVED FIXED in Firefox 58

Status

()

defect
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: ralin, Assigned: ralin)

Tracking

(Depends on 1 bug, Blocks 1 bug)

unspecified
mozilla58
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox58 fixed)

Details

(Whiteboard: [form autofill:V2], )

Attachments

(2 attachments)

Assignee

Description

2 years ago
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.


[0] http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/toolkit/components/autocomplete/nsAutoCompleteController.cpp#565-591
Assignee

Comment 1

2 years ago
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
Flags: needinfo?(MattN+bmo)
Assignee

Updated

2 years ago
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[0]->GetSearchString() before deciding we can just reopen the popup. Something like:
if (!mResults.IsEmpty() &&
    NS_SUCCEDED(mResults[0]->GetSearchString(oldSearchString)) &&
    oldSearchString.Equals(mSearchString,
                           nsCaseInsensitiveStringComparator()))
Though, I didn't verify it.
Flags: needinfo?(mak77)
Assignee

Comment 5

2 years ago
Thanks Marco! That's a great timing for the check and also makes sense to the optimization, I'll try it tomorrow. :D
Assignee

Comment 6

2 years ago
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.
Assignee

Updated

2 years ago
Assignee: nobody → ralin
Assignee

Comment 8

2 years ago
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.
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
(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.
Flags: needinfo?(ralin)
Assignee

Comment 12

2 years ago
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.
Flags: needinfo?(ralin)
Component: Form Manager → Form Autofill
Assignee

Comment 13

2 years ago
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.
Flags: needinfo?(mak77)
Assignee

Updated

2 years ago
Blocks: 1415771

Comment 14

2 years ago
mozreview-review
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.
Flags: needinfo?(mak77)

Comment 16

2 years ago
Pushed by mak77@bonardo.net:
https://hg.mozilla.org/integration/autoland/rev/c58e3a20c6f1
Call startSearch again when the new search string could not hit the cached result. r=mak
Assignee

Comment 17

2 years ago
Thank you Marco :D

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c58e3a20c6f1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Blocks: 1404768
No longer blocks: 1415771
Depends on: 1415771
You need to log in before you can comment on or make changes to this bug.