Repository navigation
Conversation
… region
Belkin's WCAG 2.2 audit raised two defects against the hosted PLP bundle. Both
reproduce in main and are byte-identical between their 1.6.0 and the latest
1.20.0, so upgrading fixed neither.
ADA-468 — Sort control not keyboard/AT accessible (WCAG 4.1.2):
- `.cio-plp-sort input { display: none }` sat outside the media query and hid the
radios from the accessibility tree and focus order at every viewport. Replaced
with a visually-hidden pattern; the checked visual is unchanged since it is drawn
by the adjacent `span:before`.
- Added a focus-visible ring on the proxy, since the input itself is invisible
(otherwise operable but with no visible focus indicator — WCAG 2.4.7).
- Every radio had a unique `name`, so the options never formed a radio group: no
arrow-key navigation and each option toggled independently. Now one shared name
per rendered group, wrapped in `role="radiogroup"`. Not previously ticketed.
- Dropped the checked state from the label `key`: a key that changes on selection
remounted the label and dropped focus mid-navigation, which would have left
arrow keys broken even after the `name` fix.
- Trigger exposes `aria-expanded` and `aria-controls`.
ADA-403 — Filter changes are silent (WCAG 4.1.3):
- New `LiveRegion` (`role="status"`, `aria-live="polite"`), mounted above the
status branches so it exists in the DOM from first render — assistive tech only
announces changes to an already-present region.
- Announcements derive from request status in `useResultsAnnouncement`, so one
announcement covers filter, sort and pagination changes. Initial load is silent
to avoid talking over the screen reader's own page-load announcement.
- Strings are overridable via the top-level `translations` prop. CDX-303 was not a
blocker: it shipped 2026-06-22 and interpolation already happens at the call
site, exactly as the existing result count header does.
Also fixed, same defect class, not previously ticketed:
- Filter checkboxes were hidden the same way. The real defect is upstream in
constructorio-ui-components (`filter-option.tsx` renders the input with
Tailwind's `hidden`); overridden here with an unlayered rule as a documented
temporary measure, pending the upstream fix.
- `MobileModal` had no dialog semantics or focus management: now `role="dialog"`,
`aria-modal`, an accessible name, focus move-in and restore, a Tab focus trap,
and Escape to close. Close now also has a timeout fallback, since `transitionend`
never fires when the transition is skipped (reduced motion, no stylesheet),
which previously left the modal stuck open.
Deviation from the ticket's DoD: `aria-haspopup` was not added to the Sort
trigger. The desktop panel is an inline disclosure, so announcing a popup would
trade one 4.1.2 failure for another; the single trigger serves both viewports via
CSS, so there is no mobile-only trigger to carry it.
Verified: 575 Jest tests and 49 Storybook interaction tests pass; lint and
check-types clean. Keyboard navigation, focus retention, focus ring, dialog
semantics, Tab trap and Escape confirmed in Chromium at desktop and mobile
viewports. axe-core reports no WCAG A/AA violations on Sort — note it reported
none before this change either, since hidden elements are skipped, which is why
the manual audit caught this and CI did not. Remaining Filters violations
(range slider labels, list nesting, contrast) are byte-identical pre-existing
and out of scope. Bundle greps that previously failed now pass: aria-live and
role="status" present, `.cio-plp-sort input{display:none}` gone.
Not verified: screen reader pass with a real AT (VoiceOver/NVDA) is still
outstanding per the DoD.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Mudaafi
changed the title
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
🟡 Changes recommended
Several unresolved accessibility issues affect hydrated results, collapsed filters, multiple sort instances, and mobile dialog relationships.
7 open findings
What changed in this PR
Improves accessibility for sort controls, filters, mobile dialogs, and asynchronous result updates.
Changes:
- Makes sort/filter inputs keyboard and assistive-technology accessible.
- Adds accessible modal semantics and focus management.
- Adds translated live-region announcements with regression tests.
| File | Description |
|---|---|
src/utils/helpers.ts |
Adds announcement defaults. |
src/types.ts |
Adds translation keys. |
src/styles.css |
Adds visually hidden utility. |
src/stories/components/Sort/Sort.stories.tsx |
Adds sort accessibility checks. |
src/stories/components/CioPlp/Translations Code Examples.mdx |
Documents announcement translations. |
src/index.ts |
Exports LiveRegion. |
src/hooks/useResultsAnnouncement.ts |
Generates result announcements. |
src/components/Sort/Sort.tsx |
Adds radio grouping and ARIA state. |
src/components/Sort/Sort.css |
Preserves radio focusability. |
src/components/MobileModal/MobileModal.tsx |
Adds dialog semantics and focus trapping. |
src/components/LiveRegion/LiveRegion.tsx |
Implements the live region. |
src/components/LiveRegion/index.ts |
Exports the component. |
src/components/Filters/FilterOptionsList.css |
Makes filter checkboxes accessible. |
src/components/CioPlpGrid/CioPlpGrid.tsx |
Integrates announcements. |
spec/components/Sort/Sort.test.jsx |
Tests sort accessibility. |
spec/components/MobileModal/MobileModal.test.jsx |
Tests modal accessibility. |
spec/components/CioPlpGrid/CioPlpGrid.test.jsx |
Tests live announcements. |
cspell.json |
Adds accessibility terminology. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| .cio-filter-multiple-option .cio-filter-option-input { | ||
| /* Must be restated: the shared `hidden` class sets `display: none` explicitly, and | ||
| * the visually-hidden properties below do not override `display` on their own. */ | ||
| display: inline-block; | ||
| position: absolute; | ||
| width: 1px; | ||
| height: 1px; | ||
| margin: -1px; | ||
| padding: 0; | ||
| border: 0; | ||
| overflow: hidden; | ||
| clip-path: inset(50%); | ||
| white-space: nowrap; | ||
| } |
| const FOCUSABLE_SELECTOR = [ | ||
| 'a[href]', | ||
| 'button:not([disabled])', | ||
| 'input:not([disabled])', | ||
| 'select:not([disabled])', | ||
| 'textarea:not([disabled])', | ||
| '[tabindex]:not([tabindex="-1"])', | ||
| ].join(','); |
| export default function MobileModal(props: React.PropsWithChildren<MobileModalProps>) { | ||
| const { side = 'left', isOpen, setIsOpen, children } = props; | ||
| const { side = 'left', isOpen, setIsOpen, label = 'Dialog', children } = props; |
| // Each rendered option set (desktop + mobile) is its own radio group, so the two | ||
| // instances don't compete over the same selection. Sharing one `name` within a group | ||
| // is what makes the options arrow-navigable and mutually exclusive. (WCAG 4.1.2) | ||
| const getGroupName = (idSuffix: string = '') => `cio-plp-sort${idSuffix}`; |
| aria-expanded={isOpen} | ||
| aria-controls={panelId} |
| return; | ||
| } | ||
|
|
||
| if (status !== RequestStatus.SUCCESS || totalNumResults === undefined) return; |
| setAnnouncement( | ||
| totalNumResults === 0 | ||
| ? translate('No results found', translations) | ||
| : `${totalNumResults} ${translate('results found', translations)}`, |
… fix collapsed filter focus
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
| /* TODO: Remove once the shared components library stops hiding the checkbox input with | ||
| * `display: none`. The real fix belongs in | ||
| * `constructorio-ui-components/src/components/filter-option.tsx`, which renders the input | ||
| * with Tailwind's `hidden` class — that removes the checkbox from the accessibility tree | ||
| * and the keyboard focus order (WCAG 4.1.2). This rule is unlayered, so it wins over the | ||
| * shared sheet's `@layer utilities .hidden` regardless of specificity. | ||
| */ |
There was a problem hiding this comment.
@HHHindawy Can you add a story/issue for the components-ui to resolve this? Thanks!
Mudaafi
left a comment
Mudaafi
left a comment
There was a problem hiding this comment.
Added some comments, otherwise looks good to me
Co-authored-by: Ahmad Mudaafi' <ahmad.mudaafi@constructor.io>
There was a problem hiding this comment.
Code Review
This PR fixes significant accessibility gaps in Sort and Filter controls — hidden radio inputs, missing dialog roles, and silent filter/sort changes — using well-structured WCAG-compliant patterns including a live region, focus management, and visually-hidden inputs.
Inline comments: 6 discussions added
Overall Assessment: ✅ Pass
| // own names and ids. `useId` would be preferable, but React >= 16.12 is supported. | ||
| // Each mounted instance takes the lowest free index, and the first keeps the original | ||
| // un-suffixed ids and names. Assigned after mount so server and client markup match | ||
| const mountedInstances = new Set<number>(); |
There was a problem hiding this comment.
Important Issue: mountedInstances is a module-level singleton, which means it persists across test runs and Hot Module Replacement cycles. In tests, if a component is mounted and then the test suite is torn down without a clean unmount (e.g. an error or a test.only), the index leaks and subsequent instances get a non-zero suffix, breaking the #cio-plp-sort-panel selector that external code and the tests themselves rely on.
The comment acknowledges useId would be preferable and notes React >=16.12 as a constraint. However, the dev dependency already pins React 18, and the peerDependencies range could be narrowed to >=18 at the next major version. For now, consider at least resetting mountedInstances in a test-only afterEach hook, or documenting this limitation so future maintainers know to test with multiple mounted instances.
| 'input:not([disabled])', | ||
| 'select:not([disabled])', | ||
| 'textarea:not([disabled])', | ||
| '[tabindex]', |
There was a problem hiding this comment.
Suggestion: The FOCUSABLE_SELECTOR includes [tabindex] (without :not([tabindex="-1"])) in the array before the .map() appends it, so the final selector becomes [tabindex]:not([tabindex="-1"]). This is technically correct but non-obvious. The [tabindex] entry is intended to catch tabindex="0" elements (e.g. custom focusable components), but a reader might assume it matches all [tabindex] before the filter is applied. A small comment clarifying this intent would help, e.g. '[tabindex]', // catches tabindex=0; :not([tabindex="-1"]) is appended by the map below.
| setAnnouncement(''); | ||
| const timeout = setTimeout(() => setAnnouncement(nextMessage), ANNOUNCEMENT_DELAY_MS); | ||
| return () => clearTimeout(timeout); | ||
| }, [status, resultId, loadingMessage, resultsMessage]); |
There was a problem hiding this comment.
Suggestion: The dependency array includes loadingMessage and resultsMessage (strings derived from translations), which correctly prevents re-announcement when a new translations object reference has the same content. However, resultsMessage is computed unconditionally on every render even when status === RequestStatus.FETCHING, introducing a minor unnecessary computation. This is negligible in practice, but for clarity the derivation could be gated: const resultsMessage = status !== RequestStatus.FETCHING && totalNumResults !== undefined ? getResultsMessage(...) : ''. This is a purely stylistic suggestion.
| const originalWindowLocation = window.location; | ||
|
|
||
| beforeEach(() => { | ||
| window.location = 'https://example.com/search?q=shoes'; |
There was a problem hiding this comment.
Suggestion: Directly assigning a string to window.location is a non-standard pattern (the property is normally a Location object). While this works in jsdom, the pattern is inconsistent with other test files in this repo that use the same pattern and it can mask issues with code that calls window.location.href vs window.location.toString(). The established pattern in this codebase (e.g. Sort.test.jsx:18, CioPlpGrid.test.jsx:73) also uses direct string assignment, so this isn't a regression introduced here, but it's worth noting that Object.defineProperty(window, 'location', { value: new URL('...') }) or using jsdom's window.location.assign would be more idiomatic.
| // (e.g. blur while handling Escape) see the latest value | ||
| const [pendingSort, setPendingSortState] = useState<PlpSortOption | null>(null); | ||
| const pendingSortRef = useRef<PlpSortOption | null>(null); | ||
| const setPendingSort = (option: PlpSortOption | null) => { |
There was a problem hiding this comment.
Suggestion: setPendingSort is an inline function defined inside the component body without useCallback. It synchronously updates both a ref and state, so it is called from multiple event handlers and a useEffect. Since it's not passed to child components as a prop, there is no rendering cost, but for consistency with the isChecked callback (which is memoized) and to avoid subtle issues if it were ever passed down, wrapping it in useCallback([]) would be cleaner.
| // still be announced. | ||
| return ( | ||
| <> | ||
| <LiveRegion message={announcement} /> |
There was a problem hiding this comment.
Important Issue: The LiveRegion is rendered twice in the "results found" branch — once at the top level (line 159) inside AnnouncementContext.Provider, and once more inside MobileModal via AnnouncementContext. When the mobile modal is closed, only the top-level LiveRegion is present. When it is open, both the top-level one AND the one inside the modal are in the DOM simultaneously (with aria-modal hiding the outer one from AT).
This design is intentional and well-documented in the code comments. However, the outer LiveRegion (line 159) will still receive DOM updates even while the modal is open — it just won't be announced by AT because aria-modal hides it. Some older AT (particularly JAWS + IE) don't fully honour aria-modal, so a belts-and-suspenders approach would be to also clear or suppress the outer LiveRegion while the modal is open. This is a minor AT compatibility edge case, not a blocker.
Sort control not keyboard/AT accessible
.cio-plp-sort input { display: none }sat outside the media query and hid the radios from the accessibility tree and focus order at every viewport. Replaced with a visually-hidden pattern; the checked visual is unchanged since it is drawn by the adjacentspan:before.name, so the options never formed a radio group: no arrow-key navigation and each option toggled independently. Now one shared name per rendered group, wrapped inrole="radiogroup".aria-expandedandaria-controls.Filter changes are silent
LiveRegion(role="status",aria-live="polite"), mounted above the status branches so it exists in the DOM from first render for assistive announceruseResultsAnnouncement, so one announcement covers filter, sort and pagination changes. Initial load is silent to avoid talking over the screen reader's own page-load announcement.translationsprop. CDX-303 was not a blocker: it shipped 2026-06-22 and interpolation already happens at the call site, exactly as the existing result count header does.Also fixed, same defect class, not previously ticketed:
MobileModalwas missing roles/arias. Added the relevant tags and accessibility controlsPull Request Checklist
Before you submit a pull request, please make sure you have to following:
PR Type
What kind of change does this PR introduce?