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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Add toHaveRole matcher #572

Merged
merged 7 commits into from Jan 30, 2024

Conversation

fpapado
Copy link
Contributor

@fpapado fpapado commented Jan 28, 2024

What:

This PR adds the toHaveRole matcher described in #143.

Why:

Sometimes, it is desirable to make assertions about the accessible role of an element. While the prioritites in testing-library encourage querying an element by its role, there are times when you already have access to an element, and want to run assertions, without changing the query itself. For example, some codebases prioritise test ids over role queries. This enables more robust accessibility checks for these scenarios :)

How:

In large part, I am porting over dom-testing-library's role helpers to this one.

Under the hood, the role helpers use aria-query which gives programmatic (and auto-updating) access to the ARIA specification and its HTML mappings.

The helpers then compile the different conditions (attributes, DOM tree relationships) into a CSS selector, and provide a function to match an element via the HTMLElement matches() method.

Checklist:

  • Documentation
  • Tests
  • Updated Type Definitions
  • Ready to be merged

Questions for the maintainers

I have two main questions, that I would want the maintainers to clarify:

  1. The current approach relies on using CSS selectors. Are they supported in all the environments that this library supports? Porting the selectors over to attribute checks is doable, in a pretty automated way (we'd change the implementation of processing aria-query mappings)
  2. Since we are copying over the function from dom-testing-library, I have a gut feeling that we could move these helpers to a shared package. I noticed that @eps1lon's dom-accessibility-api also has a getRole function, albeit with a different implementation. I have not looked enough into it, to evaluate which one is more fit for this use-case. Do you have any preferences?

Question 1. is more imminent for this PR, I suppose 馃榿 Question 2. can be an implementation detail, that we can tackle in a follow-up. I leave it up to you.

Please let me know if there is any other information that I can provide!

@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2024

Since we are copying over the function from dom-testing-library, I have a gut feeling that we could move these helpers to a shared package.

I wouldn't worry about this part too much. dom-accessibility-api is the natural fit but we don't want to use aria-query in there so it's not just about copying over impl but getting rid of aria-query entirely.

@eps1lon eps1lon changed the title Feature: Add toHaveRole matcher feat: Add toHaveRole matcher Jan 29, 2024
@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2024

Did you file a docs PR yet?

@fpapado
Copy link
Contributor Author

fpapado commented Jan 29, 2024

Did you file a docs PR yet?

I made the changes to README.md in this PR. Is there some other place to change for the docs, or should I split the docs and feature into two PRs?

@gnapse gnapse requested review from gnapse and eps1lon January 29, 2024 10:53
@gnapse
Copy link
Member

gnapse commented Jan 29, 2024

@fpapado thanks a lot for taking care of this. Very thorough PR, BTW.

About this:

I noticed that @eps1lon's dom-accessibility-api also has a getRole function, albeit with a different implementation. I have not looked enough into it, to evaluate which one is more fit for this use-case. Do you have any preferences?

I had not taken a look at both implementations closely enough before, but I did now. I must say that I prefer the one in dom-accessibility-api for two reasons:

  1. We can fully avoid having to reinvent the wheel (aka copy the code from elsewhere), since that one is already implemented, and it is a public API of that package, which BTW is already a dependency in this package.
  2. That one does something that, upon code inspection on this one you used, I'm not sure this one does: to not recognize invalid role names.
    • For instance, <div role="whatever" /> should not pass the test expect(div).toHaveRole('whatever'), because whatever is not a valid ARIA role.

However, I do sympathize with the idea of the shared implementation. I wonder why dom-testing-library does not use the one from dom-accessibility-api. Perhaps that is a change that can be contributed to that library.

@fpapado
Copy link
Contributor Author

fpapado commented Jan 29, 2024

Thank you 馃槍

However, I do sympathize with the idea of the shared implementation. I wonder why dom-testing-library does not use the one from dom-accessibility-api. Perhaps that is a change that can be contributed to that library.

My gut feeling is that dom-testing-library prioritises querying for roles, so compiling the rules to a CSS selector might be better-suited. For a matcher, where we already have a handle on the relevant element, the approach in dom-accessibility-api seems equally viable. For me personally, it's mostly a question of where the libraries pull their sources from, and what the update cadence is. It's tricky to keep up with the mapping specs (they change slowly, but also in subtle ways).

For invalid role, I believe browsers preserve it, even if the value is invalid. I guess, in a way, this enables the fallbacks and future roles to exist. For example, here is Chrome's accessibility devtools with this codepen of role="whatever":

CleanShot 2024-01-29 at 13 09 10@2x

So I can see some value in not being strict about them, but I agree that matchers can make different decisions than the browser 馃槄

@eps1lon
Copy link
Member

eps1lon commented Jan 29, 2024

I wonder why dom-testing-library does not use the one from dom-accessibility-api.

Historicaly reasons. A rewrite would not use aria-query. Ideally we'd move everything into dom-accessibility-api (the interface not the implementation) and use that everywhere.

@gnapse gnapse merged commit f7dc673 into testing-library:main Jan 30, 2024
5 checks passed
Copy link

馃帀 This PR is included in version 6.4.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

@gnapse
Copy link
Member

gnapse commented Jan 30, 2024

@all-contributors please add @fpapado for code, docs, test

Copy link
Contributor

@gnapse

I've put up a pull request to add @fpapado! 馃帀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants