Populate select elements with form autofill profile data

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks 1 bug)

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

Firefox Tracking Flags

(firefox55 fixed)

Details

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

Attachments

(1 attachment)

Assignee

Description

2 years ago
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 hidden (mozreview-request)
Assignee

Updated

2 years ago
Blocks: 1365544

Comment 3

2 years ago
mozreview-review
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)
Assignee

Updated

2 years ago
Blocks: 1365895
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
mozreview-review-reply
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 hidden (mozreview-request)

Comment 7

2 years ago
mozreview-review
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 hidden (mozreview-request)
Assignee

Comment 9

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review
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 11

2 years ago
mozreview-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
Assignee

Updated

2 years ago
Whiteboard: [form autofill:M3] → [form autofill:M3] ETA:612
Comment hidden (mozreview-request)
Assignee

Comment 14

2 years ago
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.
Assignee

Comment 16

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

Thanks Luke!
Keywords: checkin-needed

Comment 17

2 years ago
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: 2 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.