Skip to content

feat(byRole): Add name option #368

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

Merged
merged 5 commits into from
Mar 6, 2020

Conversation

eps1lon
Copy link
Member

@eps1lon eps1lon commented Jan 25, 2020

Docs for testing-library/dom-testing-library#408

Preview:

I moved byRole up in the recommendations. I'm not so sure if we should already do this though. Personally I would hope that we can reduce the queries to byRole and byTestId and then recommend asserting properties such as placeholder, alt, value etc. Choosing the right query is currently not trivial. Though I fear that by only having byRole we loose a lot of people because they are not aware of the role names and fall back to testid. While I would hope that everybody knows what roles are it's not mainstream knowledge as far as I can tell. And <input /> having so many roles with names that are not that common in frontend development. I can only speak for myself though (mentally it's still "input of type text" instead of just "a textbox"). So I'm really looking forward to input from others that have teaching experience or can quickly poll their teams.

I'm fine with leaving the name option a bit more obscure for a start and see how adoption goes.

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

This is awesome.

This will probably be used more than *ByText so maybe we could make a helper query: *ByRoleName to make it easier to use?

Copy link
Member

@kentcdodds kentcdodds left a comment

Choose a reason for hiding this comment

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

Looking good! Just a few things.

Comment on lines 17 to 21
1. `getByRole`: With the `name` option you can basically query the full
accessibility tree. It can query all the elements you can query with the
other accessility query. But consider that you still might want to assert
if an element has an actual `<label>` element or a `placeholder`
attribute.
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 many people use *ByText to select buttons, divs, and spans. Maybe we should briefly mention what the role for each of those would be and that will cover most use-cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not encountering much use cases to select div or span these days. These wouldn't be queryable with getByRole and don't have an accessible name to begin with.

After considering the feedback in the implementation PR I would actually agree that byText('some text', { role: 'listitem' }) would be a useful query. Simply because some roles can't have an accessible name and the text (i.e. .textContent) is the appropriate fallback for these.

I think I'll revert the change in the recommendation order until this is more mature (adding byText(text, { role }) etc).

Overall I'd say we should definitely add some guard rails here when people try e.g. getByRole('listitem', { name: 'first' }) by warning that certain roles don't have an accessible name and suggest using byText('first', { role: 'listitem' }). This is even a candidate for a lint rule with auto-fixing

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the wording and order again. The rule of thumb is now:

  • input? -> byLabelText (simple query that doesn't require much a11y knowledge. Personally I will still use byRole('textbox', { name: '' }). The problem is that you need to know all the various input roles but then maybe we should encourage knowing these?)
  • interactive? -> byRole
  • not-interactive? -> byText

Copy link
Collaborator

Choose a reason for hiding this comment

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

The problem is that you need to know all the various input roles but then maybe we should encourage knowing these?

There a lot of aria roles and not necessarily one perfect role for a given use case. For example you might have a list that gets refactored to the new feed role later. I think the generic queries like label are good for use cases where you don't want to overspecify the exact roles.

Copy link
Member Author

Choose a reason for hiding this comment

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

gets refactored to the new feed role later

That isn't just a refactor. You're actually changing semantics. We shouldn't be too overzealous with our testing approach. A test should actually fail some times.

I'm also not sure how a feed relates to this issue I just realized why this causes so much confusion. ByLabelText is actually quite problematic with this API. I was under the impression it only considers actual label elements. The current implementation of ByLabelText is incomplete. We should discourage ByLabelText for anything other thant input.

Copy link
Collaborator

@alexkrolick alexkrolick Feb 4, 2020

Choose a reason for hiding this comment

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

The current implementation of ByLabelText is incomplete

What do you mean by incomplete? It also understands aria-label and aria-labelledby, which would require a significant amount of work to implement in userspace if you are wanting to traverse labelled elements.

EDIT: I guess you are suggesting that you do this instead? getByRole('dialog', {name: 'this comes from an aria-labelledby element content'})

I mentioned feed because it is a subclass of list, so anything that works on list should still work on feed. I don't think the role queries understand the hierarchy yet.

https://www.w3.org/TR/wai-aria-1.1/#feed

@eps1lon eps1lon merged commit 9dd8b32 into testing-library:master Mar 6, 2020
@eps1lon eps1lon deleted the feat/byRole/name-filter branch March 6, 2020 16:42
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

Successfully merging this pull request may close these issues.

3 participants