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

Support narrowing getByRole with type #1012

Closed
G-Rath opened this issue Aug 19, 2021 · 4 comments
Closed

Support narrowing getByRole with type #1012

G-Rath opened this issue Aug 19, 2021 · 4 comments

Comments

@G-Rath
Copy link
Contributor

G-Rath commented Aug 19, 2021

Describe the feature you'd like:

I've got a component like this:

export const Form: React.FC<Props> = props => (
  <form onSubmit={props.onSubmit} onReset={props.onReset}>
    {props.children}
    <br />
    <button type="submit">{props.submitText ?? 'Submit'}</button>
    {props.onReset && <button type="reset">Reset</button>}
  </form>
);

Currently in my tests I'm doing this:

describe('when the form is submitted', () => {
  it('calls the submit handler once', () => {
    render(<Form onSubmit={handleSubmit} />);

    userEvent.click(screen.getByText('Submit'));

    expect(handleSubmit).toHaveBeenCalledTimes(1);
  });
});

As afaik that's the most efficient query I can use to get the "submit" button (without adding a test-id).

The issue is that it's dependent on the text of the button being Submit - what I really want is to do getByRole('button', { type: 'submit' }) to select "a submit button".

Likewise this would be useful for the reset button which is in the same camp.

The only drawback I can think of here is a little increase in amount of code :)

Suggested implementation:

I've not looked at the code, but I'd expect this should be doable by checking if the node being checked has a type attribute and if so compare its value to the type option if present.

Describe alternatives you've considered:

  • Adding a test-id to my element, which seems overkill.
  • Matching against the text, which is what I'm currently doing and is fine, but I think it would be easy to make this a little less brittle.
  • Using the name option of getByRole, but that'd suffer from the same issue as above.

Teachability, Documentation, Adoption, Migration Strategy:

I imagine it'd be a matter of updating the current docs for the role queries to include this as a new option, with a similar format as the docs for the existing options such as checked, selected, etc.


I'm happy to help try and implement this :)

@juanca
Copy link
Contributor

juanca commented Aug 20, 2021

I gave something similar a try and I kinda like it: https://github.com/juanca/accessible-query

It does not check role since I am more of a fan to check for labels -- I think everything on a page should be uniquely labelled; and getByRole is inherently slow*.. But if needed, we can probably import the getByRole utility if there is a role key-value in the attributes object.

Or if it's okay to include it in this library, we can also copy-paste similar looking code.

@eps1lon
Copy link
Member

eps1lon commented Aug 20, 2021

Thanks for the detailed explainer.

The type is very specifc to buttons (and not related to ARIA properties like selected or checked) so you should just use getAllByRole().filter(). If you use it often, you can create a custom query that you use instead.

But the use case itself is fairly specific so we don't intend to include it at this point. If this request becomes more popular, we'll reconsider.

PS:

I've not looked at the code, but I'd expect this should be doable by checking if the node being checked has a type attribute and if so compare its value to the type option if present.

You want to use the property instead since querying by attribute would miss the default value for the type property which is 'submit'.

@nickshanks
Copy link

nickshanks commented Feb 10, 2025

I came here looking for something similar (a way to find the default button that is triggered when you press Return when in a text field, even if there are multiple submit buttons). But after reading this, @eps1lon your proposed solution falls short in three regards:

  1. As you point out, developers should be checking the type property not looking for a type attribute. And if you do not use *ByRole() you are likely to miss all input type=button and div role=button onClick=… elements. You cannot expect thousands of developers to get this right every time. Libraries exist so that the correct implementation can be created once and shared. The ideal place for that implementation is next to all the other matchers in this library.
  2. getAllByRole('button').filter(el => el.role === 'submit') returns a result of type HTMLElement[] and does not fail if there are zero, or more than one item, nor does it print to the console the helpful error message listing out all the matching elements (as the proposed getByRole('button', {type: 'submit'}) would presumably do). Thus it provides a degraded developer experience.
  3. You say "the default value for the type property which is 'submit'." — This is not true in all cases. For at least Internet Explorer <= 7, and all user agents based on the HTML library it uses, the default value of the property is button.

Yes, this matcher option would only be needed for buttons, but buttons are a very important part of the web, with much hidden complexity. I feel it would not be excessive to provide specific support for them.

@kentcdodds thoughts?

@kentcdodds
Copy link
Member

Can you be more clear on what it is you want to have changed? For example, give us an example of code that you would like to be able to write if this were done.

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

No branches or pull requests

5 participants