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(FocusTrap): add mutation observer #7041

Merged

Conversation

EldarMuhamethanov
Copy link
Contributor


  • Unit-тесты

Описание

Сейчас в FocusTrap если children не меняется, то может и не происходить перерасчёт focusableNodesRef.current.
Например, такое происходит в модалке, если мы динамически добавляем/удаляем инпуты.

Изменения

Добавил MutationObserver для отслеживания добавления/удаления элементов внутри компонента обернутого FocusTrap. При изменении содержимого происходит перерасчет focusableNodesRef. Таким образом поддерживается актуальное состояние списка нод.

Вручную потыкал компоненты где используется FocusTrap - все работает

MutationObserver проверил на поддержку в браузерах:
image

Добавил MutationObserver для отслеживания добавления/удаления элементов внутри компонента обернутого FocusTrap
@EldarMuhamethanov EldarMuhamethanov requested a review from a team as a code owner June 20, 2024 19:20
Copy link

codesandbox-ci bot commented Jun 20, 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 Jun 21, 2024

e2e tests

Playwright Report

Copy link
Contributor

github-actions bot commented Jun 21, 2024

👀 Docs deployed

Commit 6245db1

Copy link

codecov bot commented Jun 21, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 83.55%. Comparing base (1033ec0) to head (6245db1).
Report is 7 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7041      +/-   ##
==========================================
+ Coverage   83.51%   83.55%   +0.04%     
==========================================
  Files         351      351              
  Lines       10510    10532      +22     
  Branches     3486     3489       +3     
==========================================
+ Hits         8777     8800      +23     
+ Misses       1733     1732       -1     
Flag Coverage Δ
unittests 83.55% <100.00%> (+0.04%) ⬆️

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.

Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

Шикарно!

Ещё было бы здорово проверить что происходит с фокусом элемента, который перерендерился.

Вот, например, есть форма внутри попапа, которую контролирует FocusTrap. FocusTrap у нас сейчас следит, чтобы мы не вышли за пределы попапа и перекидывает фокус на первый интерактивны элемент внутри FocusTrap если мы потенциально собираемся вывалиться за пределы FocusTrap.

А что если пользователь дошел фокусом до какого-то элемента, совершил какое-то действие и форма перерендерилась, в том числе и элемент на котором был фокус, куда уйдёт фокус? Плохо будет, если фокус просто пропадёт из поля зрения.

По хорошему бы либо вернуть фокус на тот элемент на котором он был после мутации, либо на первый элемент внутри FocusTrap. И сделать это сразу после мутации, чтобы не заставлять пользователя нажимать Tab, чтобы понять где примерно сейчас фокус.

Что думаешь? Поправь меня, пожалуйста, если я не прав.

@EldarMuhamethanov
Copy link
Contributor Author

@mendrew Согласен с тобой, добавил установку фокуса после мутации

inomdzhon
inomdzhon previously approved these changes Jun 21, 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.

Как написал Андрей, шикарно!)

Мб попробуем написать Unit-тест на кейс, который описал Андрей, чтобы не потерялось?

@EldarMuhamethanov
Copy link
Contributor Author

@inomdzhon написал два теста, которые затрагивают новый функционал

mendrew
mendrew previously approved these changes Jun 21, 2024
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

🔥 Супер, спасибо за тесты!

mendrew
mendrew previously approved these changes Jun 21, 2024
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

💅

Переделал тесты на React-way тестирование
mendrew
mendrew previously approved these changes Jun 24, 2024
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

Даже как-то, вроде, наглядней стало. 💅

mendrew
mendrew previously approved these changes Jun 24, 2024
Copy link
Contributor

@mendrew mendrew left a comment

Choose a reason for hiding this comment

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

👍

mendrew
mendrew previously approved these changes Jun 24, 2024
@inomdzhon inomdzhon added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Jun 24, 2024
@inomdzhon inomdzhon merged commit 4ee1f30 into VKCOM:master Jun 24, 2024
26 checks passed
inomdzhon pushed a commit that referenced this pull request Jun 24, 2024
h2. Описание
Сейчас в FocusTrap если children не меняется, то может и не происходить перерасчёт focusableNodesRef.current.
Например, такое происходит в модалке, если мы динамически добавляем/удаляем инпуты.

h2. Изменения
Добавил MutationObserver для отслеживания добавления/удаления элементов внутри компонента обернутого FocusTrap. При изменении содержимого происходит перерасчет focusableNodesRef. Таким образом поддерживается актуальное состояние списка нод.

Вручную потыкал компоненты где используется FocusTrap - все работает
@inomdzhon inomdzhon mentioned this pull request Jun 24, 2024
inomdzhon pushed a commit that referenced this pull request Jun 24, 2024
h2. Описание
Сейчас в FocusTrap если children не меняется, то может и не происходить перерасчёт focusableNodesRef.current.
Например, такое происходит в модалке, если мы динамически добавляем/удаляем инпуты.

h2. Изменения
Добавил MutationObserver для отслеживания добавления/удаления элементов внутри компонента обернутого FocusTrap. При изменении содержимого происходит перерасчет focusableNodesRef. Таким образом поддерживается актуальное состояние списка нод.

Вручную потыкал компоненты где используется FocusTrap - все работает
inomdzhon added a commit that referenced this pull request Jun 25, 2024
h2. Описание
Сейчас в FocusTrap если children не меняется, то может и не происходить перерасчёт focusableNodesRef.current.
Например, такое происходит в модалке, если мы динамически добавляем/удаляем инпуты.

h2. Изменения
Добавил MutationObserver для отслеживания добавления/удаления элементов внутри компонента обернутого FocusTrap. При изменении содержимого происходит перерасчет focusableNodesRef. Таким образом поддерживается актуальное состояние списка нод.

Вручную потыкал компоненты где используется FocusTrap - все работает

---

Т.к. в master тесты писались уже с удалённой строчкой const { keyboardInput } = useContext(AppRootContext); (см. #6955?files=packages/vkui/src/components/FocusTrap/FocusTrap.tsx), а в 6.1-stable она ещё есть, обернул FocusTrap в AppRootContext.Provider.

Co-authored-by: EldarMuhamethanov <61377022+EldarMuhamethanov@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча
Projects
None yet
4 participants