Matthew N. [:MattN] Reporter
	Description • 6 years ago

When the page is first shown for pre-selected rows and upon picker changes.

	Matthew N. [:MattN] Reporter
	Comment 1 • 6 years ago

If the errors are shown then the Pay button should probably also be disabled, unless there are only warnings.

	Matthew N. [:MattN] Reporter
	Comment 4 • 6 years ago
mozreview-review

Comment on attachment 8997619 [details]
Bug 1476348 - Show missing field validation errors on the summary page.

https://reviewboard.mozilla.org/r/261322/#review268538

::: commit-message-92eaf:1
(Diff revision 2)
> +Bug 1476348 - Show missing field validation errors on the summary page. r?mattn

This approach works for missing field validation but I'm not sure how well it will handle other Fx validation. Can you file a follow-up for that since when I filed this bug I was thinking that was the only kind of Fx validation that would happen. Examples of other validation we have: Luhn algorithm, valid postal code, valid region, etc.

::: browser/components/payments/res/containers/address-picker.js:32
(Diff revision 2)
> +  get fieldNames() {
> +    if (this.hasAttribute("address-fields")) {
> +      let names = this.getAttribute("address-fields").split(/\s+/);
> +      if (names.length) {

Nice cleanup

::: browser/components/payments/res/containers/address-picker.js:121
(Diff revision 2)
> +    this.classList.toggle("invalid-selected-option",
> +                          this.missingFieldsOfSelectedOption().length);

Perhaps this could go in the new `render(state)` method of rich-picker: https://dxr.mozilla.org/mozilla-central/rev/4e56a2f51ad739ca52046723448f3129a58f1666/browser/components/payments/res/containers/rich-picker.js#52-54

::: browser/components/payments/res/containers/rich-picker.js:36
(Diff revision 2)
> +    this.invalidLabel = document.createElement("label");
> +    this.invalidLabel.className = "invalid-label";
> +    this.invalidLabel.setAttribute("for", this.dropdown.popupBox.id);

If you haven't already, please double-check that both labels are used for the popupbox in the devtools accessibility inspector… I mostly don't want the error label to incorrectly override the normal label. I know multiple labels are valid but don't know how accessibility tools handle them.

::: browser/components/payments/res/containers/rich-picker.js:58
(Diff revision 2)
>    get value() {
>      return this.dropdown &&
>             this.dropdown.selectedOption;
>    }

I don't think this should be named `value` as that normally only returns @value of the selected option (a string) for <select> and <rich-select>, can you rename this to `selectedOption`

::: browser/components/payments/res/containers/rich-picker.js:73
(Diff revision 2)
> +    let selectedOption = this.value;
> +    if (!selectedOption) {
> +      return [];
> +    }
> +
> +    let fieldNames = super.fieldNames || this.fieldNames;

What is this line trying to do? I'm not sure why we would prefer the super's fieldNames over the subclass'?

::: browser/components/payments/test/mochitest/test_address_picker.html:78
(Diff revision 2)
> +      },
>      },
>    });
>    await asyncElementRendered();
>    let options = picker1.dropdown.popupBox.children;
> -  is(options.length, 2, "Check dropdown has both addresses");
> +  is(options.length, 3, "Check dropdown has both addresses");

Nit: here and below "both" is no longer correct

	Jared Wein [:jaws] (please needinfo? me) Assignee
	Comment 5 • 6 years ago
mozreview-review-reply

Comment on attachment 8997619 [details]
Bug 1476348 - Show missing field validation errors on the summary page.

https://reviewboard.mozilla.org/r/261322/#review268538

> This approach works for missing field validation but I'm not sure how well it will handle other Fx validation. Can you file a follow-up for that since when I filed this bug I was thinking that was the only kind of Fx validation that would happen. Examples of other validation we have: Luhn algorithm, valid postal code, valid region, etc.

Yeah, we will need to figure out a way to share the validation between the different views. I don't want this to start getting duplicated by the various pages of the UI.

> If you haven't already, please double-check that both labels are used for the popupbox in the devtools accessibility inspector… I mostly don't want the error label to incorrectly override the normal label. I know multiple labels are valid but don't know how accessibility tools handle them.

The combobox accessible object ends up with a name of "Contact Information Missing or invalid information" so it appears that it concatenates the two in document order.

	Pulsebot
	Comment 8 • 6 years ago

Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23889c258042
Show missing field validation errors on the summary page. r=MattN

	Cristian Brindusan [:cbrindusan]
	Comment 9 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/23889c258042

	Timea Cernea [:tbabos][inactive]
	Comment 10 • 6 years ago

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:63.0) Gecko/20100101 Firefox/63.0

Verified - Fixed on the latest Nightly 63.0a1 (2018-08-07) on Windows 10 x64, Ubuntu 16.04, Mac OS 10.13, Windows 7 x64.
The “Missing or invalid information" error is displayed under the Delivery Address or Payment Method field on the Order Summary page. The "Pay" button is also grayed out when the mentioned message appears.