1338482 - Fallback to form history if the target field doesn't have data in selected profile

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect, P1)

Description

[Steve Chung [:steveck]]

A form history list should be prompted if the form is filled with profile but the target input does not have data for the profile.

We'll need to implement an API like unmarkAsAutofillField to fallback for specific input(and bug 1338420 will need this as well), and the formAutofillContent should also have some more information after form filled, such as selected profile for form.

Comment 1

[Steve Chung [:steveck]]

Based on the discussion with UX, we also need to fallback the field for history search if it's auto filled first and user edit the value later.

Comment 2

[Steve Chung [:steveck]]

Attachment: Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

Review commit: https://reviewboard.mozilla.org/r/115200/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115200/

Comment 3

[Steve Chung [:steveck]]

Attachment: Bug 1338482 - Add filled guid for form handler and check at startSearch.

Review commit: https://reviewboard.mozilla.org/r/115238/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115238/

Comment 4

[Steve Chung [:steveck]]

Attachment: Bug 1338482 - Part 3: Add initial state for field details.

Review commit: https://reviewboard.mozilla.org/r/115656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/115656/

Comment 5

[Matthew N. [:MattN]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

https://reviewboard.mozilla.org/r/115200/#review117618

::: toolkit/components/satchel/nsFormFillController.cpp:321
(Diff revision 1)
> +  /*
> +   * Unset the field which is marked as AutofillField previously.
> +   */

This comment probably isn't needed as the method name is clear enough.

::: toolkit/components/satchel/nsFormFillController.cpp:324
(Diff revision 1)
> +nsFormFillController::UnmarkAutofillField(nsIDOMHTMLInputElement *aInput)
> +{
> +  /*
> +   * Unset the field which is marked as AutofillField previously.
> +   */
> +  nsCOMPtr<nsINode> node = do_QueryInterface(aInput);

Nit: Might as well add `NS_ENSURE_STATE(node);` here too

::: toolkit/components/satchel/nsFormFillController.cpp:325
(Diff revision 1)
> +  mAutofillInputs.Remove(node);
> +  node->RemoveMutationObserver(this);

This is fine with me assuming both of these are fine to call even if there's nothing to remove (e.g. more than once in a row).

::: toolkit/components/satchel/nsIFormFillController.idl:61
(Diff revision 1)
>     * @param aInput - The HTML <input> element to mark
>     */
>    void markAsAutofillField(in nsIDOMHTMLInputElement aInput);
>  
>    /*
> +   * Unmark the specified <input> element as being managed by a form history component.

s/history/autofill/

::: toolkit/components/satchel/nsIFormFillController.idl:62
(Diff revision 1)
>     */
>    void markAsAutofillField(in nsIDOMHTMLInputElement aInput);
>  
>    /*
> +   * Unmark the specified <input> element as being managed by a form history component.
> +   * Autocomplete requests will be handed off to the history autofill component.

…or pwmgr, right? Maybe don't specify whether it's form history that gets used.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

https://reviewboard.mozilla.org/r/115200/#review117624

It would be nice to have a trivial in the satchel directory to call this method on an unkarked input and ensure it doesn't crash or throw.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

https://reviewboard.mozilla.org/r/115200/#review117624

*trivial test

Comment 8

[Matthew N. [:MattN]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

https://reviewboard.mozilla.org/r/115238/#review117630

We'll need to test this eventually.

::: browser/extensions/formautofill/FormAutofillContent.jsm:90
(Diff revision 1)
>    startSearch(searchString, searchParam, previousResult, listener) {
>      this.log.debug("startSearch: for", searchString, "with input", formFillController.focusedInput);

Do we also need to change our AC search code to handle when the form was already filled? I think we don't want to suggest other profiles that use the same value in the focused field but I'm not sure we're already doing that correctly.

::: browser/extensions/formautofill/FormAutofillContent.jsm:319
(Diff revision 1)
>    getAllFieldNames(element) {
>      let formDetails = this.getFormDetails(element);
>      return formDetails.map(record => record.fieldName);
>    },
>  
> +  setFormProfileGuid(element, guid) {

How about `markFormAsFilled(field, guid)`?

Btw. `field` is clearer than `element` as it tells me that I need to pass a form field instead of any element.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:58
(Diff revision 1)
>    fieldDetails: null,
>  
>    /**
> +   * String of the filled profile's guid.
> +   */
> +  profileGuid: null,

How about `filledProfileGUID` to clarify that it's about which profile was filled in. The clarity is worth the extra six characters IMO.

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 8841427 [details]
Bug 1338482 - Part 3: Add initial state for field details.

https://reviewboard.mozilla.org/r/115656/#review117632

::: browser/extensions/formautofill/FormAutofillHandler.jsm:69
(Diff revision 1)
>      let autofillData = [];
>  

I guess you forgot to delete this? It seems like we should be clearing fieldDetails at the beginning though so maybe it should be `this.fieldDetails = [];`?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:94
(Diff revision 1)
>          section: info.section,
>          addressType: info.addressType,
>          contactType: info.contactType,
>          fieldName: info.fieldName,
> +        state: null,
> +        element,

Is the bug filed to switch this to a WeakRef yet using `Cu.getWeakRef…(…)`? You can do it in a separate commit of this bug if you'd like or a separate bug.

Comment 10

[Steve Chung [:steveck]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115238/diff/1-2/

Comment 11

[Steve Chung [:steveck]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115200/diff/1-2/

Comment 12

[Steve Chung [:steveck]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

https://reviewboard.mozilla.org/r/115200/#review119964

Comment 13

[Steve Chung [:steveck]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

https://reviewboard.mozilla.org/r/115238/#review117630

> Do we also need to change our AC search code to handle when the form was already filled? I think we don't want to suggest other profiles that use the same value in the focused field but I'm not sure we're already doing that correctly.

I've comfirmed with UX that we will switch the search to history no matter it's auto filled or no profile to be filled.

Comment 14

[Steve Chung [:steveck]]

Comment on attachment 8840773 [details]
Bug 1338482 - Part 2: Add state while filling the form and check state at startSearch.

Hi Matt and Luke,

Since the implementation is very different from the last patch because we'll apply the solution in bug 1338420, I simply check the new state at startSearch in the new patch.

Comment 15

[Steve Chung [:steveck]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115238/diff/2-3/

Comment 16

[Steve Chung [:steveck]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115238/diff/3-4/

Comment 17

[Matthew N. [:MattN]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

https://reviewboard.mozilla.org/r/115238/#review122362

Comment 18

[Steve Chung [:steveck]]

Comment on attachment 8840803 [details]
Bug 1338482 - Add filled guid for form handler and check at startSearch.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/115238/diff/4-5/

Comment 19

[Steve Chung [:steveck]]

Thanks!

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3810b44d85e5
Add filled guid for form handler and check at startSearch. r=MattN

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/3810b44d85e5