-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
feat(SegmentedControl): a11y - set role to radiogroup #6691
Changes from 12 commits
6bc4fd8
fbdce7c
a4ba4ea
5b2cb1b
11dfac3
373e598
73a0519
7c7083d
5c7812a
66ed61b
9170ff4
264026a
69e8717
77a6999
7d6da84
ae54d3b
fd215a8
88e0308
a38cda0
57644c3
01b5150
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -17,7 +17,8 @@ | |||||
import classNames from "classnames"; | ||||||
import * as React from "react"; | ||||||
|
||||||
import { Classes, Intent } from "../../common"; | ||||||
import { Classes, Intent, mergeRefs, Utils } from "../../common"; | ||||||
import { getArrowKeyDirection } from "../../common/utils/keyboardUtils"; | ||||||
import { | ||||||
type ControlledValueProps, | ||||||
DISPLAYNAME_PREFIX, | ||||||
|
@@ -26,6 +27,7 @@ import { | |||||
removeNonHTMLProps, | ||||||
} from "../../common/props"; | ||||||
import { Button } from "../button/buttons"; | ||||||
import type { ButtonProps } from "../button/buttonProps"; | ||||||
|
||||||
export type SegmentedControlIntent = typeof Intent.NONE | typeof Intent.PRIMARY; | ||||||
|
||||||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should keep this code more like it was before - where we use this local state index only if using the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done! Reduced the diff a ton, mostly the same code as was before, but arrow keys work as indended. |
||||||
options.findIndex(option => option.value === (controlledValue ?? defaultValue)), | ||||||
); | ||||||
|
||||||
const [localValue, setLocalValue] = React.useState<string | undefined>(defaultValue); | ||||||
const selectedValue = controlledValue ?? localValue; | ||||||
const outerRef = React.useRef<HTMLDivElement>(null); | ||||||
const optionRefs = options.map(() => React.useRef<HTMLButtonElement>(null)); | ||||||
|
||||||
const handleOptionClick = React.useCallback( | ||||||
(newSelectedValue: string, targetElement: HTMLElement) => { | ||||||
setLocalValue(newSelectedValue); | ||||||
onValueChange?.(newSelectedValue, targetElement); | ||||||
(index: number, e: React.MouseEvent<HTMLElement>) => { | ||||||
setSelectedIndex(index); | ||||||
onValueChange?.(options[index].value, e.currentTarget); | ||||||
}, | ||||||
[onValueChange], | ||||||
); | ||||||
|
||||||
const handleKeyDown = React.useCallback( | ||||||
(e: React.KeyboardEvent<HTMLDivElement>) => { | ||||||
const direction = getArrowKeyDirection(e); | ||||||
const { current: outerElement } = outerRef; | ||||||
if (direction == undefined || !outerElement) return; | ||||||
|
||||||
const focusedElement = Utils.getActiveElement(outerElement)?.closest<HTMLButtonElement>("button"); | ||||||
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could we leverage the selector query to filter out disabled buttons? e.g.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea, done! |
||||||
el => !el.disabled, | ||||||
); | ||||||
const focusedIndex = enabledOptionElements.indexOf(focusedElement); | ||||||
if (focusedIndex < 0) return; | ||||||
|
||||||
e.preventDefault(); | ||||||
// auto-wrapping at 0 and `length` | ||||||
const newIndex = (focusedIndex + direction + enabledOptionElements.length) % enabledOptionElements.length; | ||||||
const newOption = enabledOptionElements[newIndex]; | ||||||
newOption.click(); | ||||||
newOption.focus(); | ||||||
}, | ||||||
[outerRef], | ||||||
); | ||||||
|
||||||
const classes = classNames(Classes.SEGMENTED_CONTROL, className, { | ||||||
[Classes.FILL]: fill, | ||||||
[Classes.INLINE]: inline, | ||||||
}); | ||||||
|
||||||
return ( | ||||||
<div className={classes} ref={ref} {...removeNonHTMLProps(htmlProps)}> | ||||||
{options.map(option => ( | ||||||
<div | ||||||
role="radiogroup" | ||||||
{...removeNonHTMLProps(htmlProps)} | ||||||
onKeyDown={handleKeyDown} | ||||||
className={classes} | ||||||
ref={mergeRefs(ref, outerRef)} | ||||||
> | ||||||
{options.map((option, index) => ( | ||||||
<SegmentedControlOption | ||||||
{...option} | ||||||
intent={intent} | ||||||
isSelected={selectedValue === option.value} | ||||||
isSelected={index == selectedIndex} | ||||||
key={option.value} | ||||||
large={large} | ||||||
onClick={handleOptionClick} | ||||||
onClick={e => handleOptionClick(index, e)} | ||||||
ref={optionRefs[index]} | ||||||
small={small} | ||||||
// selectedIndex==-1 accounts for case where passed value/defaultValue is not one of the values of the passed options. | ||||||
tabIndex={selectedIndex == -1 && index == 0 ? 0 : undefined} | ||||||
/> | ||||||
))} | ||||||
</div> | ||||||
|
@@ -133,17 +173,27 @@ SegmentedControl.displayName = `${DISPLAYNAME_PREFIX}.SegmentedControl`; | |||||
|
||||||
interface SegmentedControlOptionProps | ||||||
extends OptionProps<string>, | ||||||
Pick<SegmentedControlProps, "intent" | "small" | "large"> { | ||||||
Pick<SegmentedControlProps, "intent" | "small" | "large">, | ||||||
Pick<ButtonProps, "tabIndex" | "ref" | "onClick"> { | ||||||
isSelected: boolean; | ||||||
onClick: (value: string, targetElement: HTMLElement) => void; | ||||||
/** | ||||||
* @default 0 if isSelected else -1 | ||||||
*/ | ||||||
tabIndex?: ButtonProps["tabIndex"]; | ||||||
} | ||||||
|
||||||
function SegmentedControlOption({ isSelected, label, onClick, value, ...buttonProps }: SegmentedControlOptionProps) { | ||||||
const handleClick = React.useCallback( | ||||||
(event: React.MouseEvent<HTMLElement>) => onClick?.(value, event.currentTarget), | ||||||
[onClick, value], | ||||||
); | ||||||
|
||||||
return <Button onClick={handleClick} minimal={!isSelected} text={label} {...buttonProps} />; | ||||||
} | ||||||
const SegmentedControlOption: React.FC<SegmentedControlOptionProps> = React.forwardRef( | ||||||
({ isSelected, label, value, tabIndex, ...buttonProps }, ref) => ( | ||||||
<Button | ||||||
role="radio" | ||||||
aria-checked={isSelected} | ||||||
minimal={!isSelected} | ||||||
text={label} | ||||||
{...buttonProps} | ||||||
ref={ref} | ||||||
// "roving tabIndex" on a radiogroup: https://www.w3.org/WAI/ARIA/apg/practices/keyboard-interface/#kbd_roving_tabindex | ||||||
tabIndex={tabIndex ?? (isSelected ? 0 : -1)} | ||||||
/> | ||||||
), | ||||||
); | ||||||
SegmentedControlOption.displayName = `${DISPLAYNAME_PREFIX}.SegmentedControlOption`; |
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.
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 rightthis 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theradiogroup
role, here in the official example of this role you can see thatup = left
. The action description is given as well: