[ButtonUnstyled] Don't set redundant role=button

[michaldudak]

Fixes the problem seen in #28429 (comment)

Preview: https://deploy-preview-28488--material-ui.netlify.app/components/buttons/

[mui-pr-bot]

Details of bundle changes

Generated by 🚫 dangerJS against 6224245

[mnajdova]

Nice! Maybe we should in addition add test that actually tests what the PR title describes (using a component that renders button).

[oliviertassinari]

Is there a way to not depend on the JS runtime to set the correct attributes? The issue seems to be still here https://validator.w3.org/nu/?doc=https%3A%2F%2Fdeploy-preview-28488--material-ui.netlify.app%2Fcomponents%2Fbuttons%2F

(Maybe it's only the demo that needs to be fixed)

[michaldudak]

Right, the issue still occurs when rendering on the server. I'll see what I can do about it.

[michaldudak]

After some investigation, I must say I'm not aware of any way to figure out the type of the rendered host component on the server.

I propose we postpone adding accessibility attributes to the client-side rendering phase. This means native buttons won't have the role=button, so the validation will pass, but neither other elements will.

This solution is IMO still better than what we have in Material Button, or how, for example, react-aria works. In both cases providing a component that renders a button does result in a redundant role=button being set (on the client). The solution I propose takes into consideration the type of the element that is actually rendered and modifies the attributes accordingly.

[oliviertassinari]

This issue makes me think of https://mui.com/guides/routing/#button

When we can't see through the custom component provided, we assume that it needs the role, and if it doesn't, we ask developers to ignore the role:

You can blame to retrieve the initial PR/issue behind it.

I would encourage we revert this PR, and instead focus on only fixing the docs. From what I understand:

• with the changes, we always render the button twice.
• React is pushing toward being able to not hydrate parts of the screen that can work with SSR only, the current solution would fail in such a context (it needs JS to resolve the ref): https://reactjs.org/blog/2020/12/21/data-fetching-with-react-server-components.html
• The behavior on https://mui.com/components/buttons/#unstyled (before we merged) is consistent with the ButtonBase, there is already a prior-art for one approach, we could build on top of it. We don't really need to force developers to learn a new arbitrary behavior (this assumes that no solution is better than another).
• In this demo: https://mui.com/components/buttons/#unstyled-component, I don't get why we use the component prop. This seems to defeat the purpose of the component prop. Shouldn't styled() wrap ButtonUnstyled as https://mui.com/components/slider/#unstyled does? So that developers can later apply a custom component, keeping the style. If we do such a change, it seems to solve the initial problem.

For instance: https://codesandbox.io/s/22igz?file=/demo.tsx, it's not rendering a span, even if had provided component="span".

[michaldudak]

Thanks for the feedback. I decided to merge it as there was no response for a week.

with the changes, we always render the button twice.

Yes, that's correct.

React is pushing toward being able to not hydrate parts of the screen that can work with SSR only, the current solution would fail in such a context (it needs JS to resolve the ref): https://reactjs.org/blog/2020/12/21/data-fetching-with-react-server-components.html

Thanks, I'll dig into this more. I've watched parts of the video and it seems to me that server-side rendering would not apply to buttons, as it's designed for non-interactive elements (i.e. ones that don't use useState or event handlers).

The behavior on https://mui.com/components/buttons/#unstyled (before we merged) is consistent with the ButtonBase, there is already a prior-art for one approach, we could build on top of it. We don't really need to force developers to learn a new arbitrary behavior (this assumes that no solution is better than another).

Yes, the behavior changed, but working on unstyled components gives us the opportunity to introduce improvements. Now, what is or isn't an improvement is of course a matter of discussion. The "give us any component you want and we'll add proper attributes when necessary so you don't have to think about it" promise would sound pretty appealing to me if I was using the library.

In this demo: https://mui.com/components/buttons/#unstyled-component, I don't get why we use the component prop. This seems to defeat the purpose of the component prop. Shouldn't styled() wrap ButtonUnstyled as https://mui.com/components/slider/#unstyled does? So that developers can later apply a custom component, keeping the style. If we do such a change, it seems to solve the initial problem.

In this case, I could have used styled(ButtonUnstyled), yes. The visual effect would be pretty much the same and the role attribute would be applied correctly.

Do you see the component prop being used as an as alias? We had a conversation with @mnajdova that we should replace the root slot with the one passed in the component (or components.Root) instead of using it as an as alias, allowing developers not to use the default styles. But then, it's a slightly different use case to the one you described.