fix(button): remove role="button" from button styled anchors

[dakahn]

Closes #5521
Closes #5112

Implicit labeling (having wrapped text in the sub-tree acting as the label) only works for native button elements. For instances where the button is an anchor tag, p tag or otherwise we need to specify the label with text matching the visible label.

Changelog

New

• added an aria-label: children to the anchorProps object
• removed role="button"

Testing / Reviewing

Since button is a widely used generic component I could totally be missing something/some use case. Otherwise just open up Chrome and run DAP with the latest ruleset and you should see zero errors reported 🏄

[netlify]

Deploy preview for carbon-elements ready!

Built with commit 3a3a336

https://deploy-preview-5522--carbon-elements.netlify.com

[netlify]

Deploy preview for carbon-components-react ready!

Built with commit 3a3a336

https://deploy-preview-5522--carbon-components-react.netlify.com

[asudoh]

A question; What happens if the button has non-text content?

[dakahn]

@asudoh Well, if there was no label whatsoever DAP would throw and error for an anchor tag not having an associated label. Is that a use case we support? For an icon I thought we set a label on the svg.

[abbeyhrt]

Looks good to me! Separately, I see that DAP is saying that p and a are form control elements, are they really? Or am I misunderstanding and it's just picking up the role='button'?

[dakahn]

@abbeyhrt right yeah! They become form control elements the moment we designate them role="button"

[joshblack]

Wow, definitely an awkward violation lol. Would you want to add a quick test to check for this too with any links to refer to why we need to do this?

[dakahn]

@joshblack added test! Though you'll notice we we had two nested divs inside our test button that I removed -- they didn't seem to be directly related to any of the tests I saw in that unit, but there might be something I'm missing.

[asudoh]

@dakahn You could detect if children is a string. If it's the case you can still propagate it to aria-label. Otherwise we can let application to populate aria-label.

[joshblack]

That's a great point ☝️, children is of type PropTypes.node so may not contain text. Would we instead need a prop type warning such that aria-label is required if href is supplied?

[dakahn]

Hey @joshblack brought up a good point that it probably makes more sense to simply remove role="button" for any anchors or whatever other non <button> elements we're only styling like buttons. 👍

[abbeyhrt]

@dakahn I'm not really sure how the non-button elements styled to look like buttons are used but is it a problem that they're no longer announced as buttons?

[tw15egan]

@abbeyhrt my understanding is they are used when you are navigating to different parts of an app, so I think it would make more sense that they were not announced as buttons. I believe they are just styled like them since a lone link doesn't seem to have the same affordance/CTA as a button.

[abbeyhrt]

@tw15egan Ah gotcha, makes sense! Thanks for explaining!

[tw15egan]

Makes sense to me, DAP errors have been eliminated ☠️

👍 ✅

[asudoh]

LGTM 👍 - Thanks @dakahn!