[ButtonBase] Improve Button accessibility

[bluSCALE4]

Revert #10339 removed safeguard and add additional one so anchors missing href's are treated as buttons.

• This is not a v0.x issue.
• I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

An anchor with an href should not be assigned role=button.

Current Behavior 😯

An anchor with prop button w/ href is being given role of button as it misleads users. A button does not alter the url path while an href does.

Steps to Reproduce 🕹

<ListItem component={Link} to={path} button>
Hi there
</ListItem>

Context 🔦

Screen reader

[oliviertassinari]

@bluSCALE4 Do you have a specific fix in mind? I think that we can apply the following but it won't help your case:

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -287,7 +287,10 @@ class ButtonBase extends React.Component {
       buttonProps.type = type;
       buttonProps.disabled = disabled;
     } else {
-      buttonProps.role = 'button';
+      if (!(ComponentProp === 'a' && other.href)) {
+        buttonProps.role = 'button';
+      }
+
       buttonProps['aria-disabled'] = disabled;

[bluSCALE4]

That looks good. It should solve the original ask. But you're right, my use case is different.

[oliviertassinari]

The alternative would be to not provide a role prop if we don't know the actual DOM node rendered. We could defer the responsibility to the component the user is providing. Yeah, it could work. I like this heuristic cc @eps1lon.

[eps1lon]

I don't understand this component anymore. Why should the ButtonBase not be a button?

[oliviertassinari]

@bluSCALE4 Have you considered not using the button prop of the list?

[eps1lon]

It's just we extend and extend this heuristic and I don't think this will ever be a good one. @bluSCALE4 Can you share some resources why anchors shouldn't have the button role?

[ryancogswell]

One potential point of confusion with the current behavior, is that the button role indicates that the user should be able to use space or enter to trigger its functionality. In the case of a link, enter still works, but space does not.

Why should the ButtonBase not be a button?

The use of ButtonBase with an a tag is primarily for aesthetic reasons (to make it look like a button). The role would ideally convey whether or not it behaves like a button in response to keyboard actions. We don't include the role when ComponentProp === 'button' because the element type already conveys what it is. Similarly when ComponentProp === 'a' and href is specified, the element type already conveys that it is a link, so no role is necessary to communicate how the element will behave.

Unfortunately, what we really want to drive off of is button.tagName, but we can't know that reliably until after rendering. The most typical use cases are going to be like the one in the initial issue comment: component={Link}, where ComponentProp will not be 'a' but button.tagName would be 'A'.

Developers can use role={null} (e.g. <ListItem component={Link} to={path} button role={null}>) to control this themselves.

[oliviertassinari]

@ryancogswell So, you are leaning toward role={null} userland and

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -287,7 +287,9 @@ class ButtonBase extends React.Component {
       buttonProps.type = type;
       buttonProps.disabled = disabled;
     } else {
-      buttonProps.role = 'button';
+      if (!(ComponentProp === 'a' && other.href)) {
+        buttonProps.role = 'button';
+      }
       buttonProps['aria-disabled'] = disabled;
     }

instead of?

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -287,7 +287,9 @@ class ButtonBase extends React.Component {
       buttonProps.type = type;
       buttonProps.disabled = disabled;
     } else {
-      buttonProps.role = 'button';
+      if (typeof ComponentProp === 'string' && !(ComponentProp === 'a' && other.href)) {
+        buttonProps.role = 'button';
+      }
       buttonProps['aria-disabled'] = disabled;
     }

[ryancogswell]

@oliviertassinari The typeof ComponentProp === 'string' condition is interesting. If ComponentProp is not a 'string', then we really don't know what the role should be. I'm not sure what is worse, including role=button when we shouldn't or potentially dropping it when it should be there. We'd be trading the former for the latter with the typeof ComponentProp === 'string' condition, but it would be more intuitive (and more straightforward to document) to explicitly add role="button" to add the role than to add role={null} to suppress it.

I suspect that when ComponentProp is not a string, that it is more often a link than something that should receive the button role, but I really don't know if that is true (but it is true in my own codebase).

When I experimented with role={null} in a CodeSandbox, it worked, but the eslint rules in the sandbox complained about it: "Elements with ARIA roles must use a valid, non-abstract ARIA role. (jsx-a11y/aria-role)".

So I like your (typeof ComponentProp === 'string' && !(ComponentProp === 'a' && other.href)) proposal, but I'm not sure how many people it might have adverse effects on.

As a side note, I need to read up on how to do those nifty diff comments.

[oliviertassinari]

@ryancogswell This synthesis my reflection. I agree with every word.
I'm slightly more in favor of the typeof ComponentProp === 'string' path. Still, what should we do 🤔?

[ryancogswell]

@oliviertassinari Given that:

• the potential adverse effect is pretty minimal (just losing role="button")
• the adverse effect is easy to remediate by adding role="button" as a prop
• I'm not aware of any common pattern that would use a non-string component prop for something that should receive the button role
• the Link component pattern is quite common

I would opt to go ahead and use your suggested:

--- a/packages/material-ui/src/ButtonBase/ButtonBase.js
+++ b/packages/material-ui/src/ButtonBase/ButtonBase.js
@@ -287,7 +287,9 @@ class ButtonBase extends React.Component {
       buttonProps.type = type;
       buttonProps.disabled = disabled;
     } else {
-      buttonProps.role = 'button';
+      if (typeof ComponentProp === 'string' && !(ComponentProp === 'a' && other.href)) {
+        buttonProps.role = 'button';
+      }
       buttonProps['aria-disabled'] = disabled;
     }