Skip to content

feat(byRole): Add name filter #408

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 10 commits into from
Mar 4, 2020
Merged

feat(byRole): Add name filter #408

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e8193d8
feat(byRole): Add `name` filter
eps1lon Nov 29, 2019
a70ed30
Use common place to define defaults
eps1lon Jan 27, 2020
5c41d96
feat(byRole): Implement TextMatch for `name` option
eps1lon Jan 27, 2020
225887a
chore(lint): fix warnings
eps1lon Jan 27, 2020
608fd2f
Final dap review
eps1lon Feb 1, 2020
3e683d7
describe test better
eps1lon Feb 4, 2020
cfce8d9
Reuse matches in name filter
eps1lon Feb 4, 2020
d6ac518
Include accname by default in logRoles
eps1lon Feb 4, 2020
3fcf430
Update src/role-helpers.js
kentcdodds Mar 4, 2020
7a53e33
remove includeName option
kentcdodds Mar 4, 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
1 change: 1 addition & 0 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,7 @@
"@sheerun/mutationobserver-shim": "^0.3.2",
"@types/testing-library__dom": "^6.0.0",
"aria-query": "3.0.0",
"dom-accessibility-api": "^0.3.0",
"pretty-format": "^24.9.0",
"wait-for-expect": "^3.0.0"
},
Expand Down
29 changes: 29 additions & 0 deletions src/__tests__/__snapshots__/role-helpers.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,15 @@
exports[`logRoles calls console.log with output from prettyRoles 1`] = `
"region:

Name "":
<section
data-testid="a-section"
/>

--------------------------------------------------
link:

Name "link":
<a
data-testid="a-link"
href="http://whatever.com"
Expand All @@ -18,135 +20,159 @@ link:
--------------------------------------------------
navigation:

Name "":
<nav
data-testid="a-nav"
/>

--------------------------------------------------
heading:

Name "Main Heading":
<h1
data-testid="a-h1"
/>

Name "Sub Heading":
<h2
data-testid="a-h2"
/>

Name "Tertiary Heading":
<h3
data-testid="a-h3"
/>

--------------------------------------------------
article:

Name "":
<article
data-testid="a-article"
/>

--------------------------------------------------
command:

Name "":
<menuitem
data-testid="a-menuitem-1"
/>

Name "":
<menuitem
data-testid="a-menuitem-2"
/>

--------------------------------------------------
menuitem:

Name "":
<menuitem
data-testid="a-menuitem-1"
/>

Name "":
<menuitem
data-testid="a-menuitem-2"
/>

--------------------------------------------------
list:

Name "":
<ul
data-testid="a-list"
/>

Name "":
<ul
data-testid="b-list"
/>

--------------------------------------------------
listitem:

Name "":
<li
data-testid="a-list-item-1"
/>

Name "":
<li
data-testid="a-list-item-2"
/>

Name "":
<li
data-testid="b-list-item-1"
/>

Name "":
<li
data-testid="b-list-item-2"
/>

--------------------------------------------------
table:

Name "":
<table
data-testid="a-table"
/>

--------------------------------------------------
rowgroup:

Name "":
<tbody
data-testid="a-tbody"
/>

--------------------------------------------------
row:

Name "Cell 1 Cell 2 Cell 3":
<tr
data-testid="a-row"
/>

--------------------------------------------------
cell:

Name "Cell 1":
<td
data-testid="a-cell-1"
/>

Name "Cell 2":
<td
data-testid="a-cell-2"
/>

Name "Cell 3":
<td
data-testid="a-cell-3"
/>

--------------------------------------------------
form:

Name "":
<form
data-testid="a-form"
/>

--------------------------------------------------
radio:

Name "":
<input
data-testid="a-radio-1"
type="radio"
/>

Name "":
<input
data-testid="a-radio-2"
type="radio"
Expand All @@ -155,16 +181,19 @@ radio:
--------------------------------------------------
textbox:

Name "":
<input
data-testid="a-input-1"
type="text"
/>

Name "":
<input
data-testid="a-input-2"
type="text"
/>

Name "":
<textarea
data-testid="a-textarea"
/>
Expand Down
143 changes: 142 additions & 1 deletion src/__tests__/role.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import {configure, getConfig} from '../config'
import {render} from './helpers/test-utils'
import {getQueriesForElement} from '../get-queries-for-element'
import {render, renderIntoDocument} from './helpers/test-utils'

test('by default logs accessible roles when it fails', () => {
const {getByRole} = render(`<h1>Hi</h1>`)
Expand All @@ -10,6 +11,7 @@ Here are the accessible roles:

heading:

Name "Hi":
<h1 />

--------------------------------------------------
Expand All @@ -32,6 +34,7 @@ Here are the available roles:

heading:

Name "Hi":
<h1 />

--------------------------------------------------
Expand Down Expand Up @@ -183,6 +186,144 @@ test('can include inaccessible roles', () => {
expect(getByRole('list', {hidden: true})).not.toBeNull()
})

test('can be filtered by accessible name', () => {
const {getByRole} = renderIntoDocument(
`
<div>
<h1>Order</h1>
<h2>Delivery Adress</h2>
<form aria-label="Delivery Adress">
<label>
<div>Street</div>
<input type="text" />
</label>
<input type="submit" />
</form>
<h2>Invoice Adress</h2>
<form aria-label="Invoice Adress">
<label>
<div>Street</div>
<input type="text" />
</label>
<input type="submit" />
</form>
</div>`,
)

const deliveryForm = getByRole('form', {name: 'Delivery Adress'})
expect(deliveryForm).not.toBeNull()

expect(
// TODO: upstream bug in `aria-query`; should be `button` role
Copy link
Member

Choose a reason for hiding this comment

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

Should we contribute a fix to aria-query before merging this?

Copy link
Member Author

Choose a reason for hiding this comment

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

Right. They released a new major. Opening a separate PR that bumps aria-query and then we'll see if that worked.

getQueriesForElement(deliveryForm).getByRole('textbox', {name: 'Submit'}),
).not.toBeNull()

const invoiceForm = getByRole('form', {name: 'Delivery Adress'})
expect(invoiceForm).not.toBeNull()

expect(
getQueriesForElement(invoiceForm).getByRole('textbox', {name: 'Street'}),
).not.toBeNull()
})

test('accessible name comparison is case sensitive', () => {
const {getByRole} = render(`<h1>Sign <em>up</em></h1>`)

// actual: "Sign up",
// queried: "Sign Up"
expect(() => getByRole('heading', {name: 'Sign Up'}))
.toThrowErrorMatchingInlineSnapshot(`
"Unable to find an accessible element with the role "heading" and name "Sign Up"

