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

Querying aria role superclass #675

Closed
wants to merge 3 commits into from
Closed

Querying aria role superclass #675

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b34806
add role-helpers function to retrieve superclass from role (#448)
Jun 27, 2020
5f9b024
match even superclass roles in queryAllByRole (#448)
Jun 27, 2020
bd1c6d9
fix tests according to superclass roles matching (#448)
Jun 27, 2020
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 src/__tests__/ariaAttributes.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,12 +63,17 @@ test('`selected: true` matches `aria-selected="true"` on supported roles', () =>
).toEqual(['selected-native-columnheader', 'selected-columnheader'])

expect(getAllByRole('gridcell', {selected: true}).map(({id}) => id)).toEqual([
'selected-rowheader',
Copy link
Member

Choose a reason for hiding this comment

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

Appreciate the work but I'm not convinced this will actually be an improvement and not cause more confusion. I'd get behind including subclasses for abstract roles (not really though) but for concrete roles it's a bit confusing that this role would also be considered a gridcell. Because it actually isn't.

In it's current form this will also be a breaking change. This is evident from the changes in existing tests.

Copy link
Member Author

@delca85 delca85 Jun 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I understand your point.
The *byRole queries could get only the abstract super classes if it sounds more reasonable. Otherwise an additional parameter could be added to decide if the hierarchies should be navigated or not.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this change make testing library behave more like a real browser would? I don't consider bug fixes to be breaking changes. Either way I think it's worth it as long as this models browser behavior effectively.

Copy link
Member

Choose a reason for hiding this comment

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

Wouldn't this change make testing library behave more like a real browser would?

I'm not aware of any behavior browsers are adding for aria roles. What do you think would getByRole('gridcell') accomplish if it would return <th> or getByRole('input') for <input type="search" />?

Copy link
Member

@nickserv nickserv Jun 30, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

As far as I understand, this part of the WAI ARIA spec requires that subclasses implement properties of superclasses. https://www.w3.org/WAI/PF/aria/roles#inheritedattributes

In that case it seems to me like these changes are important for matching browser behavior. Additionally, not supporting this would violate the Liskov Substitution Principle (replacing a type with a subclass type needs to still retain the interfaces of the original superclass type).

However one potentially issue I'm seeing is that a user may want to call getByRole when there's already another element of that role with a subclass, so the new behavior could now have multiple ambiguous results. This could be a confusing change in behavior of the tests, but as far as I understand is not actually a semver breaking change because it is a bug fix. Is there a better way to handle this situation? Perhaps we could warn users about this behavior in a minor release (FWIW this is consistent with how the React team has handled similar behavioral changes). Alternatively we could treat this as more of a getByAttribute and add support for attributes other than role while making it more clear that subclasses are intentionally not supported because the functionality is meant to represent literal attributes, not inherited rules.

Copy link

@Dreamsorcerer Dreamsorcerer Dec 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think the other concerns are mostly related to backwards compatibility. Abstract roles have no impact on that. If only abstract roles are included, then it seems like it should be easy to include while providing 90+% of what users actually want out of this.

I'm going to need to see a real use case.

A real use case for testing? The above examples are actual tests I tried to write for my application, so that is a real use case.

The application is dynamically generated from a schema, so validating that the inputs appear in the expected order is an important detail (while also saving me writing several different expect()s, one for each input).

Copy link

@Dreamsorcerer Dreamsorcerer Dec 29, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The above examples are actual tests I tried to write for my application, so that is a real use case.

e.g. Here are exactly the same kind of tests on a table when viewing the data (rather than editing in a form):
https://github.com/aio-libs/aiohttp-admin/blob/ee9498171d0f608574c5c80fb7dc80c0dac51af9/admin-js/tests/simple.test.js#L19

Copy link
Member

Choose a reason for hiding this comment

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

I'm open to looking at a new PR which handles abstract roles.

Can you show me how your test would change with this new feature?

Copy link

Choose a reason for hiding this comment

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

Can you show me how your test would change with this new feature?

The first test examples above, which operate on inputs of a form, would work. Today they don't (the last linked test is for the view mode, where data is in a table). My in-progress tests locally currently look like:

expect(within(edit).getByLabelText("Id *")).toHaveValue(1);
expect(within(edit).getByLabelText("Num *")).toHaveValue(5);
expect(within(edit).getByLabelText("Optional Num")).toHaveValue(null);
expect(within(edit).getByLabelText("Value *")).toHaveValue("first");

Which fails to test ordering while being very verbose. This would become:

expect(within(edit).getAllByRole("input").map((e) => e.value)).toEqual([1, 5, null, "first"]);

Probably with a second line to assert labels as mentioned above.

Copy link
Member

Choose a reason for hiding this comment

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

That makes sense.

I don't actively maintain this project, but I think a PR would be the next step.

'selected-native-columnheader',
'selected-columnheader',
'selected-native-rowheader',
'selected-gridcell',
])

expect(getAllByRole('option', {selected: true}).map(({id}) => id)).toEqual([
'selected-native-option',
'selected-listbox-option',
'selected-treeitem',
])

expect(
Expand Down
8 changes: 4 additions & 4 deletions src/__tests__/element-queries.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -521,11 +521,11 @@ test('queryAllByRole returns semantic html elements', () => {
</form>
`)

expect(queryAllByRole(/table/i)).toHaveLength(1)
expect(queryAllByRole(/tabl/i, {exact: false})).toHaveLength(1)
expect(queryAllByRole(/table/i)).toHaveLength(2)
expect(queryAllByRole(/tabl/i, {exact: false})).toHaveLength(2)
expect(queryAllByRole(/columnheader/i)).toHaveLength(1)
expect(queryAllByRole(/rowheader/i)).toHaveLength(1)
expect(queryAllByRole(/grid/i)).toHaveLength(1)
expect(queryAllByRole(/grid/i)).toHaveLength(3)
expect(queryAllByRole(/form/i)).toHaveLength(0)
expect(queryAllByRole(/button/i)).toHaveLength(1)
expect(queryAllByRole(/heading/i)).toHaveLength(6)
Expand All @@ -536,7 +536,7 @@ test('queryAllByRole returns semantic html elements', () => {
expect(queryAllByRole(/radio/i)).toHaveLength(1)
expect(queryAllByRole('row')).toHaveLength(3)
expect(queryAllByRole(/rowgroup/i)).toHaveLength(2)
expect(queryAllByRole(/(table)|(textbox)/i)).toHaveLength(3)
expect(queryAllByRole(/(table)|(textbox)/i)).toHaveLength(4)
expect(queryAllByRole(/img/i)).toHaveLength(1)
expect(queryAllByRole('meter')).toHaveLength(1)
expect(queryAllByRole('progressbar')).toHaveLength(0)
Expand Down
45 changes: 44 additions & 1 deletion src/__tests__/role-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import {
logRoles,
getImplicitAriaRoles,
isInaccessible,
getRolesAndSuperClassByRoles,
} from '../role-helpers'
import {render} from './helpers/test-utils'

Expand Down Expand Up @@ -181,6 +182,48 @@ test.each([
])('shouldExcludeFromA11yTree for %s returns %p', (html, expected) => {
const {container} = render(html)
container.firstChild.appendChild(document.createElement('button'))

expect(isInaccessible(container.querySelector('button'))).toBe(expected)
})

test.each([
['', []],
['roletype', ['roletype']],
['switch', ['roletype', 'widget', 'input', 'checkbox', 'switch']],
['input', ['roletype', 'widget', 'input']],
[
'select',
[
'roletype',
'widget',
'composite',
'structure',
'section',
'group',
'select',
],
],
[
['switch', 'input'],
['roletype', 'widget', 'input', 'checkbox', 'switch'],
],
[
['switch', 'select'],
[
'roletype',
'widget',
'input',
'checkbox',
'composite',
'structure',
'section',
'group',
'switch',
'select',
],
],
])(
'getSuperClassByRole returns expected roles as super class for given roles',
(roles, expected) => {
expect(getRolesAndSuperClassByRoles(roles).sort()).toEqual(expected.sort())
},
)
44 changes: 43 additions & 1 deletion src/__tests__/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,9 @@ Here are the accessible roles:
test('has no useful error message in findBy', async () => {
const {findByRole} = render(`<li />`)

await expect(findByRole('option', {timeout: 1})).rejects.toThrow('Unable to find role="option"')
await expect(findByRole('option', {timeout: 1})).rejects.toThrow(
'Unable to find role="option"',
)
})

test('explicit role is most specific', () => {
Expand Down Expand Up @@ -378,6 +380,46 @@ Here are the accessible roles:
`)
})

test('superclass roles are retrieved when are abstract roles', () => {
const {getAllByRole} = render(`
<>
<input type="text" />
<input type="checkbox" />
<textarea />
</>`)
expect(getAllByRole('textbox')).toHaveLength(2)
expect(getAllByRole('input')).toHaveLength(3)
})

test('superclass roles are retrieved for explicitly defined roles', () => {
const {getAllByRole} = render(`
<>
<input type="checkbox" />
<span role="switch" aria-checked="true" />
</>
`)
expect(getAllByRole('switch')).toHaveLength(1)
expect(getAllByRole('checkbox')).toHaveLength(2)
})

test('superclass list has found when the defined role is feed', () => {
const {getAllByRole} = render(`
<>
<ul>
<li>abc</li>
</ul>
<div role="feed">
<article role="listitem">xyz</article>
</div>
<ul role="feed">
<li role="article">def</li>
</ul>
</>
`)
expect(getAllByRole('list')).toHaveLength(3)
expect(getAllByRole('feed')).toHaveLength(2)
})

describe('configuration', () => {
let originalConfig
beforeEach(() => {
Expand Down
21 changes: 12 additions & 9 deletions src/queries/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
prettyRoles,
isInaccessible,
isSubtreeInaccessible,
getRolesAndSuperClassByRoles,
} from '../role-helpers'
import {wrapAllByQueryWithSuggestion} from '../query-helpers'
import {checkContainerType} from '../helpers'
Expand Down Expand Up @@ -58,22 +59,24 @@ function queryAllByRole(
if (isRoleSpecifiedExplicitly) {
const roleValue = node.getAttribute('role')
if (queryFallbacks) {
return roleValue
.split(' ')
.filter(Boolean)
.some(text => matcher(text, node, role, matchNormalizer))
const roles = roleValue.split(' ').filter(Boolean)
return getRolesAndSuperClassByRoles(roles).some(text =>
matcher(text, node, role, matchNormalizer),
)
}
// if a custom normalizer is passed then let normalizer handle the role value
if (normalizer) {
return matcher(roleValue, node, role, matchNormalizer)
return getRolesAndSuperClassByRoles(roleValue).some(text =>
matcher(text, node, role, matchNormalizer),
)
}
// other wise only send the first word to match
const [firstWord] = roleValue.split(' ')
return matcher(firstWord, node, role, matchNormalizer)
const roles = getRolesAndSuperClassByRoles(roleValue.split(' ')[0])
return roles.some(text => matcher(text, node, role, matchNormalizer))
}

const implicitRoles = getImplicitAriaRoles(node)

let implicitRoles = getImplicitAriaRoles(node)
implicitRoles = getRolesAndSuperClassByRoles(implicitRoles)
return implicitRoles.some(implicitRole =>
matcher(implicitRole, node, role, matchNormalizer),
)
Expand Down
25 changes: 24 additions & 1 deletion src/role-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import {elementRoles} from 'aria-query'
import {elementRoles, roles as rolesMap} from 'aria-query'
import {computeAccessibleName} from 'dom-accessibility-api'
import {prettyDOM} from './pretty-dom'

Expand Down Expand Up @@ -196,6 +196,28 @@ function computeAriaSelected(element) {
return undefined
}

function getRolesAndSuperClassByRoles(roles) {
if (!roles || roles.length === 0) {
return []
}
const rolesList = Array.isArray(roles) ? roles : [roles]
const superClasses = rolesList.reduce(
(acc, role) => {
const roleDefinition = rolesMap.get(role)
if (!roleDefinition) {
return acc
}
const superClassesFlatten = roleDefinition.superClass.reduce(
(superClassesList, superClass) => [...superClassesList, ...superClass],
[],
)
return [...acc, ...superClassesFlatten]
},
[...rolesList],
)
return Array.from(new Set(superClasses))
}

export {
getRoles,
logRoles,
Expand All @@ -204,4 +226,5 @@ export {
prettyRoles,
isInaccessible,
computeAriaSelected,
getRolesAndSuperClassByRoles,
}