Use FormAutofillHandler.autofillFormFields to fill in fields

RESOLVED FIXED in Firefox 54

Status

()

enhancement
P3
normal
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: selee, Assigned: selee)

Tracking

(Blocks 1 bug)

unspecified
mozilla54
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox54 fixed)

Details

(Whiteboard: [form autofill:M1])

Attachments

(1 attachment)

This bug should use FormAutofillHandler.autofillFormFields method to fill the form with a selected profile.

The focused input should be skipped since the focused input is filled by FormFillController.
Blocks: 1339731
The input filled by user should be skipped as well when applying auto filling.
Hey MattN,

The patch includes the implementation but the test.
Filling field value in the following cases is avoided in the patch:
1. the focused input which is filled in FormFillController.
2. a non-empty input field
3. the invalid value set
Assignee: nobody → selee
Update the patch with xpcshell-test which includes `change` event.
Blocks: 1338482
Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

https://reviewboard.mozilla.org/r/112682/#review114108

::: browser/extensions/formautofill/FormAutofillHandler.jsm:117
(Diff revision 1)
> -      let info = FormAutofillHeuristics.getInfo(fieldDetail.element);
> -      if (!info ||
> -          field.section != info.section ||
> +      let value = profile[fieldDetail.fieldName];
> +      if (value) {
> +        fieldDetail.element.setUserInput(value);
> -          field.addressType != info.addressType ||
> -          field.contactType != info.contactType ||

XXX Change doesn't check heuristics still match.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:110
(Diff revision 3)
> -   *        [{
> -   *          section: Value originally provided to the user interface.
> +   * @param {Object} focusedInput
> +   *        A focused input element which is skipped for filling.

Can you remind me why we didn't use my suggestion of an empty value in autocomplete so that we did the filling ourselves? It seems like that would be simpler.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:114
(Diff revision 3)
> -   *        Data returned by the user interface.
> -   *        [{
> -   *          section: Value originally provided to the user interface.
> -   *          addressType: Value originally provided to the user interface.
> -   *          contactType: Value originally provided to the user interface.
> -   *          fieldName: Value originally provided to the user interface.
> +   *        A profile to be filled in.
> +   * @param {Object} focusedInput
> +   *        A focused input element which is skipped for filling.
> +   */
> +  autofillFormFields(profile, focusedInput) {
> +    log.debug("profile:", profile);

Can you update this message to indicate where it's coming from e.g. add the method name as a prefix?

::: browser/extensions/formautofill/FormAutofillHandler.jsm:126
(Diff revision 3)
> -      let info = FormAutofillHeuristics.getInfo(fieldDetail.element);
> -      if (!info ||
> -          field.section != info.section ||
> +      let value = profile[fieldDetail.fieldName];
> +      if (value) {
> +        fieldDetail.element.setUserInput(value);

Sorry for the delay. I got blocked on my last pass since I was hesistant to remove this check and the associated refactoring but I think we can give it a try.

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:185
(Diff revision 3)
> +        onChangePromises.push(new Promise(resolve => {
> +          element.addEventListener("change", function changeHandler() {
> +            let id = element.id;
> +            Assert.equal(element.value, testcase.expectedResult[id],
> +                        "Check the " + id + " fields were filled with correct data");
> +            element.removeEventListener("change", changeHandler);

You can use the `once` option here instead.
Attachment #8837535 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

https://reviewboard.mozilla.org/r/112682/#review114108

> Can you remind me why we didn't use my suggestion of an empty value in autocomplete so that we did the filling ourselves? It seems like that would be simpler.

Filling an empty string still triggers DOMAutoComplete event, so I think it's a better solution to skip any operation for the focused input.
Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

https://reviewboard.mozilla.org/r/112682/#review114108

> Can you update this message to indicate where it's coming from e.g. add the method name as a prefix?

"profile in autofillFormFields:" is applied here now.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/ec0a9237ed60
Use FormAutofillHandler.autofillFormFields to fill in fields.; r=MattN
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ec0a9237ed60
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Flags: qe-verify-
No longer blocks: 1339731
You need to log in before you can comment on or make changes to this bug.