Improve time picker accessibility #4181
Conversation
theopolisme
commented
Jul 30, 2023
theopolisme
commented
- Explicitly declare the Time picker as a listbox
- Add support for keyboard Arrow events to navigate the Time picker (per listbox keyboard interaction requirements)
- When Time picker focused, initially focus the currently selected time, or whatever time was initially scrolled into view, rather than always focusing the last time (fixes Keyboard navigation when including times is very cumbersome (Accessibility) #3785)
* Explicitly declare the Time picker as a listbox * Add support for keyboard Arrow events to navigate the Time picker * When Time picker focused, initially focus the currently selected time, or whatever time was initially scrolled into view, rather than always focusing the last time
![pullrequest[bot]](https://avatars.githubusercontent.com/in/3434?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ This pull request was sent to the PullRequest network.
@theopolisme you can click here to see the review status or cancel the code review job.
There was a problem hiding this comment.
Overall the change looks good and has appropriate testing. A minor React issue is noted, but it's not something that impacts functionality.
Reviewed with ❤️ by PullRequest
| return times.map((time, i) => { | ||
| return ( | ||
| <li | ||
| key={i} |
There was a problem hiding this comment.
This code was not modified in this PR (just whitespace changes)
There was a problem hiding this comment.
Looks easy enough, some simple UX questions around focus handling.
Reviewed with ❤️ by PullRequest
| event.target.nextSibling | ||
| ) { | ||
| event.preventDefault(); | ||
| event.target.nextSibling.focus(); |
Codecov Report
@@ Coverage Diff @@
## main #4181 +/- ##
==========================================
- Coverage 94.02% 93.75% -0.28%
==========================================
Files 20 20
Lines 1890 1904 +14
Branches 458 464 +6
==========================================
+ Hits 1777 1785 +8
- Misses 40 43 +3
- Partials 73 76 +3
|
There was a problem hiding this comment.
PullRequest reviewed the updates made to #4181 since our last review was posted. This includes comments that have been posted by non-PullRequest reviewers. No further issues were found.
Reviewed by:
|
Looks good, merging. Thank you! |
