Define keyboard highlighting with open/close feature #349
Conversation
There was a problem hiding this comment.
cc @Kodylow to take a look at how this affects the behavior (clickable only on the right) and visuals (additional padding for arrow), make sure these changes are desirable.
The issue I opened was entirely just about the accessibility of it, so I want to make sure any additional changes are considered as well.
packages/ui/src/FormGroupHeading.tsx
Outdated
| tabIndex={1} | ||
| cursor='pointer' |
There was a problem hiding this comment.
Making the cursor a pointer and giving it a tabIndex across the entire header may give a user the mistaken understanding that clicking this will open / collapse the header, but only clicking the button inside will.
There was a problem hiding this comment.
I think if it's collapsed having it be "click anywhere" is preferable
packages/ui/src/FormGroupHeading.tsx
Outdated
| > | ||
| {icon && <Icon width='20px' height='20px' mr={2} as={icon} />} | ||
| <Heading fontSize='md' lineHeight='20px' fontWeight='700'> | ||
| {title} | ||
| </Heading> | ||
| <Spacer /> | ||
| {chevronIcon} | ||
| <Button onClick={onClick} tabIndex={1} variant='ghost' color='black'> |
There was a problem hiding this comment.
Buttons don't need a tabIndex, they're tabbable by default.
There was a problem hiding this comment.
Choosing to use the Chakra <Button> component added some new styling here that adds padding that wasn't there before. Maybe this is desirable, but it's a change worth considering.
There was a problem hiding this comment.
While I think the padding unintentionally introduced by the button is pleasant, the button adds a tab stop that's redundant to the functionality of the form group header and we should not introduce it
There was a problem hiding this comment.
in case we decide to keep the button, it should also have an onKeydown listener
There was a problem hiding this comment.
onClick already captures the typical accessibility keys for proceeding when focused (space bar, enter etc)
There was a problem hiding this comment.
Sorry, I was mistaken. That is only the case if you use proper elements. Even adding accessibility attributes like tabIndex and role don't work if it's not a button or a: https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/button_role#required_javascript_features
Although for what it's worth, that's a reason to get the element to use the right element instead of using a div and trying to turn it into an accessible clickable element. This would be a lot easier if it was just a <button>.
There was a problem hiding this comment.
Refactoring this to Button element seems to work just fine. No redundant functionality with tab stop, and open/close works for both onClick and keydown (Enter and Space bar) without explicitly specifying this.
There was a problem hiding this comment.
refactor.to.button.element.webm
|
echo all nits by will, and click to open should be the whole bar not just the arrow if we're updating anyway. |
|
currently validating the latest iteration |
packages/ui/src/FormGroupHeading.tsx
Outdated
| mb={3} | ||
| bg={theme.colors.gray[100]} | ||
| color={theme.colors.white[100]} |
There was a problem hiding this comment.
try apply the right button variant and colors so we fix A11y without changing visible UI. Something like variant='ghost'+ bg={theme.colors.gray[100]} + text color modification should work
There was a problem hiding this comment.
Side note: Open/Close on Follower isn't that graceful. Effect of having less fields?
|
just test with jodom, approving and merging |
This PR closes #324
Adds accessibility into collapsible fields by allowing navigation using the keyboard.