refactor: use css `outline` & new `useFocusVisibleClassName()` hook to draw visible focus #5876

eugpoloz · 2023-09-27T02:43:24Z

Чеклист

e2e-тесты
unit-тесты — для нового хука useFocusVisibleClassName()
~~дизайн-ревью~~ — не нужно, поменялся только внутряк

Описание

closes [Bug]: FocusVisible иногда является причиной появления скролла в родительском элементе #5871

В рефакторинге FocusVisible мы стали рисовать границу через border, что и приводило к "раздуванию" элемента и появлению скроллов там, где их было быть не должно. В старом подходе с box-shadow такого не было, т.к. box-shadow никак не изменяет границы элемента.

А границу стали рисовать из-за того, что добавили FocusVisible в FormField.

Однако! В процессе я внезапно обнаружила, что поддержку border-radius для outline починили! См.

Поэтому — идем путем graceful degradation. В браузерах, где баг не пофикшен, аутлайн будет без скруглений и не такой красивый, но, кажется, этим стоит пожертвовать.

Изменения

переделала обводку с границы на outline,
и полностью изменила логику FocusVisible — это теперь компонент-обертка, который передает своему ближайшему дитю на корень нужные классы,
с подачи Инома избавилась от компонента FocusVisible в пользу нового хука useFocusVisibleClassName(). Засунуть в useFocusVisible() логику не получилось, т.к. есть места, где для определения фокусного состояния мы используем хук useFocusWithin(),
подправила типы и комментарии к коду,
подправила ресет аутлайна, чтобы он работал как нужно в Windows High Contrast Mode (см. статью Maintaining Focus Outlines for Windows High Contrast Mode Бена Майерса)
добавила новые e2e-тесты на State: Active для:
- Link
- Image
- Switch
- Tappable
- Textarea
обновила скриншоты:
- Slider
- CustomSelect
- Textarea

github-actions · 2023-09-27T02:46:37Z

size-limit report 📦

Path	Size
JS	342.43 KB (-0.07% 🔽)
JS (gzip)	104.61 KB (-0.04% 🔽)
JS (brotli)	86.45 KB (+0.07% 🔺)
JS import Div (tree shaking)	2.71 KB (0%)
CSS	256.47 KB (-0.08% 🔽)
CSS (gzip)	33.63 KB (-0.29% 🔽)
CSS (brotli)	27.3 KB (-0.32% 🔽)

codesandbox-ci · 2023-09-27T02:47:28Z

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.

Latest deployment of this branch, based on commit b69bef6:

Sandbox	Source
VKUI TypeScript	Configuration
pensive-ace-n7jwvt	Issue #5871

github-actions · 2023-09-27T02:49:17Z

e2e tests

Playwright Report

github-actions · 2023-09-27T02:49:36Z

👀 Docs deployed

✅ Styleguide
✅ Storybook

Commit b69bef6

codecov · 2023-09-27T02:49:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (8d0e56b) 79.10% compared to head (b69bef6) 79.13%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5876      +/-   ##
==========================================
+ Coverage   79.10%   79.13%   +0.02%     
==========================================
  Files         312      312              
  Lines        9947     9957      +10     
  Branches     3333     3332       -1     
==========================================
+ Hits         7869     7879      +10     
  Misses       2078     2078

Flag	Coverage Δ
unittests	`79.13% <100.00%> (+0.02%)`	⬆️

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.

packages/vkui/src/components/WriteBar/WriteBar.module.css

packages/vkui/src/components/Tappable/Tappable.module.css

packages/vkui/src/components/ImageBase/ImageBaseOverlay/ImageBaseOverlay.tsx

packages/vkui/src/components/Clickable/Clickable.tsx

mendrew

Отличная работа! 🔥 💯 💠

Особенно мне нравится FocusVisible как обертка, намного лучше видно, что используется в коде. Отличная идея с передачей класса children!

packages/vkui/src/styles/constants.css

packages/vkui/src/components/TabbarItem/TabbarItem.module.css

packages/vkui/src/components/FocusVisible/FocusVisible.tsx

inomdzhon

Получилось вообще шикарно! ✨💖✨ Ещё и обводка ссылки починилась 🙂 Из-за того, что переделывали на отдельный элемент, там бага была.

Из улучшений, т.к. у нас есть хук useFocusVisible(), то я бы его и применил для определения className, который задаёт фокус. И тем самым избежал бы React.cloneElement. По возможности стоит избегать этого метода в компонентах, которые часто встречаются в интерфейсе. Его полезно использовать, например, для тултипов, т.к. реже приходится использовать (хотя даже для тултипов предпочёл бы хук с пробрской рефа).

Добавил коммент на примере компонента Clickable.

packages/vkui/src/components/Clickable/Clickable.tsx

inomdzhon

Получилось вообще шикарно! ✨💖✨ Ещё и обводка ссылки починилась 🙂 Из-за того, что переделывали на отдельный элемент, там бага была.

Из улучшений, т.к. у нас есть хук useFocusVisible(), то я бы его и применил для определения className, который задаёт фокус. И тем самым избежал бы React.cloneElement. По возможности стоит избегать этого метода в компонентах, которые часто встречаются в интерфейсе. Его полезно использовать, например, для тултипов, т.к. зачастую реже применяется в интерфейсе (хотя даже для тултипов предпочёл бы хук с пробрской рефа).

Добавил коммент на примере компонента Clickable как это может выглядеть если чисто на хуках делать.

github-actions · 2023-11-03T10:04:08Z

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

- update screenshots

- ImageOverlay: adjust overlay visibility on focus - ImageBase: add negative z-index to <img/> - Avatar: move initials to get layers under ImageBase isolation

- update screenshots

windows high contrast mode compatible

packages/vkui/src/hooks/useFocusVisibleClassName.test.ts

BlackySoul · 2023-11-22T11:20:26Z

А почему решили поменять анимации out на in? Кажется, что из-за этого сейчас outline обрезается

Было:

2023-11-22.18.09.22.mov

Стало:

2023-11-22.18.08.31.mov

for proper inside mode animation

eugpoloz · 2023-11-23T03:27:17Z

@BlackySoul

А почему решили поменять анимации out на in? Кажется, что из-за этого сейчас outline обрезается

Спасибо за бдительность! Это я класс в анимашке потеряла, как оказывается, теперь пофиксила. 😉

inomdzhon

🔥 ❤️‍🔥

vkcom-publisher · 2023-11-23T08:45:00Z

❌ Patch

Не удалось автоматически применить исправление на ветке v5.

Дальнейшие действия выполняют контрибьютеры из группы @VKCOM/vkui-core

Чтобы изменение попало в ветку v5, выполните следующие действия:

Создайте новую ветку от v5 и примените изменения используя cherry-pick

git stash # опционально
git fetch origin v5
git checkout -b patch/pr5876 origin/v5

git cherry-pick --no-commit 9d47c34ac3e47980e291f7077f5aa46ac9316b3e
git checkout HEAD **/__image_snapshots__/*.png
git diff --quiet HEAD || git commit --no-verify --no-edit

Исправьте конфликты, следуя инструкциям из терминала
Отправьте ветку на GitHub и создайте новый PR с веткой v5 (установка лейбла не требуется!)

git push --set-upstream origin patch/pr5876
gh pr create --base v5 --title "patch: pr5876" --body "- patch #5876"

…o draw visible focus (#5876)

…o draw visible focus (#5876) (#6131)

eugpoloz added the v5 Автоматизация: PR продублируется в ветку v5 label Sep 27, 2023

eugpoloz changed the title ~~fix(FocusVisible):~~ fix(FocusVisible): turn back to box-shadow for drawing visible focus Sep 27, 2023

eugpoloz changed the title ~~fix(FocusVisible): turn back to box-shadow for drawing visible focus~~ fix(FocusVisible): use box-shadow to draw visible focus Sep 27, 2023

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch 3 times, most recently from 095a0f4 to a0ca4b5 Compare September 30, 2023 03:36

eugpoloz changed the title ~~fix(FocusVisible): use box-shadow to draw visible focus~~ fix(FocusVisible): use outline to draw visible focus Oct 4, 2023

eugpoloz mentioned this pull request Oct 4, 2023

fix(TabbarItem): use single Tappable #5914

Closed

5 tasks

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch 3 times, most recently from 77bae4a to 3bc2ea7 Compare October 12, 2023 00:26

github-actions bot added the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Oct 12, 2023

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch from 3bc2ea7 to e4db235 Compare October 12, 2023 00:33

eugpoloz marked this pull request as ready for review October 12, 2023 00:34

eugpoloz requested a review from a team as a code owner October 12, 2023 00:34

SevereCloud reviewed Oct 12, 2023

View reviewed changes

eugpoloz marked this pull request as draft October 17, 2023 17:42

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch from e4db235 to 0158f3e Compare October 17, 2023 18:12

eugpoloz marked this pull request as ready for review October 19, 2023 04:19

mendrew reviewed Oct 19, 2023

View reviewed changes

packages/vkui/src/styles/constants.css Outdated Show resolved Hide resolved

packages/vkui/src/components/TabbarItem/TabbarItem.module.css Show resolved Hide resolved

SevereCloud reviewed Oct 19, 2023

View reviewed changes

packages/vkui/src/components/FocusVisible/FocusVisible.tsx Outdated Show resolved Hide resolved

inomdzhon reviewed Oct 19, 2023

View reviewed changes

packages/vkui/src/components/Clickable/Clickable.tsx Outdated Show resolved Hide resolved

inomdzhon reviewed Oct 19, 2023

View reviewed changes

github-actions bot added the pr-needs-work Автоматизация: PR автоматически закроется через 14 дней при отсутствии активности label Oct 27, 2023

eugpoloz added 4 commits November 21, 2023 18:08

refactor(Slider): useFocusVisibleClassName()

fed15dd

e2e(Slider): adjust visual test for reduced motion

9897746

- update screenshots

refactor(Switch): useFocusVisibleClassName()

04e41b2

e2e(Switch): add focus visible state visual test

258824c

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch from 07693e1 to 4c28615 Compare November 21, 2023 15:08

eugpoloz added 6 commits November 21, 2023 18:48

e2e(Textarea): update test & add focus state

a9b4151

- update screenshots

e2e(CustomSelect): update screenshots

5065bea

refactor(Image): useFocusVisibleClassName()

99dd9b7

- ImageOverlay: adjust overlay visibility on focus - ImageBase: add negative z-index to <img/> - Avatar: move initials to get layers under ImageBase isolation

e2e(Image): add focus visible state visual tests

a4c04c6

- update screenshots

fix(css): reset outline the smart way

a16a1a5

windows high contrast mode compatible

refactor: rm FocusVisible component

d1690ec

eugpoloz force-pushed the eugpoloz/5981-FocusVisible-bug branch from 4c28615 to d1690ec Compare November 21, 2023 15:52

eugpoloz requested review from inomdzhon and SevereCloud November 21, 2023 16:17

inomdzhon reviewed Nov 21, 2023

View reviewed changes

packages/vkui/src/hooks/useFocusVisibleClassName.test.ts Outdated Show resolved Hide resolved

test(hooks): useFocusVisibleClassName()

7604fca

eugpoloz removed the patch Автоматизация: PR продублируется в ветку последнего минорного релиза для выпуска патча label Nov 22, 2023

eugpoloz requested a review from inomdzhon November 22, 2023 10:56

fix(focusVisible): adjust css classes

b69bef6

for proper inside mode animation

SevereCloud approved these changes Nov 23, 2023

View reviewed changes

inomdzhon approved these changes Nov 23, 2023

View reviewed changes

eugpoloz merged commit 9d47c34 into master Nov 23, 2023
24 checks passed

eugpoloz deleted the eugpoloz/5981-FocusVisible-bug branch November 23, 2023 08:44

eugpoloz added a commit that referenced this pull request Nov 23, 2023

refactor: use css outline & new useFocusVisibleClassName() hook t…

01af924

…o draw visible focus (#5876)

eugpoloz mentioned this pull request Nov 23, 2023

patch: pr5876 #6131

Merged

eugpoloz added a commit that referenced this pull request Nov 23, 2023

refactor: use css outline & new useFocusVisibleClassName() hook t…

e0e90a6

…o draw visible focus (#5876) (#6131)

BlackySoul mentioned this pull request Feb 12, 2024

fix(FormField): rm outline #6557

Merged

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use css `outline` & new `useFocusVisibleClassName()` hook to draw visible focus #5876

refactor: use css `outline` & new `useFocusVisibleClassName()` hook to draw visible focus #5876

eugpoloz commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

codesandbox-ci bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading

mendrew left a comment

inomdzhon left a comment

inomdzhon left a comment •

edited

Loading

github-actions bot commented Nov 3, 2023

BlackySoul commented Nov 22, 2023

eugpoloz commented Nov 23, 2023

inomdzhon left a comment

vkcom-publisher commented Nov 23, 2023

refactor: use css outline & new useFocusVisibleClassName() hook to draw visible focus #5876

refactor: use css outline & new useFocusVisibleClassName() hook to draw visible focus #5876

Conversation

eugpoloz commented Sep 27, 2023 • edited Loading

Чеклист

Описание

Изменения

github-actions bot commented Sep 27, 2023 • edited Loading

size-limit report 📦

codesandbox-ci bot commented Sep 27, 2023 • edited Loading

github-actions bot commented Sep 27, 2023 • edited Loading

e2e tests

github-actions bot commented Sep 27, 2023 • edited Loading

👀 Docs deployed

codecov bot commented Sep 27, 2023 • edited Loading

Codecov Report

mendrew left a comment

Choose a reason for hiding this comment

inomdzhon left a comment

Choose a reason for hiding this comment

inomdzhon left a comment • edited Loading

Choose a reason for hiding this comment

github-actions bot commented Nov 3, 2023

BlackySoul commented Nov 22, 2023

eugpoloz commented Nov 23, 2023

inomdzhon left a comment

Choose a reason for hiding this comment

vkcom-publisher commented Nov 23, 2023

❌ Patch

refactor: use css `outline` & new `useFocusVisibleClassName()` hook to draw visible focus #5876

refactor: use css `outline` & new `useFocusVisibleClassName()` hook to draw visible focus #5876

eugpoloz commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

codesandbox-ci bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

github-actions bot commented Sep 27, 2023 •

edited

Loading

codecov bot commented Sep 27, 2023 •

edited

Loading

inomdzhon left a comment •

edited

Loading