Skip to content

Conversation

@Kartik-Murthy
Copy link
Contributor

Summary:

This PR enhances the Button component by adding support for anchor-specific attributes like href, target, rel, and download when the component is rendered as an anchor tag. It ensures correct TypeScript typing for these attributes and fixes type errors when target and download are used without href.

Related Issue: #45729

(You can refer to the issue for more details, including the provided Stackblitz example and image.)

Reproduction:

You can test the behavior using the original Stackblitz example provided by the author here:
Stackblitz Example

Changes:

  • Updated ButtonProps to support anchor tag attributes when href is provided.
  • Added type constraints to ensure proper type handling.
  • Fixed TypeScript errors related to the invalid usage of target, download, or other anchor-specific attributes without href.
  • Updated and added tests to ensure the correct behavior of anchor props in the Button component.

Testing:

  • Tests have been added to verify that the Button component behaves correctly when used with anchor-specific attributes.

  • Type errors are now correctly thrown when target or download is used without href.

  • All related tests pass, including edge cases where component="a" is used with anchor attributes.

  • I have followed (at least) the PR section of the contributing guide.

  • I have tested the changes in this PR.

@mui-bot
Copy link

mui-bot commented Mar 31, 2025

Netlify deploy preview

https://deploy-preview-45765--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against e92ec49

@zannager zannager added the scope: button Changes related to the button. label Mar 31, 2025
@zannager zannager requested a review from siriwatknp March 31, 2025 09:48
@Kartik-Murthy Kartik-Murthy force-pushed the bugfix/button-anchor-attributes-typing branch 3 times, most recently from 834c953 to d94b7ab Compare April 2, 2025 11:12
@Kartik-Murthy Kartik-Murthy force-pushed the bugfix/button-anchor-attributes-typing branch from 1f121b8 to cf030c6 Compare April 3, 2025 11:51
@Kartik-Murthy Kartik-Murthy force-pushed the bugfix/button-anchor-attributes-typing branch 2 times, most recently from 9b5b2b0 to 7b4576a Compare April 5, 2025 05:38
- Updated ButtonProps to allow anchor tag attributes (href, target, rel, download) when Button acts as an anchor.
- Added type constraints to ensure correct type inference for anchor tag props.
- Fixed TypeScript type errors related to usage of target and download without href.
- Updated tests to verify anchor tag behavior and type validation.
- Ran `pnpm docs:typescript:formatted` to automatically fix import order and formatting.
- Restore original import order as per project conventions
- Combined base button props with separate types for anchor and non-anchor buttons.
- Removed the need for ConstrainedButtonProps by using union types to handle href-specific attributes.
- Ensured proper type restrictions for buttons with and without href.
@Kartik-Murthy Kartik-Murthy force-pushed the bugfix/button-anchor-attributes-typing branch from 41d51fa to e92ec49 Compare April 7, 2025 04:11

export type ButtonProps<
// Anchor-specific attributes that should only be available with href
type AnchorAttributes = 'target' | 'rel' | 'download' | 'hrefLang' | 'ping' | 'referrerPolicy';
Copy link
Member

Choose a reason for hiding this comment

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

Is this all of anchor attributes? How do we ensure that this is enough 🤔.

To be honest, I don't lean on this solution that much. I would update the docs on how to apply these props to the Button instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're absolutely right about the anchor attributes—creating an explicit list does risk missing some attributes or falling out of sync with future HTML specs. I really appreciate your suggestion to focus on documentation instead.

As an alternative approach that doesn’t require listing specific attributes, we could document this pattern:

import { Button, ButtonProps } from '@mui/material';

// This correctly adds all anchor props only when href is present
type CustomButtonProps<C extends React.ElementType = 'button'> = ButtonProps<C, {}> &
  (({ href: string } & React.ComponentProps<'a'>) | { href: never });

export const CustomButton = (props: CustomButtonProps) => {
  return <Button {...props} />;
};

This approach leverages TypeScript's built-in types to automatically include all anchor attributes, helping us stay aligned with future HTML specs while minimizing maintenance overhead.

I hadn’t considered that earlier, but it’s a great idea, and I’m definitely on board with it. Would it make sense to close my PR and open a new one that addresses issue #45729 by updating the documentation instead of modifying the type definitions?

@siriwatknp @ZeeshanTamboli , I’d love to get your input on whether this seems like the best course of action.

Copy link
Member

@ZeeshanTamboli ZeeshanTamboli Apr 8, 2025

Choose a reason for hiding this comment

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

Before adding docs, we should first understand why the target attribute isn't supported on custom buttons that are wrapped with Material UI Buttons in TS.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the issue with target not being properly recognized on custom button component lies with the ButtonProps, though I'm not entirely certain about all the details.

ButtonProps type definition:-

export type ButtonProps
  RootComponent extends React.ElementType = ButtonTypeMap['defaultComponent'],
  AdditionalProps = {},
> = OverrideProps<ButtonTypeMap<AdditionalProps, RootComponent>, RootComponent> & {
  component?: React.ElementType;
}

This definition doesn't seem to explicitly include the anchor element attributes like target when an href is provided.

What's interesting is that the ExtendButtonBase type in ButtonBase.d.ts file does seem to handle anchor transformation:

export type ExtendButtonBase<TypeMap extends OverridableTypeMap> = ((
  props: { href: string } & OverrideProps<ExtendButtonBaseTypeMap<TypeMap>, 'a'>,
) => React.JSX.Element) &
  OverridableComponent<ExtendButtonBaseTypeMap<TypeMap>>;

From what I can observe:

  • When using Button directly, href and target work perfectly
  • When wrapping Button in our custom component using ButtonProps, TypeScript doesn't recognize anchor attributes
  • The component still works correctly at runtime anyway despite TypeScript errors when using ButtonProps

Copy link
Member

Choose a reason for hiding this comment

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

When wrapping Button in our custom component using ButtonProps, TypeScript doesn't recognize anchor attributes

Is this a TypeScript limitation? And how? If so, we should document how to make it recognize the target attribute and mention that it's a TS limitation.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I spent some time looking into this, and while I couldn’t find anything concrete in the TypeScript documentation or issue tracker, it seems like TypeScript might be struggling with narrowing types when they’re passed through a generic wrapper.

It seems the Button component handles href and target fine because of how the overloads are set up in ExtendButtonBase. But when using ButtonProps in a wrapper, that conditional behavior seems to get flattened or lost, and target is no longer recognized.

I’m not sure if this is due to TypeScript limitations or just a side effect of how the types are structured. If anyone has more insight, I’d be happy to learn more or help clarify this further in the docs if it turns out to be expected behavior.

Copy link
Member

Choose a reason for hiding this comment

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

I looked into it — this is a TypeScript limitation.

When you wrap Button in a custom component like CustomButton, TypeScript can't infer that target is valid when href is used.

As a workaround (though not ideal), you can explicitly extend both ButtonProps and anchor attributes:

import { Button, ButtonProps } from '@mui/material';

type AnchorButtonProps = ButtonProps & React.AnchorHTMLAttributes<HTMLAnchorElement>;

const CustomButton = (props: AnchorButtonProps) => {
  return <Button {...props} />;
};

export default function App() {
  return (
    <CustomButton href="https://example.com" target="_blank">
      My custom button
    </CustomButton>
  );
}

That said, if the custom component isn’t adding any new logic or props, I don't see much value in wrapping Button just to forward props — so I raised that question in the issue: #45729 (comment)

@ZeeshanTamboli
Copy link
Member

Closing this PR since the related issue is a TS limitation. See #45765 (comment) and #45729 (comment)

@Kartik-Murthy Kartik-Murthy deleted the bugfix/button-anchor-attributes-typing branch April 11, 2025 07:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: button Changes related to the button. typescript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants