UI: Ensure Link without href is keyboard-reachable#34163
Conversation
Sidnioulz
added
bug
accessibility
ci:normal
a11y: keyboard
|
View your CI Pipeline Execution ↗ for commit fda8f06
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughDeprecates Link's Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing Touches📝 Generate docstrings
📝 Coding Plan
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
code/core/src/components/components/typography/link/link.tsx (1)
197-211: UpdateforwardReftype to reflect polymorphic rendering.The
Linkcomponent renders as either<a>or<button>depending on thehrefprop (line 220), but theforwardReftype is fixed toHTMLAnchorElement. Update the type to union both element types:Suggested patch
-export const Link = forwardRef<HTMLAnchorElement, LinkProps>( +export const Link = forwardRef<HTMLAnchorElement | HTMLButtonElement, LinkProps>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/typography/link/link.tsx` around lines 197 - 211, The forwardRef typing for the Link component is wrong because the component can render as an anchor or a button; update the forwardRef generic to accept a union of HTMLAnchorElement | HTMLButtonElement instead of only HTMLAnchorElement so the ref type matches both rendering paths (change the signature on forwardRef used for the Link component and any related LinkProps that reference the ref type). Ensure the change references the Link function and the forwardRef call so consumers using href (anchor) or button rendering get the correct element type for refs.code/core/src/components/components/typography/link/link.stories.tsx (1)
56-69: Good test coverage for the accessibility fix.The story correctly validates that a
Linkwithouthrefrenders as a button and is keyboard-reachable. UsinggetByRole('button')confirms the correct semantic element is rendered.Consider using
userEventfrom@storybook/testfor more realistic interaction testing that better simulates user behavior:♻️ Optional: Use userEvent for more realistic interactions
+import { expect, fn, userEvent } from 'storybook/test'; -import { expect, fn } from 'storybook/test'; export const ButtonLink = { args: { children: 'Click me', onClick: fn(), }, name: 'Link-styled button', play: async ({ args, canvas }) => { const link = canvas.getByRole('button', { name: 'Click me' }); - link.focus(); + await userEvent.tab(); expect(link).toHaveFocus(); - link.click(); + await userEvent.click(link); expect(args.onClick).toHaveBeenCalled(); }, } satisfies Story;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/typography/link/link.stories.tsx` around lines 56 - 69, Replace the direct DOM click/focus calls in the ButtonLink story play function with the userEvent helpers to simulate real user interactions: add an import for userEvent from '@storybook/test', then in the play function for ButtonLink use await userEvent.focus(link) (or await userEvent.tab() if you want keyboard navigation) instead of link.focus(), and replace link.click() with await userEvent.click(link); keep the existing assertions (expect(link).toHaveFocus(), expect(args.onClick).toHaveBeenCalled()) and update the async flow accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/components/components/typography/link/link.tsx`:
- Around line 219-225: The Link component renders a native <button> when href is
not provided, so add an explicit type="button" for that path to avoid implicit
type="submit"; update the A element props (the JSX line that sets as={href ? 'a'
: 'button'} and isButton={!href || isButton === true}) to include type={href ?
undefined : 'button'} (or type="button" when as is 'button'), ensuring the prop
is passed alongside ref, onClick, isButton, cancelled, cancel and {...rest}.
In `@MIGRATION.md`:
- Line 684: The migration note text uses incorrect article "a" before "HTML
`button`"; update the sentence starting "Link now renders automatically as a
HTML `button`..." to use "an HTML `button`" instead, leaving the rest of the
sentence and references to the `isButton` prop and Storybook 10/11 behavior
unchanged.
---
Nitpick comments:
In `@code/core/src/components/components/typography/link/link.stories.tsx`:
- Around line 56-69: Replace the direct DOM click/focus calls in the ButtonLink
story play function with the userEvent helpers to simulate real user
interactions: add an import for userEvent from '@storybook/test', then in the
play function for ButtonLink use await userEvent.focus(link) (or await
userEvent.tab() if you want keyboard navigation) instead of link.focus(), and
replace link.click() with await userEvent.click(link); keep the existing
assertions (expect(link).toHaveFocus(), expect(args.onClick).toHaveBeenCalled())
and update the async flow accordingly.
In `@code/core/src/components/components/typography/link/link.tsx`:
- Around line 197-211: The forwardRef typing for the Link component is wrong
because the component can render as an anchor or a button; update the forwardRef
generic to accept a union of HTMLAnchorElement | HTMLButtonElement instead of
only HTMLAnchorElement so the ref type matches both rendering paths (change the
signature on forwardRef used for the Link component and any related LinkProps
that reference the ref type). Ensure the change references the Link function and
the forwardRef call so consumers using href (anchor) or button rendering get the
correct element type for refs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4ebf4b0e-5b4c-4570-8bc9-faf072159a9d
📒 Files selected for processing (5)
MIGRATION.mdcode/addons/vitest/src/components/Description.tsxcode/core/src/components/components/typography/link/link.stories.tsxcode/core/src/components/components/typography/link/link.tsxcode/core/src/manager/components/sidebar/RefBlocks.tsx
| <A | ||
| as={href ? 'a' : 'button'} | ||
| href={href} | ||
| {...rest} | ||
| ref={ref} | ||
| isButton={!href || isButton === true} | ||
| onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the exact file
find code -name "link.tsx" -path "*/typography/*" | head -20Repository: storybookjs/storybook
Length of output: 125
🏁 Script executed:
# Get the file size and content around the specified lines
wc -l code/core/src/components/components/typography/link/link.tsxRepository: storybookjs/storybook
Length of output: 129
🏁 Script executed:
# Read the file to see the full context
cat -n code/core/src/components/components/typography/link/link.tsxRepository: storybookjs/storybook
Length of output: 7286
🏁 Script executed:
# Search for Link component usage with form elements
rg -A 5 -B 5 "Link\b.*href=\{false\}|Link\b.*href=\{undefined\}|as=.*button" code/core/src --type tsx --type ts | head -100Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Check if Link is used in form contexts
rg "<form" code/core/src --type tsx --type ts -l | head -10Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Look for any existing patterns where Link might be used without href
rg "<Link" code/core/src --type tsx --type ts -A 2 | grep -E "(Link|href|onClick)" | head -50Repository: storybookjs/storybook
Length of output: 92
🏁 Script executed:
# Search for Link component without href to understand actual usage patterns
rg "<Link" code/core/src -A 3 | grep -E "(Link|href|onClick|as=)" | head -60Repository: storybookjs/storybook
Length of output: 5586
🏁 Script executed:
# Check if there are any form contexts where Link is used
rg -B 5 -A 5 "as=.*button" code/core/src | head -80Repository: storybookjs/storybook
Length of output: 987
🏁 Script executed:
# Let's verify the A styled component's prop handling
ast-grep --pattern 'const A = styled.a'Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Look at the GuidePage.tsx usage to confirm button usage without href
cat -n code/core/src/manager/settings/GuidePage.tsx | grep -A 2 -B 2 "Link onClick"Repository: storybookjs/storybook
Length of output: 563
🏁 Script executed:
# Check UpgradeBlock.tsx usage
cat -n code/core/src/manager/components/upgrade/UpgradeBlock.tsx | grep -A 2 -B 2 "Link onClick"Repository: storybookjs/storybook
Length of output: 252
🏁 Script executed:
# Verify the AProps type definition and what it includes
sed -n '66,66p' code/core/src/components/components/typography/link/link.tsxRepository: storybookjs/storybook
Length of output: 119
🏁 Script executed:
# Check if styled-components passes through type attribute on button elements
# Search for other button type handling patterns in the codebase
rg "type=.*button" code/core/src --max-count 20Repository: storybookjs/storybook
Length of output: 1010
Set an explicit button type for the no-href path.
When href is missing (lines 220–225), the component renders a real <button> element. Without an explicit type attribute, HTML defaults to type="submit", which can cause unintended form submissions if the Link is used within a form.
Real usages exist in the codebase (e.g., GuidePage.tsx, UpgradeBlock.tsx) where Link is called without href, triggering button rendering. The codebase's other button-based components consistently use explicit type="button".
Suggested patch
<A
as={href ? 'a' : 'button'}
href={href}
+ type={href ? undefined : 'button'}
{...rest}
ref={ref}
isButton={!href || isButton === true}
onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick}
className={className}
>📝 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.
| <A | |
| as={href ? 'a' : 'button'} | |
| href={href} | |
| {...rest} | |
| ref={ref} | |
| isButton={!href || isButton === true} | |
| onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} | |
| <A | |
| as={href ? 'a' : 'button'} | |
| href={href} | |
| type={href ? undefined : 'button'} | |
| {...rest} | |
| ref={ref} | |
| isButton={!href || isButton === true} | |
| onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} | |
| className={className} | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/components/components/typography/link/link.tsx` around lines
219 - 225, The Link component renders a native <button> when href is not
provided, so add an explicit type="button" for that path to avoid implicit
type="submit"; update the A element props (the JSX line that sets as={href ? 'a'
: 'button'} and isButton={!href || isButton === true}) to include type={href ?
undefined : 'button'} (or type="button" when as is 'button'), ensuring the prop
is passed alongside ref, onClick, isButton, cancelled, cancel and {...rest}.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/core/src/components/components/typography/link/link.tsx (1)
66-66:⚠️ Potential issue | 🟠 MajorFix ref/prop typings to match conditional
<a>/<button>rendering.The
Linkcomponent conditionally renders<a>or<button>based on thehrefprop (line 221:as={href ? 'a' : 'button'}), but the ref is typed asHTMLAnchorElementonly (line 198). This creates a type safety mismatch: developers can passLinkPropswithhrefundefined and render a button, but TypeScript claims the ref is an anchor.Use a discriminated union to align types with runtime behavior:
Type-shape direction
-import type { AnchorHTMLAttributes, MouseEvent } from 'react'; +import type { AnchorHTMLAttributes, ButtonHTMLAttributes, MouseEvent } from 'react'; ... -type AProps = AnchorHTMLAttributes<HTMLAnchorElement>; +type AnchorProps = AnchorHTMLAttributes<HTMLAnchorElement>; +type ButtonProps = ButtonHTMLAttributes<HTMLButtonElement>; ... -export interface LinkProps extends LinkInnerProps, LinkStylesProps, AProps { - cancel?: boolean; - className?: string; - style?: object; - onClick?: (e: MouseEvent) => void; - href?: string; -} +type LinkBaseProps = LinkInnerProps & + LinkStylesProps & { + cancel?: boolean; + className?: string; + style?: object; + onClick?: (e: MouseEvent) => void; + }; + +export type LinkProps = + | (LinkBaseProps & AnchorProps & { href: string }) + | (LinkBaseProps & ButtonProps & { href?: undefined }); ... -export const Link = forwardRef<HTMLAnchorElement, LinkProps>( +export const Link = forwardRef<HTMLAnchorElement | HTMLButtonElement, LinkProps>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/typography/link/link.tsx` at line 66, The Link component currently types AProps as AnchorHTMLAttributes<HTMLAnchorElement> while rendering either an <a> or <button> (as={href ? 'a' : 'button'}), causing a ref/type mismatch; change the prop typing to a discriminated union (e.g., one variant for when href is present using AnchorHTMLAttributes and ref HTMLAnchorElement, and another for when href is absent using ButtonHTMLAttributes and ref HTMLButtonElement), update the LinkProps and the ref/react.forwardRef signature to accept the union so TypeScript knows which element/ref type corresponds to href presence, and adjust any usages of AProps, LinkProps, or the Link component's forwarded ref typing to match the new discriminated union.
♻️ Duplicate comments (1)
code/core/src/components/components/typography/link/link.tsx (1)
220-227:⚠️ Potential issue | 🟠 MajorSet an explicit
type="button"for the no-hrefrendering path.Line 221 renders a native
<button>whenhrefis absent, but no explicittypeis set. Inside forms, this defaults to submit behavior.Suggested patch
<A as={href ? 'a' : 'button'} href={href} + type={href ? undefined : 'button'} {...rest} ref={ref} isButton={!href || isButton === true} onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick} className={className} >#!/bin/bash # Verify the current render path and whether `type` is explicitly set. sed -n '218,232p' code/core/src/components/components/typography/link/link.tsx # Optional impact check: find Link usages without href. rg -nP --type=tsx --type=ts '<Link(?![^>]*\bhref=)' code/core/src -C1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/components/components/typography/link/link.tsx` around lines 220 - 227, The A element renders as a native button when href is absent but no explicit type is set, causing unintended form submits; update the A JSX props so that when href is falsy (the no-href path) you pass type="button" (or type={rest.type ?? 'button'} to respect an explicit type prop) — change the A element usage (the component with props as, href, isButton, onClick, ref, className) to include type="button" for the button rendering path to prevent default submit behavior inside forms.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@code/core/src/components/components/typography/link/link.tsx`:
- Around line 221-225: The truthy checks on href in the Link component cause
empty-string hrefs to be treated as absent; update the conditional checks in
link.tsx (the JSX props using as={...} and isButton={...}) to use explicit
null/undefined comparison: change as={href ? 'a' : 'button'} to as={href != null
? 'a' : 'button'} and change isButton={!href || isButton === true} to
isButton={href == null || isButton === true} so an empty string href is treated
as a valid link while preserving existing isButton override behavior.
---
Outside diff comments:
In `@code/core/src/components/components/typography/link/link.tsx`:
- Line 66: The Link component currently types AProps as
AnchorHTMLAttributes<HTMLAnchorElement> while rendering either an <a> or
<button> (as={href ? 'a' : 'button'}), causing a ref/type mismatch; change the
prop typing to a discriminated union (e.g., one variant for when href is present
using AnchorHTMLAttributes and ref HTMLAnchorElement, and another for when href
is absent using ButtonHTMLAttributes and ref HTMLButtonElement), update the
LinkProps and the ref/react.forwardRef signature to accept the union so
TypeScript knows which element/ref type corresponds to href presence, and adjust
any usages of AProps, LinkProps, or the Link component's forwarded ref typing to
match the new discriminated union.
---
Duplicate comments:
In `@code/core/src/components/components/typography/link/link.tsx`:
- Around line 220-227: The A element renders as a native button when href is
absent but no explicit type is set, causing unintended form submits; update the
A JSX props so that when href is falsy (the no-href path) you pass type="button"
(or type={rest.type ?? 'button'} to respect an explicit type prop) — change the
A element usage (the component with props as, href, isButton, onClick, ref,
className) to include type="button" for the button rendering path to prevent
default submit behavior inside forms.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 616181a9-8746-4556-b549-52cb3e3dcb8d
📒 Files selected for processing (1)
code/core/src/components/components/typography/link/link.tsx
| as={href ? 'a' : 'button'} | ||
| href={href} | ||
| {...rest} | ||
| ref={ref} | ||
| isButton={!href || isButton === true} |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check for empty-string href usage that would be affected by truthiness checks.
rg -n --type=tsx --type=ts 'href=""|href=\{""\}' code/core/src -C1
rg -n --type=tsx --type=ts "href=''" code/core/src -C1Repository: storybookjs/storybook
Length of output: 120
🏁 Script executed:
cat -n code/core/src/components/components/typography/link/link.tsx | sed -n '215,235p'Repository: storybookjs/storybook
Length of output: 821
🏁 Script executed:
# Correct rg syntax for TypeScript/TSX files
rg 'href=""' code/core/src --type=ts -C1 | head -40Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Also check for href={""} pattern
rg 'href=\{""\}' code/core/src --type=ts -C1 | head -40Repository: storybookjs/storybook
Length of output: 47
🏁 Script executed:
# Find the Link component type definition and href parameter type
rg -n 'interface.*Link|type.*Link' code/core/src/components/components/typography/link/link.tsx --type=ts -A5Repository: storybookjs/storybook
Length of output: 599
🏁 Script executed:
# Look for the function signature of the Link component
head -50 code/core/src/components/components/typography/link/link.tsx | cat -nRepository: storybookjs/storybook
Length of output: 1695
Improve clarity of href presence check using explicit null/undefined comparison.
Lines 221 and 225 use truthiness checks on href, which would treat an empty string href="" as falsy and switch to button behavior. Since the type definition allows href?: string, use href != null to explicitly check for the absence of the prop rather than relying on truthiness.
Suggested patch
+ const hasHref = href != null;
return (
<A
- as={href ? 'a' : 'button'}
+ as={hasHref ? 'a' : 'button'}
href={href}
{...rest}
ref={ref}
- isButton={!href || isButton === true}
+ isButton={!hasHref || isButton === true}
onClick={onClick && cancel ? (e) => cancelled(e, onClick) : onClick}
className={className}
>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@code/core/src/components/components/typography/link/link.tsx` around lines
221 - 225, The truthy checks on href in the Link component cause empty-string
hrefs to be treated as absent; update the conditional checks in link.tsx (the
JSX props using as={...} and isButton={...}) to use explicit null/undefined
comparison: change as={href ? 'a' : 'button'} to as={href != null ? 'a' :
'button'} and change isButton={!href || isButton === true} to isButton={href ==
null || isButton === true} so an empty string href is treated as a valid link
while preserving existing isButton override behavior.
github-project-automation
bot
moved this from In Progress
to Done
in Core Team Projects
4 participants
What I did
Tied the
isButtonlogic of theLinkcomponent to the absence of ahref. Without ahref, an HTML anchor will not be keyboard-reachable. This implies buttons that use theLinkstyling with<Link isButton onClick={() => ... } />are not accessible.As all our usages of
<Link isBUttonare cases where we don't have ahref, it makes sense for us to tie the logic to that criteria. This will avoid future regressions and ensure those "links" have the proper HTML markup.I've gone through a deprecation phase for external users of the component, adding the traditional
deprecatecall.Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
🦋 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