feat(byRole): Add `name` filter #408

eps1lon · 2019-11-29T12:19:21Z

What:
Add a name option to ByRole queries. If a name is provided then the error messages will include the accessible name of each element with a role.

Why:

Better alternative to byText (especially for buttons) or byLabelText when aria-label or aria-labelledby is used (e.g. for custom Select implementations) or byTestId for "semantic" elements e.g. menuitem.

There's a bit more to write in https://testing-library.com/docs/guide-which-query to explain why you still might want to use byLabelText('street') instead of byRole('textbox', { name: 'street' }).

How:

Using https://github.com/eps1lon/dom-accessibility-api#readme which implements https://www.w3.org/TR/accname-1.2/ and is tested against web-platform-tests

Checklist:

Documentation added to the
docs site. See feat(byRole): Add name option testing-library-docs#368 (Preview)
I've prepared a PR for types targeting
DefinitelyTyped
Tests
Ready to be merged

I would hope that (after review and testing phase) we can move that package under the testing-library umbrella and move isInaccessible into dom-accessibility-api. In the end I'd like to add a single configuration that allows mocking window.getComputedStyle for the a11y context. That operation is quite expensive and I think allowing to pretend that every element is visible is sufficient for unit testing. The rationale being that we don't exclude certain groups of people by (accidentally) implementing inaccessible component but instead exclude everyone if we assert on elements that are actually visually hidden.

codesandbox-ci · 2019-11-29T12:20:08Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 7a53e33:

Sandbox	Source
inspiring-morning-z3ptt	Configuration

kentcdodds

Very very cool. Really looking forward to this feature!

src/queries/role.js

kentcdodds · 2019-11-30T02:49:01Z

Also, I completely agree with your comment about moving things around a bit 👍

alexkrolick · 2019-12-02T18:51:07Z

One thing I am wondering is why this shouldn't support combining the other matchers as well:

// combine with displayValue
getByRole('textbox', {displayValue: 'username'})

// combine with aria-labelledby or htmlFor
getByRole('dialog', {label: 'uh oh'})

// Inverted? Anything that takes a `selector` option could take `role`?
// This actually seems like a more natural modifier - it increases specificity
// of content-based queries.
getByLabelText('uh oh', {role: 'dialog'})

Seems like a one off when we should be figuring out how to intersect queries in general. There have been a few proposals on how to do that.

eps1lon · 2019-12-03T20:00:28Z

getByLabelText('uh oh', {role: 'dialog'})

Not sure what this is supposed to query. A dialog doesn't have a label. It's important to note that getByLabelText('street') is not equivalent to getByRole('textbox', { name: 'street' }) because the <label> element has other behavior as well: Clicking a label will focus the control it is associated with.

getByRole('textbox', {displayValue: 'username'})

I would have to look into this more. I don't know how display value is handled currently but we need consider that one might include the actual value attribute which is sent to the server/form library while e.g. <div role="textbox">value</div> does no such thing necessarily.

But this will definitely be part of the docs since it's apparently not obvious when to use what.

alexkrolick · 2019-12-03T21:26:21Z

That was just an example, but this is from MDN:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/dialog_role

<div role="dialog" aria-labelledby="dialog1Title" aria-describedby="dialog1Desc">
  <h2 id="dialog1Title">Your personal details were successfully updated</h2>
  <p id="dialog1Desc">You can change your details at any time in the user account section.</p>
  <button>Close</button>
</div>

eps1lon · 2019-12-04T14:21:30Z

That was just an example, but this is from MDN:

Sure. But be careful to not mix a <label> with the accessible name. They share some features but also have distinct features (e.g. clicking a <label> focuses the associated input).

If you used getByLabelText for the accessible name only then you can switch. If your test implied the click-focus feature then you shouldn't (or if you do add another test for click-focus).

eps1lon · 2020-01-15T19:29:53Z

Plan for this in the next week:

update branch
write docs (most important part of this feature): feat(byRole): Add name option testing-library-docs#368
~~[ ] unify approach for "loose mode" (where styles are ignored)~~ Will propose later. Since this is not used by default we don't introduce performance regressions that nobody asked for. We can iterate later if this is an issue.
another review pass for dom-accessibility-api
Implement feat(byRole): Add name filter #408 (comment)
then this is ready to go from my perspective

codecov · 2020-01-25T16:54:02Z

Codecov Report

Merging #408 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #408   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          22     22           
  Lines         385    397   +12     
  Branches       91     94    +3     
=====================================
+ Hits          385    397   +12

Impacted Files	Coverage Δ
src/role-helpers.js	`100% <100%> (ø)`	⬆️
src/queries/role.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed36447...7a53e33. Read the comment docs.

kentcdodds · 2020-01-28T21:46:55Z

Awesome! Let us know when you're ready for a review. Thanks a bunch!

bowernite · 2020-02-02T16:01:04Z

I'm also curious about the inversion that @alexkrolick mentioned. Maybe the example he gave wasn't applicable (finding a dialog by label text), but others would be.

getByText(/my website/i, { role: 'heading' })

This seems a little more expressive, at least for this use case than the query added in this PR:

getByRole('heading', { name: /my website/i })

I want to get something with specific text, and the fact that it's a heading is supplementary to that. I concede that this is nitpicky, though.

eps1lon · 2020-02-02T16:44:54Z

I want to get something with specific text, and the fact that it's a heading is supplementary to that. I concede that this is nitpicky, though.

I wouldn't agree with that since roles might be button, dialog, heading etc. These are not secondary at all.

The point is to encourage writing out what kind of element you're dealing with. It's more important that the element is e.g. a button than that it reads "log in". The first one is part of the UI/UX. The second one is subject to localization.

bowernite · 2020-02-03T18:53:36Z

Fair enough. I honestly can see scenarios in which either the role or the text are the primary thing you're concerned with.

kentcdodds

Looking awesome. I'm excited for this one :)

kentcdodds · 2020-02-04T16:25:13Z

src/__tests__/role.js

+  expect(deliveryForm).not.toBeNull()
+
+  expect(
+    // TODO: upstream bug in `aria-query`; should be `button` role


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

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

kentcdodds · 2020-02-04T16:28:40Z

src/__tests__/role.js

+
+  heading:
+
+  Name "Sign up":


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!

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.

kentcdodds · 2020-02-04T16:29:44Z

src/__tests__/role.js

+</div>"
+`)
+
+  expect(() => getByRole('heading', {name: /Login/}))


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.

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 😉

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...

src/queries/role.js

kentcdodds · 2020-02-04T16:32:48Z

src/role-helpers.js

        .join('\n\n')

      return `${role}:\n\n${elementsString}\n\n${delimiterBar}`
    })
    .join('\n')
 }

-const logRoles = (dom, {hidden = false} = {}) =>
-  console.log(prettyRoles(dom, {hidden}))
+const logRoles = (dom, {hidden = false, includeName = false} = {}) =>


Why would we not want to always include the name?

Haven't thought much about this. Just wanted to not change existing behavior.
Since this is only used by library consumers it might be a good idea to include the name by default to promote that we now support accessible name queries?

Yes, since it's debug output, it seems fine to change.

eps1lon · 2020-02-04T17:10:43Z

Addressed #408 (comment) and #408 (comment) in 3c7dd0e.

kentcdodds

Cool cool! Looking good :)

kentcdodds · 2020-03-03T16:02:52Z

src/role-helpers.js

+        .map(
+          el =>
+            `${
+              includeName === true
+                ? `Name "${computeAccessibleName(el)}":\n`
+                : ''
+            }${prettyDOM(el.cloneNode(false))}`,
+        )


Prettier makes this look funny. Why don't we do this:

Suggested change

.map(

el =>

`${

includeName === true

? `Name "${computeAccessibleName(el)}":\n`

: ''

}${prettyDOM(el.cloneNode(false))}`,

)

.map(el =>

[

includeName === true

? `Name "${computeAccessibleName(el)}":\n`

: '',

prettyDOM(el.cloneNode(false)),

].join(''),

)

src/role-helpers.js

kentcdodds · 2020-03-03T16:05:04Z

src/role-helpers.js

        .join('\n\n')

      return `${role}:\n\n${elementsString}\n\n${delimiterBar}`
    })
    .join('\n')
 }

-const logRoles = (dom, {hidden = false} = {}) =>
-  console.log(prettyRoles(dom, {hidden}))
+const logRoles = (dom, {hidden = false, includeName = true} = {}) =>


Why do we provide the option to not include the name?

kentcdodds · 2020-03-04T16:01:09Z

I'm working on this now. I'll go ahead and remove the includeName feature because I think it's easier to add it later if we really need it than to include it if we don't really have a use case. Thanks!

kentcdodds · 2020-03-04T16:10:57Z

🎉 This PR is included in version 6.14.0 🎉

The release is available on:

npm package (@latest dist-tag)
GitHub release

Your semantic-release bot 📦🚀

eps1lon · 2020-03-04T16:18:44Z

Agreed with everything. Thanks for finishing this up!

eps1lon added the enhancement New feature or request label Nov 29, 2019

kentcdodds reviewed Nov 30, 2019

View reviewed changes

src/queries/role.js Outdated Show resolved Hide resolved

alexkrolick mentioned this pull request Dec 20, 2019

within for mutliple elements #420

Closed

eps1lon mentioned this pull request Jan 15, 2020

getClosestBy* #327

Closed

eps1lon force-pushed the feat/name branch from dc2fd95 to 21945af Compare January 25, 2020 16:52

eps1lon mentioned this pull request Jan 28, 2020

feat(byRole): Add name option testing-library/testing-library-docs#368

Merged

eps1lon force-pushed the feat/name branch from 670707e to 2e78cb1 Compare February 1, 2020 20:46

eps1lon requested review from alexkrolick and kentcdodds February 1, 2020 20:53

eps1lon mentioned this pull request Feb 2, 2020

Add toHaveRole testing-library/jest-dom#143

Open

kentcdodds reviewed Feb 4, 2020

View reviewed changes

eps1lon mentioned this pull request Feb 4, 2020

fix: Various implicit roles in ByRole #446

Merged

2 tasks

kentcdodds reviewed Mar 3, 2020

View reviewed changes

eps1lon added 3 commits March 4, 2020 09:02

feat(byRole): Add name filter

e8193d8

Use common place to define defaults

a70ed30

feat(byRole): Implement TextMatch for name option

5c41d96

eps1lon and others added 7 commits March 4, 2020 09:02

chore(lint): fix warnings

225887a

Final dap review

608fd2f

describe test better

3e683d7

Reuse matches in name filter

cfce8d9

Include accname by default in logRoles

d6ac518

Update src/role-helpers.js

3fcf430

remove includeName option

7a53e33

kentcdodds force-pushed the feat/name branch from 1de9c26 to 7a53e33 Compare March 4, 2020 16:03

kentcdodds merged commit a98dc9f into testing-library:master Mar 4, 2020

kentcdodds added the released label Mar 4, 2020

eps1lon deleted the feat/name branch March 4, 2020 21:40

eps1lon mentioned this pull request Mar 6, 2020

feat(testing-library/dom): Add name option to byRole DefinitelyTyped/DefinitelyTyped#42892

Merged

9 tasks

CarlosCortizasCT mentioned this pull request Mar 28, 2022

Make <Notification> accessible commercetools/merchant-center-application-kit#2059

Closed

PaquitoSoft mentioned this pull request Apr 1, 2022

feat(byRole): Add description filter #1120

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(byRole): Add `name` filter #408

feat(byRole): Add `name` filter #408

eps1lon commented Nov 29, 2019 •

edited

Loading

codesandbox-ci bot commented Nov 29, 2019 •

edited

Loading

kentcdodds left a comment

kentcdodds commented Nov 30, 2019

alexkrolick commented Dec 2, 2019 •

edited

Loading

eps1lon commented Dec 3, 2019 •

edited

Loading

alexkrolick commented Dec 3, 2019

eps1lon commented Dec 4, 2019

eps1lon commented Jan 15, 2020 •

edited

Loading

codecov bot commented Jan 25, 2020 •

edited

Loading

kentcdodds commented Jan 28, 2020

bowernite commented Feb 2, 2020

eps1lon commented Feb 2, 2020

bowernite commented Feb 3, 2020

kentcdodds left a comment

kentcdodds Feb 4, 2020

eps1lon Feb 4, 2020

kentcdodds Feb 4, 2020

eps1lon Feb 4, 2020

kentcdodds Feb 4, 2020

eps1lon Feb 4, 2020

kentcdodds Mar 3, 2020

kentcdodds Feb 4, 2020

eps1lon Feb 4, 2020

alexkrolick Feb 4, 2020

eps1lon commented Feb 4, 2020

kentcdodds left a comment

kentcdodds Mar 3, 2020

kentcdodds Mar 3, 2020

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

eps1lon commented Mar 4, 2020

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

feat(byRole): Add name filter #408

feat(byRole): Add name filter #408

Conversation

eps1lon commented Nov 29, 2019 • edited Loading

codesandbox-ci bot commented Nov 29, 2019 • edited Loading

kentcdodds left a comment

Choose a reason for hiding this comment

kentcdodds commented Nov 30, 2019

alexkrolick commented Dec 2, 2019 • edited Loading

eps1lon commented Dec 3, 2019 • edited Loading

alexkrolick commented Dec 3, 2019

eps1lon commented Dec 4, 2019

eps1lon commented Jan 15, 2020 • edited Loading

codecov bot commented Jan 25, 2020 • edited Loading

Codecov Report

kentcdodds commented Jan 28, 2020

bowernite commented Feb 2, 2020

eps1lon commented Feb 2, 2020

bowernite commented Feb 3, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eps1lon commented Feb 4, 2020

kentcdodds left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kentcdodds commented Mar 4, 2020

kentcdodds commented Mar 4, 2020

eps1lon commented Mar 4, 2020

feat(byRole): Add `name` filter #408

feat(byRole): Add `name` filter #408

eps1lon commented Nov 29, 2019 •

edited

Loading

codesandbox-ci bot commented Nov 29, 2019 •

edited

Loading

alexkrolick commented Dec 2, 2019 •

edited

Loading

eps1lon commented Dec 3, 2019 •

edited

Loading

eps1lon commented Jan 15, 2020 •

edited

Loading

codecov bot commented Jan 25, 2020 •

edited

Loading