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.

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

feat(byRole): Add description filter #1120

Merged

Conversation

PaquitoSoft
Copy link
Contributor

@PaquitoSoft PaquitoSoft commented Apr 1, 2022

What:

Add a new description option to ByRole queries.
This helps finding elements which are aria described.

Why:

We were implementing a React component to display the usual popup notifications and, when reading about accessibility considerations, we found we should use ariadialog role and we also should use aria-describedby to reference the message (spec documentation).

When writing some tests we wanted to have one where we could close an specific notification when a list of them were rendered in a view.

ByRole queries currently support filtering results by accessible name, but no filtering by accessible description.

How:

Since this project is already using the dom-accessibility-api dependency to resolve the accessible name and that library also expose a function to resolve the accessible description, I would like to introduce this proposal where we implement filtering by description when using ByRole queries.

Given some HTML markup like this one:

<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">Your session is about to expire!</div>
  </li>
</ul>

Now we can find alertdialog elements with this kind of query:

const notification = getByRole('alertdialog', {
  description: 'Your session is about to expire!',
});

I used this PR (when namefiltering was added to ByRole queries) to guide my proposed implementation.

I just wanted to create the proposal with some actual code so maybe it's easier to reason about and maybe there are other thing to take into account that we might discuss as a follow-up of this starting point.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • TypeScript definitions updated
  • Ready to be merged

References
These are the documents I used as reference for this proposal:

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 1, 2022

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 2c18fc5:

Sandbox Source
react-testing-library-examples Configuration

@PaquitoSoft PaquitoSoft changed the title feat(byrole): Add description filter feat(byRole): Add description filter Apr 1, 2022
@codecov
Copy link

codecov bot commented Apr 4, 2022

Codecov Report

Merging #1120 (2c18fc5) into main (6b99a7e) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##              main     #1120   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           25        25           
  Lines          970       985   +15     
  Branches       316       322    +6     
=========================================
+ Hits           970       985   +15     
Flag Coverage Δ
node-12 100.00% <100.00%> (ø)
node-14 100.00% <100.00%> (ø)
node-16 100.00% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/queries/role.js 100.00% <100.00%> (ø)
src/role-helpers.js 100.00% <100.00%> (ø)

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 6b99a7e...2c18fc5. Read the comment docs.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Very nice, thanks!

We need documentation in https://github.com/testing-library/testing-library-docs/edit/main/docs/queries/byrole.mdx (under "Options") for this to be mergable.

@eps1lon eps1lon added the enhancement New feature or request label Apr 4, 2022
@PaquitoSoft
Copy link
Contributor Author

PaquitoSoft commented Apr 4, 2022

Very nice, thanks!

We need documentation in https://github.com/testing-library/testing-library-docs/edit/main/docs/queries/byrole.mdx (under "Options") for this to be mergable.

Hey @eps1lon, you mention to add the new option under the Options section but I noticed the name property is not there (it is explained before that section) so I added on that.

Should I move that explanation to the Options section?
Should another PR (maybe out of current's scope) do the same for name prop?

Thanks!

@eps1lon
Copy link
Member

eps1lon commented Apr 4, 2022

Hey @eps1lon, you mention to add the new option under the Options section but I noticed the name property is not there (it is explained before that section) so testing-library/testing-library-docs#1035.

We should move the name section to options as well. But let's do that in a follow-up

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.

src/__tests__/role.js Outdated Show resolved Hide resolved
Co-authored-by: Tim Deschryver <28659384+timdeschryver@users.noreply.github.com>
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Very well crafted PR. Thanks!

@eps1lon eps1lon merged commit 84c7290 into testing-library:main Apr 5, 2022
@github-actions
Copy link

github-actions bot commented Apr 5, 2022

🎉 This PR is included in version 8.13.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@PaquitoSoft PaquitoSoft deleted the pr/role-filter-description branch April 5, 2022 13:30
@PaquitoSoft
Copy link
Contributor Author

Thanks for accepting this little feature and for your help 👍

@vctormb
Copy link

vctormb commented May 26, 2022

I had some tests like this:

<button title="this is the title">click me</button>

screen.getByRole('button', { name: /this is the title/i }) // <- before it was getting by its title

now it only gets by its children

<button title="this is the title">click me</button>

screen.getByRole('button', { name: /this is the title/i }) // <- failing
screen.getByRole('button', { name: /click me/i }) // <- OK

is this problem related to this change?

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants