Steve Chung [:steveck] Assignee
	Description • 7 years ago

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.

	Steve Chung [:steveck] Assignee
	Comment 1 • 7 years ago

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.

	Matthew N. [:MattN]
	Comment 5 • 7 years ago
mozreview-review

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.

	Matthew N. [:MattN]
	Comment 6 • 7 years ago
mozreview-review

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.

	Matthew N. [:MattN]
	Comment 7 • 7 years ago
mozreview-review-reply

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

	Matthew N. [:MattN]
	Comment 8 • 7 years ago
mozreview-review

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.

	Matthew N. [:MattN]
	Comment 9 • 7 years ago
mozreview-review

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.

	Steve Chung [:steveck] Assignee
	Comment 12 • 7 years ago
mozreview-review

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

	Steve Chung [:steveck] Assignee
	Comment 13 • 7 years ago
mozreview-review-reply

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.

	Steve Chung [:steveck] Assignee
	Comment 14 • 7 years ago

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.

	Matthew N. [:MattN]
	Comment 17 • 7 years ago
mozreview-review

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

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

	Steve Chung [:steveck] Assignee
	Comment 19 • 7 years ago

Thanks!

	Pulsebot
	Comment 20 • 7 years ago

Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/3810b44d85e5
Add filled guid for form handler and check at startSearch. r=MattN

	Carsten Book [:Tomcat]
	Comment 21 • 7 years ago
bugherder

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