[a11y] Update dropdown component for keyboard usage #261
Conversation
| }; | ||
|
|
||
| removeOffClickListener = () => { | ||
| document.removeEventListener('click', this.handleOffClick, false); | ||
| document.removeEventListener('keydown', this.handleKeyPress, false); |
There was a problem hiding this comment.
This is used in a couple of places, including in the ImmersiveModal to handle outside clicks. I can't think of a reason not to also respond to Esc for those, but I may need to also do a quick search in the PocketDerm repo to see if we are duplicating this logic there.
| ).toEqual(true); | ||
| }); | ||
|
|
||
| describe('onSelectClick callback', () => { | ||
| it('should be invoked onClick', () => { | ||
| const spy = jest.fn(); | ||
| const wrapper = shallow( | ||
| const wrapper = mount( |
There was a problem hiding this comment.
These tests were failing when using shallow - I believe the issue is that the hook was still firing, but it seemed like it was coming from the render of the class component, which violates the laws of Hooks. Using mount does a more full render of the tree and seemed to fix the issue.
| @@ -5,6 +5,20 @@ import MobileDropdown from './mobileDropdown'; | |||
| import DesktopDropdown from './desktopDropdown'; | |||
| import allowNullPropType from '../../utils/allowNullPropType'; | |||
|
|
|||
| export type OptionType = { | |||
There was a problem hiding this comment.
This file is just TS conversion changes.
There was a problem hiding this comment.
@ipc103 I think we'll also want to convert this to a Function component. We're typing optionsContainerMaxHeight as optional in children components, but it's provided via defaultProps. If we convert this to a Function component we can rely on ES6 defaults to inform TypeScript that it is optional but, downstream, being required is not type inconsistent.
Fortunately the only non-render methods aren't lifecycle methods so it should be pretty straightforward to convert.
| DropdownOptionsContainer, | ||
| DropdownOption, | ||
| } from './style'; | ||
|
|
There was a problem hiding this comment.
File CHANGELOG:
- Convert to TS
- Add
useResetFocushook to move focus back to the button when an option is selected - Update roles, tabIndex, and aria-labels for the
DropdownOptionsContainerul andDropdownOptionli. - Add a keydown handler to allow selecting an option using the
enterkey.
|
I mentioned this to @michaeljaltamirano in Slack, but I'll be away from the office until 7/20. Feel free to pick up where this leaves off, reject this, or ignore it entirely and leave for me when I come back 😄 |
| const { onChange } = this.props; | ||
|
|
||
| const { value, selectedOptions } = event.target; | ||
| const {target} = event; |
There was a problem hiding this comment.
Did this not auto-format to be { target }? I've noticed this before in this repo... wonder if something is up
There was a problem hiding this comment.
It did not - and apparently I've gotten very dependent on our linting to fix my bad whitespace habits 😅
We could also build off of |
| } = option; | ||
|
|
||
| return ( | ||
| <DropdownOption |
There was a problem hiding this comment.
Did you play around at all with trying to achieve the (kind of) opposite of useResetFocus, but rather than resetting focus back to another element, we actually set the focus ahead to the first descendent? (Kind of like the menu button when using keyboard navigation here: https://reach.tech/menu-button/#reach-skip-nav).
I also have not dug into the internals of all this as much as you have, so I'm not use if that kind of functionality is consistent with "best practices" or we'd be potentially introducing hacky behavior that we ought to achieve with other code.
There was a problem hiding this comment.
This is a good idea - I'm going to write up a separate ticket for this and will investigate further. Looking at the reach-ui implementation, it looks like they generally send focus to the first list element, but it may make sense to focus on the selected element if there is one.
Asana Task
Our
Dropdowncomponent renders two different components depending on the type of device. For devices with touchscreen enabled, it uses theMobileDropdown, which takes advantage of semanticselectandoptionelements. However, for styling purposes, theDesktopDropdownis using aulwith behavior modifications to effectively function as a select box. Doing this causes a few different accessibility issues:In order to preserve the existing visuals, I attempted to fix these issues by adding accessibility information and additional keyboard event handlers to the component. Definitely curious to get other opinions - it may be easier to rebuild this component using semantic HTML, where we'll get most of the correct behavior for free, and then re-style to match instead of what I've done here. Included in this PR:
Dropdowndirectory to TypeScriptlistboxto theulrepresenting the dropdown, andoptionto eachli.liis tab-able and select-able.OffClickWrapperto fire the callback when theEscapekey is pressed. This component is also used by ourImmersiveModal, which I see as a benefit (pressing the Escape key should allow for the same functionality as doing an off-click with the mouse)useResetFocushook to send focus back to the trigger button after an option is selected.Future TODO/Other Potential Improvements:
listboxhas been opened, you should be able to scroll the elements using the arrow keys instead of thetabkey. Testing in Chrome and Safari with Voiceover turned off, this did not work as implemented - the options are only select-able using thetabkey.