Skip to content

feat(byRole): Add description filter #1120

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

Merged
merged 3 commits into from
Apr 5, 2022
Merged

feat(byRole): Add description filter #1120

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fe9e665
feat(byrole): Add description filter
PaquitoSoft Apr 1, 2022
3bca028
refactor(byRole): Add new property to byRole options type
PaquitoSoft Apr 4, 2022
2c18fc5
Update src/__tests__/role.js
eps1lon Apr 5, 2022
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
174 changes: 174 additions & 0 deletions src/__tests__/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -572,3 +572,177 @@ test('should find the input using type property instead of attribute', () => {
const {getByRole} = render('<input type="124">')
expect(getByRole('textbox')).not.toBeNull()
})

test('can be filtered by accessible description', () => {
const targetedNotificationMessage = 'Your session is about to expire!'
const {getByRole} = renderIntoDocument(
`
<ul>
<li role="alertdialog" aria-describedby="notification-id-1">
<div><button>Close</button></div>
<div id="notification-id-1">You have unread emails</div>
</li>
<li role="alertdialog" aria-describedby="notification-id-2">
<div><button>Close</button></div>
<div id="notification-id-2">${targetedNotificationMessage}</div>
</li>
</ul>`,
)

const notification = getByRole('alertdialog', {
description: targetedNotificationMessage,
})

expect(notification).not.toBeNull()
expect(notification).toHaveTextContent(targetedNotificationMessage)

expect(
getQueriesForElement(notification).getByRole('button', {name: 'Close'}),
).not.toBeNull()
})

test('error should include description when filtering a no results are found', () => {
const targetedNotificationMessage = 'Your session is about to expire!'
const {getByRole} = renderIntoDocument(
`<div role="dialog" aria-describedby="some-id"><div><button>Close</button></div><div id="some-id">${targetedNotificationMessage}</div></div>`,
)

expect(() =>
getByRole('alertdialog', {description: targetedNotificationMessage}),
).toThrowErrorMatchingInlineSnapshot(`
Unable to find an accessible element with the role "alertdialog" and description "Your session is about to expire!"

Here are the accessible roles:

dialog:

Name "":
Description "Your session is about to expire!":
<div
aria-describedby="some-id"
role="dialog"
/>

--------------------------------------------------
button:

Name "Close":
Description "":
<button />

--------------------------------------------------

Ignored nodes: comments, <script />, <style />
<body>
<div
aria-describedby="some-id"
role="dialog"
>
<div>
<button>
Close
</button>
</div>
<div
id="some-id"
>
Your session is about to expire!
</div>
</div>
</body>
`)
})

test('TextMatch serialization for description filter in error message', () => {
const {getByRole} = renderIntoDocument(
`<div role="alertdialog" aria-describedby="some-id"><div><button>Close</button></div><div id="some-id">Your session is about to expire!</div></div>`,
)

expect(() => getByRole('alertdialog', {description: /unknown description/}))
.toThrowErrorMatchingInlineSnapshot(`
Unable to find an accessible element with the role "alertdialog" and description \`/unknown description/\`

Here are the accessible roles:

alertdialog:

Name "":
Description "Your session is about to expire!":
<div
aria-describedby="some-id"
role="alertdialog"
/>

--------------------------------------------------
button:

Name "Close":
Description "":
<button />

--------------------------------------------------

Ignored nodes: comments, <script />, <style />
<body>
<div
aria-describedby="some-id"
role="alertdialog"
>
<div>
<button>
Close
</button>
</div>
<div
id="some-id"
>
Your session is about to expire!
</div>
</div>
</body>
`)

expect(() => getByRole('alertdialog', {description: () => false}))
.toThrowErrorMatchingInlineSnapshot(`
Unable to find an accessible element with the role "alertdialog" and description \`() => false\`

Here are the accessible roles:

alertdialog:

Name "":
Description "Your session is about to expire!":
<div
aria-describedby="some-id"
role="alertdialog"
/>

--------------------------------------------------
button:

Name "Close":
Description "":
<button />

--------------------------------------------------

Ignored nodes: comments, <script />, <style />
<body>
<div
aria-describedby="some-id"
role="alertdialog"
>
<div>
<button>
Close
</button>
</div>
<div
id="some-id"
>
Your session is about to expire!
</div>
</div>
</body>
`)
})
36 changes: 33 additions & 3 deletions src/queries/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
import {computeAccessibleName} from 'dom-accessibility-api'
import {
computeAccessibleDescription,
computeAccessibleName,
} from 'dom-accessibility-api'
import {roles as allRoles, roleElements} from 'aria-query'
import {
computeAriaSelected,
Expand Down Expand Up @@ -30,6 +33,7 @@ function queryAllByRole(
collapseWhitespace,
hidden = getConfig().defaultHidden,
name,
description,
trim,
normalizer,
queryFallbacks = false,
Expand Down Expand Up @@ -169,6 +173,22 @@ function queryAllByRole(
text => text,
)
})
.filter(element => {
if (description === undefined) {
// Don't care
return true
}

return matches(
computeAccessibleDescription(element, {
computedStyleSupportsPseudoElements:
getConfig().computedStyleSupportsPseudoElements,
}),
element,
description,
text => text,
)
})
.filter(element => {
return hidden === false
? isInaccessible(element, {
Expand Down Expand Up @@ -216,7 +236,7 @@ const getMultipleError = (c, role, {name} = {}) => {
const getMissingError = (
container,
role,
{hidden = getConfig().defaultHidden, name} = {},
{hidden = getConfig().defaultHidden, name, description} = {},
) => {
if (getConfig()._disableExpensiveErrorDiagnostics) {
return `Unable to find role="${role}"`
Expand All @@ -227,6 +247,7 @@ const getMissingError = (
roles += prettyRoles(childElement, {
hidden,
includeName: name !== undefined,
includeDescription: description !== undefined,
})
})
let roleMessage
Expand Down Expand Up @@ -257,10 +278,19 @@ Here are the ${hidden === false ? 'accessible' : 'available'} roles:
nameHint = ` and name \`${name}\``
}

let descriptionHint = ''
if (description === undefined) {
descriptionHint = ''
} else if (typeof description === 'string') {
descriptionHint = ` and description "${description}"`
} else {
descriptionHint = ` and description \`${description}\``
}

return `
Unable to find an ${
hidden === false ? 'accessible ' : ''
}element with the role "${role}"${nameHint}
}element with the role "${role}"${nameHint}${descriptionHint}

${roleMessage}`.trim()
}
Expand Down
20 changes: 18 additions & 2 deletions src/role-helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import {elementRoles} from 'aria-query'
import {computeAccessibleName} from 'dom-accessibility-api'
import {
computeAccessibleDescription,
computeAccessibleName,
} from 'dom-accessibility-api'
import {prettyDOM} from './pretty-dom'
import {getConfig} from './config'

Expand Down Expand Up @@ -178,7 +181,7 @@ function getRoles(container, {hidden = false} = {}) {
}, {})
}

function prettyRoles(dom, {hidden}) {
function prettyRoles(dom, {hidden, includeDescription}) {
const roles = getRoles(dom, {hidden})
// We prefer to skip generic role, we don't recommend it
return Object.entries(roles)
Expand All @@ -191,7 +194,20 @@ function prettyRoles(dom, {hidden}) {
computedStyleSupportsPseudoElements:
getConfig().computedStyleSupportsPseudoElements,
})}":\n`

const domString = prettyDOM(el.cloneNode(false))

if (includeDescription) {
Copy link
Member

Choose a reason for hiding this comment

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

@eps1lon not related to this PR, but should we also do a includeName?
It seems like we're already providing the property here but we're not doing anything with it.

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 we originally decide to always include it to help people discover the name option (would have to check the original PR though).

Though I don't think this necessarily applies to description since it's

  1. more likely to not be set
  2. quite noisy (concerning length) if you don't care about it

Copy link
Member

Choose a reason for hiding this comment

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

I'll take a look at the history.
If we indeed decided to always include it (which makes sense imho), I'll remove the code where we now add the includeName argument.

const descriptionString = `Description "${computeAccessibleDescription(
el,
{
computedStyleSupportsPseudoElements:
getConfig().computedStyleSupportsPseudoElements,
},
)}":\n`
return `${nameString}${descriptionString}${domString}`
}

return `${nameString}${domString}`
})
.join('\n\n')
Expand Down
7 changes: 7 additions & 0 deletions types/queries.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,6 +115,13 @@ export interface ByRoleOptions extends MatcherOptions {
| RegExp
| string
| ((accessibleName: string, element: Element) => boolean)
/**
* Only considers elements with the specified accessible description.
*/
description?:
| RegExp
| string
| ((accessibleDescription: string, element: Element) => boolean)
}

export type AllByRole<T extends HTMLElement = HTMLElement> = (
Expand Down