-
Notifications
You must be signed in to change notification settings - Fork 2.9k
(7.0) ComboBox: fix multi-select checkbox role; improve docs and examples #16329
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
(7.0) ComboBox: fix multi-select checkbox role; improve docs and examples #16329
Conversation
|
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 ef8b9de:
|
Perf AnalysisNo significant results to display. All results
|
Asset size changes
Baseline commit: c98610f4fc38ed65b3a0a89350970681393fb0a3 (build) |
a94613a to
77793f6
Compare
7a43cdd to
3aaae03
Compare
3aaae03 to
4e9b8b2
Compare
smhigley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one change, otherwise it looks good!
| aria-selected={isSelected ? 'true' : 'false'} | ||
| ariaLabel={item.ariaLabel} | ||
| aria-selected={isChecked ? 'true' : 'false'} | ||
| ariaLabel={this._getPreviewText(item)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this one case, I'd suggest leaving this as ariaLabel={item.ariaLabel}, since defining it as item._getPreviewText means the aria-label will always be set, even if an author hasn't defined item.ariaLabel. That actually caused an a11y bug in v8 (fixed now) when a team had different option text in the dropdown than the text used in the input value.
|
Because this pull request has not had activity for over 150 days, we're automatically closing it for house-keeping purposes. The pull request will still be available for reference. If it's still relevant to merge at some point, you can reopen or make a new version based on the latest code. |
Pull request was closed
4e9b8b2 to
ef8b9de
Compare
|
@smhigley Finally updated this--would you mind taking another look at the core ComboBox and Dropdown changes and ensuring they look correct? |
smhigley
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Pull request checklist
data-props #16307, fixes ComboBox multi-select has invalid/missing aria attributes #15223, fixes ComboBox multi-select has no children with role="option" #15224$ yarn changeDescription of changes
ComboBox tries to pass
role="option"and somedata-*props as top-level props to an option Checkbox in multi-select mode. As documented in #16307, Checkbox currently doesn't apply most native props, so these props aren't applied. This is definitely a problem (Accessibility Insights flags it) but probably got missed since none of the ComboBox examples on the website used multi-select.Related changes:
data-*props are passed as top-level props to the Checkbox (the more significant issue with the role had already been fixed)(master version: #16331)