Skip to content

Passing { selected: true } to getByRole of a radio / checkbox throws an error #691

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

Closed
idanen opened this issue Jul 12, 2020 · 8 comments · Fixed by #692
Closed

Passing { selected: true } to getByRole of a radio / checkbox throws an error #691

idanen opened this issue Jul 12, 2020 · 8 comments · Fixed by #692
Labels
enhancement New feature or request released

Comments

@idanen
Copy link
Collaborator

idanen commented Jul 12, 2020

Describe the feature you'd like:

Currently trying to find checked radio / checkbox throws an error "aria-selected" is not supported on role "radio".
I'd expect it to work for natively selected elements.

Suggested implementation:

Just try to find by checked first.

Describe alternatives you've considered:

Currently to get those, we need to filter a getAllBy* query:

const selected = getAllByLabelText('preference').filter(control => control.checked);

Teachability, Documentation, Adoption, Migration Strategy:

The WAI-ARIA spec separates aria-checked from aria-selected so maybe we can introduce this change too.
Or just update the docs to link to both.
Suggested tab example after implementing the change:

you can get the "Native"-tab by calling getByRole('tab', { selected: true }). To learn more about the selected state and which elements can have this state see ARIA aria-selected or ARIA aria-checked.

@idanen
Copy link
Collaborator Author

idanen commented Jul 12, 2020

The example for tab is in the *ByRole docs

idanen added a commit to idanen/dom-testing-library that referenced this issue Jul 12, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 12, 2020

Why is a checked checkbox considered selected? As far as I know these are two different concepts.

@eps1lon eps1lon added the enhancement New feature or request label Jul 12, 2020
@idanen
Copy link
Collaborator Author

idanen commented Jul 12, 2020

They differ in the spec. The context might make the difference.
For example, in a group of checkboxes:

<label><input type="checkbox" value="onion" />Onion</label>
<label><input type="checkbox" value="tomato" />Tomato</label>
<label><input type="checkbox" value="lettuce" />Lettuce</label>
<label><input type="checkbox" value="pickles" />Pickles</label>

getting all selected options sounds fit.
But for say a preference:

<label><input type="checkbox" value"yes" />Take away</label>

getting the checked option, might make more sense.
Maybe I'm not that keen in english, but both feels the same for me.
Either way, currently the checked option isn't supported in any way, so that's the main feature suggestion.
If the difference is important, can we just add that option ({ checked: true })?

@eps1lon
Copy link
Member

eps1lon commented Jul 12, 2020

There should be a stronger argument that it "sounds fit" to you. What are screen readers announcing? What does the spec say? etc. There's most likely a reason why we have aria-selected and aria-checked . Though I do remember some discussion on the ARIA repo about improving documentation for it because the distinction isn't all that obvious at the moment.

If you want to implement a checked filter then I'm all ears. This seems reasonable given that you might want to query the checked role="radio" in a group.

@idanen
Copy link
Collaborator Author

idanen commented Jul 12, 2020

The argument regarding "sounds fit" was for English, not the spec 😊
For the spec I totally agree that I should have a better argument, but other than comparing to English, I have no good argument, other than this might be a simpler API for both cases.

In case this library needs to be perfectly aligned with the WAI-ARIA spec, I guess there's no choice other than using checked.
In this case, I can change the PR to support it.

idanen added a commit to idanen/dom-testing-library that referenced this issue Jul 13, 2020
@eps1lon
Copy link
Member

eps1lon commented Jul 13, 2020

In case this library needs to be perfectly aligned with the WAI-ARIA spec, I guess there's no choice other than using checked.

That'd be great, thanks!

@idanen
Copy link
Collaborator Author

idanen commented Jul 13, 2020

I was trying to check VoiceOver for what is used in my native tongue (Hebrew).
Unfortunately, they don't seem to translate state values like checked or selected 🤷‍♂️
Going forward with changing to checked

idanen added a commit to idanen/dom-testing-library that referenced this issue Jul 13, 2020
@kentcdodds
Copy link
Member

🎉 This issue has been resolved in version 7.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants