From 13258fe6b633f8c043c4fae59454adac8cbcfe8f Mon Sep 17 00:00:00 2001 From: Andrey Medvedev Date: Tue, 20 Aug 2024 15:22:30 +0300 Subject: [PATCH] fix(CustomSelect): a11y allow NVDA to read selected option (#7235) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Оказалось, что `NVDA` не зачитывает элементы расположенные рядом с `input`, если фокус есть на `input`, в отличии от `VoiceOver`. Есть несколько решений. 1. Отказаться от использования `input` как основного элемента `CustomSelect` c ролью `combobox`. Не хотелось бы от него отказываться, чтобы продолжал работать нативный фокус на `CustomSelect` при клике на связанный с ним `label`. > [!NOTE] > Но если проблемы продолжатся, и это будет причиной, то стоит пожертвовать нативным фокусом. 2. Переделать `input` так, чтобы он всегда хранил `label` выбранной опции, тогда `NVDA` сможет его зачитывать. Тут проблема в том, что` option.label `может быть не только строкой, но и react-компонентом, поэтому нельзя `option.label` выбранной в данный момент опции, просто так передать в `input`.\. https://github.com/VKCOM/VKUI/blob/1662eeae1a24e36f6f9f0c0331b16bd414d895cd/packages/vkui/src/components/CustomSelect/CustomSelect.tsx#L126 Про такой вариант даже немножка в доке про [combobox](https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Roles/combobox_role) на MDN сказано > If the combobox element is an [](https://developer.mozilla.org/en-US/docs/Web/HTML/Element/input) element, the value of the combobox is the input's value. Otherwise, the value of the combobox comes from its descendant elements. Изменения В качестве решения выбран второй вариант с передачей в `input.value` `option.label`. Для того, что мы могли передать туда и текстовое представление react-компонента используем утилитарную функцию [getTextFromChildren](https://github.com/VKCOM/VKUI/blob/1662eeae1a24e36f6f9f0c0331b16bd414d895cd/packages/vkui/src/lib/children.ts#L19) Чтобы минимизировать различия с дизайном, в том числе когда `option.label` это реакт-компонент, мы продолжаем input рендерить скрыто, с `opacity: 0`, а поверх рендерим контейнер для option.label, где спокойно может лежать react-компонент. В обычном режиме `CustomSelect` (не `searchable`) `input` не виден даже при фокусе. В режиме `searchable` мы продолжаем рендерить контейнер поверх `input`, но при фокусе на `CustomSelect` (а значит на `input`) мы `input` показываем, чтобы пользователь мог взаимодействовать с ним для ввода текста и поиска опций. Если в `CustomSelect` уже выбрана какая-то опция, при фокусе мы оставляем текст инпута на месте, чтобы пользователи скринридера могли прочитать выбранное в данный момент значение. - Отрефакторил классы `CustomSelectInput` чтобы было понятнее к чему они относятся. - Убрал из CustomSelect отдельный скрытый текст лэйбла, добавленный ранее для скринридеров, так как он никак не помогает `NVDA` и его теперь заменяет значение `value` у `CustomSelectInput`. - На `blur` мы устанавливаем значение` input.value` равным `option.label`, если есть выбранная опция, либо ''. - Также стараемся реагировать на изменения значения `select.value`, чтобы `input.value` всегда обновлялось в соответствии с текущей выбранной опцией. Это особенно актуально, когда `CustomSelect` `value` устанавливается снаружи. - Изменился способ передачи в `CustomSelectInput` значения `label` текущей выбранной опции, раньше label передавался как `children` и было не понятно что в этом свойстве хранится, пока не посмотришь на то, как `CustomSelectInput` используется внутри `CustomSelect`. Теперь`label` мы передаем в свойстве `selectedOptionLabel` - Убрал `aria-owns` из `combobox` так как аттрибут устарел и мешает `NVDA` в `Firefox` правильно зачитывать опции (c `aria-owns` вместо названия опции зачитывается "секция") --- .../CustomSelect/CustomSelect.stories.tsx | 133 +++++++++++++++- .../CustomSelect/CustomSelect.test.tsx | 140 ++++++++++------- .../components/CustomSelect/CustomSelect.tsx | 146 ++++++------------ .../CustomSelect/CustomSelectInput.module.css | 90 ++++++----- .../CustomSelect/CustomSelectInput.tsx | 59 +++---- .../src/components/CustomSelect/helpers.tsx | 61 ++++++++ .../vkui/src/components/CustomSelect/types.ts | 15 ++ .../SelectTypography/SelectTypography.tsx | 7 +- 8 files changed, 417 insertions(+), 234 deletions(-) create mode 100644 packages/vkui/src/components/CustomSelect/helpers.tsx create mode 100644 packages/vkui/src/components/CustomSelect/types.ts diff --git a/packages/vkui/src/components/CustomSelect/CustomSelect.stories.tsx b/packages/vkui/src/components/CustomSelect/CustomSelect.stories.tsx index 2799fb0bb6..4cb0ebbde2 100644 --- a/packages/vkui/src/components/CustomSelect/CustomSelect.stories.tsx +++ b/packages/vkui/src/components/CustomSelect/CustomSelect.stories.tsx @@ -1,7 +1,15 @@ +import * as React from 'react'; import { Meta, StoryObj } from '@storybook/react'; import { fn } from '@storybook/test'; +import { Icon24User } from '@vkontakte/icons'; import { CanvasFullLayout, DisableCartesianParam } from '../../storybook/constants'; -import { cities } from '../../testing/mock'; +import { cities, getRandomUsers } from '../../testing/mock'; +import { Avatar } from '../Avatar/Avatar'; +import { CustomSelectOption } from '../CustomSelectOption/CustomSelectOption'; +import { Div } from '../Div/Div'; +import { FormItem } from '../FormItem/FormItem'; +import { FormLayoutGroup } from '../FormLayoutGroup/FormLayoutGroup'; +import { Header } from '../Header/Header'; import { CustomSelect, SelectProps } from './CustomSelect'; const story: Meta = { @@ -21,3 +29,126 @@ export const Playground: Story = { options: cities, }, }; + +function getUsers(usersArray: ReturnType) { + return usersArray.map((user) => ({ + label: user.name, + value: `${user.id}`, + avatar: user.photo_100, + description: user.screen_name, + })); +} + +export const QAPlayground: Story = { + render: function Render() { + const selectTypes = [ + { + label: 'default', + value: 'default', + }, + { + label: 'plain', + value: 'plain', + }, + { + label: 'accent', + value: 'accent', + }, + ]; + + const [selectType, setSelectType] = React.useState( + undefined, + ); + const users = [...getUsers(getRandomUsers(10))]; + return ( +
+
Custom Select на десктопе
+
Базовые примеры использования
+ + + + + + + + setSelectType(e.target.value as SelectProps['selectType'])} + renderOption={({ option, ...restProps }) => ( + + )} + /> + + + + + ( + } + description={option.description} + /> + )} + /> + + + + + + +
Поиск
+ + } + placeholder="Введите имя пользователя" + searchable + id="administrator-select-searchable-id-3" + options={users} + allowClearButton + /> + +
+ ); + }, +}; diff --git a/packages/vkui/src/components/CustomSelect/CustomSelect.test.tsx b/packages/vkui/src/components/CustomSelect/CustomSelect.test.tsx index 0017455848..f3ac4f80e1 100644 --- a/packages/vkui/src/components/CustomSelect/CustomSelect.test.tsx +++ b/packages/vkui/src/components/CustomSelect/CustomSelect.test.tsx @@ -6,7 +6,10 @@ import { Avatar } from '../Avatar/Avatar'; import { CustomSelectOption } from '../CustomSelectOption/CustomSelectOption'; import { CustomSelect, type CustomSelectRenderOption, type SelectProps } from './CustomSelect'; -const getCustomSelectValue = () => screen.getByTestId('labelTextTestId').textContent; +const checkCustomSelectLabelValue = (label: string) => { + expect(screen.getByTestId('labelTextTestId').textContent).toEqual(label); + expect(screen.getByRole('combobox').value).toEqual(label); +}; const CustomSelectControlled = ({ options, @@ -52,39 +55,49 @@ describe('CustomSelect', () => { />, ); - expect(getCustomSelectValue()).toEqual(''); + checkCustomSelectLabelValue(''); fireEvent.click(screen.getByTestId('labelTextTestId')); const unselectedOption = screen.getByRole('option', { selected: false, name: 'Josh' }); fireEvent.mouseEnter(unselectedOption); fireEvent.click(unselectedOption); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); }); it('works correctly as controlled component', () => { const SelectController = () => { - const [value, setValue] = useState(0); + const [value, setValue] = useState('0'); const options = [ - { value: 0, label: 'Mike' }, - { value: 1, label: 'Josh' }, + { value: '0', label: 'Mike' }, + { value: '1', label: 'Josh' }, ]; return ( - setValue(Number(e.target.value))} - /> + + setValue(e.target.value)} + /> + + ); }; render(); - expect(getCustomSelectValue()).toEqual('Mike'); + + checkCustomSelectLabelValue('Mike'); + fireEvent.click(screen.getByTestId('labelTextTestId')); const unselectedOption = screen.getByRole('option', { selected: false, name: 'Josh' }); fireEvent.mouseEnter(unselectedOption); fireEvent.click(unselectedOption); - expect(getCustomSelectValue()).toEqual('Josh'); + + checkCustomSelectLabelValue('Josh'); + + fireEvent.click(screen.getByRole('button', { name: /Clear controlled value/ })); + + checkCustomSelectLabelValue(''); }); it('works correctly with pinned value', () => { @@ -95,12 +108,14 @@ describe('CustomSelect', () => { render(); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); + fireEvent.click(screen.getByTestId('labelTextTestId')); const unselectedOption = screen.getByRole('option', { selected: false, name: 'Josh' }); fireEvent.mouseEnter(unselectedOption); fireEvent.click(unselectedOption); - expect(getCustomSelectValue()).toEqual('Mike'); + + checkCustomSelectLabelValue('Mike'); }); it('correctly reacts on options change', () => { @@ -115,7 +130,7 @@ describe('CustomSelect', () => { />, ); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); rerender( { />, ); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); rerender( { />, ); - expect(getCustomSelectValue()).toEqual('Felix'); + checkCustomSelectLabelValue('Felix'); }); it('correctly converts from controlled to uncontrolled state', () => { @@ -156,7 +171,7 @@ describe('CustomSelect', () => { />, ); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); rerender( { />, ); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); fireEvent.click(screen.getByTestId('labelTextTestId')); const unselectedOption = screen.getByRole('option', { selected: false, name: 'Mike' }); fireEvent.mouseEnter(unselectedOption); fireEvent.click(unselectedOption); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); }); - it('accept defaultValue', () => { + it('accepts defaultValue', () => { render( { />, ); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); }); it('is searchable', async () => { @@ -206,6 +221,8 @@ describe('CustomSelect', () => { />, ); + checkCustomSelectLabelValue(''); + fireEvent.click(screen.getByTestId('labelTextTestId')); await waitFor(() => expect(screen.getByTestId('inputTestId')).toHaveFocus()); @@ -219,7 +236,8 @@ describe('CustomSelect', () => { key: 'Enter', code: 'Enter', }); - expect(getCustomSelectValue()).toEqual('Mike'); + + checkCustomSelectLabelValue('Mike'); }); it('is custom searchable', () => { @@ -240,6 +258,8 @@ describe('CustomSelect', () => { />, ); + checkCustomSelectLabelValue(''); + fireEvent.click(screen.getByTestId('inputTestId')); fireEvent.change(screen.getByTestId('inputTestId'), { target: { value: 'usa' }, @@ -252,7 +272,8 @@ describe('CustomSelect', () => { key: 'Enter', code: 'Enter', }); - expect(getCustomSelectValue()).toEqual('New York'); + + checkCustomSelectLabelValue('New York'); }); it('is searchable and keeps search results up to date during props.options updates', async () => { @@ -293,6 +314,7 @@ describe('CustomSelect', () => { const { rerender } = render( { />, ); + checkCustomSelectLabelValue('Josh'); + fireEvent.click(screen.getByTestId('inputTestId')); await waitForFloatingPosition(); @@ -315,6 +339,7 @@ describe('CustomSelect', () => { rerender( { rerender( { ); expect(screen.getByRole('option', { selected: true })).toHaveTextContent('Joe'); + checkCustomSelectLabelValue('Joe'); }); // см. https://github.com/VKCOM/VKUI/issues/3600 @@ -361,6 +388,8 @@ describe('CustomSelect', () => { />, ); + checkCustomSelectLabelValue('Категория 3'); + fireEvent.click(screen.getByTestId('inputTestId')); expect(screen.getByRole('option', { selected: true })).toHaveTextContent('Категория 3'); @@ -373,7 +402,7 @@ describe('CustomSelect', () => { fireEvent.mouseEnter(unselectedOption); fireEvent.click(unselectedOption); - expect(getCustomSelectValue()).toEqual('Категория 2'); + checkCustomSelectLabelValue('Категория 2'); }); it('fires onOpen and onClose correctly', async () => { @@ -459,7 +488,7 @@ describe('CustomSelect', () => { await waitForFloatingPosition(); - expect(getCustomSelectValue()).toEqual('Bob'); + checkCustomSelectLabelValue('Bob'); fireEvent.keyDown(screen.getByTestId('inputTestId'), { key: 'Enter', @@ -475,7 +504,7 @@ describe('CustomSelect', () => { code: 'Enter', }); - expect(getCustomSelectValue()).toEqual('Josh'); + checkCustomSelectLabelValue('Josh'); rerender( { code: 'Enter', }); - expect(getCustomSelectValue()).toEqual('Bob'); + checkCustomSelectLabelValue('Bob'); }); // https://github.com/VKCOM/VKUI/issues/4066 @@ -540,6 +569,8 @@ describe('CustomSelect', () => { />, ); + checkCustomSelectLabelValue('Josh'); + rerender( { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual(''); + checkCustomSelectLabelValue(''); }); it('clear value with default clear button', async () => { @@ -576,10 +607,11 @@ describe('CustomSelect', () => { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); + expect(screen.getByTestId('inputTestId')).not.toHaveFocus(); fireEvent.click(screen.getByRole('button', { hidden: true })); - expect(getCustomSelectValue()).toEqual(''); + checkCustomSelectLabelValue(''); // focus goes to select input await waitFor(() => expect(screen.getByTestId('inputTestId')).toHaveFocus()); @@ -602,10 +634,10 @@ describe('CustomSelect', () => { />, ); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); fireEvent.click(screen.getByTestId('clearButtonTestId')); - expect(getCustomSelectValue()).toEqual(''); + checkCustomSelectLabelValue(''); expect(onChange).toHaveBeenCalledTimes(2); }); @@ -649,7 +681,7 @@ describe('CustomSelect', () => { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); fireEvent.click(screen.getByTestId('labelTextTestId')); expect(screen.getByRole('option', { selected: true })).toHaveTextContent('Mike'); @@ -688,7 +720,7 @@ describe('CustomSelect', () => { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); // clear input fireEvent.click(screen.getByRole('button', { hidden: true })); @@ -758,7 +790,7 @@ describe('CustomSelect', () => { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); // первый клик по не выбранной опции без изменения value fireEvent.click(screen.getByTestId('labelTextTestId')); @@ -813,15 +845,15 @@ describe('CustomSelect', () => { labelTextTestId="labelTextTestId" initialValue="0" options={[ - { value: 0, label: 'Mike' }, - { value: 1, label: 'Josh' }, + { value: '0', label: 'Mike' }, + { value: '1', label: 'Josh' }, ]} onChangeStub={onChangeStub} />, ); expect(onChangeStub).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual('Mike'); + checkCustomSelectLabelValue('Mike'); // первый клик по не выбранной опции с изменением value fireEvent.click(screen.getByTestId('labelTextTestId')); @@ -890,7 +922,7 @@ describe('CustomSelect', () => { ); expect(onChange).toHaveBeenCalledTimes(0); - expect(getCustomSelectValue()).toEqual(''); + checkCustomSelectLabelValue(''); // первый клик по не выбранной опции без изменения value fireEvent.click(screen.getByTestId('inputTestId')); @@ -1036,34 +1068,22 @@ describe('CustomSelect', () => { ); }); - it('shows input placeholder for screen readers only if option is not selected', () => { - // Это позволяет скринридеру зачитывать placeholder, если опция не выбрана. - const { rerender } = render( + it('has placeholder', () => { + render( , ); + // input placeholder expect(screen.queryByPlaceholderText('Не выбрано')).toBeTruthy(); - - rerender( - , - ); - - expect(screen.queryByPlaceholderText('Не выбрано')).toBeFalsy(); + // элемент поверх скрытого инпута + expect(screen.getByTestId('labelTextTestId').textContent).toEqual('Не выбрано'); }); it('native select is reachable via nativeSelectTestId', () => { diff --git a/packages/vkui/src/components/CustomSelect/CustomSelect.tsx b/packages/vkui/src/components/CustomSelect/CustomSelect.tsx index fc5a69a1c6..ba237635a3 100644 --- a/packages/vkui/src/components/CustomSelect/CustomSelect.tsx +++ b/packages/vkui/src/components/CustomSelect/CustomSelect.tsx @@ -2,7 +2,6 @@ import * as React from 'react'; import { classNames, debounce } from '@vkontakte/vkjs'; import { useAdaptivity } from '../../hooks/useAdaptivity'; import { useExternRef } from '../../hooks/useExternRef'; -import { useFocusWithin } from '../../hooks/useFocusWithin'; import { useDOM } from '../../lib/dom'; import type { PlacementWithAuto } from '../../lib/floating'; import { defaultFilterFn, type FilterFn } from '../../lib/select'; @@ -13,21 +12,24 @@ import { CustomSelectDropdown, CustomSelectDropdownProps, } from '../CustomSelectDropdown/CustomSelectDropdown'; -import { - CustomSelectOption, - CustomSelectOptionProps, -} from '../CustomSelectOption/CustomSelectOption'; import { DropdownIcon } from '../DropdownIcon/DropdownIcon'; import { FormFieldProps } from '../FormField/FormField'; import { NativeSelectProps } from '../NativeSelect/NativeSelect'; import { SelectType } from '../Select/Select'; import { Footnote } from '../Typography/Footnote/Footnote'; -import { VisuallyHidden } from '../VisuallyHidden/VisuallyHidden'; import { CustomSelectClearButton, type CustomSelectClearButtonProps, } from './CustomSelectClearButton'; import { CustomSelectInput } from './CustomSelectInput'; +import { + calculateInputValueFromOptions, + defaultRenderOptionFn, + findIndexAfter, + findIndexBefore, + findSelectedIndex, +} from './helpers'; +import type { CustomSelectOptionInterface, CustomSelectRenderOption } from './types'; import styles from './CustomSelect.module.css'; const sizeYClassNames = { @@ -35,32 +37,6 @@ const sizeYClassNames = { ['compact']: styles['CustomSelect--sizeY-compact'], }; -const findIndexAfter = (options: CustomSelectOptionInterface[] = [], startIndex = -1) => { - if (startIndex >= options.length - 1) { - return -1; - } - return options.findIndex((option, i) => i > startIndex && !option.disabled); -}; - -const findIndexBefore = ( - options: CustomSelectOptionInterface[] = [], - endIndex: number = options.length, -) => { - let result = -1; - if (endIndex <= 0) { - return result; - } - for (let i = endIndex - 1; i >= 0; i--) { - let option = options[i]; - - if (!option.disabled) { - result = i; - break; - } - } - return result; -}; - const warn = warnOnce('CustomSelect'); const checkOptionsValueType = (options: T[]) => { @@ -72,33 +48,10 @@ const checkOptionsValueType = (options: T } }; -function defaultRenderOptionFn({ - option, - ...props -}: CustomSelectRenderOption): React.ReactNode { - return ; -} - const handleOptionDown: MouseEventHandler = (e: React.MouseEvent) => { e.preventDefault(); }; -function findSelectedIndex( - options: T[] = [], - value: SelectValue, - withClear: boolean, -) { - if (withClear && value === '') { - return -1; - } - return ( - options.findIndex((item) => { - value = typeof item.value === 'number' ? Number(value) : value; - return item.value === value; - }) ?? -1 - ); -} - const filter = ( options: SelectProps['options'], inputValue: string, @@ -109,21 +62,7 @@ const filter = ( : options; }; -type SelectValue = React.SelectHTMLAttributes['value']; - -export interface CustomSelectOptionInterface { - value: SelectValue; - label: React.ReactElement | string; - disabled?: boolean; - [index: string]: any; -} - -export interface CustomSelectRenderOption - extends CustomSelectOptionProps { - option: T; -} - -export type { CustomSelectClearButtonProps }; +export type { CustomSelectClearButtonProps, CustomSelectOptionInterface, CustomSelectRenderOption }; export interface SelectProps< OptionInterfaceT extends CustomSelectOptionInterface = CustomSelectOptionInterface, @@ -283,10 +222,14 @@ export function CustomSelect(-1); const [isControlledOutside, setIsControlledOutside] = React.useState(props.value !== undefined); - const [inputValue, setInputValue] = React.useState(''); const [nativeSelectValue, setNativeSelectValue] = React.useState( () => props.value ?? defaultValue ?? (allowClearButton ? '' : undefined), ); + + const [inputValue, setInputValue] = React.useState(() => + calculateInputValueFromOptions(optionsProp, nativeSelectValue), + ); + const [popperPlacement, setPopperPlacement] = React.useState( popupDirection, ); @@ -427,7 +370,6 @@ export function CustomSelect { resetKeyboardInput(); - setInputValue(''); setOpened(false); resetFocusedOption(); onClose?.(); @@ -437,8 +379,8 @@ export function CustomSelect { const item = options[index]; - setNativeSelectValue(item?.value); close(); + setNativeSelectValue(item?.value); const shouldTriggerOnChangeWhenControlledAndInnerValueIsOutOfSync = isControlledOutside && @@ -474,7 +416,9 @@ export function CustomSelect { const event = new Event('focusin', { bubbles: true }); @@ -509,27 +453,40 @@ export function CustomSelect = (e) => { @@ -830,7 +787,6 @@ export function CustomSelect = { 'role': 'combobox', 'aria-controls': popupAriaId, - 'aria-owns': popupAriaId, 'aria-expanded': opened, ['aria-activedescendant']: ariaActiveDescendantId && opened ? `${popupAriaId}-${ariaActiveDescendantId}` : undefined, @@ -839,8 +795,6 @@ export function CustomSelect - {focusWithin && selected && !opened && ( - {selected.label} - )} - {selected?.label} - + selectedOptionLabel={selected?.label} + />