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

Add toHaveRole #143

Open
bowernite opened this issue Oct 17, 2019 · 16 comments
Open

Add toHaveRole #143

bowernite opened this issue Oct 17, 2019 · 16 comments
Labels
needs discussion We need to discuss this to come up with a good solution

Comments

@bowernite
Copy link

Describe the feature you'd like:

A toHaveRole matcher that uses a similar implementation to @testing-library's described here. In other words, checking implicit roles in addition to aria roles (i.e. using aria-query).

Motivation

The use case that prompted this was a page that had a button (example text "Create Issue") that was supposed to navigate to a page with a heading with the same text ("Create Issue").

Ideally, the test would look like this:

const createIssueBtn = getByText(/create issue/i)
expect(createIssueBtn).toHaveRole('button')
fireEvent.click(createIssueBtn)

const createIssueHeader = getByText(/create issue/i)
expect(createIssueHeader).toHaveRole('header')

Instead, the opposite can only be done (i.e. getByRole, then assert with toHaveTextContent). This is a little more fickle, since it relies on these elements being the first of the specified role on the page, which may not always be the case. Querying by text first is much more specific, and hopefully more robust.

Concretely, here's what I had to implement as an alternative:

    const utils = render(<App />)
    const createIssueBtn = utils.getByText(/create issue/i)
    // Since the heading on the page we're going to also has the text "create issue", we need to be a little dirty and make sure this element is a button
    expect(createIssueBtn.tagName).toBe('BUTTON')

    fireEvent.click(createIssueButton)

    // Wait for the "create issue" element to be a heading instead of a button -- necessary since the click won't trigger the page change immediately
    await wait(() =>
      expect(utils.getByRole('heading')).toHaveTextContent(/create issue/i)
    )
    const createIssueHeader = utils.getByText(/create issue/i)
    // Again, just being paranoid and making sure this element isn't just the button that's on the home page
    expect(examSchedulerHeader.tagName).not.toBe('BUTTON')
@connormeredith

This comment has been minimized.

@alexkrolick
Copy link
Contributor

alexkrolick commented Oct 17, 2019

A toHaveRole matcher that uses a similar implementation to @testing-library's described here

Yes, it seems like every query should have a corresponding jest-dom method

off topic

@connormeredith I think we still want this for general usage even if there is an alternative way for this particular example

This exact flow is really common, where a button opens a modal or page with the same text in a heading.

Some Page...

[New Message]
New Message

To:      [                ]
Subject: [                ]

@bowernite

This comment has been minimized.

@alexkrolick
Copy link
Contributor

alexkrolick commented Oct 17, 2019

I've actually never used waitForDomChange before -- at first glance my instinct was that it might be brittle

Yes, wrapping the specific assertion with wait or using the await findBy... variant is probably preferably in most cases

Can you take the part of the discussion about this specific scenario to https://spectrum.chat/testing-library? The feature request for toHaveRole has been logged 😄

@bowernite
Copy link
Author

Sure thing 🙂

One last thing in response that I think might still be relevant -- the findBy... method you suggested also doesn't work in this case -- the button you clicked will be found immediately, instead of waiting for the new heading you're trying to find.

@alexkrolick
Copy link
Contributor

-    await wait(() =>
-      expect(utils.getByRole('heading')).toHaveTextContent(/create issue/i)
-    )
+ expect(await utils.findByRole('heading')).toHaveTextContent(/create issue/i)

@bowernite
Copy link
Author

Yeah -- so in that solution utils.findByRole('heading') will find any heading that was on the original page, and not even run after the DOM changes

@gnapse
Copy link
Member

gnapse commented Jan 31, 2020

I'm confused as to what the conclusion was in the above discussion? Do we think we need this matcher? Isn't it .toHaveAttribute('role', 'dialog') enough? (maybe it is not, if we want to support the implicit roles of some elements, such as lis having an implicit listitem role).

