Closed Bug 1476348 Opened 4 years ago Closed 4 years ago

Show missing field validation errors on the summary page

Categories

(Firefox :: WebPayments UI, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 63
Tracking Status
firefox63 --- verified

People

(Reporter: MattN, Assigned: jaws)

References

(Depends on 1 open bug)

Details

(Whiteboard: [webpayments] [user-testing])

Attachments

(1 file)

When the page is first shown for pre-selected rows and upon picker changes.
Whiteboard: [webpayments] → [webpayments] [user-testing]
If the errors are shown then the Pay button should probably also be disabled, unless there are only warnings.
Flags: qe-verify+
QA Contact: hani.yacoub
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Priority: P2 → P1
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
Attachment #8997619 - Flags: review?(MattN+bmo) → review+
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.
Depends on: 1481481
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23889c258042
Show missing field validation errors on the summary page. r=MattN
https://hg.mozilla.org/mozilla-central/rev/23889c258042
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.