Repository navigation
A4A: extract A4APopoverTrigger with built-in keyboard a11y - #111038
Conversation
|
Link to live branch is being generated... |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| onKeyDown?.( event ); | ||
| if ( event.key === 'Enter' || event.key === ' ' ) { | ||
| event.preventDefault(); | ||
| onActivate(); | ||
| } |
There was a problem hiding this comment.
[follow-up] Issue: No tests for the shared keyboard-handling logic.
The handleKeyDown has three branches (Enter, Space with preventDefault, and passthrough for other keys). Since this component is now shared across 5 surfaces, a regression here would be wide-reaching. A minimal test covering Enter activation, Space activation with default prevention, and that other keys don't fire onActivate would satisfy the guideline.
Suggestion: Add a small test file (trigger.test.tsx) in this directory covering the key branches.
When implementing non-trivial or complicated logic, add a minimal set of non-redundant tests that cover the key branches and edge cases.
There was a problem hiding this comment.
I found two non-blocking things worth checking:
The icon-only triggers still do not expose an accessible name. Screen readers may announce them only as “button”. Could we add aria-label or aria-labelledby at the icon-only call sites?
In the popover, the “Learn more” link appears focused immediately after opening. It looks a bit odd visually. Ideally, focus would stay on the trigger when the popover opens, and move to the link only after the first Tab press.
— ClemenAgent (Powered by Codex)
Note: This review was prepared fully or partially by an AI agent on my behalf, with my oversight. Please raise any issues, inconsistencies, or missing context in the review.
|
@cleacos thanks for the review — applied both suggestions:
Pushed in 811b9b6. |
|
This PR modifies the release build for the following Calypso Apps: For info about this notification, see here: PCYsg-OT6-p2
To test WordPress.com changes, run |
Jetpack Cloud Live (direct link)
Automattic for Agencies Live (direct link)
|
|
Thank you @tiagonoronha Mmmm, I'm not sure if it's a regression, but now I cannot close the popover, and I can open multiple popovers by clicking in the 
|
|
✅ Re-approved! |
The popover trigger pattern (span with role="button", tabIndex, click and keydown handlers) was duplicated across multiple call sites, all missing a visible focus indicator and Space-key activation. Extract a shared A4APopoverTrigger component that centralizes these accessibility concerns and migrate the existing trigger sites.
Address review feedback on the popover triggers used for agency-tier influenced revenue, WooPayments commission eligibility, consolidated stats cards, and referral cancellation: - Add aria-label to the icon-only triggers so screen readers announce the action instead of an unnamed "button". - Forward focusOnMount through A4APopover and set it to false on the three info tooltips that previously moved focus onto a "Learn more" link the moment the popover opened. Focus now stays on the trigger and tabs into the popover content.
Switching focusOnMount to false kept focus on the trigger but also broke the popovers' close-on-outside-focus behavior — popovers couldn't be dismissed and clicking other triggers stacked them. Using "container" instead moves focus into the popover wrapper (so the "Learn more" link is no longer auto-focused) while keeping onFocusOutside functional.
a8cfdcd to
60e13d4
Compare
|
This Pull Request is now available for translation here: https://translate.wordpress.com/deliverables/33689804 Some locales (Hebrew) have been temporarily machine-translated due to translator availability. All other translations are usually ready within a few days. Untranslated and machine-translated strings will be sent for translation next Monday and are expected to be completed by the following Friday. Thank you @tiagonoronha for including a screenshot in the description! This is really helpful for our translators. |
The bump of `@wordpress/compose` from 7.45 to 8.0 in this PR added a `useFocusOnMount.Mode` namespace export with a strict literal union type: `boolean | 'firstElement' | 'firstInputElement'`. Previously the namespace didn't exist, so `useFocusOnMount.Mode` resolved to `any` and the `"container"` value used in three A4A popovers (introduced in #111038) type-checked without complaint. At runtime, `"container"` and `true` are equivalent — both fall through to `setFocus(node)` on the popover wrapper in both 7.45 and 8.0, since the implementation only special-cases `false`, `'firstElement'`, and `'firstInputElement'`. Switching to the JSX prop shorthand `focusOnMount` (which evaluates to `true`) preserves behavior — focus moves into the popover wrapper, `onFocusOutside` keeps working, and "Learn more" links inside are no longer auto-focused — while satisfying the narrowed type.
4 participants
Follow-up to #111013.
Proposed Changes
A4APopoverTriggercomponent (client/a8c-for-agencies/components/a4a-popover/trigger.tsx) that centralizes the popover-trigger pattern:role="button",tabIndex={0}, Enter + Space activation (withpreventDefaultfor Space), forwarded ref, and a built-in:focus-visibleoutline.<span role="button" tabIndex={0} onClick onKeyDown>boilerplate and the per-sitecursor: pointer/display: inline-flex/ redundant:focus-visiblerules:agency-tier/overview-content/influenced-revenue.tsxcomponents/consolidated-stats-card/index.tsxwoopayments/dashboard-content/site-columns.tsxpurchases/licenses/license-preview/index.tsx(TransferredBadge)referrals/referral-details/components/assigned-to.tsxWhy are these changes being made?
Review feedback on #111013 noted that tabbing to the influenced-revenue info icon left no visible focus indicator. The same
<span role="button" tabIndex={0}>pattern was duplicated across multiple A4A surfaces, all sharing the same accessibility gaps (no focus indicator, no Space-key activation). Extracting a shared component fixes the indicator everywhere at once and prevents the pattern from drifting further.Two existing popover-related files are intentionally untouched:
partner-directory/dashboard/status-badge.tsx— hover-driven, different interaction model (and was already missing a keyboard handler).marketplace/checkout/pending-payment-popover.tsx— renders the popover itself, not a trigger.Testing Instructions
Pre-merge Checklist