fix(payments-next): Implement a11y improvements #17056

xlisachan · 2024-06-03T16:57:49Z

This pull request

Added LinkExternal component to have screen readers announce when links open in a new window
Moved form elements within and added role='alert' for each error message, so screen reader reads if there is an error for the form element
Added tabindex=-1 to disabled form elements that should not be reachable
Removed disabled from div so screen reader does not mis-read that the “Name as it appears on your card” field is dimmed when checkbox is checked
Increased banner’s z-index as the ‘You need to complete this before moving forward’ tooltip displayed on top of the page banner when scrolling
Updated CSS to use border properties instead of <hr> as <hr> was separating information (e.g., 1. Create a Mozilla account / Already have a Mozilla account? Sign in heading and form section, Purchase details list prices and total section)
Converted Total label and value to a <li></li> and moved into <ul></ul> as price items should be grouped and improve readability of list price items in Purchase details
Updated StripeWrapper so it can be reused later on in Upgrade and Subscription Management

Issue that this pull request solves

Closes: FXA-9708

Checklist

Put an x in the boxes that apply

My commit is GPG signed.

netlify · 2024-06-03T18:28:43Z

❌ Deploy Preview for mozilla-fxa-react failed.

Name	Link
🔨 Latest commit	`3035ef8`
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-fxa-react/deploys/665e09b3dfd46f00082f6088

netlify · 2024-06-03T18:28:53Z

❌ Deploy Preview for mozilla-fxa failed.

Name	Link
🔨 Latest commit	`3035ef8`
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-fxa/deploys/665e09b35ff4b10008cfa9eb

netlify · 2024-06-03T18:29:03Z

❌ Deploy Preview for mozilla-fxa-payments-server failed.

Name	Link
🔨 Latest commit	`3035ef8`
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-fxa-payments-server/deploys/665e09b3f865930008c7d1ba

netlify · 2024-06-03T18:29:40Z

❌ Deploy Preview for mozilla-fxa-settings failed.

Name	Link
🔨 Latest commit	`3035ef8`
🔍 Latest deploy log	https://app.netlify.com/sites/mozilla-fxa-settings/deploys/665e09b32281a1000805d003

pdehaan · 2024-06-04T14:51:41Z

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

+            serverInvalid={hasFullNameError}
+            className="my-6"
+          >
+            <Form.Label className="text-[15px] text-grey-400 block mb-1 text-start">


Would built-in text-sm (14px) or text-base (16px) work instead of a custom font size?

I updated this to use a custom size for a more unified look with the labels and error messages used in the Stripe Payment Element (15px).

Note: the base font size for the Stripe Payment Element is currently set to 16px.

Fly-by comment:

I guess there's no way to update the stripe payment element? Sad. Can we at least use rems, or is stripe payment also px?

Apologies, clarification on the note - we had set the Stripe Payment Element to have a base font-size of 16px.

It looks like the Element does its own calculations from the base font size, so that its labels and error message use 15px (.93rem).

Looking at the Stripe Appearance API, it might be possible to set the Stripe Elements sizes. https://docs.stripe.com/elements/appearance-api

pdehaan · 2024-06-04T14:52:34Z

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

