Do basic shipping address validation in the Payment Request dialog

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P1
normal
RESOLVED FIXED
a year ago
6 months ago

People

(Reporter: MattN, Assigned: jaws)

Tracking

(Blocks 1 bug)

Trunk
Firefox 63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [webpayments])

Attachments

(4 attachments, 2 obsolete attachments)

To help users have a better experience, we can hint to users when fields are missing or invalid in their shipping address.

Examples:
* missing state/province
* missing zip/postal code in countries that use it

This will have to interact well with the shippingaddresschange validation done by the merchant.
Priority: P3 → P2
Whiteboard: [webpayments]
Component: WebPayments UI → WebPayments UI
Product: Toolkit → Firefox
Assignee: nobody → jaws
Status: NEW → ASSIGNED

Updated

11 months ago
Priority: P2 → P1
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8986674 - Attachment is obsolete: true
(Reporter)

Comment 13

10 months ago
mozreview-review
Comment on attachment 8986670 [details]
Bug 1427961 - Regular expression validation for zip/postal codes.

https://reviewboard.mozilla.org/r/251974/#review259916

::: browser/components/payments/res/containers/address-form.js:282
(Diff revision 2)
>      paymentRequest.updateAutofillRecord("addresses", record, addressPage.guid, state);
>    }
> +
> +  updateCountryDependentFields(event) {
> +    let country = event.target.value;
> +    let zipCodeInput = this.form.querySelector("#postal-code");

Nit: postalCodeInput to align with the ID

::: browser/components/payments/test/mochitest/test_address_form.html:61
(Diff revision 2)
>    element.focus();
> -  if (string == "") {
> -    while (element.value) {
> +  while (element.value) {
> -      sendKey("BACK_SPACE");
> +    sendKey("BACK_SPACE");
> -    }
> +  }
> -  } else {
> -    sendString(string);
> +  sendString(string);
> -  }

Would it be possible to move this change to the first commit where the function was introduced or would that require bigger changes?

::: browser/extensions/formautofill/FormAutofillUtils.jsm:786
(Diff revision 2)
>      const dataset = this.getCountryAddressData(country);
>      return {
>        "addressLevel1Label": dataset.state_name_type || "province",
>        "postalCodeLabel": dataset.zip_name_type || "postalCode",
>        "fieldsOrder": this.parseAddressFormat(dataset.fmt || "%N%n%O%n%A%n%C, %S %Z"),
> +      "zipPattern": dataset.zip,

To align with more countries and the existing `postalCodeLabel`, please call this `postalCodePattern`.
Attachment #8986670 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 14

10 months ago
mozreview-review
Comment on attachment 8986671 [details]
Bug 1427961 - Put the asterisk next to required fields.

https://reviewboard.mozilla.org/r/251976/#review259918
Attachment #8986671 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 15

10 months ago
mozreview-review
Comment on attachment 8984791 [details]
Bug 1427961 - Basic shipping address validation for state/province and zip/postal code.

https://reviewboard.mozilla.org/r/250584/#review259914

::: commit-message-199a0:3
(Diff revision 3)
> +Bug 1427961 - Basic shipping address validation for state/province and zip/postal code. r?mattn
> +
> +todo: need to clear validation messages when changing which page is shown

reminder about this

::: browser/components/payments/res/containers/address-form.js:90
(Diff revision 3)
> +      // Add validation to some address fields
> +      let zipCodeInput = form.querySelector("#postal-code");
> +      let stateInput = form.querySelector("#address-level1");
> +      for (let element of [zipCodeInput, stateInput]) {
> +        if (element.closest(`#${element.id}-container`).style.display != "none") {
> +          element.setAttribute("required", "true");

Is there a reason you're using the attribute instead of the property? A DOM property generally provides more validation (it can throw) and can avoid DOM mutations in the cases where the property value isn't reflected on the attribute.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 21

10 months ago
mozreview-review-reply
Comment on attachment 8984791 [details]
Bug 1427961 - Basic shipping address validation for state/province and zip/postal code.

https://reviewboard.mozilla.org/r/250584/#review259914

> reminder about this

This was fixed in my previous push but I had forgotten to remove it from the commit message. This line has now been removed.

> Is there a reason you're using the attribute instead of the property? A DOM property generally provides more validation (it can throw) and can avoid DOM mutations in the cases where the property value isn't reflected on the attribute.

I've switched to using the property for the form elements, but the container needs to continue using the attribute since non-form elements don't have a .required property that is reflected as an attribute and I'm using the attribute to place the asterisk next to the label.
(Assignee)

Comment 22

10 months ago
mozreview-review-reply
Comment on attachment 8986670 [details]
Bug 1427961 - Regular expression validation for zip/postal codes.

https://reviewboard.mozilla.org/r/251974/#review259916

> Nit: postalCodeInput to align with the ID

I also changed the other variable names to match their ID, though I think the naming of address-level2 and address-level1 can be confusing and take some memorization. I would prefer if we used a name that wasn't correct for all contexts but was easier to understand for the majority of contexts.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Comment 25

10 months ago
mozreview-review-reply
Comment on attachment 8986670 [details]
Bug 1427961 - Regular expression validation for zip/postal codes.

https://reviewboard.mozilla.org/r/251974/#review259916

> I also changed the other variable names to match their ID, though I think the naming of address-level2 and address-level1 can be confusing and take some memorization. I would prefer if we used a name that wasn't correct for all contexts but was easier to understand for the majority of contexts.

I think trying to hide the complexity from developers is likely to lead to bugs due to over simplifications. The address level stuff is complicated out of necessity and I think having the code reflect that is useful so we don't have to do a lot of work later due to bad assumptions when we add new countries.
(Reporter)

Comment 26

10 months ago
mozreview-review
Comment on attachment 8984791 [details]
Bug 1427961 - Basic shipping address validation for state/province and zip/postal code.

https://reviewboard.mozilla.org/r/250584/#review260494

::: browser/components/payments/res/containers/address-form.js:47
(Diff revision 4)
> -      city: "#address-level2-container",
> -      country: "#country-container",
> -      organization: "#organization-container",
> -      phone: "#tel-container",
> -      postalCode: "#postal-code-container",
> -      recipient: "#name-container",
> +      city: "#address-level2",
> +      country: "#country",
> +      organization: "#organization",
> +      phone: "#tel",
> +      postalCode: "#postal-code",
> +      recipient: "#given-name",

Make sure to file a follow-up for this.
Attachment #8984791 - Flags: review?(MattN+bmo) → review+
(Reporter)

Comment 27

10 months ago
mozreview-review
Comment on attachment 8986672 [details]
Bug 1427961 - Disable the Save button if the form isn't valid.

https://reviewboard.mozilla.org/r/251978/#review260502

::: browser/components/payments/res/containers/address-form.js:188
(Diff revision 4)
> +      let fieldNames = [
> +        "name", "email", "tel",
> +        "organization", "street-address", "address-level2",
> +        "address-level1", "postal-code", "country",
> +      ];

I would rather we didn't hard-code this here where it can get out of sync with the autofill dialog (we're going to have to add more fields soon). Can we iterate over `this.form.elements` instead, (potentially excluding <button>s) and maybe including an attribute on form field to indicate what payment request field name they map to (e.g. for the case of the name fields)? Or else make a custom element for the name field to propagate the the required attribute and setCustomValidity appropriately.

::: browser/components/payments/res/containers/address-form.js:193
(Diff revision 4)
> +      for (let fieldName of fieldNames) {
> +        if (!addressPage.addressFields.includes(fieldName)) {
> +          let container = document.querySelector(`#${fieldName}-container`);
> +          let inputs = [...container.querySelectorAll("input, textarea, select")];
> +          for (let input of inputs) {
> +            input.required = false;

I'm a bit confused about how this loop interacts with the `for (let formElement of requiredFormElements) {` loop and why the latter one uses .style.display to determine visibility but this one does it a different way. Can't we have one check to indicate when a field isn't visible for both cases:
* Field isn't applicable to the country
* Field isn't applicable to this form (e.g. payer)

::: browser/components/payments/test/mochitest/test_address_form.html:332
(Diff revision 4)
> +  // Need to fake a "change" event since sendString won't trigger it
> +  postalCodeInput.dispatchEvent(new Event("change"));

We really should get `setUserInput` added to `HTMLSelectElement` so we can use that instead of dispatching the event everywhere. Can you file a follow-up for that? An alternative would be changing the tests to use eventUtils to type instead of using .value. We can figure out the solution in the follow-up.

::: browser/extensions/formautofill/FormAutofillUtils.jsm:786
(Diff revision 4)
>      const dataset = this.getCountryAddressData(country);
>      return {
>        "addressLevel1Label": dataset.state_name_type || "province",
>        "postalCodeLabel": dataset.zip_name_type || "postalCode",
>        "fieldsOrder": this.parseAddressFormat(dataset.fmt || "%N%n%O%n%A%n%C, %S %Z"),
> -      "zipPattern": dataset.zip,
> +      "postalCodePattern": dataset.zip,

Looks like this change was made in the wrong commit
(Reporter)

Comment 28

10 months ago
mozreview-review
Comment on attachment 8986670 [details]
Bug 1427961 - Regular expression validation for zip/postal codes.

https://reviewboard.mozilla.org/r/251974/#review260686

::: browser/components/payments/res/containers/address-form.js:146
(Diff revision 3)
>      // Add validation to some address fields
> +    let countrySelect = this.form.querySelector("#country");
> +    countrySelect.addEventListener("change", this);
>      let postalCodeInput = this.form.querySelector("#postal-code");

I think it would have been a bit cleaner to put the `updateCountryDependentFields` logic in https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/browser/extensions/formautofill/content/autofillEditForms.js#117 (making no changes to address-form.js) and add @novalidate to the <form> in editAddress.xhtml for when it's used in prefs. The reason is because I would like to keep adress-form as light as possible and eventually  combine it with EditAutofillForm once custom elements are shipping by default. It also already calls `getFormFormat`, and knows whether the postal code field will be visible from `fieldsOrder`.

::: browser/components/payments/res/containers/address-form.js:277
(Diff revision 3)
> +      let {postalCodePattern} = PaymentDialogUtils.getFormFormat(country);
> +      // Note, this doesn't handle level1, such as CA--fr
> +      postalCodeInput.setAttribute("pattern", postalCodePattern);

I think you should also handle postalCodePattern being undefined since I don't think every country in libaddressinput has one defined. One way would be:
```js
let postalCodePattern;
if (country && (postalCodePattern = PaymentDialogUtils.getFormFormat(country).postalCodePattern)) {
…
```

::: browser/extensions/formautofill/FormAutofillUtils.jsm:786
(Diff revision 3)
>      const dataset = this.getCountryAddressData(country);
>      return {
>        "addressLevel1Label": dataset.state_name_type || "province",
>        "postalCodeLabel": dataset.zip_name_type || "postalCode",
>        "fieldsOrder": this.parseAddressFormat(dataset.fmt || "%N%n%O%n%A%n%C, %S %Z"),
> +      "zipPattern": dataset.zip,

Here is where this should be postalCodePattern
(Reporter)

Comment 29

10 months ago
mozreview-review
Comment on attachment 8986671 [details]
Bug 1427961 - Put the asterisk next to required fields.

https://reviewboard.mozilla.org/r/251976/#review260688

::: browser/components/payments/res/containers/address-form.css:34
(Diff revision 3)
>  address-form .error textarea {
>    border: 1px solid #ce001a;
>  }
>  
> +label[required] > span::after {
> +  content: "*";

I guess this should be localized too so please add a comment with a bug number for that.

::: browser/components/payments/res/containers/address-form.js:149
(Diff revision 3)
> -    let postalCodeInput = this.form.querySelector("#postal-code");
> -    let addressLevel1Input = this.form.querySelector("#address-level1");
> -    for (let element of [postalCodeInput, addressLevel1Input]) {
> -      element.required = element.closest(`#${element.id}-container`).style.display != "none";
> +    let requiredFormElements = [
> +      this.form.querySelector("#given-name"),
> +      this.form.querySelector("#street-address"),
> +      this.form.querySelector("#address-level2"),
> +      this.form.querySelector("#postal-code"),
> +      this.form.querySelector("#address-level1"),
> +      countrySelect,
> +    ];
> +    for (let formElement of requiredFormElements) {
> +      let container = formElement.closest(`#${formElement.id}-container`);
> +      let required = container.style.display != "none";
> +      formElement.required = required;
> +      if (required) {
> +        container.setAttribute("required", "true");
> +      } else {

I also think it would be cleaner with the @required attributes statically in editAddress.xhtml.

To replace the .display check we could modify https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/browser/extensions/formautofill/content/autofillEditForms.js#144-152 to set .disabled appropriately on each field (so that the hidden field isn't validated)
(Reporter)

Comment 30

10 months ago
mozreview-review
Comment on attachment 8986672 [details]
Bug 1427961 - Disable the Save button if the form isn't valid.

https://reviewboard.mozilla.org/r/251978/#review260696

I think we should move this feature to a follow-up bug so we can get the basic validation landed.

::: browser/components/payments/res/containers/address-form.js:175
(Diff revision 4)
> -        span = document.createElement("span");
> -        span.className = "error-text";
> -        container.appendChild(span);
> +        // added if the merchant provided a specific error for them.
> +        if (["organization", "phone"].includes(errorName)) {
> +          field.addEventListener("change", this);

I think it would be cleaner to use a single `change` listener on the whole <form> since it bubbles.
Attachment #8986672 - Flags: review?(MattN+bmo)
(Reporter)

Comment 31

10 months ago
mozreview-review
Comment on attachment 8986673 [details]
Bug 1427961 - Implement error tooltip styling.

https://reviewboard.mozilla.org/r/251980/#review260698

Maybe this commit can also move to a follow-up

::: commit-message-d3cd2:1
(Diff revision 4)
> +Bug 1427961 - Implement error tooltip styling and hook up custom validation for errors. r?mattn

Maybe I'm missing something but I don't see how this patch "hook[s] up custom validation for errors"?
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8986672 - Attachment is obsolete: true
(Reporter)

Comment 36

10 months ago
mozreview-review
Comment on attachment 8986673 [details]
Bug 1427961 - Implement error tooltip styling.

https://reviewboard.mozilla.org/r/251980/#review260780
Attachment #8986673 - Flags: review?(MattN+bmo) → review+

Comment 37

10 months ago
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a5982366f6f1
Basic shipping address validation for state/province and zip/postal code. r=MattN
https://hg.mozilla.org/integration/autoland/rev/4b668c00c226
Regular expression validation for zip/postal codes. r=MattN
https://hg.mozilla.org/integration/autoland/rev/5be363cf7ea3
Put the asterisk next to required fields. r=MattN
https://hg.mozilla.org/integration/autoland/rev/ae034e2f6e1a
Implement error tooltip styling. r=MattN
You need to log in before you can comment on or make changes to this bug.