Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/seven-chefs-confess.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@primer/react': patch
---

Update Radio to only use disabled when provided and no longer set aria-disabled
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
81 changes: 81 additions & 0 deletions e2e/components/RadioGroup.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,81 @@
import {test, expect} from '@playwright/test'
import {visit} from '../test-helpers/storybook'
import {themes} from '../test-helpers/themes'

test.describe('RadioGroup', () => {
test.describe('Default', () => {
for (const theme of themes) {
test.describe(theme, () => {
test('default @vrt', async ({page}) => {
await visit(page, {
id: 'components-forms-radiogroup-examples--default',
globals: {
colorScheme: theme,
},
})

// Default state
expect(await page.screenshot()).toMatchSnapshot(`RadioGroup.Default.${theme}.png`)
})

test('disabled @vrt', async ({page}) => {
await visit(page, {
id: 'components-forms-radiogroup-examples--default',
globals: {
colorScheme: theme,
},
args: {
disabled: true,
},
})

expect(await page.screenshot()).toMatchSnapshot(`RadioGroup.Default.disabled.${theme}.png`)

await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})

test('disabled @aat', async ({page}) => {
await visit(page, {
id: 'components-forms-radiogroup-examples--default',
globals: {
colorScheme: theme,
},
args: {
disabled: true,
},
})

await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})

test('axe @aat', async ({page}) => {
await visit(page, {
id: 'components-forms-radiogroup-examples--default',
globals: {
colorScheme: theme,
},
})
await expect(page).toHaveNoViolations({
rules: {
'color-contrast': {
enabled: theme !== 'dark_dimmed',
},
},
})
})
})
}
})
})
64 changes: 51 additions & 13 deletions e2e/matchers/toHaveNoViolations.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import {Page, expect, test} from '@playwright/test'
import AxeBuilder from '@axe-core/playwright'
import {AxeResults} from 'axe-core'
import {AxeResults, source} from 'axe-core'
import path from 'node:path'
import fs from 'node:fs'

Expand All @@ -21,23 +20,62 @@ const defaultOptions = {
region: {
enabled: false,
},
// Custom rules
'avoid-both-disabled-and-aria-disabled': {
enabled: true,
},
},
}

expect.extend({
async toHaveNoViolations(page: Page, options = {rules: {}}) {
// @ts-ignore Page from @playwright/test should satisfy Page from
// playwright-core
const result = await new AxeBuilder({page})
.options({
...defaultOptions,
...options,
rules: {
...defaultOptions.rules,
...options.rules,
},
const runConfig = {
...defaultOptions,
...options,
rules: {
...defaultOptions.rules,
...options.rules,
},
}

await page.evaluate(source)

const result: AxeResults = await page.evaluate(runConfig => {
// @ts-ignore `axe` is a global variable defined by page.evaluate() above
const axe = window.axe

axe.configure({
rules: [
{
id: 'avoid-both-disabled-and-aria-disabled',
excludeHidden: true,
selector: 'button, fieldset, input, optgroup, option, select, textarea',
all: ['check-avoid-both-disabled-and-aria-disabled'],
any: [],
metadata: {
help: '[aria-disabled] may be used in place of native HTML [disabled] to allow tab-focus on an otherwise ignored element. Setting both attributes is contradictory.',
helpUrl: 'https://www.w3.org/TR/html-aria/#docconformance-attr',
},
tags: ['custom-github-rule'],
},
],
checks: [
{
id: 'check-avoid-both-disabled-and-aria-disabled',
/**
* Check an element with native `disabled` support doesn't have both `disabled` and `aria-disabled` set.
*/
evaluate: (el: Element) => !(el.hasAttribute('aria-disabled') && el.hasAttribute('disabled')),
metadata: {
impact: 'critical',
},
},
],
})
.analyze()

// @ts-ignore `axe` is a global variable defined by page.evaluate() above
return axe.run(runConfig)
}, runConfig)

saveResult(result)

Expand Down
10 changes: 9 additions & 1 deletion e2e/test-helpers/storybook.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,27 @@ import {waitForImages} from './waitForImages'

interface Options {
id: string
args?: Record<string, string | boolean>
globals?: Record<string, string>
}

const {STORYBOOK_URL = 'http://localhost:6006'} = process.env

export async function visit(page: Page, options: Options) {
const {id, globals} = options
const {id, args, globals} = options
// In CI, the static server strips `.html` extensions
const url = process.env.CI ? new URL(`${STORYBOOK_URL}/iframe`) : new URL(`${STORYBOOK_URL}/iframe.html`)

url.searchParams.set('id', id)
url.searchParams.set('viewMode', 'story')

if (args) {
const serialized = Object.entries(args)
.map(([key, value]) => `${key}:${value}`)
.join(',')
url.searchParams.set('args', serialized)
}

if (globals) {
let params = ''
for (const [key, value] of Object.entries(globals)) {
Expand Down
22 changes: 0 additions & 22 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@
"styled-system": "^5.1.5"
},
"devDependencies": {
"@axe-core/playwright": "4.5.0",
"@babel/cli": "7.19.3",
"@babel/core": "7.14.8",
"@babel/eslint-parser": "7.15.7",
Expand Down
11 changes: 11 additions & 0 deletions script/generate-e2e-tests.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,17 @@ const components = new Map([
],
},
],
[
'RadioGroup',
{
stories: [
{
id: 'components-forms-radiogroup-examples--default',
name: 'Default',
},
],
},
],
[
'UnderlineNav',
{
Expand Down
1 change: 0 additions & 1 deletion src/Radio.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ const Radio = React.forwardRef<HTMLInputElement, RadioProps>(
name={name}
ref={ref}
disabled={disabled}
aria-disabled={disabled ? 'true' : 'false'}
Copy link
Member

Choose a reason for hiding this comment

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

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 😊

Copy link
Member Author

Choose a reason for hiding this comment

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

@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 👀

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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 🚀

checked={checked}
aria-checked={checked ? 'true' : 'false'}
required={required}
Expand Down
4 changes: 0 additions & 4 deletions src/__tests__/Radio.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -104,18 +104,14 @@ describe('Radio', () => {

expect(radio.disabled).toEqual(true)
expect(radio).not.toBeChecked()
expect(radio).toHaveAttribute('aria-disabled', 'true')

fireEvent.change(radio)

expect(radio.disabled).toEqual(true)
expect(radio).not.toBeChecked()
expect(radio).toHaveAttribute('aria-disabled', 'true')

// remove disabled attribute and retest
rerender(<Radio {...defaultProps} onChange={handleChange} />)

expect(radio).toHaveAttribute('aria-disabled', 'false')
})

it('renders an uncontrolled component correctly', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ exports[`CheckboxOrRadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down Expand Up @@ -146,7 +145,6 @@ exports[`CheckboxOrRadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down Expand Up @@ -178,7 +176,6 @@ exports[`CheckboxOrRadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down
1 change: 0 additions & 1 deletion src/__tests__/__snapshots__/Radio.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ exports[`Radio renders consistently 1`] = `

<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c0"
Expand Down
3 changes: 0 additions & 3 deletions src/__tests__/__snapshots__/RadioGroup.test.tsx.snap
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,6 @@ exports[`RadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down Expand Up @@ -146,7 +145,6 @@ exports[`RadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down Expand Up @@ -178,7 +176,6 @@ exports[`RadioGroup renders consistently 1`] = `
>
<input
aria-checked="false"
aria-disabled="false"
aria-invalid="false"
aria-required="false"
className="c6"
Expand Down
Loading