Closed Bug 1364823 Opened 3 years ago Closed 3 years ago

Populate select elements with form autofill profile data

Categories

(Toolkit :: Form Manager, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: scottwu, Assigned: scottwu)

References

(Blocks 1 open bug)

Details

(Whiteboard: [form autofill:M3] ETA:612)

Attachments

(1 file)

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.
Blocks: 1365544
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.
Attachment #8868079 - Flags: review?(lchang)
Blocks: 1365895
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 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."
Attachment #8868079 - Flags: review?(lchang)
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 on attachment 8868079 [details]
Bug 1364823 - Populate select elements with form autofill profile data.

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

Thanks.
Attachment #8868079 - Flags: review?(lchang) → review+
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.
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
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
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.
Fine with me.
Try looks fine with a few unrelated intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fa88b5d4c57f

Thanks Luke!
Keywords: checkin-needed
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/96ed52d76547
Populate select elements with form autofill profile data. r=lchang
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/96ed52d76547
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
As per San-Francisco meeting with :vchen, marking this bug as qe-.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.