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

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Form Manager
P1
normal
RESOLVED FIXED
6 months ago
5 months ago

People

(Reporter: steveck, Assigned: steveck)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [form autofill:M1])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

6 months 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.
(Assignee)

Updated

6 months ago
Blocks: 1338485
Summary: Fallback to form history the target field doesn't have data in selected profile → Fallback to form history if the target field doesn't have data in selected profile
(Assignee)

Comment 1

6 months 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.
(Assignee)

Updated

6 months ago
Depends on: 1339721
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee: nobody → schung
Status: NEW → ASSIGNED
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.
Attachment #8840773 - Flags: review?(MattN+bmo) → 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.
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 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.
Attachment #8840803 - Flags: review?(MattN+bmo) → 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.
Attachment #8841427 - Flags: review?(MattN+bmo) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8841427 - Attachment is obsolete: true
(Assignee)

Comment 12

5 months 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
Attachment #8840773 - Flags: review?(schung)
(Assignee)

Comment 13

5 months 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.
(Assignee)

Comment 14

5 months 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.
Attachment #8840773 - Flags: review?(schung)
Attachment #8840773 - Flags: review?(MattN+bmo)
Attachment #8840773 - Flags: review+
Attachment #8840773 - Flags: feedback?(lchang)
Comment hidden (mozreview-request)
(Assignee)

Updated

5 months ago
Attachment #8840773 - Attachment is obsolete: true
Attachment #8840773 - Flags: review?(MattN+bmo)
Attachment #8840773 - Flags: feedback?(lchang)

Updated

5 months ago
Blocks: 1340468
Comment hidden (mozreview-request)
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 hidden (mozreview-request)
(Assignee)

Comment 19

5 months ago
Thanks!
Keywords: checkin-needed

Comment 20

5 months 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
Keywords: checkin-needed

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/3810b44d85e5
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.