Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Refactor SelectNext into [role=combobox] to improve a11y #1330

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Refactor SelectNext into [role=combobox] to improve a11y #1330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions src/components/input/SelectNext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,8 @@ import { useFocusAway } from '../../hooks/use-focus-away';
import { useKeyPress } from '../../hooks/use-key-press';
import { useSyncedRef } from '../../hooks/use-synced-ref';
import type { PresentationalProps } from '../../types';
import { downcastRef } from '../../util/typing';
import { MenuCollapseIcon, MenuExpandIcon } from '../icons';
import Button from './Button';
import { inputGroupStyles } from './InputGroup';
import SelectContext from './SelectContext';

Expand Down Expand Up @@ -149,6 +149,9 @@ export type SelectProps<T> = PresentationalProps & {
*/
buttonId?: string;

'aria-label'?: string;
'aria-labelledby'?: string;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An issue I noticed when reading the docs, but that I don't think we should try to address in this PR:

The documentation creates some slight confusion here because at the top of the "Component API" section it links to http://localhost:4001/using-components#presentational-components-api, which states that HTML attributes are accepted. For this particular control, those attributes are not available however. I'm not sure that accepting HTML attributes as rest props makes sense because it might be unclear which element they are applied to (the container? the button? the listbox?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this was a confusion on my part. SelectNextProps extends PresentationalProps, but that type does not mention anything about HTML attributes.

I have noticed though, that all other components extending PresentationalProps also extend some form of JSX.HTMLAttributes<...>, which is not the case for SelectNext.

So I don't know if PresentationalProps should actually implicitly extend JSX.HTMLAttributes and we should have some other type with a different name for what is currently PresentationalProps.


/** @deprecated Use buttonContent instead */
label?: ComponentChildren;
};
Expand All @@ -163,6 +166,8 @@ function SelectMain<T>({
elementRef,
classes,
buttonId,
'aria-label': ariaLabel,
'aria-labelledby': ariaLabelledBy,
}: SelectProps<T>) {
const [listboxOpen, setListboxOpen] = useState(false);
const closeListbox = useCallback(() => setListboxOpen(false), []);
Expand Down Expand Up @@ -212,11 +217,11 @@ function SelectMain<T>({
className={classnames('relative w-full border rounded', inputGroupStyles)}
ref={wrapperRef}
>
<Button
<button
id={buttonId ?? defaultButtonId}
variant="custom"
classes={classnames(
'w-full flex justify-between',
className={classnames(
'focus-visible-ring transition-colors whitespace-nowrap',
'w-full flex items-center justify-between gap-x-2 p-2',
Copy link
Contributor Author

@acelaya acelaya Oct 23, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These classes were automatically applied by our Button component, but we have moved to the regular button because our own only allows to set aria-label via title.

'bg-grey-0 disabled:bg-grey-1 disabled:text-grey-6',
// Add inherited rounded corners so that the toggle is consistent with
// the wrapper, which is the element rendering borders.
Expand All @@ -225,11 +230,15 @@ function SelectMain<T>({
'rounded-[inherit]',
classes,
)}
expanded={listboxOpen}
type="button"
role="combobox"
disabled={disabled}
aria-expanded={listboxOpen}
aria-haspopup="listbox"
aria-controls={listboxId}
elementRef={buttonRef}
aria-label={ariaLabel}
aria-labelledby={ariaLabelledBy}
ref={downcastRef(buttonRef)}
onClick={() => setListboxOpen(prev => !prev)}
onKeyDown={e => {
if (e.key === 'ArrowDown' && !listboxOpen) {
Expand All @@ -243,7 +252,7 @@ function SelectMain<T>({
<div className="text-grey-6">
{listboxOpen ? <MenuCollapseIcon /> : <MenuExpandIcon />}
</div>
</Button>
</button>
<SelectContext.Provider value={{ selectValue, value }}>
<ul
className={classnames(
Expand Down
4 changes: 2 additions & 2 deletions src/components/input/test/SelectNext-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -289,15 +289,15 @@ describe('SelectNext', () => {
name: 'Closed Select listbox',
content: () =>
createComponent(
{ buttonContent: 'Select' },
{ buttonContent: 'Select', 'aria-label': 'Select' },
{ optionsChildrenAsCallback: false },
),
},
{
name: 'Open Select listbox',
content: () => {
const wrapper = createComponent(
{ buttonContent: 'Select' },
{ buttonContent: 'Select', 'aria-label': 'Select' },
{ optionsChildrenAsCallback: false },
);
toggleListbox(wrapper);
Expand Down
2 changes: 1 addition & 1 deletion src/pattern-library/components/Library.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ export type LibraryDemoProps = {
classes?: string | string[];
/** Inline styles to apply to the demo container */
style?: JSX.CSSProperties;
title?: string;
title?: ComponentChildren;
/**
* Should the demo also render the source? When true, a "Source" tab will be
* rendered, which will display the JSX source of the Demo's children.
Expand Down
252 changes: 154 additions & 98 deletions src/pattern-library/components/patterns/prototype/SelectNextPage.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import classnames from 'classnames';
import { useCallback, useMemo, useState } from 'preact/hooks';
import { useCallback, useId, useMemo, useState } from 'preact/hooks';

import { ArrowLeftIcon, ArrowRightIcon } from '../../../../components/icons';
import type { SelectNextProps } from '../../../../components/input';
import { IconButton, InputGroup } from '../../../../components/input';
import SelectNext from '../../../../components/input/SelectNext';
import Library from '../../Library';
Expand All @@ -23,65 +24,77 @@ const defaultItems: ItemType[] = [
function SelectExample({
disabled,
textOnly,
classes,
items = defaultItems,
}: {
disabled?: boolean;
...rest
}: Pick<
SelectNextProps<ItemType>,
'aria-label' | 'aria-labelledby' | 'classes' | 'disabled'
> & {
textOnly?: boolean;
classes?: string;
items?: typeof defaultItems;
items?: ItemType[];
}) {
const [value, setValue] = useState<ItemType>();
const buttonId = useId();

return (
<SelectNext
value={value}
onChange={setValue}
classes={classes}
disabled={disabled}
buttonContent={
value ? (
<>
{textOnly && value.name}
{!textOnly && (
<div className="flex">
<div className="truncate">{value.name}</div>
<div className="rounded px-2 ml-2 bg-grey-7 text-white">
{value.id}
</div>
</div>
)}
</>
) : disabled ? (
<>This is disabled</>
) : (
<>Select one...</>
)
}
>
{items.map(item => (
<SelectNext.Option value={item} key={item.id} disabled={item.disabled}>
{({ disabled }) =>
textOnly ? (
item.name
) : (
<>
{item.name}
<div className="grow" />
<div
className={classnames('rounded px-2 ml-2 text-white', {
'bg-grey-7': !disabled,
'bg-grey-4': disabled,
})}
>
{item.id}
<>
{!rest['aria-label'] && !rest['aria-labelledby'] && (
<label htmlFor={buttonId}>Select a person</label>
)}
<SelectNext
{...rest}
buttonId={buttonId}
value={value}
onChange={setValue}
disabled={disabled}
buttonContent={
value ? (
<>
{textOnly && value.name}
{!textOnly && (
<div className="flex">
<div className="truncate">{value.name}</div>
<div className="rounded px-2 ml-2 bg-grey-7 text-white">
{value.id}
</div>
</div>
</>
)
}
</SelectNext.Option>
))}
</SelectNext>
)}
</>
) : disabled ? (
<>This is disabled</>
) : (
<>Select one…</>
)
}
>
{items.map(item => (
<SelectNext.Option
value={item}
key={item.id}
disabled={item.disabled}
>
{({ disabled }) =>
textOnly ? (
item.name
) : (
<>
{item.name}
<div className="grow" />
<div
className={classnames('rounded px-2 ml-2 text-white', {
'bg-grey-7': !disabled,
'bg-grey-4': disabled,
})}
>
{item.id}
</div>
</>
)
}
</SelectNext.Option>
))}
</SelectNext>
</>
);
}

Expand All @@ -99,53 +112,58 @@ function InputGroupSelectExample({ classes }: { classes?: string }) {
const newIndex = selectedIndex - 1;
setSelected(defaultItems[newIndex] ?? selected);
}, [selected, selectedIndex]);
const buttonId = useId();

return (
<InputGroup>
<IconButton
icon={ArrowLeftIcon}
title="Previous student"
variant="dark"
onClick={previous}
disabled={selectedIndex <= 0}
/>
<SelectNext
value={selected}
onChange={setSelected}
classes={classes}
buttonContent={
selected ? (
<div className="flex">
<div className="truncate">{selected.name}</div>
<div className="rounded px-2 ml-2 bg-grey-7 text-white">
{selected.id}
<>
<label htmlFor={buttonId}>Select a person</label>
<InputGroup>
<IconButton
icon={ArrowLeftIcon}
title="Previous student"
variant="dark"
onClick={previous}
disabled={selectedIndex <= 0}
/>
<SelectNext
buttonId={buttonId}
value={selected}
onChange={setSelected}
classes={classes}
buttonContent={
selected ? (
<div className="flex">
<div className="truncate">{selected.name}</div>
<div className="rounded px-2 ml-2 bg-grey-7 text-white">
{selected.id}
</div>
</div>
</div>
) : (
<>Select one...</>
)
}
>
{defaultItems.map(item => (
<SelectNext.Option value={item} key={item.id}>
{item.name}
<div className="grow" />
<div
className={classnames('rounded px-2 ml-2 text-white bg-grey-7')}
>
{item.id}
</div>
</SelectNext.Option>
))}
</SelectNext>
<IconButton
icon={ArrowRightIcon}
title="Next student"
variant="dark"
onClick={next}
disabled={selectedIndex >= defaultItems.length - 1}
/>
</InputGroup>
) : (
<>Select one…</>
)
}
>
{defaultItems.map(item => (
<SelectNext.Option value={item} key={item.id}>
{item.name}
<div className="grow" />
<div
className={classnames('rounded px-2 ml-2 text-white bg-grey-7')}
>
{item.id}
</div>
</SelectNext.Option>
))}
</SelectNext>
<IconButton
icon={ArrowRightIcon}
title="Next student"
variant="dark"
onClick={next}
disabled={selectedIndex >= defaultItems.length - 1}
/>
</InputGroup>
</>
);
}

Expand Down Expand Up @@ -228,6 +246,44 @@ export default function SelectNextPage() {
</Library.Demo>
</Library.Example>

<Library.Example title="Labeling SelectNext">
<p>
There are three ways to label a <code>SelectNext</code>. Make sure
you always use one of them.
</p>

<Library.Demo
title={
<>
Via{' '}
<code>
{'<'}label {'/>'}
</code>{' '}
linked to <code>buttonId</code>
</>
}
>
<div className="w-96 mx-auto">
<SelectExample />
</div>
</Library.Demo>

<Library.Demo title="Via aria-label">
<div className="w-96 mx-auto">
<SelectExample aria-label="Select a person with aria label" />
</div>
</Library.Demo>

<Library.Demo title="Via aria-labelledby">
<div className="w-96 mx-auto">
<p id="select-next-meta-label">
Select a person with aria labelledby
</p>
<SelectExample aria-labelledby="select-next-meta-label" />
</div>
</Library.Demo>
</Library.Example>

<Library.Example title="Select with long content">
<p>
<code>SelectNext</code> makes sure the button content never
Expand Down Expand Up @@ -403,7 +459,7 @@ export default function SelectNextPage() {
</div>
</>
) : (
<>Select one...</>
<>Select one…</>
)
}
>
Expand Down