Closed Bug 1304634 Opened 8 years ago Closed 7 years ago

Support populating autocomplete results from form autofill code

Categories

(Toolkit :: Form Manager, defect, P3)

Product:
Toolkit
Component:
Form Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Iteration:
53.4 - Jan 9
Tracking Flags:
Tracking Status
firefox53 --- fixed

People

(Reporter: MattN, Assigned: steveck)

References

Details

(Whiteboard: [form autofill:M1])

Attachments

(4 files, 1 obsolete file)

Bug 1304634 - Support populating autocomplete results from form autofill code
58 bytes, text/x-review-board-request
Details
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1304634 - Part 2: Introduce form autofill search factory and registration in FormAutofillcontent,
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1304634 - Part 3: Ensure the profile search is registed while search started,
59 bytes, text/x-review-board-request
MattN
: review+
Details 
Add support for populating autocomplete results from code in the Form Autofill code base.
Assignee: MattN+bmo → nobody
Status: ASSIGNED → NEW
I could take over this patch except for the C++ part because I'm not familiar with. If the markAsAutofillField API is ready and we don't need to have specific XPCShell test for the C++ changes.

BTW, maybe we can split the markAsAutofillField API to another bug and keep rest of codes in this one, and I think there items are still missing in the patch:
- Apply markAsAutofillField API at the proper timing.
- Apply storage API in startSearch method.

Please let me know if I missed any critical item for this patch, thanks!
Flags: needinfo?(MattN+bmo)
(In reply to Steve Chung [:steveck] from comment #2)
> I could take over this patch except for the C++ part because I'm not
> familiar with. If the markAsAutofillField API is ready and we don't need to
> have specific XPCShell test for the C++ changes.

I think the API is ready other than the TODO to check that the autofill service exists (so we can handle if our system add-on is disabled).

The effects of markAsAutofillField are only visible to autocomplete so I don't think we can unit test it.

> BTW, maybe we can split the markAsAutofillField API to another bug and keep
> rest of codes in this one,

The problem is that I think you need it in order to test the rest of the code so it will have to be done first regardless.

> and I think there items are still missing in the
> patch:
> - Apply markAsAutofillField API at the proper timing.

That is for bug 1300993 and we can just test in this bug by calling markAsAutofillField on our test field.

> - Apply storage API in startSearch method.

Right, that can be done in this bug if you want.

> Please let me know if I missed any critical item for this patch, thanks!
Flags: needinfo?(MattN+bmo)
Comment on attachment 8809341 [details]
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,

https://reviewboard.mozilla.org/r/91936/#review91890

::: toolkit/components/satchel/AutoCompletePopup.jsm:42
(Diff revision 1)
>    getLabelAt(index) {
>      // Unused by richlist autocomplete - see getCommentAt.
>      return "";
>    },
>  
>    getCommentAt(index) {

I removed the rest of the chagnes in https://reviewboard.mozilla.org/r/80364/diff/1#4 because this getCommentAt API  should be able to handle the autocomplete popup properly
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

https://reviewboard.mozilla.org/r/91938/#review91886

::: browser/extensions/formautofill/content/ProfileAutocomplete.jsm:73
(Diff revision 1)
>     * @param searchParam
>     * @param previousResult a previous result to use for faster searchinig
>     * @param listener the listener to notify when the search is complete
>     */
>    startSearch(searchString, searchParam, previousResult, listener) {
> +    // TODO: These mock data should be replaced by form autofill API

I think it should be the API implemented in bug 1300990, something like FormAutofillParent.getProfileStore().getAll()?
(In reply to Steve Chung [:steveck] from comment #7)
> Comment on attachment 8809342 [details]
> Bug 1304634 - Part 2: Apply form autofill API
> 
> https://reviewboard.mozilla.org/r/91938/#review91886
> 
> ::: browser/extensions/formautofill/content/ProfileAutocomplete.jsm:73
> (Diff revision 1)
> >     * @param searchParam
> >     * @param previousResult a previous result to use for faster searchinig
> >     * @param listener the listener to notify when the search is complete
> >     */
> >    startSearch(searchString, searchParam, previousResult, listener) {
> > +    // TODO: These mock data should be replaced by form autofill API
> 
> I think it should be the API implemented in bug 1300990, something like
> FormAutofillParent.getProfileStore().getAll()?

Hi Matt, Luke and I just discussed about the possible way to call the storage API, and we found a critical issue that might need to be addressed in this bug. For the search result, we'll need to know the current input's autocomplete's property for filtering out the proper results. We came out 2 ideas that:

- Is it possible to get the autocomplete field type from searchParam? It seems like we'll need to set the parameters in nsAutoCompleteController.cpp, but handling the profile autocomplete in nsAutoCompleteController seems not an ideal solution since it should only handle the general parameter instead of profile autofill specific params.

- Could we reference login manager's solution that we call LoginManager's AutoCompleteSearchAsync API directly instead of any further binding? Since we could get the focusedInputNode here, it's also easier to get all the input's information.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

https://reviewboard.mozilla.org/r/91938/#review94870

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:1
(Diff revision 2)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

This common.js is very similar to the one in satchel since it was designed to handle the menu popup test as well
Assignee: nobody → schung
Status: NEW → ASSIGNED
Comment on attachment 8809341 [details]
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,

https://reviewboard.mozilla.org/r/91936/#review95462

::: browser/extensions/formautofill/content/ProfileAutocomplete.jsm:72
(Diff revision 2)
> +   * @param {string} searchString the string to search for
> +   * @param searchParam
> +   * @param previousResult a previous result to use for faster searchinig
> +   * @param listener the listener to notify when the search is complete
> +   */
> +  startSearch(searchString, searchParam, previousResult, listener) {

Hi Matt, we had some discussion in Taipei for addressing the input indentification with searchParams, and came out a thought to leverage the MarkAsAutofillField API. If we can call the API with additional autocomplete type parameter, and store the type along with input node in nsFormFillController.cpp, would it be accessible to pass the autocomplete type as the searchParam?
Code to test marking a field:

> Cc["@mozilla.org/satchel/form-fill-controller;1"].getService(Ci.nsIFormFillController).markAsAutofillField(input)
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

https://reviewboard.mozilla.org/r/91938/#review95896

::: browser/extensions/formautofill/test/mochitest/parent_utils.js:1
(Diff revision 4)
> +const { classes: Cc, interfaces: Ci, utils: Cu } = Components;

Why aren't we just using the version from satchel? I suspect we would want to keep our test helpers somewhat independent since we're a system add-on but I would wait for that. If we are copying this, I would do an `hg copy` to make it easier to see what changed and preserve blame.

I have the same question about satchel_common. We can augment with a custom file too but why fork the stuff that's identical? at least why now?
(In reply to Steve Chung [:steveck] from comment #8)
> (In reply to Steve Chung [:steveck] from comment #7)
> > Comment on attachment 8809342 [details]
> > Bug 1304634 - Part 2: Apply form autofill API
> > 
> > https://reviewboard.mozilla.org/r/91938/#review91886
> > 
> 
> Hi Matt, Luke and I just discussed about the possible way to call the
> storage API, and we found a critical issue that might need to be addressed
> in this bug. For the search result, we'll need to know the current input's
> autocomplete's property for filtering out the proper results.

Very true.

> We came out 2 ideas that:
> 
> - Is it possible to get the autocomplete field type from searchParam?

No, searchParam is for a different purpose

> It seems like we'll need to set the parameters in nsAutoCompleteController.cpp,
> but handling the profile autocomplete in nsAutoCompleteController seems not
> an ideal solution since it should only handle the general parameter instead
> of profile autofill specific params.

Yeah, I agree it's not ideal

> - Could we reference login manager's solution that we call LoginManager's
> AutoCompleteSearchAsync API directly instead of any further binding? Since
> we could get the focusedInputNode here, it's also easier to get all the
> input's information.

Perhaps in the short term but I was hoping to fix that hack however we solve the problem for autofill. See https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/toolkit/components/passwordmgr/nsLoginManager.js#473 for how this is documented as gross.
Flags: needinfo?(MattN+bmo)
(In reply to Matthew N. [:MattN] (PM me if requests are blocking you) from comment #17)
> (In reply to Steve Chung [:steveck] from comment #8)
> > (In reply to Steve Chung [:steveck] from comment #7)
> > We came out 2 ideas that:
> > 
> > - Is it possible to get the autocomplete field type from searchParam?
> 
> No, searchParam is for a different purpose
> 
> > It seems like we'll need to set the parameters in nsAutoCompleteController.cpp,
> > but handling the profile autocomplete in nsAutoCompleteController seems not
> > an ideal solution since it should only handle the general parameter instead
> > of profile autofill specific params.
> 
> Yeah, I agree it's not ideal

Example: https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/toolkit/components/search/nsSearchSuggestions.js#132-139
Here is my current thinking but I still have some other ideas I haven't fully investigated and I haven't thought through all the pros/cons yet:

Context: We need the ability for our form autofill extension to register to handle autocomplete results for relevant fields using attributes on the <input> while trying not to hard-code a dependency on such a service like we currently have with password manager[1]. See also bug 239380 / bug 380963. In my patch (attachment 8793640 [details]) on this bug I have an approach using a custom nsIAutoCompleteSearch which is a somewhat looser coupling than password manager's approach.

The problem with an nsIAutoCompleteSearch implementation is that it doesn't currently provide enough information about the node/input that the search is being triggered from. Our autocomplete results need to take input attributes (e.g. minlength, maxlength, autocomplete, etc.) and other fields in the form e.g. input.form into account but there currently isn't a clean way to get a reference to the mFocusedInput[2].

Here are some possible solutions:
a) Handle this the same way we do with password manager and call some special method which takes the node instead of the regular StartSearch.
b) Like (a) but define a new interface to use for the three components to get called e.g. nsIFormAutoCompleteSearch
c) Hack something into the searchParam like comment 18.
d) Add an [optional] node/element argument to the end of StartSearch. I'm not sure if making it "[optional]" helps with updating all implementors or add-on compat though? That would probably also be useful to replace the hack in comment 18.
e) New interface extending nsIAutoCompleteSearch which has a method like StartSearch but that also gets the element/node as an argument.
f) Expose `mFocusedInput`[2] via nsIFormFillController.
g) Expose the element/node on nsIAutoCompleteInput which is available from nsIAutoCompletePopup and nsIAutoCompleteController already. I'm not sure if this idea makes sense yet.

[1] https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/toolkit/components/passwordmgr/nsLoginManager.js#473
[2] https://dxr.mozilla.org/mozilla-central/rev/05328d3102efd4d5fc0696489734d7771d24459f/toolkit/components/satchel/nsFormFillController.h#94
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

Hi MattN, thanks for all the suggestions! Luke and I will discuss about the solutions locally before Hawaii. Hope that we can have a conclusion over there.

I updated the test per your comments that assertion should wait for onpopupshown event, but I realized that the main problem is ProfileAutocomplete is not loaded correctly. Resource path is valid but the log shows error[1] while running the bootstrapt. Did you ever face this issue after applying the patch?

[1] Error loading bootstrap.js for formautofill@mozilla.org: [Exception... "Component returned failure code: 0x80040111 (NS_ERROR_NOT_AVAILABLE)
Attachment #8809342 - Flags: feedback?(MattN+bmo)
(In reply to Steve Chung [:steveck] from comment #21)
> Comment on attachment 8809342 [details]
> Bug 1304634 - Part 2: Apply form autofill API and add mochitest
> 
> I updated the test per your comments that assertion should wait for
> onpopupshown event, but I realized that the main problem is
> ProfileAutocomplete is not loaded correctly. Resource path is valid but the
> log shows error[1] while running the bootstrapt. Did you ever face this
> issue after applying the patch?

BTW the error only happened while running the mochitest.
And Bug 1319995 remind me that maybe we also need to set the substituting resources handler of formautofill for mochitest. Will give it a try.
Comment on attachment 8809341 [details]
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,

https://reviewboard.mozilla.org/r/91936/#review96700

::: browser/extensions/formautofill/bootstrap.js:11
(Diff revision 2)
>  
>  /* exported startup, shutdown, install, uninstall */
>  
> -function startup() {}
> -function shutdown() {}
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> +
> +Cu.import("resource://formautofill/ProfileAutocomplete.jsm");

I think the resource URI path is only registered once the extension is installed which only happens from a call to `startup` so this should move inside there (but perhaps can still assign to a global?)

::: browser/extensions/formautofill/bootstrap.js:14
(Diff revision 2)
> -function startup() {}
> -function shutdown() {}
> +const {classes: Cc, interfaces: Ci, results: Cr, utils: Cu} = Components;
> +
> +Cu.import("resource://formautofill/ProfileAutocomplete.jsm");
> +
> +function startup() {
> +  ProfileAutocomplete.ensureRegistered();

This (and the import) should be behind a pref check for the feature so we don't do any extra work when the pref is off.

Satchel stuff is already under browser.formfill but I don't see any overlap with those prefs so how about `browser.formautofill.enabled`?
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

https://reviewboard.mozilla.org/r/91938/#review95896

> Why aren't we just using the version from satchel? I suspect we would want to keep our test helpers somewhat independent since we're a system add-on but I would wait for that. If we are copying this, I would do an `hg copy` to make it easier to see what changed and preserve blame.
> 
> I have the same question about satchel_common. We can augment with a custom file too but why fork the stuff that's identical? at least why now?

I think making these helpers independent is reasonable since they might have some differences from satchel one, so I applied hg copy here to keep the blame.
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

After some experiment with Luke, we found that moving PorfileAutocomplete.jsm to content might address the problem on e10s, and it would be easier to reference input in startSearch. So this patch might need to wait for bug 1300993 since it will implement the formAutofill content script, and we don't need to call the markas API during the mochitest after bug 1300993 landed.
Attachment #8809342 - Flags: feedback?(MattN+bmo)
Some results during the discussion:

- There should be some checks need to be done before we actually mark inputs: 1) Check if the from(or form like) element is the proper target for profile autofill. 2) Check if the storage is empty or not. If the profile exists. The mark API and heuristic matching will be called only when both checks passed.

- The existing events might not be sufficient for proper timing to trigger mark API. The DomContentLoaded could not fulfil if the input is created dynamically, and the input focus event might to too later since there's lots of tasks need to be done. So we may need to invent a proper event for profile autofill case here.
Comment on attachment 8819836 [details]
Bug 1304634 - Part 2: Introduce form autofill search factory and registration in FormAutofillcontent,

https://reviewboard.mozilla.org/r/99500/#review99786

::: browser/extensions/formautofill/test/mochitest/formautofill_common.js:255
(Diff revision 1)
>  
>  function formAutoFillCommonSetup() {
>    var chromeURL = SimpleTest.getTestFileURL("parent_utils.js");
>    gChromeScript = SpecialPowers.loadChromeScript(chromeURL);
> +
> +  SpecialPowers.setBoolPref("browser.formautofill.enabled", true);

Actually it didn't work here :/  The flag is enabled while running the mochitest(in the config), but the bootstrap startup will still treat it as disabled.
Blocks: 1324631
I created another follow up bug 1324631 for showing the multiple column popup, since multi-column popup would be more complicate after richlist popup landed.
Comment on attachment 8819836 [details]
Bug 1304634 - Part 2: Introduce form autofill search factory and registration in FormAutofillcontent,

https://reviewboard.mozilla.org/r/99500/#review99786

> Actually it didn't work here :/  The flag is enabled while running the mochitest(in the config), but the bootstrap startup will still treat it as disabled.

We could set the pref to true for all mochitest profiles: https://dxr.mozilla.org/mozilla-central/source/testing/profiles/prefs_general.js
Target Milestone: --- → mozilla54
Iteration: --- → 53.4 - Jan 9
Target Milestone: mozilla54 → ---
Comment on attachment 8809342 [details]
Bug 1304634 - Part 4: Apply form autofill API and add mochitest,

https://reviewboard.mozilla.org/r/91938/#review91886

> I think it should be the API implemented in bug 1300990, something like FormAutofillParent.getProfileStore().getAll()?

Yeah, but I would like to address this in bug 1300992 and make this patch simpler
Comment on attachment 8809341 [details]
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,

https://reviewboard.mozilla.org/r/91936/#review100844

r=me with the `IsSingleLineTextControl` change reverted.

::: toolkit/components/satchel/nsFormFillController.cpp:954
(Diff revision 4)
>    nsCOMPtr<nsINode> inputNode = do_QueryInterface(aInput);
>    if (!inputNode)
>      return;
>  
>    nsCOMPtr<nsIFormControl> formControl = do_QueryInterface(aInput);
> -  if (!formControl || !formControl->IsSingleLineTextControl(false))
> +  if (!formControl || !formControl->IsSingleLineTextControl(true))

Why are you changing this? I think this is just a rebasing issue since we changed this recently for the insecure password warning.
Attachment #8809341 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8809341 [details]
Bug 1304634 - Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller,

https://reviewboard.mozilla.org/r/91936/#review100848

::: browser/extensions/formautofill/content/ProfileAutocomplete.jsm:1
(Diff revision 4)
> +/* This Source Code Form is subject to the terms of the Mozilla Public

Remove this from part 1 and just land this code in part 2 since part 1 shouldn't cause problems without the factory
Comment on attachment 8819836 [details]
Bug 1304634 - Part 2: Introduce form autofill search factory and registration in FormAutofillcontent,

https://reviewboard.mozilla.org/r/99500/#review100850

::: browser/extensions/formautofill/content/FormAutofillContent.js:135
(Diff revision 3)
> +
> +// Register/unregister a constructor as a factory.
> +function AutocompleteFactory() {}

As I said in part 1 just now: don't have the ProfileAutocomplete.jsm, just land this code in part 2.
Attachment #8819836 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8820612 [details]
Bug 1304634 - Part 3: Ensure the profile search is registed while search started,

https://reviewboard.mozilla.org/r/100102/#review100852

::: toolkit/components/satchel/nsFormFillController.cpp:528
(Diff revision 1)
>  }
>  
>  NS_IMETHODIMP
>  nsFormFillController::GetSearchAt(uint32_t index, nsACString & _retval)
>  {
> -  if (mAutofillInputs.Get(mFocusedInputNode)) {
> +  nsCOMPtr<nsIAutoCompleteSearch> profileSearch =

I would also be fine with you removing the GetSearchAt hunk from part 1 and only having this in Part 4. That way we don't land the intermediate TODO.

::: toolkit/components/satchel/nsFormFillController.cpp:532
(Diff revision 1)
>  {
> -  if (mAutofillInputs.Get(mFocusedInputNode)) {
> -    // TODO: check for autofill component
> +  nsCOMPtr<nsIAutoCompleteSearch> profileSearch =
> +    do_GetService("@mozilla.org/autocomplete/search;1?name=autofill-profiles");
> +
> +  // Make sure the form autofill search is registered.
> +  if (mAutofillInputs.Get(mFocusedInputNode) && profileSearch) {

Since this only runs as a result of SetInput I guess it's find to do the do_GetService every time but to be safe we could rewrite this to do an early return after `_retval.AssignLiteral("autofill-profiles");` and fallback to "form-history" if the service doesn't exist.
```
if (mAutofillInputs.Get(mFocusedInputNode)) {
  nsCOMPtr<nsIAutoCompleteSearch> profileSearch = do_GetService("@mozilla.org/autocomplete/search;1?name=autofill-profiles");
  if (profileSearch) {
    _retval.AssignLiteral("autofill-profiles");
    return NS_OK;
  }
}

_retval.AssignLiteral("form-history");
return NS_OK;
```
Attachment #8820612 - Flags: review?(MattN+bmo) → review+
Attachment #8809342 - Attachment is obsolete: true
Attachment #8809342 - Flags: review?(MattN+bmo)
Blocks: 1325537
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/d8168eadbf34
Part 1: Introduce MarkAsAutofillField API and necessary logic changes in controller, r=MattN
https://hg.mozilla.org/integration/autoland/rev/fbf7e620479d
Part 2: Introduce form autofill search factory and registration in FormAutofillcontent, r=MattN
https://hg.mozilla.org/integration/autoland/rev/56678d58795c
Part 3: Ensure the profile search is registed while search started, r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/d8168eadbf34
https://hg.mozilla.org/mozilla-central/rev/fbf7e620479d
https://hg.mozilla.org/mozilla-central/rev/56678d58795c
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Steve, would this fix benefit from manual testing? If yes, then what should manual QA focus on?
Flags: qe-verify?
Flags: needinfo?(schung)
Maybe not for now, I think Vance will reply your concern immediately, thanks.
Flags: needinfo?(schung) → needinfo?(vchen)
Andrei, thanks for reaching out! In fact this bug is only one of the bugs for Form Autofill implementation and right now it is still not manually testable. We will reach out to you once the feature is ready for manual test!
Flags: needinfo?(vchen)
(In reply to Vance Chen [:vchen][skype:vance.lucida][vchen@mozilla.com] from comment #60)
> Andrei, thanks for reaching out! In fact this bug is only one of the bugs
> for Form Autofill implementation and right now it is still not manually
> testable. We will reach out to you once the feature is ready for manual test!

Thank you for following up, Vance! Removing the qe-verify flag for now.
Flags: qe-verify?
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: