[Form Autofill] Support "country-name" fields

RESOLVED FIXED in Firefox 56

Status

()

enhancement
P3
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: lchang, Assigned: lchang)

Tracking

(Blocks 2 bugs)

unspecified
mozilla56
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 attachment)

Create "country-name" fields based on "country" when loading data from the storage and vice versa.
Assignee

Updated

2 years ago
Assignee: nobody → lchang
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review148046

::: browser/extensions/formautofill/ProfileStorage.jsm:102
(Diff revision 1)
>                                     "nsIUUIDGenerator");
>  
> +XPCOMUtils.defineLazyGetter(this, "REGION_NAMES", function() {
> +  let regionNames = {};
> +
> +  // We only support US in MVP.

This is also assuming that we want to use the English name "United States" instead of the translated: https://dxr.mozilla.org/l10n-central/search?q=%22us%3D%22+path%3AregionNames.properties&redirect=false

I think you should link to the bug that you file for proper support and also mention that it's US in en-US.

::: browser/extensions/formautofill/ProfileStorage.jsm:104
(Diff revision 1)
> +  let countries = Services.strings.createBundle("chrome://global/locale/regionNames.properties").getSimpleEnumeration();
> +  while (countries.hasMoreElements()) {
> +    let country = countries.getNext().QueryInterface(Components.interfaces.nsIPropertyElement);
> +    regionNames[country.key.toUpperCase()] = country.value;
> +  }
> +  */

Don't leave this commented code in mozilla-central… put it in a bug to implement this

::: browser/extensions/formautofill/ProfileStorage.jsm:110
(Diff revision 1)
> +  while (countries.hasMoreElements()) {
> +    let country = countries.getNext().QueryInterface(Components.interfaces.nsIPropertyElement);
> +    regionNames[country.key.toUpperCase()] = country.value;
> +  }
> +  */
> +  regionNames.US = "United States";

What about "United States of America"?

::: browser/extensions/formautofill/ProfileStorage.jsm:433
(Diff revision 1)
> +    // Compute country name
> +    if (profile.country) {
> +      let countryName = REGION_NAMES[profile.country];
> +      if (countryName) {
> +        profile["country-name"] = countryName;
> +      }
> +    }

In the future the country name we want to fill may depend on the locale of the page possibly. For example, if I'm in an en-US build and go to a zh-TW website then we may need to look for "United States", "United States of America" and "美國" in the <option>s for a <select>.

This is another case where having the returned profile be an object that has a method to get country translations may be better since a single value won't be great.
Assignee

Comment 3

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review148046

> This is also assuming that we want to use the English name "United States" instead of the translated: https://dxr.mozilla.org/l10n-central/search?q=%22us%3D%22+path%3AregionNames.properties&redirect=false
> 
> I think you should link to the bug that you file for proper support and also mention that it's US in en-US.

I'll file the bug. Thanks.

> What about "United States of America"?

"United States" is aligned with the naming in our preferences dialog and "regionNames.properties". Should we change that?

> In the future the country name we want to fill may depend on the locale of the page possibly. For example, if I'm in an en-US build and go to a zh-TW website then we may need to look for "United States", "United States of America" and "美國" in the <option>s for a <select>.
> 
> This is another case where having the returned profile be an object that has a method to get country translations may be better since a single value won't be great.

The reason I compute the country name here is mainly for displaying a full name in dropdown and preferences dialog, and for collecting "autofillSavedFieldNames". We will implement another mechanism to handle filling-in issue like what Scott is going to do in bug 1365544.

Comment 4

2 years ago
mozreview-review
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

::: browser/extensions/formautofill/FormAutofillHeuristics.jsm:38
(Diff revision 1)
>      "address-line3",
>      "address-level2",
>      "address-level1",
>      "postal-code",
>      "country",
> +    "country-name",

Both `country` and `country-name` indicate the field is related to "country", but I am fine with replacing `country` with `country-name` if needed. Just like what the patch did for `ProfileAutoCompleteResult.jsm`.

I agree these two affect how to fill the country value into a field.

May I know what kind of cases that we need both `country` and `country-name` in prediction heuristics?

::: browser/extensions/formautofill/ProfileStorage.jsm:483
(Diff revision 1)
>          profile["street-address"] = addressLines.join("\n");
>        }
>      }
> +
> +    // Normalize country
> +    if (profile.country || profile["country-name"]) {

`test_transformFields.js` is still pass without this this `if` statement.

Can't this `if` statement be removed? If not, I guess there are some cases not covered in the related test.

IIUC, `profile["country-name"]` is meaningful here only when `profile.country` does not exist.

::: browser/extensions/formautofill/ProfileStorage.jsm:490
(Diff revision 1)
> +        let country = profile.country.toUpperCase();
> +        // We only support US in MVP.
> +        if (country == "US") {
> +          profile.country = country;
> +        } else {
> +          delete profile.country;

Does this mean any country except `US` is going to be removed before writing to JSON file?

What happen if an address record comes from a new version add-on? Do users lose their `country` field here?
Attachment #8872558 - Flags: review?(selee)

Comment 5

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Does this mean any country except `US` is going to be removed before writing to JSON file?
> 
> What happen if an address record comes from a new version add-on? Do users lose their `country` field here?

I should address the comment more clearly here.
A new version add-on means it's ready to support other country/region. It's possible if users use Sync feature.
Assignee

Comment 6

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Both `country` and `country-name` indicate the field is related to "country", but I am fine with replacing `country` with `country-name` if needed. Just like what the patch did for `ProfileAutoCompleteResult.jsm`.
> 
> I agree these two affect how to fill the country value into a field.
> 
> May I know what kind of cases that we need both `country` and `country-name` in prediction heuristics?

Since `country` explicitly stands for the country code and `country-name` means the full name, I think we should support both and fill in different values if these attributes are set on text inputs rather than <select>s.

> `test_transformFields.js` is still pass without this this `if` statement.
> 
> Can't this `if` statement be removed? If not, I guess there are some cases not covered in the related test.
> 
> IIUC, `profile["country-name"]` is meaningful here only when `profile.country` does not exist.

Yes, it can be removed and you understand correctly.

> I should address the comment more clearly here.
> A new version add-on means it's ready to support other country/region. It's possible if users use Sync feature.

All these normalizations are supposed to be applied to local modifications only so it shouldn't affect profiles coming from sync engine. I intend to remove the value other than `US` because there is no other value that user can choose from preferences dialog in our current version.

Comment 7

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Since `country` explicitly stands for the country code and `country-name` means the full name, I think we should support both and fill in different values if these attributes are set on text inputs rather than <select>s.

Yes, I agree that "we should support both and fill in different values". Does this mean `country-name` and `country` can be regconized during filling value stage?

BTW, `VALID_FIELDS` is renamed to `FIELD_GROUPS` in m-c. (well, the changes is more than renaming but also grouping.)
Adding `country-name` into `FIELD_GROUPS` implies that we should add a `country-name` regexp rule.
If we can find an efficient regexp rule to distinguish `country-name` and `country`, we can consider that.
Or I would suggest to do that during filling value stage.

Alternative way is to check `tagName`, `max-length`, etc. to determine it's `country-name` or `country` during prediction stage.

Comment 8

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Yes, I agree that "we should support both and fill in different values". Does this mean `country-name` and `country` can be regconized during filling value stage?
> 
> BTW, `VALID_FIELDS` is renamed to `FIELD_GROUPS` in m-c. (well, the changes is more than renaming but also grouping.)
> Adding `country-name` into `FIELD_GROUPS` implies that we should add a `country-name` regexp rule.
> If we can find an efficient regexp rule to distinguish `country-name` and `country`, we can consider that.
> Or I would suggest to do that during filling value stage.
> 
> Alternative way is to check `tagName`, `max-length`, etc. to determine it's `country-name` or `country` during prediction stage.

After the offline discussion, we don't have to add `country-name` to the heuristics.
Assignee

Updated

2 years ago
Whiteboard: [form autofill:M2] → [form autofill:M2] ETA:612
Assignee

Updated

2 years ago
Whiteboard: [form autofill:M2] ETA:612 → [form autofill:M2]
Assignee

Updated

2 years ago
Blocks: 1370193
Assignee

Comment 9

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review148046

> I'll file the bug. Thanks.

Filed bug 1370193.
Comment hidden (mozreview-request)
Whiteboard: [form autofill:M2] → [form autofill:M3]
Comment hidden (mozreview-request)
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review151772
Attachment #8872558 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> All these normalizations are supposed to be applied to local modifications only so it shouldn't affect profiles coming from sync engine. I intend to remove the value other than `US` because there is no other value that user can choose from preferences dialog in our current version.

Why wouldn't `recordWriteProcessor` be used by Sync's writes? If we receive a record from an older version then we may need `recordWriteProcessor` to compute some things or upgrade the record.

Comment 14

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Why wouldn't `recordWriteProcessor` be used by Sync's writes? If we receive a record from an older version then we may need `recordWriteProcessor` to compute some things or upgrade the record.

If we want to prevent users to select other countries except US, I think `_recordReadProcessor` is the right place to force the users to see US before supporting others.

I have the same question with MattN. Does ProfileStorage provide another way to write JSON file for Sync?

Comment 15

2 years ago
mozreview-review
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review156026

Thanks for the update. It looks good to me except the `recordWriteProcessor` part.
Attachment #8872558 - Flags: review?(selee)
Assignee

Comment 16

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> If we want to prevent users to select other countries except US, I think `_recordReadProcessor` is the right place to force the users to see US before supporting others.
> 
> I have the same question with MattN. Does ProfileStorage provide another way to write JSON file for Sync?

Do you mean we have the country with the value other than US saved anyway but blank out it while reading the record?

According to Mark's WIP patch, there will be an additional argument in `add`/`update` method to indicate that the action is triggered by sync or not, so the saving process doesn't necessarily have to be the same -- i.e. `recordWriteProcessor` doesn't necessarily have to be invoked.

However, as we are implementing the caching mechanism in bug 1368008, a migrating function should be always invoked after syncing.
Comment hidden (mozreview-request)
Assignee

Comment 18

2 years ago
mozreview-review-reply
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review149098

> Do you mean we have the country with the value other than US saved anyway but blank out it while reading the record?
> 
> According to Mark's WIP patch, there will be an additional argument in `add`/`update` method to indicate that the action is triggered by sync or not, so the saving process doesn't necessarily have to be the same -- i.e. `recordWriteProcessor` doesn't necessarily have to be invoked.
> 
> However, as we are implementing the caching mechanism in bug 1368008, a migrating function should be always invoked after syncing.

The patch has been updated per offline discussion to save any valid (in the region list) country code and hide its value upon reading if it's not US.

Comment 19

2 years ago
mozreview-review
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review156992

LGTM! Thanks.
Attachment #8872558 - Flags: review?(selee)

Comment 20

2 years ago
mozreview-review
Comment on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review156994
Attachment #8872558 - Flags: review+
Comment hidden (mozreview-request)

Comment 22

2 years ago
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd36449c21cc
[Form Autofill] Support "country-name" fields. r=MattN,seanlee

Comment 23

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd36449c21cc
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.