Skip to content

[button] Warn in dev mode when rendering a native link#3423

Open
mj12albert wants to merge 8 commits intomui:masterfrom
mj12albert:use-button-link-warning
Open

[button] Warn in dev mode when rendering a native link#3423
mj12albert wants to merge 8 commits intomui:masterfrom
mj12albert:use-button-link-warning

Conversation

@mj12albert
Copy link
Copy Markdown
Member

@mj12albert mj12albert added the component: button Changes related to the button component. label Dec 4, 2025
@pkg-pr-new
Copy link
Copy Markdown

pkg-pr-new bot commented Dec 4, 2025

commit: c39a5b1

@mui-bot
Copy link
Copy Markdown

mui-bot commented Dec 4, 2025

Bundle size report

Bundle Parsed size Gzip size
@base-ui/react 0B(0.00%) 0B(0.00%)

Details of bundle changes


Check out the code infra dashboard for more information about this PR.

@netlify
Copy link
Copy Markdown

netlify bot commented Dec 4, 2025

Deploy Preview for base-ui ready!

Name Link
🔨 Latest commit c39a5b1
🔍 Latest deploy log https://app.netlify.com/projects/base-ui/deploys/699d582ea90d6a00081cf8a5
😎 Deploy Preview https://deploy-preview-3423--base-ui.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@mj12albert mj12albert force-pushed the use-button-link-warning branch from 72b9049 to beb4f7e Compare December 4, 2025 20:41
Copy link
Copy Markdown
Member

@LukasTy LukasTy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense to me. 👍

'A component that acts as a button was rendered as a native <button>, which does not match the default. Ensure that the element passed to the `render` prop of the component is not a real <button>, or set the `nativeButton` prop on the component to `true`.',
);
} else if (elementRef.current.tagName === 'A') {
warn(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT, would error work here as well? 🤔

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 9, 2025
@mj12albert mj12albert force-pushed the use-button-link-warning branch from 74367ec to 9e0bca1 Compare December 9, 2025 15:09
@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Dec 9, 2025
@mj12albert mj12albert marked this pull request as ready for review December 9, 2025 15:09
@atomiks
Copy link
Copy Markdown
Contributor

atomiks commented Jan 28, 2026

It might be too late to add this warning given shadcn's integration:

Screenshot 2026-01-28 at 1 35 55 pm

Might be better to have a bullet point under "Usage guidelines" and a doc section about not using <Button> to render links (and get shadcn to update on it)

@mj12albert
Copy link
Copy Markdown
Member Author

Maybe we should warn against passing disabled if rendering a link instead? That's more of an actual a11y mistake than simply replacing the element with a link 🤔

@atomiks
Copy link
Copy Markdown
Contributor

atomiks commented Jan 28, 2026

Maybe we should warn against passing disabled if rendering a link instead? That's more of an actual a11y mistake than simply replacing the element with a link 🤔

Another issue is that role="button" is set but shouldn't be if it's an <a> (but it still should be if it's a <div>). So nativeButton doesn't cover this during SSR

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 3, 2026
@atomiks
Copy link
Copy Markdown
Contributor

atomiks commented Feb 9, 2026

My current thoughts on this:

  • Coordinate with shadcn to remove the invalid usage. A quick patch would be to get shadcn to reuse the Radix button logic for Base UI (given Radix doesn't have a button component anyway). @colmtuite
  • In the meantime (interim, 1.2.0 release), detect <a> in an effect and remove role="button". No warning.
  • In 1.3.0 or some later release eventually, add a warning for this usage and recommend what to do instead.

);
} else if (elementRef.current.tagName === 'A') {
warn(
'A component that acts as a button was rendered as an <a> tag, which could cause usability issues for keyboard and assistive tech users. Prefer using `<a>` directly.',
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadcn updated, so we can likely warn now.

  • This should say "causes" not "could cause"
  • Mention Menu.LinkItem for the old Menu case, or possibly silence/ignore the warning in that case?

Copy link
Copy Markdown
Member Author

@mj12albert mj12albert Feb 25, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shadcn isn't using links in their regular <Menu>, it's dev mode and just a warning (not error) so maybe it's not worth making a special case for it? @atomiks

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think a lot of users who were using the render pattern might not know of Menu.LinkItem yet, so silencing might be better or at least mentioning that component name in the warning message to let them know to migrate

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged. label Feb 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

component: button Changes related to the button component.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants