All users were logged out of Bugzilla on October 13th, 2018

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

RESOLVED FIXED in Firefox 59

Status

()

P3
normal
RESOLVED FIXED
a year ago
10 months ago

People

(Reporter: scottwu, Assigned: scottwu)

Tracking

(Blocks: 3 bugs)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [form autofill:V2])

Attachments

(1 attachment)

(Assignee)

Description

a year ago
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
(Assignee)

Updated

a year ago
Whiteboard: [form autofill:MVP] → [form autofill]
Priority: -- → P3
Whiteboard: [form autofill] → [form autofill:V2]
(Assignee)

Updated

a year ago
Blocks: 1411508
(Assignee)

Updated

11 months ago
Assignee: nobody → scwwu

Updated

11 months ago
Component: Form Manager → Form Autofill

Updated

11 months ago
Blocks: 1415112
(Assignee)

Updated

11 months ago
Duplicate of this bug: 1022898
(Assignee)

Updated

11 months ago
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 4

11 months ago
mozreview-review
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.
Attachment #8932042 - Flags: review?(schung)
(Assignee)

Comment 5

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

Comment 7

11 months ago
mozreview-review
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?
Attachment #8932042 - Flags: review?(schung) → review+

Comment 8

11 months ago
mozreview-review
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.
Attachment #8932042 - Flags: review?(lchang) → review+
(Assignee)

Comment 9

10 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

10 months ago
Keywords: checkin-needed

Comment 12

10 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/6597ee8b5b88
Order edit dialog fields based on country selected. r=lchang,steveck
Keywords: checkin-needed

Comment 13

10 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6597ee8b5b88
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.