fix(Radio): remove redundant aria-disabled

[joshblack]

Closes #2634

Removes redundant aria-disabled when the <input> is disabled. In order to get a solid test case, I needed to adopt a custom rule from GitHub that was being run against axe. Unfortunately, the default playwright package from axe doesn't allowing configuring axe-core directly so I updated the matcher to run axe-core directly and configure it with this custom rule.

Changelog

New

• Add e2e test for RadioGroup, including a disabled state check

Changed

• Update Radio to no longer set aria-disabled
• Update toHaveNoViolations to use axe-core directly and configure with custom rules from dotcom

Removed

[changeset-bot]

🦋 Changeset detected

Latest commit: dcda7c4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
@primer/react	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[github-actions]

size-limit report 📦

Path	Size
dist/browser.esm.js	82.8 KB (-0.01% 🔽)
dist/browser.umd.js	83.45 KB (0%)

[broccolinisoup]

Hey @joshblack! Sorry, I am a bit confused about why we removed aria-disabled . I understand that the GitHub axe rule of having both attribute are conflicting and it makes sense but here it says

Unless otherwise stated, authors MAY use aria-* attributes in place of their HTML equivalents on HTML elements where the aria-* semantics would be expected. For example, authors MAY specify aria-disabled=true on a button element, while also implementing the necessary scripting to functionally disable the button, rather than the use disabled attribute.

I would expect us to remove the disabled and update styling accordingly? 🤔 I might be missing something, sorry but would be great to clarify this, then I'll continue to review 😊

[joshblack]

@broccolinisoup makes sense to me! Happy to make that change 👍

Maybe what I can do is add this to our next major release ticket with the following changes in mind:

• Radio should no longer accept a disabled prop (this we would have to remove in the next major)
• Radio in the interim accepts disabled or aria-disabled (but not both) and styling/behavior should apply in either scenario

Curious what you think 👀

[broccolinisoup]

Thanks @joshblack!

Radio should no longer accept a disabled prop (this we would have to remove in the next major)

I really like that 🙌🏼 Can we also check it with the accessibility team to make sure if they don't suggest anything else or anything more?

Radio in the interim accepts disabled or aria-disabled (but not both) and styling/behaviour should apply in either scenario

My gut feeling says we should allow aria-disabled not disabled but I recognise the effort around updating the styles to make sure the ones with aria-disabled look disabled too. So maybe we can merge your PR as is now as the interim solution and make sure we have issue to address removing disabled in the next major realised? Does that sound good at all?

[joshblack]

Can we also check it with the accessibility team to make sure if they don't suggest anything else or anything more?

Definitely! I'll reach out.

My gut feeling says we should allow aria-disabled not disabled

Makes sense to me! I think the only catch is that we can't remove support for disabled without it being a breaking change. I think that's where some of the interim ideas were coming from (support either case but not both at the same time). Are you thinking of having disabled map to aria-disabled or were you wanting to remove it as part of addressing the parent issue?

[broccolinisoup]

Yes like mapping. We could still keep the disabled prop in the api but render as aria-disabled in the HTML however I just checked the implementation and it is mostly used within the FormControl which makes sense and mapping would make things unnecessarily complicated. Sorry that was a bit non-sense 😆

Your solution to remove the warning is the most effective and doable way, I 100% agree. Hope to bring aria-disabled back with a major update to make it more accessible.

Sorry for lingering around with not very doable solutions, I'll approved the PR 🚀