UI: Fix trivial RefBlocks ARIA violations#33026
Conversation
Sidnioulz
added
bug
accessibility
ci:normal
a11y: aria
📝 WalkthroughWalkthroughAdds an Changes
Sequence Diagram(s)No sequence diagram — changes are prop/semantic/typing updates and do not alter control flow. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Possibly related issues
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,json,html,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/!(*.test).{ts,tsx,js,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (3)📓 Common learnings📚 Learning: 2025-11-05T09:36:55.944ZApplied to files:
📚 Learning: 2025-11-05T09:38:47.712ZApplied to files:
🧬 Code graph analysis (1)code/core/src/components/components/typography/link/link.tsx (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (3)
Comment |
|
View your CI Pipeline Execution ↗ for commit b75ad9d
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
force-pushed
the
a11y/fix-basic-refblocks-aria-issues
branch
2 times, most recently
from
4fb9eb6 to
062b955
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/core/src/components/components/typography/link/link.tsx (2)
22-28:isButtonprop wiring and styling look coherentThe addition of
isButtontoLinkStylesPropsand the corresponding style variant onAare consistent and non‑breaking (default isfalse), and the visual reset to a text‑like button is appropriate for a link-acting-as-button pattern.If you expect
isButtonto be used frequently, consider documenting it alongsideLinkin your docs/Storybook stories so consumers know to preferisButtonover manually settingrole="button".Also applies to: 168-177
188-209: Add Space key activation to achieve full ARIA button semanticsThe Link component with
role="button"lacks Space key handling. Browser behavior for<a>elements activates on Enter (native link behavior) but not Space—ARIArole="button"elements should respond to both per the ARIA Authoring Practices Guide. Current usages in the codebase (Description.tsx, RefBlocks.tsx) rely on click/Enter, but adding Space support completes the keyboard accessibility contract.Consider adding an
onKeyDownhandler to trigger the sameonClicklogic when Space is pressed and preventing default to avoid page scroll. Optionally, document thatisButtonusages should omithrefor consider atabIndex={0}fallback if isButton links ever appear outside normal tab flow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/components/components/typography/link/link.tsx(1 hunks)code/core/src/manager/components/sidebar/RefIndicator.tsx(2 hunks)code/core/src/manager/components/sidebar/Refs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- code/core/src/manager/components/sidebar/Refs.tsx
- code/core/src/manager/components/sidebar/RefIndicator.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/components/components/typography/link/link.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/components/components/typography/link/link.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/typography/link/link.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/typography/link/link.tsx (1)
code/core/src/components/index.ts (1)
A(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/components/components/typography/link/link.tsx (1)
188-200:rolehandling may unintentionally override consumer-providedroleBecause
role={isButton ? 'button' : undefined}is always passed after{...rest}, anyroleprovided by the caller is effectively ignored, and whenisButtonisfalsethe explicitundefinedwill clear a caller’srole.
[submit_recommended_refactor]
A safer pattern is to let explicit consumer intent win when present, e.g.:-export const Link = forwardRef<HTMLAnchorElement, LinkProps>( - ( - { - cancel = true, - children, - onClick = undefined, - withArrow = false, - containsIcon = false, - className = undefined, - style = undefined, - isButton = false, - ...rest - }, - ref - ) => ( +export const Link = forwardRef<HTMLAnchorElement, LinkProps>( + ( + { + cancel = true, + children, + onClick = undefined, + withArrow = false, + containsIcon = false, + className = undefined, + style = undefined, + isButton = false, + role, + ...rest + }, + ref + ) => ( <A - {...rest} + {...rest} ref={ref} isButton={isButton} - role={isButton ? 'button' : undefined} + role={isButton ? 'button' : role} onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} className={className} >This preserves existing behavior while still enforcing
role="button"whenisButtonis true, but allows advanced consumers to override if needed.Please double-check against your intended public API: if
Linkis meant to own all role semantics, you may decide to keep the existing behavior.Also applies to: 203-209
Sidnioulz
force-pushed
the
a11y/fix-basic-refblocks-aria-issues
branch
from
062b955 to
918de80
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/core/src/components/components/typography/link/link.tsx (1)
198-217: Consider adding Space key handler whenrole="button"is set.The addition of
role="button"correctly addresses the ARIA violations mentioned in the PR objectives. However, when an element hasrole="button", users expect it to respond to the Space key in addition to Enter. Currently, the component only handles click events.For complete button semantics, consider adding a keyboard event handler:
) => ( <A {...rest} ref={ref} isButton={isButton} role={isButton ? 'button' : undefined} onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} + onKeyDown={ + isButton && onClick + ? (e) => { + if (e.key === ' ' || e.key === 'Spacebar') { + e.preventDefault(); + if (cancel) { + cancelled(e as any, onClick); + } else { + onClick(e as any); + } + } + } + : undefined + } className={className} >Note: This enhancement can be deferred if the current implementation already meets your accessibility requirements.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
code/core/src/components/components/typography/link/link.tsx(1 hunks)code/core/src/manager/components/sidebar/RefIndicator.tsx(2 hunks)code/core/src/manager/components/sidebar/Refs.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/core/src/manager/components/sidebar/RefIndicator.tsx
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/manager/components/sidebar/Refs.tsxcode/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/manager/components/sidebar/Refs.tsxcode/core/src/components/components/typography/link/link.tsx
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/manager/components/sidebar/Refs.tsxcode/core/src/components/components/typography/link/link.tsx
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/manager/components/sidebar/Refs.tsxcode/core/src/components/components/typography/link/link.tsx
🧠 Learnings (2)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-11-05T09:36:55.944Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
Applied to files:
code/core/src/components/components/typography/link/link.tsx
🧬 Code graph analysis (1)
code/core/src/components/components/typography/link/link.tsx (1)
code/core/src/components/index.ts (1)
A(7-7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (1)
code/core/src/manager/components/sidebar/Refs.tsx (1)
97-97: LGTM!Removing the explicit
HTMLElementtype annotation allows TypeScript to correctly infer the type asHTMLDivElementbased on the updatedRefIndicatorcomponent's ref type. This aligns with the change fromasidetodivin RefIndicator.
Sidnioulz
force-pushed
the
a11y/fix-basic-refblocks-aria-issues
branch
from
918de80 to
b75ad9d
Compare
2 participants
What I did
Incorrect use of aside: https://www.chromatic.com/test?appId=635781f3500dd2c49e189caf&id=6913769e99ba81e2674e5463
Error link missing button role: https://635781f3500dd2c49e189caf-siuwlkquds.chromatic.com/?path=/story/manager-sidebar-refs--errored
Tip
If I'm correct, adding the button role to
isButtonlinks might actually fix more a11y violations in Chromatic.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
ø
Documentation
ø
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
New Features
Accessibility
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.