fix: a11y baseline pass across components#805
Open
paanSinghCoder wants to merge 8 commits into
Open
Conversation
Address the cross-component a11y gaps tracked in #673 with one PR. Each change is targeted at the specific WCAG / ARIA issue called out in the child issues — no broader refactors. Component-level fixes - Tooltip: aria-label / aria-labelledby flow to Popup for ReactNode messages - TextArea: aria-invalid / aria-required propagated from Field context - Table: Table.Head defaults scope="col"; SectionHeader uses scope="colgroup"; new Table.Caption sub-component - Spinner: drop conflicting aria-hidden + status combo; add ariaLabel (default "Loading"); aria-hidden override cleanly demotes to decorative - Skeleton: aria-hidden="true" on decorative placeholder container - Sidebar: remove orphan role="listitem" (items rely on native element semantics) - SidePanel: title renders as <h2> with generated id, new titleId prop for aria-labelledby wiring - Drawer: aria-label / aria-labelledby customisable; new closeLabel prop (default "Close") - Separator: new `decorative` prop -> role="presentation" + aria-hidden - Select: remove conflicting aria-multiselectable on Combobox list (data-multiselectable retained for styling) - ScrollArea: aria-label / aria-labelledby apply role="region" to viewport - List: keep explicit role="list" (Safari drops implicit role when list-style:none); drop redundant role="listitem"; remove generic default aria-label="List"; new `level` prop on List.Header (default 3, was hardcoded) - Link: drop redundant role="link"; use children for aria-label only when string (no more "[object Object]") - Label: new requiredText + showRequiredIndicator props to balance the existing (optional) indicator - Input: aria-invalid / aria-required from Field context; leading / trailing icon wrappers marked aria-hidden - Image: drop redundant role="img" and aria-label={alt} (native alt is the accessible name) - IconButton: drop redundant aria-disabled; strengthen aria-label guidance in JSDoc - Container: no default role="region"; role applied only when an aria-label / aria-labelledby is supplied - Button: aria-busy when loading; internal spinner marked aria-hidden so the button speaks for itself - AnnouncementBar: action rendered as a real <button> with focus styles (was <Text onClick>); decorative icons aria-hidden Tests + docs - Existing component tests updated to assert the corrected a11y contract (1735 passing, +28 vs. main). - props.ts updated for every new prop (Spinner.ariaLabel, Drawer.closeLabel, Separator.decorative, List.Header.level, Label.requiredText / showRequiredIndicator, SidePanel.Header.titleId, Table.Head.scope + Table.Caption, Tooltip.Content aria-label / aria-labelledby). - Accessibility sections in index.mdx rewritten where the previous text claimed roles / attributes that are no longer emitted. Behavioural notes for downstream consumers - getByRole('listitem') will not find Sidebar items or List items. - getByRole('img') will not find decorative (alt="") images. - getByRole('region') will not find a Container without a label. - getByLabelText('Close Drawer') is now getByLabelText('Close') (override via closeLabel for older copy). - Test suites that depended on these strings will need a small update.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 WalkthroughWalkthroughThis pull request establishes an accessibility baseline across the component library by systematically clarifying semantic HTML rendering, standardizing ARIA attribute patterns, adding new accessible configuration props, and updating tests. Changes span documentation updates explaining accessible behaviors, new optional props like Possibly related issues
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Document the PR #805 accessibility changes in V1-migration.md under Cross-Cutting Changes so consumers upgrading to v1.0 see the full set of test / CSS / i18n migration patterns alongside the per-component notes.
Apsara components consistently expose the native `aria-label` attribute rather than a camelCase `ariaLabel` prop (see Chip, IconButton, Tooltip, Drawer, ScrollArea, etc.). Spinner was the lone exception in the a11y baseline pass — drop the alias and consume `aria-label` directly with "Loading" as the default. Docs and migration notes updated.
6 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/raystack/components/side-panel/side-panel.tsx (1)
23-29:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe
<aside>landmark should be labeled by the heading ID.While the header now generates an ID for the
<h2>title (Line 56), the<aside>element rendered bySidePanelRootdoesn't reference it viaaria-labelledby. According to WCAG, landmark regions should have accessible names to help users navigate.The
SidePanelRootandSidePanelHeaderare separate components, so you'll need to:
- Pass the heading ID from Header up to Root (via context or composition), or
- Accept
aria-labelledbyas a prop onSidePanelRootand require consumers to pass the heading IDWithout this connection, screen reader users cannot distinguish between multiple side panels on the same page.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/side-panel/side-panel.tsx` around lines 23 - 29, SidePanelRoot currently renders the <aside> without an accessible name; update SidePanelProps to accept an optional ariaLabelledBy (string) prop and set it on the aside as aria-labelledby={ariaLabelledBy} inside SidePanelRoot, and ensure consumers pass the heading ID generated by SidePanelHeader (or lift the heading ID via context if preferred); reference the SidePanelRoot and SidePanelHeader symbols and ensure the header exposes its generated id so it can be forwarded to SidePanelRoot via the new ariaLabelledBy prop (or via shared context).
🧹 Nitpick comments (4)
packages/raystack/components/label/label.tsx (1)
43-45: ⚡ Quick winConsider renaming or using a more generic CSS class.
The
requiredTextindicator reusesstyles.optional, which is semantically mismatched. While this may be intentional for consistent styling, consider either:
- Renaming the CSS class to something generic like
styles.indicatororstyles.labelSuffix- Creating a separate
styles.requiredclass if the styling should differ in the future🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/label/label.tsx` around lines 43 - 45, The JSX uses styles.optional for the requiredText span which is semantically incorrect; update the component (in label.tsx around the span that renders requiredText) to use a generic or semantically appropriate class name—either rename styles.optional to styles.indicator or styles.labelSuffix (and update the CSS module accordingly) or add a new styles.required class in the CSS module and use that on the span so the class name matches the intent and future styling changes are clearer.apps/www/src/content/docs/components/label/index.mdx (1)
57-57: 💤 Low valueConsider hyphenating compound modifier.
The phrase "WCAG compliant color contrast" should use a hyphen: "WCAG-compliant color contrast" per standard English grammar for compound modifiers.
📝 Suggested grammar fix
-- Maintains WCAG compliant color contrast ratios. +- Maintains WCAG-compliant color contrast ratios.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/www/src/content/docs/components/label/index.mdx` at line 57, Update the phrase "Maintains WCAG compliant color contrast ratios." to hyphenate the compound modifier so it reads "Maintains WCAG-compliant color contrast ratios." Locate and edit the exact string "Maintains WCAG compliant color contrast ratios." in the label docs (the line containing that sentence) and replace it with the hyphenated version to follow standard English grammar for compound modifiers.packages/raystack/components/spinner/__tests__/spinner.test.tsx (1)
10-10: ⚡ Quick winRemove
{ hidden: true }from role queries for consistency.The Spinner component no longer sets
aria-hidden="true"by default (confirmed by the updated accessibility test on lines 50-55). AllgetByRole('status')queries should work without the{ hidden: true }option for consistency with the new default behavior.♻️ Proposed fix to update query pattern
- const spinner = screen.getByRole('status', { hidden: true }); + const spinner = screen.getByRole('status');Apply this change to lines 10, 21, 27, 34, 41, 77, and 89.
Also applies to: 21-21, 27-27, 34-34, 41-41, 77-77, 89-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/spinner/__tests__/spinner.test.tsx` at line 10, Tests in spinner.test.tsx still use getByRole('status', { hidden: true }); update all role queries to remove the hidden option so they read getByRole('status') for consistency with the Spinner's new default (apply to the occurrences currently using { hidden: true } - e.g., the spinner variable assignments and other getByRole('status', { hidden: true }) calls found in this test file).packages/raystack/components/drawer/drawer-content.tsx (1)
41-41: ⚡ Quick winAdd
closeLabelto the exported props interface for API consistency.
closeLabelis part of the component API but not part ofDrawerContentProps, which can cause typing drift for consumers reusing the exported type.Suggested fix
export interface DrawerContentProps extends Omit<DrawerPrimitive.Popup.Props, 'children'>, VariantProps<typeof drawerPopup> { showCloseButton?: boolean; overlayProps?: DrawerPrimitive.Backdrop.Props; children?: ReactNode; + closeLabel?: string; } @@ -}: DrawerContentProps & { closeLabel?: string }) { +}: DrawerContentProps) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/raystack/components/drawer/drawer-content.tsx` at line 41, The DrawerContent component accepts a closeLabel prop but the exported type DrawerContentProps doesn't include it; update the exported DrawerContentProps interface to add closeLabel?: string so consumers typing against DrawerContentProps see the prop, and ensure any references to DrawerContentProps (and the component signature where closeLabel is destructured) remain compatible after the addition.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/V1-migration.md`:
- Around line 274-289: The document contains contradictory guidance for the
Separator component: the summary table claims Separator has a new decorative
prop while the detailed “### Separator” section later says decorative was
removed—pick the correct API behavior and make both sections consistent. If the
prop was added: update the “### Separator” section to document the new
decorative prop on the Separator component, show usage examples, and state that
decorative -> role="presentation" + aria-hidden are applied; if the prop was
removed: update the summary table to remove the decorative entry and adjust any
examples. Ensure all references to Separator, decorative, role="presentation",
and aria-hidden are aligned and update the migration notes and examples
accordingly.
In `@packages/raystack/components/announcement-bar/announcement-bar.module.css`:
- Line 56: Change the CSS keyword value to lowercase to satisfy the stylelint
value-keyword-case rule: in announcement-bar.module.css replace the outline
property value "currentColor" with "currentcolor" (the rule applies where
outline: 2px solid currentColor; is defined) so the keyword is all lowercase and
the lint violation is resolved.
In `@packages/raystack/components/announcement-bar/announcement-bar.tsx`:
- Around line 58-69: The action button currently renders when actionIcon exists
even if actionLabel is missing, creating an unlabeled button; update the render
condition in the AnnouncementBar component so the button is only rendered when
actionLabel is present (require actionLabel truthy) and remove/avoid rendering
an icon-only button; locate the JSX that checks {actionLabel || actionIcon ?
(...) : null} and change it to require actionLabel (use actionLabel) for
rendering the <button> (keeping onActionClick, styles['action-btn'], actionIcon
handling inside) so the button always has an accessible label.
In `@packages/raystack/components/separator/separator.tsx`:
- Around line 44-56: The decorative semantics can be overridden because props
are spread after decorativeProps; in the Separator component ensure
decorativeProps win by applying them after consumer props (or explicitly set
role and aria-hidden on the final props sent to SeparatorPrimitive). Update the
render so SeparatorPrimitive receives {...props} first and then decorativeProps
(or assign role:'presentation' and aria-hidden:true after spreading props) so
that when decorative is true the role/aria-orientation/aria-hidden cannot be
replaced by incoming props.
In `@packages/raystack/components/spinner/spinner.tsx`:
- Around line 48-55: The component's computed accessibility attributes (role,
aria-label, aria-live, aria-hidden) are being overridden by user-supplied props
because {...props} is spread last; to make decorative mode authoritative, spread
{...props} before the computed attributes in the Spinner component (the JSX
block that builds the <div className={spinner({ size, color, className })}
...>), or explicitly derive final attributes by taking props but forcing
role/aria-label/aria-live/aria-hidden from the computed values when isDecorative
is true so passed props cannot override them.
---
Outside diff comments:
In `@packages/raystack/components/side-panel/side-panel.tsx`:
- Around line 23-29: SidePanelRoot currently renders the <aside> without an
accessible name; update SidePanelProps to accept an optional ariaLabelledBy
(string) prop and set it on the aside as aria-labelledby={ariaLabelledBy} inside
SidePanelRoot, and ensure consumers pass the heading ID generated by
SidePanelHeader (or lift the heading ID via context if preferred); reference the
SidePanelRoot and SidePanelHeader symbols and ensure the header exposes its
generated id so it can be forwarded to SidePanelRoot via the new ariaLabelledBy
prop (or via shared context).
---
Nitpick comments:
In `@apps/www/src/content/docs/components/label/index.mdx`:
- Line 57: Update the phrase "Maintains WCAG compliant color contrast ratios."
to hyphenate the compound modifier so it reads "Maintains WCAG-compliant color
contrast ratios." Locate and edit the exact string "Maintains WCAG compliant
color contrast ratios." in the label docs (the line containing that sentence)
and replace it with the hyphenated version to follow standard English grammar
for compound modifiers.
In `@packages/raystack/components/drawer/drawer-content.tsx`:
- Line 41: The DrawerContent component accepts a closeLabel prop but the
exported type DrawerContentProps doesn't include it; update the exported
DrawerContentProps interface to add closeLabel?: string so consumers typing
against DrawerContentProps see the prop, and ensure any references to
DrawerContentProps (and the component signature where closeLabel is
destructured) remain compatible after the addition.
In `@packages/raystack/components/label/label.tsx`:
- Around line 43-45: The JSX uses styles.optional for the requiredText span
which is semantically incorrect; update the component (in label.tsx around the
span that renders requiredText) to use a generic or semantically appropriate
class name—either rename styles.optional to styles.indicator or
styles.labelSuffix (and update the CSS module accordingly) or add a new
styles.required class in the CSS module and use that on the span so the class
name matches the intent and future styling changes are clearer.
In `@packages/raystack/components/spinner/__tests__/spinner.test.tsx`:
- Line 10: Tests in spinner.test.tsx still use getByRole('status', { hidden:
true }); update all role queries to remove the hidden option so they read
getByRole('status') for consistency with the Spinner's new default (apply to the
occurrences currently using { hidden: true } - e.g., the spinner variable
assignments and other getByRole('status', { hidden: true }) calls found in this
test file).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c941fa5c-3756-4e3e-9aa9-75fde3e687e1
📒 Files selected for processing (73)
apps/www/src/content/docs/components/announcement-bar/index.mdxapps/www/src/content/docs/components/button/index.mdxapps/www/src/content/docs/components/container/index.mdxapps/www/src/content/docs/components/drawer/index.mdxapps/www/src/content/docs/components/drawer/props.tsapps/www/src/content/docs/components/image/index.mdxapps/www/src/content/docs/components/label/index.mdxapps/www/src/content/docs/components/label/props.tsapps/www/src/content/docs/components/link/index.mdxapps/www/src/content/docs/components/list/index.mdxapps/www/src/content/docs/components/list/props.tsapps/www/src/content/docs/components/scroll-area/index.mdxapps/www/src/content/docs/components/separator/index.mdxapps/www/src/content/docs/components/separator/props.tsapps/www/src/content/docs/components/sidepanel/index.mdxapps/www/src/content/docs/components/sidepanel/props.tsapps/www/src/content/docs/components/skeleton/index.mdxapps/www/src/content/docs/components/spinner/index.mdxapps/www/src/content/docs/components/spinner/props.tsapps/www/src/content/docs/components/table/index.mdxapps/www/src/content/docs/components/table/props.tsapps/www/src/content/docs/components/tooltip/index.mdxapps/www/src/content/docs/components/tooltip/props.tsdocs/V1-migration.mdpackages/raystack/components/announcement-bar/__tests__/announcement-bar.test.tsxpackages/raystack/components/announcement-bar/announcement-bar.module.csspackages/raystack/components/announcement-bar/announcement-bar.tsxpackages/raystack/components/announcement-bar/index.tsxpackages/raystack/components/button/__tests__/button.test.tsxpackages/raystack/components/button/button.module.csspackages/raystack/components/button/button.tsxpackages/raystack/components/button/index.tsxpackages/raystack/components/container/__tests__/container.test.tsxpackages/raystack/components/container/container.module.csspackages/raystack/components/container/container.tsxpackages/raystack/components/container/index.tsxpackages/raystack/components/drawer/__tests__/drawer.test.tsxpackages/raystack/components/drawer/drawer-content.tsxpackages/raystack/components/icon-button/__tests__/icon-button.test.tsxpackages/raystack/components/icon-button/icon-button.module.csspackages/raystack/components/icon-button/icon-button.tsxpackages/raystack/components/icon-button/index.tsxpackages/raystack/components/image/__tests__/image.test.tsxpackages/raystack/components/image/image.module.csspackages/raystack/components/image/image.tsxpackages/raystack/components/image/index.tsxpackages/raystack/components/input/input.tsxpackages/raystack/components/label/label.tsxpackages/raystack/components/link/__tests__/link.test.tsxpackages/raystack/components/link/index.tsxpackages/raystack/components/link/link.tsxpackages/raystack/components/list/__tests__/list.test.tsxpackages/raystack/components/list/index.tsxpackages/raystack/components/list/list.tsxpackages/raystack/components/scroll-area/scroll-area.tsxpackages/raystack/components/select/index.tspackages/raystack/components/select/select-content.tsxpackages/raystack/components/separator/index.tsxpackages/raystack/components/separator/separator.module.csspackages/raystack/components/separator/separator.tsxpackages/raystack/components/side-panel/index.tsxpackages/raystack/components/side-panel/side-panel.tsxpackages/raystack/components/skeleton/index.tspackages/raystack/components/skeleton/skeleton.module.csspackages/raystack/components/skeleton/skeleton.tsxpackages/raystack/components/spinner/__tests__/spinner.test.tsxpackages/raystack/components/spinner/index.tsxpackages/raystack/components/spinner/spinner.tsxpackages/raystack/components/table/index.tspackages/raystack/components/table/table.tsxpackages/raystack/components/text-area/text-area.tsxpackages/raystack/components/tooltip/tooltip-content.tsxpackages/raystack/components/tooltip/tooltip.module.css
💤 Files with no reviewable changes (1)
- packages/raystack/components/image/image.tsx
| } | ||
|
|
||
| .action-btn:focus-visible { | ||
| outline: 2px solid currentColor; |
Contributor
There was a problem hiding this comment.
Fix Stylelint keyword-case violation on Line 56.
currentColor should be currentcolor to satisfy the configured value-keyword-case rule.
Proposed fix
-.action-btn:focus-visible {
- outline: 2px solid currentColor;
+.action-btn:focus-visible {
+ outline: 2px solid currentcolor;
outline-offset: 2px;
border-radius: var(--rs-radius-1);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| outline: 2px solid currentColor; | |
| .action-btn:focus-visible { | |
| outline: 2px solid currentcolor; | |
| outline-offset: 2px; | |
| border-radius: var(--rs-radius-1); | |
| } |
🧰 Tools
🪛 Stylelint (17.11.0)
[error] 56-56: Expected "currentColor" to be "currentcolor" (value-keyword-case)
(value-keyword-case)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/announcement-bar/announcement-bar.module.css` at
line 56, Change the CSS keyword value to lowercase to satisfy the stylelint
value-keyword-case rule: in announcement-bar.module.css replace the outline
property value "currentColor" with "currentcolor" (the rule applies where
outline: 2px solid currentColor; is defined) so the keyword is all lowercase and
the lint violation is resolved.
Comment on lines
58
to
69
| {actionLabel || actionIcon ? ( | ||
| <Text | ||
| <button | ||
| type='button' | ||
| className={styles['action-btn']} | ||
| size='small' | ||
| weight='medium' | ||
| onClick={onActionClick} | ||
| > | ||
| {actionLabel} | ||
| {actionIcon} | ||
| </Text> | ||
| <Text size='small' weight='medium'> | ||
| {actionLabel} | ||
| </Text> | ||
| {actionIcon && <span aria-hidden='true'>{actionIcon}</span>} | ||
| </button> | ||
| ) : null} |
Contributor
There was a problem hiding this comment.
Prevent rendering an unlabeled action button.
The current condition allows icon-only actions, but the icon is hidden from AT and no accessible name is provided for the <button>.
Suggested fix (require label for button render)
- {actionLabel || actionIcon ? (
+ {actionLabel ? (
<button
type='button'
className={styles['action-btn']}
onClick={onActionClick}
>
<Text size='small' weight='medium'>
{actionLabel}
</Text>
{actionIcon && <span aria-hidden='true'>{actionIcon}</span>}
</button>
) : null}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/announcement-bar/announcement-bar.tsx` around
lines 58 - 69, The action button currently renders when actionIcon exists even
if actionLabel is missing, creating an unlabeled button; update the render
condition in the AnnouncementBar component so the button is only rendered when
actionLabel is present (require actionLabel truthy) and remove/avoid rendering
an icon-only button; locate the JSX that checks {actionLabel || actionIcon ?
(...) : null} and change it to require actionLabel (use actionLabel) for
rendering the <button> (keeping onActionClick, styles['action-btn'], actionIcon
handling inside) so the button always has an accessible label.
Comment on lines
+44
to
56
| const decorativeProps = decorative | ||
| ? { | ||
| role: 'presentation', | ||
| 'aria-hidden': true, | ||
| 'aria-orientation': undefined | ||
| } | ||
| : {}; | ||
| return ( | ||
| <SeparatorPrimitive | ||
| orientation={orientation} | ||
| className={separator({ size, color, className })} | ||
| {...decorativeProps} | ||
| {...props} |
Contributor
There was a problem hiding this comment.
Ensure decorative semantics cannot be overridden by passthrough ARIA props.
When decorative is true, spreading ...props after decorativeProps lets consumers reintroduce conflicting role/aria-* values.
Suggested fix
<SeparatorPrimitive
orientation={orientation}
className={separator({ size, color, className })}
- {...decorativeProps}
{...props}
+ {...decorativeProps}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const decorativeProps = decorative | |
| ? { | |
| role: 'presentation', | |
| 'aria-hidden': true, | |
| 'aria-orientation': undefined | |
| } | |
| : {}; | |
| return ( | |
| <SeparatorPrimitive | |
| orientation={orientation} | |
| className={separator({ size, color, className })} | |
| {...decorativeProps} | |
| {...props} | |
| return ( | |
| <SeparatorPrimitive | |
| orientation={orientation} | |
| className={separator({ size, color, className })} | |
| {...props} | |
| {...decorativeProps} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/separator/separator.tsx` around lines 44 - 56,
The decorative semantics can be overridden because props are spread after
decorativeProps; in the Separator component ensure decorativeProps win by
applying them after consumer props (or explicitly set role and aria-hidden on
the final props sent to SeparatorPrimitive). Update the render so
SeparatorPrimitive receives {...props} first and then decorativeProps (or assign
role:'presentation' and aria-hidden:true after spreading props) so that when
decorative is true the role/aria-orientation/aria-hidden cannot be replaced by
incoming props.
Comment on lines
48
to
55
| <div | ||
| className={spinner({ size, color, className })} | ||
| role='status' | ||
| aria-hidden='true' | ||
| role={isDecorative ? undefined : 'status'} | ||
| aria-label={isDecorative ? undefined : ariaLabel} | ||
| aria-live={isDecorative ? undefined : 'polite'} | ||
| aria-hidden={isDecorative || undefined} | ||
| {...props} | ||
| > |
Contributor
There was a problem hiding this comment.
Keep decorative mode authoritative when aria-hidden is set.
...props currently comes after computed accessibility props, so a passed role/aria-live can override decorative behavior.
Suggested fix
<div
+ {...props}
className={spinner({ size, color, className })}
role={isDecorative ? undefined : 'status'}
aria-label={isDecorative ? undefined : ariaLabel}
aria-live={isDecorative ? undefined : 'polite'}
aria-hidden={isDecorative || undefined}
- {...props}
>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <div | |
| className={spinner({ size, color, className })} | |
| role='status' | |
| aria-hidden='true' | |
| role={isDecorative ? undefined : 'status'} | |
| aria-label={isDecorative ? undefined : ariaLabel} | |
| aria-live={isDecorative ? undefined : 'polite'} | |
| aria-hidden={isDecorative || undefined} | |
| {...props} | |
| > | |
| <div | |
| {...props} | |
| className={spinner({ size, color, className })} | |
| role={isDecorative ? undefined : 'status'} | |
| aria-label={isDecorative ? undefined : ariaLabel} | |
| aria-live={isDecorative ? undefined : 'polite'} | |
| aria-hidden={isDecorative || undefined} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/raystack/components/spinner/spinner.tsx` around lines 48 - 55, The
component's computed accessibility attributes (role, aria-label, aria-live,
aria-hidden) are being overridden by user-supplied props because {...props} is
spread last; to make decorative mode authoritative, spread {...props} before the
computed attributes in the Spinner component (the JSX block that builds the <div
className={spinner({ size, color, className })} ...>), or explicitly derive
final attributes by taking props but forcing
role/aria-label/aria-live/aria-hidden from the computed values when isDecorative
is true so passed props cannot override them.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Related issue #673
Component changes (21 source files):
aria-label/aria-labelledbyflow to Popup so ReactNode messages get an accessible namearia-invalid/aria-requiredpropagated from Field contextTable.Headdefaultsscope=\"col\"; SectionHeader usesscope=\"colgroup\"; newTable.Captionaria-hidden+role=\"status\"; addariaLabel(default\"Loading\");aria-hiddenoverride demotes to decorative cleanlyaria-hidden=\"true\"on decorative placeholder containerrole=\"listitem\"<h2>with generated id; newtitleIdprop foraria-labelledbyaria-label/aria-labelledbycustomisable; newcloseLabelprop (default\"Close\")decorativeprop →role=\"presentation\"+aria-hiddenaria-multiselectableon Combobox list (data-multiselectableretained for styling)aria-label/aria-labelledbyapplyrole=\"region\"to viewportrole=\"list\"(Safari drops the implicit role underlist-style: none); drop redundantrole=\"listitem\"; remove generic defaultaria-label=\"List\"; newlevelprop onList.Headerrole=\"link\"; usechildrenfor aria-label only when it's a stringrequiredText+showRequiredIndicatorprops to balance the existing(optional)indicatoraria-invalid/aria-requiredfrom Field context; leading / trailing icon wrappersaria-hiddenrole=\"img\"andaria-label={alt}(nativealtis the accessible name)aria-disabled; strengthenaria-labelguidance in JSDocrole=\"region\"; role applied only when anaria-label/aria-labelledbyis suppliedaria-busywhenloading; internal spinner markedaria-hiddenso the button speaks for itself<button>with focus styles (was<Text onClick>); decorative iconsaria-hiddenTests + docs:
apps/wwwprops.tsupdated for every new prop.apps/wwwAccessibility sections rewritten where the previous text claimed roles / attributes that are no longer emitted.Behavioural changes downstream callers should know about
Bumping to this version is mostly a drop-in, but a few queries / assertions need updating:
getByRole('listitem')no longer finds Sidebar or List items — they rely on native element semantics now.getByRole('img')does not find decorative images (alt=\"\").getByRole('region')no longer finds aContainerunless anaria-label/aria-labelledbyis supplied.getByLabelText('Close Drawer')is nowgetByLabelText('Close')(override via thecloseLabelprop if the old copy is needed).getByRole('status')to detect a Button's loading state should switch toexpect(button).toHaveAttribute('aria-busy', 'true').Test plan
pnpm test:apsara— 1735 / 1735 passing locallypnpm --filter @raystack/apsara build— cleanSidePanel.Header(now<h2>— confirm default heading margins don't shift layout)AnnouncementBaraction (now<button>with reset CSS + focus ring — confirm rendering is unchanged)aria-multiselectabledoesn't regress AT announcementsCloses #673.