@gnapse gnapse added the needs discussion We need to discuss this to come up with a good solution label Jan 31, 2020
@bowernite
Copy link
Author

(maybe it is not, if we want to support the implicit roles of some elements, such as lis having an implicit listitem role)

This is the meat of what I'm getting at.

An alternative solution is similar to add a role option similar to the selector option that some queries in @testing-libary/dom has.

// This would fail if an element that's a heading with the 'create issue' text doesn't exist
const createIssueButton = getByText(/create issue/i, { role: 'heading' })

@eps1lon
Copy link
Member

eps1lon commented Feb 2, 2020

An alternative solution is similar to add a role option similar to the selector option that some queries in @testing-libary/dom has.

@babramczyk With testing-library/dom-testing-library#408 you'll be able to use byRole('heading', { name: /create issue/i })

@marcysutton
Copy link

I came up with a use case for this where I wish I had a role matcher:

Query by TestId for robustness and then assert it has a certain role for accessibility.

What I wish existed:

// tab with user-event to reach the next focusable element
await user.tab()

// use a testId to query the right element
const editHandle = getByTestId(container, 'edit-handle')

// assert the element has an interactive role and receives focus
expect(editHandle).toHaveRole('button')
expect(editHandle).toHaveFocus()

The toHaveRole matcher would make it so a specific role could be asserted from an explicit (bolted on attribute) or implicit element, such as role="button" or <button>. It would be super useful.

@alexandrsashin
Copy link

@babramczyk
Is this topic actual now?

@gnapse
Copy link
Member

gnapse commented Jun 20, 2023

I think it could be of use to have a toHaveRole custom matcher, because it is not as straightforward as simply checking it with .toHaveAttribute('role', 'button'). Native buttons won't have that attribute, and they are indeed the canonical button role element.

If someone wants to contribute with a pull request implementing this, that'd be great. I cannot promise I'll get to it anytime soon.

@alexandrsashin
Copy link

alexandrsashin commented Jun 20, 2023

There is "Priority" section (https://testing-library.com/docs/queries/about/#priority) where in case with buttons we should use

const createIssueBtn = screen.getByRole('button', { name: /create issue/i });

So @babramczyk initial comment can be rewritten and no needs in toHaveRole().


Case of @marcysutton can be also rewritten using toHaveAttribute() (by example of @gnapse). Or in case of native button element check may be:

expect(editHandle).toHaveProperty('tagName', 'BUTTON');

@gnapse
Copy link
Member

gnapse commented Jun 20, 2023

The problem with…

expect(editHandle).toHaveProperty('tagName', 'BUTTON');

…is that such a test will break if for some reason a <button /> is changed for something with role="button". Albeit uncommon, there could be legitimate reasons to do this, and I'd argue that having a test not failing when this happens is desirable, because semantically nothing changed (assuming whoever did the change made sure that the role="button" element is accessible in all the other ways it should be accessible).

More commonly, it is possible to use <button /> elements with a different role. The native button gives us the keyboard accessibility, but we want a different role for some reason. One prominent use case is using a <button role="switch" /> (ref). So checking for the button tag name does not guarantee is a role="button".

In sum, I do think this could be a useful nice-to-have API.

@fpapado
Copy link
Contributor

fpapado commented Jan 27, 2024

At my current workplace, we have the exact "query by test id and assert the role" use-case described above. Using the implicit role is important, to account for shuffling around the implementations in existing applications. Moving away from test ids is not always possible; there are different testing practices and historical reasons for them. All I want to do is provide a way to improve accessibility, without arguing about those :)

Long story short, I'm interested in contributing a PR for this! If someone hasn't started on it yet, I'm happy to give it a shot. I glanced at the contributing guide, and the path seems relatively clear to me. If the maintainers want to pitch any ideas or direction, I'm happy to take them into account 😌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs discussion We need to discuss this to come up with a good solution
Projects
None yet
Development

No branches or pull requests

8 participants