feat(SegmentedControl): a11y - set role to radiogroup #6691
Conversation
|
Various open source UI toolkits have different approaches for Segmented Control a11y:
Can you explain the motivations for your change and make an argument for setting the default as you've done in this PR? I think a safer, better approach might be to add good documentation to the component docs suggesting that users should set their own ARIA role appropriately by specifying the |
|
@adidahiya that is tricky. After reading through those, I agree my original choice of It sounds like If you would like me to add a |
bvandercar-vt
changed the title
There was a problem hiding this comment.
why it shouldn't be radiogroup when that's exactly how it behaves
Right now the individual buttons are focusable, not the whole element with focus moved between with arrow keys as in a radio group. The non-checked buttons should have tabIndex={-1} and we should hook up arrow key listeners to move between the options if we want this to behave like a radio group.
I too do not see the argument against treating this like a radio group for the reason of "which would require a save button to apply the changes" - I see no reference to this in this w3 example or the mdn docs.
The SegmentedControl docs do say "similar to a Radio group" so this seems worth doing to me, as long as we hook up the expected interaction behavior as well.
|
@evansjohnson Done, added arrow key control and the proper |
There was a problem hiding this comment.
this is looking good - some last comments and then will leave to @ggdouglas for approval since he'll be owning the next BP release
| * If false, down arrow returns same as left arrow, up arrow returns same as right arrow. | ||
| * @returns -1 for left, 1 for right, undefined if not an arrow keypress. | ||
| */ | ||
| export function getArrowKeyDirection(event: React.KeyboardEvent<HTMLElement>, upMovesLeft: boolean = true) { |
There was a problem hiding this comment.
upMovesLeft is interesting - are we currently following expected behavior correctly? it definitely makes sense in the slider case for up to move right but I'm wondering if up should always be to the right
this feels like something that may change based on locale, possibly in addition to component usage
not sure we need to figure this out now since this keeps existing behavior but leaving this comment for posterity at least
There was a problem hiding this comment.
-
Slider: Definitely makes sense forup = right. -
Tabs: the official aria tabs example doesn't have any effect from the up/down arrow keys, only left/right. So if anything, we should remove the effect of an up/down keypress at some point. But with this PR, the existing implementation goes unchanged. -
SegmentedControl: since we are giving this theradiogrouprole, here in the official example of this role you can see thatup = left. The action description is given as well:
| @@ -92,34 +94,72 @@ export const SegmentedControl: React.FC<SegmentedControlProps> = React.forwardRe | |||
| value: controlledValue, | |||
| ...htmlProps | |||
| } = props; | |||
| const [selectedIndex, setSelectedIndex] = React.useState<number>( | |||
There was a problem hiding this comment.
we should keep this code more like it was before - where we use this local state index only if using the defaultValue option to avoid duplicating state, then on every render reconcile the index of the user provided controlledValue with this local index as a fallback
There was a problem hiding this comment.
Hmm it's tricky because we want to add keypress control with this PR, which is much easier to change to the next index vs. the next value. I can take a shot at it though.
There was a problem hiding this comment.
Done! Reduced the diff a ton, mostly the same code as was before, but arrow keys work as indended.
|
@ggdouglas this should be ready for final review! |
| if (!focusedElement) return; | ||
|
|
||
| // must rely on DOM state because we have no way of mapping `focusedElement` to a React.JSX.Element | ||
| const enabledOptionElements = Array.from(outerElement.querySelectorAll("button")).filter( |
There was a problem hiding this comment.
Could we leverage the selector query to filter out disabled buttons? e.g.
| const enabledOptionElements = Array.from(outerElement.querySelectorAll("button")).filter( | |
| const enabledOptionElements = Array.from(outerElement.querySelectorAll("button:not(:disabled)")); |
There was a problem hiding this comment.
Good idea, done!
|
@ggdouglas made some additional changes actually-- added a |
|
@ggdouglas the lint error is actually unrelated to this PR, it's happening in my other PRs as well. Here's the fix: #6972 |
Improves a11y of new SegmentedControl item, giving it the
radiogrouprole and also allowing it to be controlled with arrow keys.