Closed Bug 1358944 Opened 4 years ago Closed 4 years ago

[Form Autofill] Support "country-name" fields

Categories

(Toolkit :: Form Manager, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 --- fixed

People

(Reporter: lchang, Assigned: lchang)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [form autofill:M3])

Attachments

(1 file)

Create "country-name" fields based on "country" when loading data from the storage and vice versa.
Assignee: nobody → lchang
Status: NEW → ASSIGNED
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.
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 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 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.
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 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 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.
Whiteboard: [form autofill:M2] → [form autofill:M2] ETA:612
Whiteboard: [form autofill:M2] ETA:612 → [form autofill:M2]
Blocks: 1370193
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.
Whiteboard: [form autofill:M2] → [form autofill:M3]
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 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 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)
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 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 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 on attachment 8872558 [details]
Bug 1358944 - [Form Autofill] Support "country-name" fields.

https://reviewboard.mozilla.org/r/144086/#review156994
Attachment #8872558 - Flags: review+
Pushed by lchang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/fd36449c21cc
[Form Autofill] Support "country-name" fields. r=MattN,seanlee
https://hg.mozilla.org/mozilla-central/rev/fd36449c21cc
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.