1383687 - [Form Autofill] Order edit dialog fields based on country selected

Status: RESOLVED FIXED

(Toolkit :: Form Autofill, enhancement, P3)

Description

[Scott Wu [:scottwu]]

When we support multiple countries, the order of edit dialog input fields should be arranged based on the country selected. The data is available in the `fmt` attribute for each country in `formautofill/content/addressReferences.js`.

Details: https://github.com/googlei18n/libaddressinput/wiki/AddressValidationMetadata
Source: https://github.com/googlei18n/libaddressinput/blob/master/testdata/countryinfo.txt

Comment 2

[Scott Wu [:scottwu]]

Attachment: Bug 1383687 - Order edit dialog fields based on country selected.

Review commit: https://reviewboard.mozilla.org/r/203108/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203108/

Comment 3

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203108/diff/1-2/

Comment 4

[Steve Chung [:steveck]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

https://reviewboard.mozilla.org/r/203108/#review210148

::: browser/extensions/formautofill/FormAutofillUtils.jsm:651
(Diff revision 2)
>    getFormFormat(country) {
>      const dataset = this.getCountryAddressData(country);
>      return {
>        "addressLevel1Label": dataset.state_name_type || "province",
>        "postalCodeLabel": dataset.zip_name_type || "postalCode",
> +      "fieldsOrder": this.parseAddressFormat(dataset.fmt),

Although it's an edge case, there's still some countries/regions that don't have format in metadata. It's out of scope but it might be easy to return a default order in `parseAddressFormat` instead of the throwing error.

::: browser/extensions/formautofill/content/editDialog.js:226
(Diff revision 2)
> +      "street-address",
> +      "address-level2",
> +      "address-level1",
> +      "postal-code",
> +    ];
> +    let tabIndex = 1;

nit: It's just my preference that maybe we can initiate an empty inputs array here and `inputs.push(...container.querySelectorAll("input, textarea, select"))` to collect all the inputs. So we can update the tab index out of this loop instead of nested loop.

::: browser/extensions/formautofill/content/editDialog.js:230
(Diff revision 2)
> +    ];
> +    let tabIndex = 1;
> +    for (let i = 0; i < fieldsOrder.length; i++) {
> +      let {fieldId, newLine} = fieldsOrder[i];
> +      let container = document.getElementById(`${fieldId}-container`);
> +      let inputs = container.querySelectorAll(`input, textarea, select`);

nit: doulbe quote if it's not template string

::: browser/extensions/formautofill/content/editDialog.js:242
(Diff revision 2)
> +      }
> +      // Remove the field from the list of fields
> +      fields.splice(fields.indexOf(fieldId), 1);
> +    }
> +    // Hide the remaining fields
> +    for (let i = 0; i < fields.length; i++) {

nit: Since we don't need index here, why not simply `for (let field of fields)` and set the style directly without container?

::: browser/extensions/formautofill/skin/shared/editDialog.css:18
(Diff revision 2)
>    flex-wrap: wrap;
>    /* Add extra space to ensure invalid input box is displayed properly */
>    padding: 2px;
>  }
>  
> +div {

nit: I'm just curious about why there's no class applied in the css. Is there any reason that we want to avoid using class? Maybe we can have a `.wrap {flex-wrap: wrap;}` for the elements which will need it.

::: browser/extensions/formautofill/skin/shared/editDialog.css:22
(Diff revision 2)
>  
> +div {
> +  flex-wrap: wrap;
> +}
> +
>  form > label,

nit: How about `form label` selector? I didn't see any label in the from without this margin.

::: browser/extensions/formautofill/test/browser/browser_editAddressDialog.js:159
(Diff revision 2)
> +  });
> +  let addresses = await getAddresses();
> +  for (let [fieldName, fieldValue] of Object.entries(TEST_ADDRESS_CA_1)) {
> +    is(addresses[0][fieldName], fieldValue, "check " + fieldName);
> +  }
> +  await removeAddresses([addresses[0].guid]);

If you want to clear the saved record, I think it might be better to clear all the records(`removeAllRecords`) at the end of the test.

Comment 5

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

https://reviewboard.mozilla.org/r/203108/#review210148

> Although it's an edge case, there's still some countries/regions that don't have format in metadata. It's out of scope but it might be easy to return a default order in `parseAddressFormat` instead of the throwing error.

Ok I'll set default to the US format `%N%n%O%n%A%n%C, %S %Z`

> nit: It's just my preference that maybe we can initiate an empty inputs array here and `inputs.push(...container.querySelectorAll("input, textarea, select"))` to collect all the inputs. So we can update the tab index out of this loop instead of nested loop.

Good idea. Thanks.

> nit: Since we don't need index here, why not simply `for (let field of fields)` and set the style directly without container?

You're right. I'll use `for..of` instead.

> nit: I'm just curious about why there's no class applied in the css. Is there any reason that we want to avoid using class? Maybe we can have a `.wrap {flex-wrap: wrap;}` for the elements which will need it.

There's no special for not using classes. The form structure is just simple enough that I didn't find the need for them. I'll just combine the `flex-wrap: wrap` rules.

Comment 6

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203108/diff/2-3/

Comment 7

[Steve Chung [:steveck]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

https://reviewboard.mozilla.org/r/203108/#review210896

::: browser/extensions/formautofill/FormAutofillUtils.jsm:647
(Diff revision 3)
>     *           {string} postalCodeLabel
> +   *           {object} fieldsOrder
>     *         }
>     */
>    getFormFormat(country) {
>      const dataset = this.getCountryAddressData(country);

It's not about this patch, but I just wondering if it's expected behavior that we always return US country data if we can not get country/default region metadata, which means that unsupported country will always apply US format intead of these default format. Maybe we should let the `getCountryAddressData` only fallback to default region and let `getFormFormat` and `getCollator` handle the default value?

::: browser/extensions/formautofill/test/browser/browser_editAddressDialog.js:162
(Diff revision 3)
> +    is(addresses[0][fieldName], fieldValue, "check " + fieldName);
> +  }
> +  await removeAllRecords();
> +});
> +
> +add_task(async function test_saveAddressDE() {

Shouldn't we verify if the address level1 label is hidden in this test?

Comment 8

[Luke Chang [:lchang] (inactive)]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

https://reviewboard.mozilla.org/r/203108/#review210924

Looks good. Thanks.

Comment 9

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

https://reviewboard.mozilla.org/r/203108/#review210896

> It's not about this patch, but I just wondering if it's expected behavior that we always return US country data if we can not get country/default region metadata, which means that unsupported country will always apply US format intead of these default format. Maybe we should let the `getCountryAddressData` only fallback to default region and let `getFormFormat` and `getCollator` handle the default value?

As discussed offline we'll handle this either in Bug 1418884 or Bug 1421217.

> Shouldn't we verify if the address level1 label is hidden in this test?

Thanks I've added that check.

Comment 10

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203108/diff/3-4/

Comment 11

[Scott Wu [:scottwu]]

Comment on attachment 8932042 [details]
Bug 1383687 - Order edit dialog fields based on country selected.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203108/diff/4-5/

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6597ee8b5b88
Order edit dialog fields based on country selected. r=lchang,steveck

Comment 13

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/6597ee8b5b88