Skip to content

support {checked: true} for checkbox / radio #692

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 9 commits into from
Jul 15, 2020

Conversation

idanen
Copy link
Collaborator

@idanen idanen commented Jul 12, 2020

Closes #691

What:

Allow querying checkboxes and radios that are checked using the selected option in ByRole queries.

Why:

Better experience than filtering for them

How:

Checking for both aria-selected and checked property.

Checklist:

  • Documentation added to the
    docs site
  • Tests
  • Typescript definitions updated
  • Ready to be merged

@codesandbox-ci
Copy link

codesandbox-ci bot commented Jul 12, 2020

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 0bfd8e1:

Sandbox Source
kentcdodds/react-testing-library-examples Configuration

@codecov
Copy link

codecov bot commented Jul 12, 2020

Codecov Report

Merging #692 into master will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #692   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines          603       614   +11     
  Branches       151       156    +5     
=========================================
+ Hits           603       614   +11     
Impacted Files Coverage Δ
src/queries/role.js 100.00% <100.00%> (ø)
src/role-helpers.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2215770...0bfd8e1. Read the comment docs.

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 fine with me, though I'd like to hear other's opinions.

Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Referencing #691 (comment) to make it clear that it we haven't decided whether the problem in the linked issue is something we should solve.

@idanen idanen force-pushed the pr/getbyrole-checked branch from a7432f1 to 4de1412 Compare July 13, 2020 07:41
@idanen idanen requested review from eps1lon and kentcdodds July 13, 2020 19:59
@eps1lon eps1lon changed the title support {selected: true} for checkbox / radio support {checked: true} for checkbox / radio Jul 13, 2020
Copy link
Member

@eps1lon eps1lon left a comment

Choose a reason for hiding this comment

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

Now that I think about it: How is mixed treated? Right now we're treating it like selected but selected only has two states while checked has three: true, false and mixed (or indeterminated): https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/checkbox_role

@idanen
Copy link
Collaborator Author

idanen commented Jul 14, 2020

Good point.
According to the spec:

The control is never a true tri-state control, even if the element's indeterminate IDL attribute is set to true. The indeterminate IDL attribute only gives the appearance of a third state.

Also from the spec:

If the element's indeterminate IDL attribute is set to true, then the control's selection should be obscured as if the control was in a third, indeterminate, state.

So I don't really know how to move forward..
Any suggestions?

@idanen
Copy link
Collaborator Author

idanen commented Jul 14, 2020

Another point: the spec uses "positive selection" and "negative selection" when describing checkedness state, so maybe that's not such a bad idea to use the same API (selected)?

@eps1lon
Copy link
Member

eps1lon commented Jul 14, 2020

So I don't really know how to move forward..

How is aria-checked mapped from and to <input type="checkbox" />?

@idanen
Copy link
Collaborator Author

idanen commented Jul 14, 2020

From what I went over (checked spec, and WAI-ARIA for aria-checked), it's implied that:
The WAI-ARIA spec describes it like this:

  • When checked, the checkbox element has state aria-checked set to true.
  • When not checked, it has state aria-checked set to false.
  • When partially checked, it has state aria-checked set to mixed.

There's no direct "mapping", but from how I understand it should map like so:

  • When checked, the checkbox element's checked property is set to true.
  • When not checked, the checkbox element's checked property is set to false.
  • When partially checked, the checkbox element's checked property is set to false and indeterminate property is set to true.

So when using ByRole with {checked: true} or {checked: false} it's pretty obvious, non should include mixed.
Do we want to add also {indeterminate: true}? Make the checked option tri-state?

This discussion makes me lean more towards {selected: true}, since it really helps users find controls by state, without diving too deep into the HTML spec.
If a label like "Please select one or more of these" is semantically correct, than querying with "selected" should also be correct.
WDYT?

@eps1lon
Copy link
Member

eps1lon commented Jul 14, 2020

checkbox role, with the aria-checked state set to "mixed" if the element's indeterminate IDL attribute is true, or "true" if the element's checkedness is true, or "false" otherwise

-- https://www.w3.org/TR/html-aam-1.0/#details-id-56

So indeterminate=true takes presedence.

@idanen idanen requested a review from eps1lon July 15, 2020 05:44
@kentcdodds kentcdodds merged commit fcdeb31 into testing-library:master Jul 15, 2020
@kentcdodds
Copy link
Member

@all-contributors please add @idanen for code and tests

@allcontributors
Copy link
Contributor

@kentcdodds

I've put up a pull request to add @idanen! 🎉

@kentcdodds
Copy link
Member

Thanks so much for your help! I've added you as a collaborator on the project. Please make sure that you review the other/MAINTAINING.md and CONTRIBUTING.md files (specifically the bit about the commit messages and the git hooks) and familiarize yourself with the code of conduct (we're using the contributor covenant). You might also want to watch the repo to be notified when someone files an issue/PR. Please continue to make PRs as you feel the need (you can make your branches directly on the repo rather than your fork if you want). Thanks! And welcome to the team :)

@kentcdodds
Copy link
Member

🎉 This PR is included in version 7.21.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

jluxenberg added a commit to jluxenberg/dom-testing-library that referenced this pull request Aug 6, 2020
Hat tip to Sebastian Silbermann for his prior work on PR testing-library#692
kentcdodds pushed a commit that referenced this pull request Aug 6, 2020
Hat tip to Sebastian Silbermann for his prior work on PR #692
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

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