Here are the accessible roles:

heading:

Name "Sign up":
Copy link
Member

Choose a reason for hiding this comment

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

The only difference between this and the query is the u is not capitalized. Could we make this difference more stark so this makes more sense why it failed to find an element please? Thanks!

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 had trouble understanding it as well. I would like to keep the test to make sure that { name: string } is always case-sensitive. I'll add a comment to the test explaining the issue.

<h1 />

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

<div>
<h1>
Sign
<em>
up
</em>
</h1>
</div>"
`)
})

test('accessible name filter implements TextMatch', () => {
const {getByRole} = render(
`<h1>Sign <em>up</em></h1><h2>Details</h2><h2>Your Signature</h2>`,
)

// subset via regex
expect(getByRole('heading', {name: /gn u/})).not.toBeNull()
// regex
expect(getByRole('heading', {name: /^sign/i})).not.toBeNull()
// function
expect(
getByRole('heading', {
name: (name, element) => {
return element.nodeName === 'H2' && name === 'Your Signature'
},
}),
).not.toBeNull()
})

test('TextMatch serialization in error message', () => {
const {getByRole} = render(`<h1>Sign <em>up</em></h1>`)

expect(() => getByRole('heading', {name: /Login/}))
Copy link
Member

Choose a reason for hiding this comment

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

Not sure it's necessary to have all three of these because they're all testing the same codepath and their differences are covered in other tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's more about showcasing how the name argument is serialized. It may be too verbose in which case the codepath should change. And overall: Isn't that testing implementation details 😉

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
expect(() => getByRole('heading', {name: /Login/}))
expect(() => getByRole('heading', {name: /something that does not match/}))

I think this makes it more clear that it's not just a typo...

.toThrowErrorMatchingInlineSnapshot(`
"Unable to find an accessible element with the role "heading" and name \`/Login/\`

Here are the accessible roles:

heading:

Name "Sign up":
<h1 />

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

<div>
<h1>
Sign
<em>
up
</em>
</h1>
</div>"
`)

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

Here are the accessible roles:

heading:

Name "Sign up":
<h1 />

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

<div>
<h1>
Sign
<em>
up
</em>
</h1>
</div>"
`)
})

describe('configuration', () => {
let originalConfig
beforeEach(() => {
Expand Down
Loading