+            {hasFullNameError && (
+              <Form.Message asChild>
+                <Localized id="next-payment-validate-name-error">
+                  <p className="text-[15px] mt-1 text-alert-red" role="alert">


Same Q as above, re: font-size.

See above for reasoning.

pdehaan · 2024-06-04T14:56:56Z

libs/payments/ui/src/lib/client/components/CheckoutCheckbox.tsx

+              id="next-payment-confirm-with-legal-links-static-3"
+              elems={{
+                termsOfServiceLink: (
+                  <LinkExternal


I think all these 2-4 <LinkExternal> components will end up having the same default data-testid prop, if that's an issue. 🤷

'data-testid': testid = 'link-external',

Updated, thanks Peter!

StaberindeZA

I've left a few comments. I appended the comments with either "Clarification question", where I thought it might be worth while having a discussion, or "Change required" where it seems a change is necessary.

StaberindeZA · 2024-06-04T14:27:43Z

libs/payments/ui/tsconfig.lib.json

@@ -19,5 +19,5 @@
    "src/**/*.spec.jsx",
    "src/**/*.test.jsx"
  ],
-  "include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx"]
+  "include": ["src/**/*.js", "src/**/*.jsx", "src/**/*.ts", "src/**/*.tsx", "../../shared/react/src/components/LinkExternal.tsx"]


Change required. This should be removed. The payments-ui lib shouldn't be responsible for building components from shared-react

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

libs/payments/ui/src/lib/client/components/CheckoutCheckbox.tsx

StaberindeZA · 2024-06-04T14:49:32Z

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

+        {!isPaymentElementLoading && (
+          <Form.Submit asChild>
+            <Localized id="next-new-user-submit">
+              <PrimaryButton


Change required. The submit button is clickable, even during the disabled state, when the checkbox has not been selected yet. The rest of the disabled area has a no-cursor icon.

I pushed an update for all the other changes - working on this one now.

apps/payments/next/app/[locale]/[offeringId]/checkout/[interval]/[cartId]/start/page.tsx

StaberindeZA · 2024-06-04T15:09:03Z

tsconfig.base.json

@@ -58,6 +58,7 @@
      "@fxa/shared/notifier": ["libs/shared/notifier/src/index.ts"],
      "@fxa/shared/otp": ["libs/shared/otp/src/index.ts"],
      "@fxa/shared/pem-jwk": ["libs/shared/pem-jwk/src/index.ts"],
+      "@fxa/shared/react/*": ["libs/shared/react/src/*"],


Change required. Is this the alias that was generated by the nx generator? Why is it different from the typescript libraries?

No, I updated it to match shared-assets. I'll revert.

StaberindeZA · 2024-06-05T14:26:56Z

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

+              className={
+                formEnabled
+                  ? ''
+                  : 'cursor-not-allowed relative focus:border-blue-400 focus:outline-none focus:shadow-input-blue-focus after:absolute after:content-[""] after:top-0 after:left-0 after:w-full after:h-full after:bg-white after:opacity-50 after:z-10'
+              }


This shouldn't be necessary. The
creating the disabled layer encapsulates the button, so the cursor-not-allowed should also apply to the button.

I had a look at SP2, and it looks like the PrimaryButton is missing class z-[5], which we can probably replace with z-10. Adding that should work. (Note, this can be added to the PrimaryButton component directly)

StaberindeZA · 2024-06-05T14:27:54Z

libs/payments/ui/src/lib/client/components/CheckoutForm.tsx

-      {!isPaymentElementLoading && (
-        <Form.Submit asChild>
-          <Localized id="next-new-user-submit">
+      <div


This is missing the mt-14 which adds a bit of space between the checkbox and the Enter your card information.

StaberindeZA

lgtm, ty!

xlisachan force-pushed the FXA-9708 branch from e3096f9 to 3035ef8 Compare June 3, 2024 18:21

xlisachan force-pushed the FXA-9708 branch 5 times, most recently from 1884120 to 7b0e3ca Compare June 4, 2024 00:04

xlisachan marked this pull request as ready for review June 4, 2024 14:10

xlisachan requested review from a team as code owners June 4, 2024 14:10

xlisachan changed the title ~~WIP: fix(payments-next): Implement a11y improvements~~ fix(payments-next): Implement a11y improvements Jun 4, 2024

pdehaan reviewed Jun 4, 2024

View reviewed changes

pdehaan requested a review from a team June 4, 2024 14:58

StaberindeZA reviewed Jun 4, 2024

View reviewed changes

xlisachan force-pushed the FXA-9708 branch from 7b0e3ca to 1323f41 Compare June 4, 2024 17:49

bcolsson approved these changes Jun 4, 2024

View reviewed changes

xlisachan force-pushed the FXA-9708 branch 2 times, most recently from 8a36b05 to 1351176 Compare June 4, 2024 22:25

StaberindeZA reviewed Jun 5, 2024

View reviewed changes

fix(payments-next): Implement a11y improvements

f9f670f

xlisachan force-pushed the FXA-9708 branch from 95b0b35 to f9f670f Compare June 5, 2024 14:55

StaberindeZA self-requested a review June 5, 2024 15:41

StaberindeZA approved these changes Jun 5, 2024

View reviewed changes

xlisachan merged commit 18e5d17 into main Jun 5, 2024
23 checks passed

xlisachan deleted the FXA-9708 branch June 5, 2024 15:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(payments-next): Implement a11y improvements #17056

fix(payments-next): Implement a11y improvements #17056

xlisachan commented Jun 3, 2024 •

edited

Loading

netlify bot commented Jun 3, 2024

netlify bot commented Jun 3, 2024

netlify bot commented Jun 3, 2024

netlify bot commented Jun 3, 2024

pdehaan Jun 4, 2024

xlisachan Jun 4, 2024

LZoog Jun 4, 2024

xlisachan Jun 4, 2024

StaberindeZA Jun 4, 2024

pdehaan Jun 4, 2024

xlisachan Jun 4, 2024

pdehaan Jun 4, 2024

xlisachan Jun 4, 2024

StaberindeZA left a comment

StaberindeZA Jun 4, 2024

xlisachan Jun 4, 2024

StaberindeZA Jun 4, 2024

xlisachan Jun 4, 2024

StaberindeZA Jun 4, 2024

xlisachan Jun 4, 2024

StaberindeZA Jun 5, 2024

StaberindeZA Jun 5, 2024

StaberindeZA left a comment

fix(payments-next): Implement a11y improvements #17056

fix(payments-next): Implement a11y improvements #17056

Conversation

xlisachan commented Jun 3, 2024 • edited Loading

This pull request

Issue that this pull request solves

Checklist

netlify bot commented Jun 3, 2024

❌ Deploy Preview for mozilla-fxa-react failed.

netlify bot commented Jun 3, 2024

❌ Deploy Preview for mozilla-fxa failed.

netlify bot commented Jun 3, 2024

❌ Deploy Preview for mozilla-fxa-payments-server failed.

netlify bot commented Jun 3, 2024

❌ Deploy Preview for mozilla-fxa-settings failed.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StaberindeZA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

StaberindeZA left a comment

Choose a reason for hiding this comment

xlisachan commented Jun 3, 2024 •

edited

Loading