1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement, P3)

Description

[Sean Lee [:seanlee][:weilonge]]

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.

Comment 1

[Sean Lee [:seanlee][:weilonge]]

The input filled by user should be skipped as well when applying auto filling.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Attachment: Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

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

Comment 3

[Sean Lee [:seanlee][:weilonge]]

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

Comment 4

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

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

Comment 5

[Sean Lee [:seanlee][:weilonge]]

Update the patch with xpcshell-test which includes `change` event.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

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

Comment 7

[Matthew N. [:MattN]]

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.

Comment 8

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8837535 [details]
Bug 1339721 - Use FormAutofillHandler.autofillFormFields to fill in fields.;

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

Comment 9

[Sean Lee [:seanlee][:weilonge]]

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 10

[Sean Lee [:seanlee][:weilonge]]

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.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec0a9237ed60
Use FormAutofillHandler.autofillFormFields to fill in fields.; r=MattN

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec0a9237ed60