[Form Autofill] Handle filling in country field select element

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
Form Manager
RESOLVED FIXED
4 months ago
2 months ago

People

(Reporter: scottwu, Assigned: lchang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla57
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

(Whiteboard: [form autofill:M3])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Reporter)

Description

4 months ago
Form autofill should try to fill country field select element. FormAutofillHandler will first look for the country key (ex. US) on both the option values and texts of a country select element. If there's no exact match, we should try to identify them.
(Assignee)

Updated

3 months ago
Whiteboard: [form autofill:MVP] → [form autofill:M4]
Comment hidden (mozreview-request)
(Assignee)

Updated

3 months ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

3 months ago
mozreview-review
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review169160

::: browser/extensions/formautofill/FormAutofillUtils.jsm:289
(Diff revision 3)
> +          break;
> -    }
> +        }
>  
> -    if (fieldName === "country") {
> -      // TODO: Support matching countries (Bug 1375382)
> +        let matchingOption;
> +        alternativeCountryNames.some(name => {
> +          let reName = new RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i");

`reName` should be kept in a cache since `name` is a const value in a const array.
This makes `RegExp("\\b" + this.escapeRegExp(name) + "\\b", "i")` can not be precompiled.

::: browser/extensions/formautofill/ProfileStorage.jsm:1325
(Diff revision 3)
> +      if (!address.country) {
> +        for (let country in FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES) {
> +          let found = FormAutofillUtils.ALTERNATIVE_COUNTRY_NAMES[country].find(name => {
> +            let reName = new RegExp("\\b" + FormAutofillUtils.escapeRegExp(name) + "\\b", "i");
> +            return reName.test(countryName);
> +          });

In this snippet, there are three cross-JSM calls in the `for` loop.
I suggest to write a helper in `FormAutofillUtils` and use it here.
```JS
// This should be in FormAutofillUtils.
findCountryCode(countryName) {
  for (let country in this.ALTERNATIVE_COUNTRY_NAMES) {
    let found = this.ALTERNATIVE_COUNTRY_NAMES[country].find(name => {
      let reName = this.getCountryRegexp(name);
      return reName.test(countryName);
    });
    if (found) {
      return country;
    }
  }
  return null;
},

```

You seem want to handle more matching cases, so we should have a test for the above cases in `test_transformFields.js` as well.
Attachment #8891179 - Flags: review?(selee)
Comment hidden (mozreview-request)
(Assignee)

Comment 6

3 months ago
Updated accordingly. Thanks.
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171040

::: browser/extensions/formautofill/FormAutofillUtils.jsm:235
(Diff revision 4)
> +      let dataset = this.getCountryAddressData(country);
> +      this._collator[country] = new Intl.Collator(dataset.lang, {sensitivity: "base", ignorePunctuation: true});

This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly.

I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:305
(Diff revision 4)
> +      case "address-level1":
> -      if (!Array.isArray(dataset.sub_keys)) {
> +        if (!Array.isArray(dataset.sub_keys)) {

If you're switching to use `case` then you should add {} braces so that variables have block scope
Whiteboard: [form autofill:M4] → [form autofill:M3]

Comment 8

2 months ago
mozreview-review
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171990

The patch looks good to me. I will review it again once the issues that MattN addressed are fixed. Thanks.
Attachment #8891179 - Flags: review?(selee)
(Assignee)

Comment 9

2 months ago
mozreview-review-reply
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review171040

> This assumes that there is one language per country which isn't correct so that should be noted here. You can get other languages by splitting the `languages` property on "~" but that still isn't ideal as I think only one gets actually used. It seems like the locale of the page should be taken into account to do this properly.
> 
> I'm fine with this for the MVP if there's a comment and a bug. Maybe we should have a whiteboard for V2 stuff involving rollout in more locales: "[form autofill:V2]"?

Thanks for pointing it out. I should have noticed that. I'll add a comment for now and file a bug to follow up how we can better deal with the languages.
Comment hidden (mozreview-request)

Comment 11

2 months ago
mozreview-review
Comment on attachment 8891179 [details]
Bug 1375382 - [Form Autofill] Handle filling in country field select element.

https://reviewboard.mozilla.org/r/162384/#review172112

LGTM! Thank you.
Attachment #8891179 - Flags: review?(selee) → review+

Comment 12

2 months ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/40da3955d4a1
[Form Autofill] Handle filling in country field select element. r=seanlee

Comment 13

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/40da3955d4a1
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
https://hg.mozilla.org/releases/mozilla-beta/rev/64e427f215f7
status-firefox56: --- → fixed
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.