Skip to content

refactor(settings): make pair2 illustrations aria-hidden - #21215

Merged
dschom merged 1 commit into
mainfrom
FXA-14337
Sep 17, 2026
Merged

dschom merged 1 commit into
mainfrom
FXA-14337

Conversation

@dschom

@dschom dschom commented Sep 14, 2026 •

Copy link
Copy Markdown
Contributor

Because

  • The five shared pairing illustrations landed in feat(settings): add shared pairing illustrations #20979 with placeholder labels and a TODO, pending an a11y decision.
  • Per the a11y guidance on the ticket, an illustration whose card states the same thing in text is presentational under WCAG 1.1.1.

This pull request

  • Renders the five pairing illustrations aria-hidden rather than labelling them with empty strings.
  • Replaces the aria-label test's exclusion regex with a named decorative list, covering both the hidden and the labelled images.
  • Corrects seven Pair2 page tests whose names and comments described the illustrations as exposed.

Issue that this pull request solves

Closes: FXA-14337

Checklist

Put an x in the boxes that apply

  • My commit is GPG signed.
  • If applicable, I have modified or added tests which pass locally.
  • I have added necessary documentation (if appropriate).
  • I have verified that my changes render correctly in RTL (if appropriate).
  • I have manually reviewed all AI generated code.

How to review (Optional)

  • Key files/areas to focus on: packages/fxa-settings/src/components/images/ — index.tsx and index.test.tsx. The other seven files change test names and comments only.
  • Suggested review order: the two components/images files, then the Pair2 page tests.
  • Risky or complex parts: none. Rendered DOM is unchanged — the empty-string labels already resolved to the same aria-hidden branch of PreparedImage.

Screenshots (Optional)

Please attach the screenshots of the changes made in case of change in user interface.

Other information (Optional)

  • The audit found adequate surrounding context for all five illustrations, so the list of "images that need aria labels" the ticket asks for is empty, and no new FTL strings were needed.
  • SyncDevicesImage is still exposed on five Pair2 cards with a verbose label and is a candidate for the same treatment. Left alone here — it is a pre-existing translated string.

@dschom dschom changed the title refactor(settings): make pair2 illustrations explicitly decorative refactor(settings): make pair2 illustrations aria-hidden Sep 15, 2026
@dschom
dschom marked this pull request as ready for review September 16, 2026 02:09
@dschom
dschom requested a review from a team as a code owner September 16, 2026 02:09
@vbudhram
vbudhram requested a balanced review from Copilot September 16, 2026 18:52

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The implementation preserves existing rendering while making decorative-image intent explicit and adequately tested.

Pull request overview

Marks five Pair2 illustrations as decorative while retaining accessible labels for meaningful images and QR codes.

Changes:

  • Forces shared Pair2 illustrations into aria-hidden rendering.
  • Adds explicit coverage for decorative and labelled image exports.
  • Updates Pair2 accessibility test descriptions and expectations.
File summaries
File Description
components/images/index.tsx Marks five pairing illustrations decorative.
components/images/index.test.tsx Tests decorative and labelled image groups.
Pair2/Supplicant/TimeoutAndCancel/index.test.tsx Clarifies accessibility expectation.
Pair2/Supplicant/SyncSuccess/index.test.tsx Clarifies accessibility expectation.
Pair2/Supplicant/ReadyToScan/index.test.tsx Clarifies accessibility expectation.
Pair2/Authority/TimeoutAndCancel/index.test.tsx Clarifies accessibility expectation.
Pair2/Authority/SyncSuccess/index.test.tsx Clarifies accessibility expectation.
Pair2/Authority/ScanQR/index.test.tsx Distinguishes QR labeling from decorative artwork.
Pair2/Authority/ContinueOnMobile/index.test.tsx Clarifies accessibility expectation.
Review details
  • Files reviewed: 9/9 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@vbudhram vbudhram left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dschom Seems reasonable, low risk too

@LZoog

LZoog commented Sep 16, 2026

Copy link
Copy Markdown
Contributor

an illustration whose card states the same thing in text

FWIW, I don't think that's what these images are doing, and there's a "user delight" angle and big screen real estate and low vision user argument. However, these are easy enough for us to add back later and it's really up to the accessibility team based on the page context. I've sent Laurel a message and if she gives the thumbs up I'm good to r+.

@dschom

dschom commented Sep 16, 2026

Copy link
Copy Markdown
Contributor Author

an illustration whose card states the same thing in text

FWIW, I don't think that's what these images are doing, and there's a "user delight" angle and big screen real estate and low vision user argument. However, these are easy enough for us to add back later and it's really up to the accessibility team based on the page context. I've sent Laurel a message and if she gives the thumbs up I'm good to r+.

@LZoog I won't land until you r+

@LZoog LZoog left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm r+'ing since you're right that it's better to have aria-hidden=true on these instead of an empty aria-label, because that'll make it read more consistently across OS/screen readers. VO on Mac skips these but it's possible another could read these as "image" with nothing else.

I think it's worth filing a follow up to have the accessibility folks look at all of our pages with images similar to this for a more official recommendation on what makes sense for user delight / low sighted users. I'm betting they'll say most should be hidden but some deserve some text. This might also be better as a Zoom call with someone on their side so we can show them Storybook pages, or something like that, but either way, I know that's low priority and we can do it at any point later. 👍

/>
/*
* The pairing illustrations below are decorative. Every card that renders one
* states the same thing in its heading and body copy, so WCAG 1.1.1 is met

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a nit pick but I think the wording in another comment, "already carry their meaning in text" is more accurate than, "Every card that renders one states the same thing in its heading and body copy." As written it just makes me think of an image with text in it that "states the same thing" but eh.

Because:
 - The five shared pairing illustrations landed in #20979 with placeholder
   labels and a TODO, pending an a11y decision.
 - Every card that renders one states the same thing in text, so WCAG 1.1.1
   treats them as presentational.

This commit:
 - Renders the five illustrations aria-hidden rather than relying on empty
   label strings falling through a falsy check.
 - Replaces the aria-label test's exclusion regex with a named decorative
   list, asserting both the hidden and the labelled images.
 - Corrects seven Pair2 page tests whose names and comments described the
   illustrations as exposed.

Closes #FXA-14337
@dschom
dschom merged commit 0a92c2e into main Sep 17, 2026
4 of 7 checks passed
@dschom
dschom deleted the FXA-14337 branch September 17, 2026 20:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants