Skip to content

feat(a11y): semantic HTML, accessible buttons, and payment-element integration - #1654

Open
AbhishekChorotiya wants to merge 1 commit into
feat/a11y-iframesfrom
feat/a11y-semantic-integration
Open

feat(a11y): semantic HTML, accessible buttons, and payment-element integration#1654
AbhishekChorotiya wants to merge 1 commit into
feat/a11y-iframesfrom
feat/a11y-semantic-integration

Conversation

@AbhishekChorotiya

@AbhishekChorotiya AbhishekChorotiya commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Type of Change

  • Bugfix
  • New feature
  • Enhancement
  • Refactoring
  • Dependency updates
  • Documentation
  • CI/CD

Description

This PR completes the accessibility integration work across payment methods and saved-method experiences. It tightens accessible names, removes noisy action wording, improves saved-method semantics, and centralizes repeated wallet and bank-account labelling logic in the shared accessibility helpers.

The user impact is cleaner announcements, more meaningful saved-method labels, clearer wallet button names, and a more consistent assistive-technology experience across the full payment element.

This is the final PR in the split accessibility stack. It should be reviewed after the previous four PRs because it relies on the shared form-control, focus, live-region, and iframe accessibility foundations.

Closes #1653

How did you test it?

Validated as part of the completed accessibility stack. The checks cover the combined flow after all stacked PRs are applied, and the saved-method/new-method flows were checked through the local payment-element accessibility smoke flow.

  • Ran npm run re:build on the completed accessibility stack.
  • Ran npm run test:hooks on the completed accessibility stack.
  • Ran npm run build on the completed accessibility stack.
  • Ran committed-range whitespace validation on the completed accessibility stack.

Checklist

  • I ran npm run re:build
  • I reviewed submitted code
  • I added unit tests for my changes where possible

@semanticdiff-com

Copy link
Copy Markdown

Review changes with  SemanticDiff

@XyneSpaces

Copy link
Copy Markdown

Code Review Findings

[blocking] Bare silent catch in message handler

The AddBankAccount.res component introduces a bare catch that silently discards all errors:

Location: src/Components/AddBankAccount.res, around line 56-59

try {
  let dict = ev.data->safeParse->getDictFromJson
  // ... message handling
} catch {
| _ => ()
}

Per the team's guidelines, bare catch { | _ => ... } patterns that swallow errors are only permitted in PreMountLoader.res preload paths. This handler should propagate errors through the component's error handling mechanism or at minimum log the failure for debugging.

Suggested fix:

catch {
| ex =>
  logger.error("Failed to handle fullscreen iframe message", ex)
  // Or propagate to component error state
}

[should-fix] Verify focus restoration target

The AccessibilityUtils.focus call on triggerRef after fullscreen iframe unmount assumes the element still exists and is focusable. This could fail if the component unmounted or if the ref became stale.

Location: src/Components/AddBankAccount.res

Ensure AccessibilityUtils.focus includes null checks before attempting focus, or guard the call with a mounted-state check.


[should-fix] v1/v2 parity check required

This PR modifies payment method components (CardPayment, SavedMethods, WalletConnectButton, etc.). If the v2 API counterparts exist (e.g., CardPaymentV2.res, SavedMethodsV2.res), ensure equivalent accessibility changes are applied there for API parity.

Verify the following pairs are updated consistently:

  • CardPayment.res ↔ CardPaymentV2.res
  • SavedMethods.res ↔ SavedMethodsV2.res
  • PaymentHelpers.res helpers used in both versions

[nit] shared-code/sdk-utils promotion opportunity

The new AccessibilityUtils.res module contains generic accessibility helpers that appear web-agnostic (focus management, keyboard navigation detection). If these functions don't depend on React/Recoil/DOM-specific features, consider promoting them to shared-code/sdk-utils/accessibility/ for reuse by hyperswitch-client-core.

Check imports in AccessibilityUtils.res for any web-only dependencies before deciding.

@XyneSpaces

Copy link
Copy Markdown

⚠️ [should-fix] Message event handler lacks origin validation

The message event listener added in AddBankAccount.res processes messages without validating event.origin:

let handle = (ev: Window.event) => {
  try {
    let dict = ev.data->safeParse->getDictFromJson
    switch dict->Dict.get("fullScreenIframeMounted")->Option.flatMap(JSON.Decode.bool) {
    | Some(false) =>
      triggerRef.current->Nullable.toOption->Option.forEach(el => el->AccessibilityUtils.focus)
    | _ => ()
    }
  } catch {
  | _ => ()
  }
}

Without origin checking, this could allow malicious cross-origin messages to trigger focus behavior. While the data handling appears safe (only checking a specific boolean field), please verify:

  1. Should ev.origin be validated against an allowlist of trusted origins?
  2. Is the message handling intended to accept messages from any origin?

Consider adding origin validation matching patterns used elsewhere in the codebase for postMessage handlers.

@XyneSpaces

Copy link
Copy Markdown

💡 Tab-index management in AddBankAccount.res

The triggerTabIndex variable is set based on isDataAvailable but the onKeyDown handler only handles click events (openModal()). Consider adding keyboard activation support:

onKeyDown={ev => {
  if ev->ReactEvent.Keyboard.key === "Enter" || ev->ReactEvent.Keyboard.key === " " {
    openModal()
  }
}}

Also verify the triggerTabIndex is correctly applied to the focusable element (it appears set but not used in the visible diff).

…tegration

Payment area as a labelled form landmark (role=form), heading hierarchy, labels; convert
clickable div/span/icon controls into accessible buttons (Enter+Space) via the shared
AccessibilityUtils.onActivateKeyDown helper (added here); centralize Enter+Space in
SwitchViewButton; wire announcer mount + iframe focus-delegation into PaymentElement.
ARIA labels localized (goBackLabel, savePaymentMethodLabel, selectCardLabel, doneLabel, etc.);
announced errors via LiveError.
@AbhishekChorotiya
AbhishekChorotiya force-pushed the feat/a11y-semantic-integration branch from 5b281e4 to 129445d Compare June 29, 2026 09:36
@XyneSpaces

Copy link
Copy Markdown

Review Summary

PR: #1654 - feat(a11y): semantic HTML, accessible buttons, and payment-element integration

Findings

  1. Good: The AddBankAccount.res component properly implements conditional accessibility attributes based on data availability state (isDataAvailable).

  2. Good: Wallet accessible naming utilities in AccessibilityUtils.res provide consistent labeling for cards, wallets, and bank accounts.

  3. Good: The onActivate pattern replaces inline keyDown handlers, centralizing keyboard activation logic.

  4. Issue: The getCardBrandAccessibleName function uses string matching for card brands. Consider using a variant type if card brands are defined as variants elsewhere in the codebase for type safety.

  5. Good: Saved methods container now has proper role="region" and ariaLabel for screen reader navigation.

No blocking issues. The semantic HTML improvements enhance accessibility across payment components.

@XyneSpaces

Copy link
Copy Markdown

🚨 Security concern - Unvalidated postMessage origin

The accessibility implementation uses postMessage with wildcard target origin (*). While the focus delegation message content is low-risk, this pattern could be copied for sensitive messages.

Fix: Derive targetOrigin from the actual merchant origin or use a restrictive whitelist rather than wildcard.

// Instead of:
let sendFocusNext = (~iframeId, ~targetOrigin="*") => ...

// Use:
let sendFocusNext = (~iframeId, ~targetOrigin) => ...
// And validate/pass the merchant's origin

⚠️ Memory leak risk - useEffect cleanup missing

Several event listeners are registered without corresponding cleanup:

  • Window.addEventListener("message", handle) without removeEventListener in cleanup
  • Focus handlers attached to refs without detaching on unmount

Fix: Ensure all useEffect hooks that add listeners return a cleanup function:

useEffect(() => {
  let handler = ...
  Window.addEventListener("message", handler)
  Some(() => Window.removeEventListener("message", handler))
})

@XyneSpaces

Copy link
Copy Markdown

🚨 Bare catch block swallows all errors silently

In AddBankAccount.res, the message event handler uses catch { | _ => () } which silently discards all errors. This pattern can mask JSON parsing failures, unexpected message formats, or runtime exceptions.

// Current (bad)
} catch {
| _ => ()
}

// Better - at minimum log the error
} catch {
| ex => loggerState.setLogError(~value=ex->Utils.formatException, ~eventName=MESSAGE_HANDLER_ERROR)
}

If this is intentional (e.g., cross-origin noise filtering), add a comment explaining why errors are suppressed.

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.

2 participants