1364823 - Populate select elements with form autofill profile data

Status: RESOLVED FIXED

(Toolkit :: Form Manager, enhancement)

Description

[Scott Wu [:scottwu]]

Data from autofill profile should also populate select elements when their `fieldName`s match. Special care should be taken to make sure `address-level1` (state for US) could be filled in correctly.

Comment 1

[Scott Wu [:scottwu]]

Attachment: Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 2

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 3

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review143738

You need to modify `FormAutofillContent`, `FormAutofillHandler` and `FormAutofillHeuristics` to allow `<select>` elements be processed in our core flow. Note that you might need to exclude `<select>` elements with `multiple` attribute. Besides, as our offline discussion, there might be a bug related to `radio` and `checkbox`, which we should ignore, in `FormAutofillHeuristics`. It would be better if you can take care of that when you refactor it.

::: browser/extensions/formautofill/FormAutofillHandler.jsm:113
(Diff revision 2)
> +              continue;
> +            }
> +            for (let option of element.options) {
> +              if (value === option.textContent || value === option.value) {
> +                element.value = option.value;
> +                element.dispatchEvent(new Event("change", {"bubbles": true}));

`dispatchEvent` can't make the event "trusted". I imagine you will need to implement an API like `setUserInput`. However, it's ok to file another bug to handle it and leave a "TODO" here.

Comment 4

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 5

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review143738

> `dispatchEvent` can't make the event "trusted". I imagine you will need to implement an API like `setUserInput`. However, it's ok to file another bug to handle it and leave a "TODO" here.

`dispatchEvent` actually makes the event "trusted" because it's done with chrome privilege, however it's not exactly the same as event triggered by user actions. Filed a bug and added TODO there.

Comment 6

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 7

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review146312

::: browser/extensions/formautofill/FormAutofillHandler.jsm:106
(Diff revision 4)
> -        element.setUserInput(value);
> +            element.setUserInput(value);
> +            break;
> +          }
> +          case "SELECT": {
> +            // Do not change value if the option is already selected.
> +            let selected = element.selectedOptions[0];

If we're going to support multiple options, the condition to early return should happen only when there's only one option selected and it's already been the expected value.

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:287
(Diff revision 4)
> +do_test(TESTCASES, (testcase, element) => {
> +  return new Promise(resolve => {
> +    element.addEventListener("change", () => {
> +      let id = element.id;
> +      Assert.equal(element.value, testcase.expectedResult[id],
> +                  "Check the " + id + " fields were filled with correct data");

nit: "... field was ..."

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:295
(Diff revision 4)
> +  });
> +});
> +
> +do_test(TESTCASES_INPUT_UNCHANGED, (testcase, element) => {
> +  return new Promise((resolve, reject) => {
> +    element.addEventListener("change", reject);

I would recommand wrapping `reject` in a function that also calls `clearTimeout`.

::: browser/extensions/formautofill/test/unit/test_autofillFormFields.js:302
(Diff revision 4)
> +    setTimeout(() => {
> +      let id = element.id;
> +      element.removeEventListener("change", reject);
> +      element.removeEventListener("input", reject);
> +      Assert.equal(element.value, testcase.expectedResult[id],
> +                  "Check the " + id + " fields were filled with correct data");

The message needs an update. e.g. "Check no event is triggered on the `id` field."

Comment 8

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review146312

> If we're going to support multiple options, the condition to early return should happen only when there's only one option selected and it's already been the expected value.

Having talked with Juwei, we agree we can ignore multiple-select for the time being because it's very unlikely users will run into that situation. I changed `selectedOptions` to using `selectedIndex` so it's clear that we are only interested in single select.

> nit: "... field was ..."

Got it.

> I would recommand wrapping `reject` in a function that also calls `clearTimeout`.

Got it. Turns out `reject` must have a reason in our test suite or Promise.all will keep waiting.

> The message needs an update. e.g. "Check no event is triggered on the `id` field."

Thanks that's better.

Comment 10

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review146892

Thanks.

Comment 11

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

https://reviewboard.mozilla.org/r/139696/#review146960

::: browser/extensions/formautofill/FormAutofillHandler.jsm:108
(Diff revision 5)
> +          }
> +          case "SELECT": {
> +            // Do not change value if the option is already selected.
> +            // Use case for multiple select is not considered here.
> +            let selected = element.options[element.selectedIndex];
> +            if (value === selected.textContent || value === selected.value) {

Forgot to say that you should use case-insensitive string comparison. Besides, I think you don't need to compare with `textContent` if we're going to preprocess the profile data before showing the dropdown. Let's discuss it next week.

Comment 12

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

Hey Scott,

This patch is going to provide autofilling value based on the exact matching.
Could you add the mochitest in [1]?

BTW, since bug 1349490 and bug 1360370 are both landed, please rebase this patch based on the latest m-c.

Thank you.

[1] http://searchfox.org/mozilla-central/rev/1a0d9545b9805f50a70de703a3c04fc0d22e3839/browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html#169

Comment 13

[Scott Wu [:scottwu]]

Comment on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Comment 14

[Scott Wu [:scottwu]]

Thanks Sean, I've made changes to `test_basic_autocomplete_form.html` to test if a select element is filled-in correctly. Originally the `country` field was a text input and it was used to test falling-back to history search result. I've changed it to `email` and created a select element for `country` field instead.

Luke, this patch has been rebased to m-c. As for case-insensitive string comparison, I'll address it in Bug 1365544, and use `.localeCompare`, which does more than case-insensitive comparison.

Comment 15

[Luke Chang [:lchang] (inactive)]

Fine with me.

Comment 16

[Scott Wu [:scottwu]]

Try looks fine with a few unrelated intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa88b5d4c57f

Thanks Luke!

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/96ed52d76547
Populate select elements with form autofill profile data. r=lchang

Comment 18

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/96ed52d76547

Comment 19

[Adrian Florinescu [:aflorinescu]]

As per San-Francisco meeting with :vchen, marking this bug as qe-.