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.

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

Jump to bottom

getByRole's error message is incorrect on button with role "tab" #553

Closed
mattstobbs opened this issue May 7, 2020 · 9 comments · Fixed by #568
Closed

getByRole's error message is incorrect on button with role "tab" #553

mattstobbs opened this issue May 7, 2020 · 9 comments · Fixed by #568
Labels
bug Something isn't working released

Comments

@mattstobbs
Copy link

  • DOM Testing Library version: 5.5.0
  • node version: 13.12.0
  • npm (or yarn) version: 1.22.4

Relevant code or config:

import React from 'react';
import { render, screen } from '@testing-library/react';

const Button = () => <button role="tab" aria-label="my-button" />;

test('button exists', () => {
  render(<Button />);

  screen.getByRole('button', { name: 'my-button' });
});

What happened:

The test (correctly) failed because it could not find an element with the role of button (the button has a role of "tab"). However, when the error message listed the accessible roles, it listed the button's role under "button":

TestingLibraryElementError: Unable to find an accessible element with the role "button" and name "my-button"

Here are the accessible roles:

  document:

  Name "":
  <body />

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

  Name "my-button":
  <button
    aria-label="my-button"
    role="tab"
  />

Reproduction:

Code Sandbox

Problem description:

The current behaviour is confusing because the error message says there is no element with the role of "button" and then shows that there is an element with the role of "button".

Suggested solution:

Buttons with other roles (e.g. tab) should be shown under their correct roles in the error message.

I'm happy to put up a PR for this but would need pointing in the right direction as I'm unfamiliar with the code 😁
@eps1lon
Copy link
Member

eps1lon commented May 7, 2020

Thanks for the report. That is indeed weird.

You should start with adding a test to src/tests/role.js. The code for ByRole lives in src/queries/role.js.

@eps1lon eps1lon added the bug Something isn't working label May 7, 2020
@mattstobbs
Copy link
Author

mattstobbs commented May 8, 2020 •
edited

It looks like there are actually 2 issues here.

The first is the library aria-query, which we're using to get a map of the elements to their possible roles. Under elementRoles, the button element only has the role "button". This would need to be extended to include other roles, such as tab.

The second is that the matches function correctly determines that <button role="tab /> has the role of tab, but currentNode.matches determines the node has the role of button. Do you know where that function is defined?

@mattstobbs
Copy link
Author

Just dug a bit more into this. currentNode.matches returns true for the selector "button" because it is a button. The way this is handled in the aria-query library is to have more specific selectors first, so they will match first (i.e. the button would match with button[role="tab"] before it matches with button).

Therefore, the solution would be to put up an issue and PR in the aria-query library, and once that is merged, update the version used by dom-testing-library. If you're happy with that, @eps1lon, I can go ahead and do that.

@eps1lon
Copy link
Member

eps1lon commented May 9, 2020 •
edited

Great investigation! We do sort by specificity:

dom-testing-library/src/role-helpers.js

Line 118 in 429de04

return result.sort(bySelectorSpecificity)
I think we just need to refine the logic so that an explicit "role" has the highest specificity:

dom-testing-library/src/role-helpers.js

Lines 92 to 94 in 429de04

function getSelectorSpecificity({attributes = []}) {
return attributes.length
}

@mattstobbs
Copy link
Author

That would make sense. It looks like that logic is still manipulating the map from aria-query, so it still wouldn't include button[role="tab"] because that library doesn't include it?

@eps1lon
Copy link
Member

eps1lon commented May 9, 2020

aria-query definitely includes tab.

@mattstobbs
Copy link
Author

When I console.log(elementRoleList), which is the output of

dom-testing-library/src/role-helpers.js

Line 5 in 429de04

const elementRoleList = buildElementRoleList(elementRoles)

I get the following list: elementRoleList.txt

which doesn't include tab, or a selector called button[role="tab"]. Am I missing something?

@eps1lon eps1lon mentioned this issue May 12, 2020
Merged
2 tasks
kentcdodds pushed a commit that referenced this issue May 13, 2020
@eps1lon
fix: Consider explicit role when pretty printing roles (#568)
04b027c 
Closes #553
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.5.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

@anndroow

This comment has been minimized.

Sign in to view
@Mansi1 Mansi1 mentioned this issue Feb 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working released
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix: Consider explicit role when pretty printing roles eps1lon/dom-testing-library
4 participants
@kentcdodds @anndroow @eps1lon @mattstobbs