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.

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

fix(CustomSelect): a11y allow NVDA to read selected option #7235

Merged
merged 8 commits into from
Aug 20, 2024

Conversation

mendrew
Copy link
Contributor

@mendrew mendrew commented Jul 23, 2024


  • Unit-тесты
  • e2e-тесты (обновил, был конфликт в режиме disabled, не доставало opacity, согласно дизайн системе)
  • Дизайн-ревью
  • QA - проверил специалист в области доступности (Тестировалось на этом примере)
  • Документация фичи

Описание

Оказалось, что NVDA не зачитывает элементы расположенные рядом с input, если фокус есть на input, в отличии от VoiceOver.
Есть несколько решений.

  1. Отказаться от использования input как основного элемента CustomSelect c ролью combobox.
    Не хотелось бы от него отказываться, чтобы продолжал работать нативный фокус на CustomSelect при клике на связанный с ним label.

Note

Но если проблемы продолжатся, и это будет причиной, то стоит пожертвовать нативным фокусом.

  1. Переделать input так, чтобы он всегда хранил label выбранной опции, тогда NVDA сможет его зачитывать.
    Тут проблема в том, чтоoption.labelможет быть не только строкой, но и react-компонентом, поэтому нельзя option.label выбранной в данный момент опции, просто так передать в input..
    label: React.ReactElement | string;

    Про такой вариант даже немножка в доке про combobox на MDN сказано

    If the combobox element is an 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
Чтобы минимизировать различия с дизайном, в том числе когда 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 вместо названия опции зачитывается "секция")

Видео

Поведение в режиме я searchable.
При фокусе выделяется текст выбранной опции.

Screen.Recording.2024-08-09.at.13.49.57.mov

@github-actions github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Jul 23, 2024
Copy link
Contributor

github-actions bot commented Jul 23, 2024

size-limit report 📦

Path Size
JS 378.1 KB (+0.03% 🔺)
JS (gzip) 114.66 KB (+0.04% 🔺)
JS (brotli) 94.11 KB (-0.05% 🔽)
JS import Div (tree shaking) 1.39 KB (0%)
CSS 306.03 KB (+0.16% 🔺)
CSS (gzip) 39.26 KB (+0.14% 🔺)
CSS (brotli) 31.5 KB (+0.18% 🔺)

Copy link

codesandbox-ci bot commented Jul 23, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Copy link
Contributor

github-actions bot commented Jul 23, 2024

e2e tests

⚠️ Some screenshots were failed. See Playwright Report.

Playwright Report

Copy link
Contributor

github-actions bot commented Jul 23, 2024

👀 Docs deployed

Commit 2608468

Copy link

codecov bot commented Jul 23, 2024

Codecov Report

Attention: Patch coverage is 91.93548% with 5 lines in your changes missing coverage. Please review.

Project coverage is 92.76%. Comparing base (88727c8) to head (2608468).

Files Patch % Lines
...kages/vkui/src/components/CustomSelect/helpers.tsx 86.11% 5 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7235      +/-   ##
==========================================
+ Coverage   92.74%   92.76%   +0.01%     
==========================================
  Files         374      375       +1     
  Lines       11103    11111       +8     
  Branches     3646     3643       -3     
==========================================
+ Hits        10298    10307       +9     
+ Misses        805      804       -1     
Flag Coverage Δ
unittests 92.76% <91.93%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mendrew mendrew force-pushed the mendrew/6974/CustomSelect/a11y-selected-option branch from ec03446 to 006d11f Compare July 24, 2024 18:46
@vkcom-publisher vkcom-publisher added pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности and removed pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности labels Aug 2, 2024
@mendrew mendrew force-pushed the mendrew/6974/CustomSelect/a11y-selected-option branch from 4b42895 to 479d29e Compare August 8, 2024 08:05
@mendrew mendrew marked this pull request as ready for review August 9, 2024 10:51
@mendrew mendrew requested a review from a team as a code owner August 9, 2024 10:51
inomdzhon
inomdzhon previously approved these changes Aug 16, 2024
Copy link
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

🤯🤯🤯.... кропотливая работёнка 🧑‍🔬

спасибо 🙏


Вопрос

Ничего что в скриншотах placeholder теперь ещё прозрачней?

mendrew and others added 7 commits August 19, 2024 14:56
Refactor SelectInput classes

Always set input value for screen readers
Reset inputValue in case selected option is not found

Move extra functions and types to types and helpers files
Check input value as well as it is important for screen readers
Add preselected value
Fix test
Don't read first option when dropdown just opened and nothing is
selected
Turned out aria-own forced Firefox to read first option
when no options selected on dropdown open.
Removing it let us avoid this frustration
Turned out section, section, section NVDA was reading
because of aria-owns
@mendrew
Copy link
Contributor Author

mendrew commented Aug 19, 2024

Вопрос

Ничего что в скриншотах placeholder теперь ещё прозрачней?

Это как раз хорошо, у нас такая opacity по умолчанию в FormField.

.FormField--disabled {
opacity: var(--vkui--opacity_disable_accessibility);

И теперь такая же в CustomSelect.

.CustomSelectInput__input:disabled ~ .CustomSelectInput__label-wrapper {
opacity: var(--vkui--opacity_disable_accessibility);

У нас как раз такая прозрачность в дизайне
Screenshot 2024-08-19 at 14 53 17

@mendrew mendrew force-pushed the mendrew/6974/CustomSelect/a11y-selected-option branch from 0f3d72b to 2608468 Compare August 19, 2024 11:57
Copy link
Contributor

@inomdzhon inomdzhon left a comment

Choose a reason for hiding this comment

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

👏

@BlackySoul
Copy link
Contributor

VID_20240820172448.mp4

Понимаю, что это к NVDA не относится, но, кажется, на андроиде (talk back), че т совсем неважно себя чувствуют селекты, они почему-то все зачитываются как "disabled", и активировать их никак нельзя 🤔

@mendrew
Copy link
Contributor Author

mendrew commented Aug 20, 2024

VID_20240820172448.mp4

Понимаю, что это к NVDA не относится, но, кажется, на андроиде (talk back), че т совсем неважно себя чувствуют селекты, они почему-то все зачитываются как "disabled", и активировать их никак нельзя 🤔

Да, дело в том, что CustomSelect предназначен только для десктопа. Для мобилок он недоступен. Для мобилок мы рекомендуем использовать нативный Select, либо наш NativeSelect, который визульно только выглядит как input VKUI, но не имеет множества кастомных свойств CustomSelect. По этой причине наш Select в зависимости от устройства использует под капотом либо NativeSelect либо CustomSelect.

@mendrew mendrew merged commit f48a3cf into master Aug 20, 2024
1 check passed
@mendrew mendrew deleted the mendrew/6974/CustomSelect/a11y-selected-option branch August 20, 2024 12:22
vkcom-publisher pushed a commit that referenced this pull request Aug 20, 2024
Оказалось, что `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 [<input>](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` вместо названия опции зачитывается "секция")
SevereCloud added a commit that referenced this pull request Sep 18, 2024
SevereCloud added a commit that referenced this pull request Sep 19, 2024
* Revert "fix(CustomSelect): fix custom select cursor updating (#7508)"

This reverts commit dd77217.

* Revert "fix(CustomSelect): a11y allow NVDA to read selected option (#7235)"

This reverts commit 13258fe.
vkcom-publisher pushed a commit that referenced this pull request Sep 19, 2024
* Revert "fix(CustomSelect): fix custom select cursor updating (#7508)"

This reverts commit dd77217.

* Revert "fix(CustomSelect): a11y allow NVDA to read selected option (#7235)"

This reverts commit 13258fe.
vkcom-publisher pushed a commit that referenced this pull request Sep 19, 2024
* Revert "fix(CustomSelect): fix custom select cursor updating (#7508)"

This reverts commit dd77217.

* Revert "fix(CustomSelect): a11y allow NVDA to read selected option (#7235)"

This reverts commit 13258fe.
SevereCloud added a commit that referenced this pull request Sep 24, 2024
* Revert "fix(CustomSelect): fix custom select cursor updating (#7508)"

This reverts commit dd77217.

* Revert "fix(CustomSelect): a11y allow NVDA to read selected option (#7235)"

This reverts commit 13258fe.
SevereCloud added a commit that referenced this pull request Sep 24, 2024
* Revert "fix(CustomSelect): fix custom select cursor updating (#7508)"

This reverts commit dd77217.

* Revert "fix(CustomSelect): a11y allow NVDA to read selected option (#7235)"

This reverts commit 13258fe.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
None yet
5 participants