Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(ByRole): Allow filter by disabled state #1221

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

feat(ByRole): Allow filter by disabled state #1221

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
eeebfc2
feat(ByRole): Allow filter by disabled state
eps1lon Mar 8, 2023
542128a
TODO: reword+fixup
eps1lon Mar 8, 2023
876fef0
Update src/__tests__/ariaAttributes.js
eps1lon Mar 24, 2023
a42f42e
Merge branch 'main' into feat/byrole-disabled
timdeschryver May 25, 2023
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
54 changes: 54 additions & 0 deletions src/__tests__/ariaAttributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,3 +379,57 @@ test('`expanded: true|false` matches `expanded` elements with proper role', () =
expect(getByRole('button', {expanded: true})).toBeInTheDocument()
expect(getByRole('button', {expanded: false})).toBeInTheDocument()
})

test('`disabled` throws on unsupported roles', () => {
const {getByRole} = render(
`<div role="alert" aria-disabled="true">Hello, Dave!</div>`,
)
expect(() =>
getByRole('alert', {disabled: true}),
).toThrowErrorMatchingInlineSnapshot(
`"aria-disabled" is not supported on role "alert".`,
)
})

test('`disabled: true|false` matches `disabled` buttons', () => {
const {getByRole} = renderIntoDocument(
`<div>
<button disabled="true" />
<button />
</div>`,
)
expect(getByRole('button', {disabled: true})).toBeInTheDocument()
expect(getByRole('button', {disabled: false})).toBeInTheDocument()
})

test('`disabled: true|false` matches `aria-disabled` buttons', () => {
const {getByRole} = renderIntoDocument(
`<div>
<button aria-disabled="true" />
<button aria-disabled="false" />
</div>`,
)
expect(getByRole('button', {disabled: true})).toBeInTheDocument()
expect(getByRole('button', {disabled: false})).toBeInTheDocument()
})

test('`disabled` attributes overrides `aria-disabled`', () => {
const {getByRole} = renderIntoDocument(
`<div>
<button disabled="true" aria-disabled="false" />
<button />
</div>`,
)
expect(getByRole('button', {disabled: true})).toBeInTheDocument()
Copy link
Member

Choose a reason for hiding this comment

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

Just an idea, maybe for the completeness of this test, we should also add:

expect(queryByRole('button', {disabled: false})).not.toBeInTheDocument()

})

test('consider `disabled` attribute only if supported', () => {
const {getByRole, queryByRole} = renderIntoDocument(
`<div>
<button disabled="true" />
<div role="slider" disabled="true" />
</div>`,
)
expect(getByRole('button', {disabled: true})).toBeInTheDocument()
expect(queryByRole('slider', {disabled: true})).toBe(null)
})
15 changes: 15 additions & 0 deletions src/queries/role.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import {
computeAriaChecked,
computeAriaPressed,
computeAriaCurrent,
computeAriaDisabled,
computeAriaExpanded,
computeAriaValueNow,
computeAriaValueMax,
Expand Down Expand Up @@ -52,6 +53,7 @@ const queryAllByRole: AllByRole = (
checked,
pressed,
current,
disabled,
level,
expanded,
value: {
Expand Down Expand Up @@ -174,6 +176,16 @@ const queryAllByRole: AllByRole = (
}
}

if (disabled !== undefined) {
// guard against unknown roles
if (
allRoles.get(role as ARIARoleDefinitionKey)?.props['aria-disabled'] ===
undefined
) {
throw new Error(`"aria-disabled" is not supported on role "${role}".`)
}
}

const subtreeIsInaccessibleCache = new WeakMap<Element, Boolean>()
function cachedIsSubtreeInaccessible(element: Element) {
if (!subtreeIsInaccessibleCache.has(element)) {
Expand Down Expand Up @@ -227,6 +239,9 @@ const queryAllByRole: AllByRole = (
if (current !== undefined) {
return current === computeAriaCurrent(element)
}
if (disabled !== undefined) {
return disabled === computeAriaDisabled(element)
}
if (expanded !== undefined) {
return expanded === computeAriaExpanded(element)
}
Expand Down
24 changes: 24 additions & 0 deletions src/role-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -290,6 +290,29 @@ function computeAriaCurrent(element) {
)
}

const elementsSupportingDisabledAttribute = new Set([
Copy link
Member

Choose a reason for hiding this comment

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

I might be missing something, but this seems to me like something that should be defined in aria-query because it can change as part of the spec and having it here is like mixing the ARIA logic with our role implementation.
Wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

Also, I see we have something like this in jest-dom too, so it makes me think that this indeed shouldn't belong in one of our packages.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't want to put more stuff into aria-query. dom-accesibility-api makes more sense at this point. Feel free to raise a PR.

Copy link
Member

Choose a reason for hiding this comment

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

Done: eps1lon/dom-accessibility-api#919
Please lmk if I can change stuff, I'm not happy with putting the change in the util file but I'm not familiar with the codebase, so I'm happy to change it.
Thanks!

'button',
'fieldset',
'input',
'optgroup',
'option',
'select',
'textarea',
])

/**
* @param {Element} element -
* @returns {boolean} -
*/
function computeAriaDisabled(element) {
return elementsSupportingDisabledAttribute.has(element.localName) &&
element.hasAttribute('disabled')
? // https://www.w3.org/TR/html-aam-1.0/#html-attribute-state-and-property-mappings
true
: // https://www.w3.org/TR/wai-aria-1.1/#aria-disabled
element.getAttribute('aria-disabled') === 'true'
Copy link
Member

Choose a reason for hiding this comment

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

I think it's worth adding this to jest-dom assertion too (I can do this on my own if you agree with me).
Having two libraries in the organization considering different things as disabled is quite confusing IMO.

Copy link
Member Author

Choose a reason for hiding this comment

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

Querying should not be used for assertions. For jest-dom i.e. there was a clear case why this wasn't as straight forward because it might contain footguns.

/dom: queries based on a11y tree
For assertions I'd rather remove the convenience query from jest-dom altogether in favor of an explicit query for just a11y disabled.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with that querying and asserting are two different operations.
Just so I'll understand, what are the footguns you're referring to?
I don't think removing the toBeDisabled assertion is a good idea as it's widely used but maybe I'm missing something.

}

/**
* @param {Element} element -
* @returns {boolean | undefined} - false/true if (not)expanded, undefined if not expand-able
Expand Down Expand Up @@ -382,6 +405,7 @@ export {
computeAriaChecked,
computeAriaPressed,
computeAriaCurrent,
computeAriaDisabled,
computeAriaExpanded,
computeAriaValueNow,
computeAriaValueMax,
Expand Down
5 changes: 5 additions & 0 deletions types/queries.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,11 @@ export interface ByRoleOptions {
* Filters elements by their `aria-current` state. `true` and `false` match `aria-current="true"` and `aria-current="false"` (as well as a missing `aria-current` attribute) respectively.
*/
current?: boolean | string
/**
* If true only includes elements in the query set that are marked as
* disabled in the accessibility tree, i.e., `aria-disabled="true"` or `disabled="true"`.
*/
disabled?: boolean
/**
* If true only includes elements in the query set that are marked as
* expanded in the accessibility tree, i.e., `aria-expanded="true"`
Expand Down