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(PasswordInput): focus input after clicking on the eye icon #2892

Merged
merged 10 commits into from
Jun 24, 2022

Conversation

JackUait
Copy link
Contributor

@JackUait JackUait commented May 18, 2022

fixes IF-472

При повторном клике на иконку глаза в PasswordInput перестал возвращаться фокуc (см. видео)

2022-05-19.03.18.50.mov

Это было вызвано двумя причинами:

  1. После смены иконки не вызывался onClick. Это полечилось добавлением pointer-events: none на обе иконки (на самом деле достаточно добавить эти стили на одну иконку, но для уверенности добавил на обе)

  2. После фикса первого пункта, при потере фокуса инпут стал мгновенно переходить в режим скрытых символов. Из-за чего при ручном переходе из открытых символов в скрытые пользователь попадал в бесконечный цикл (см. видео)

    2022-05-25.13.19.40.mov

    Это удалось полечить проверкой того, что пользователь в данный момент сфокусирован на инпуте перед переходом в режим скрытых символов при блюре (эта логика выполняется в setTimeout так как при клике на иконку с глазом инпут на некоторое время теряет фокус)


Не уверен, насколько легальны обращения к document.activeElement (в том плане не упадут ли они в SSR окружении) без дополнительных проверок, но увидел несколько мест в публичных компонентах где также используется объект document без проверок, так что не стал добавлять и к себе. Но хотелось бы понять благодаря чему такой код не упадёт в SSR


Добавил новый тест, отрефакторил существующие, удалил лишние. Не получилось отрефакторить тест с использованием CapsLock, вероятно, это баг в библиотеке user-event, так как при использовании "специального символа" {capslock} буквы при вводе не заменяются на буквы верхнего регистра, при этом другие специальные символы, например {backspace} отрабатывают как ожидается

@JackUait JackUait changed the title fix(PasswordInput): get focus back after clicking on the eye icon fix(PasswordInput): focus input after clicking on the eye icon May 20, 2022
@zhzz
Copy link
Member

zhzz commented May 24, 2022

Как воспроизвести бесконечный цикл? У меня не получается. Есть какое-то особое условие?

@JackUait
Copy link
Contributor Author

JackUait commented May 25, 2022

Чтобы воспроизвести бесконечный цикл нужно убрать setTimeout со 163-ей строки в PasswordInput:

setTimeout(() => {
  // @ts-expect-error: private property
  if (document.activeElement !== this.input?.input) {
    this.setState({ visible: false });
  }
}, 200);

Обновил описание и видео у второй причины

Copy link
Member

@zhzz zhzz left a comment

Choose a reason for hiding this comment

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

Все проблемы от того, что при нажатии на иконку у инпута тоже происходит blur. А blur в #2766 стал переключать видимость пароля, что и вызывает все перечисленные эффекты.

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

У меня есть пара варинтов:

  1. Детектить только тот blur, который происходит за пределы компонента. Это основная специализация компонента RenderLayer.
  2. Фильтровать все blur на основе e.relatedTarget. Если фокус переходит на элемент, который может иметь фокус, то этот элемент попадает в relatedTarget (иначе там null). Но с этим вариантом проблема в том, что пока иконка не фокусируема, и в relatedTarget она не попадает. Можно было бы завернуть ее в кнопку и улучшить a11y заодно.

Текущее решение не работает, если на иконку кликать медленно, отпуская клик через >200 мс.


Вот здесь статейка по основам поддержки SSR.

@JackUait
Copy link
Contributor Author

JackUait commented Jun 2, 2022

Можно было бы завернуть ее в кнопку и улучшить a11y заодно.

Мне второй вариант нравится больше, но оборачивание иконки в кнопку вызовет ряд других проблем, которые я описал в #2779. Так что пока предлагаю отложить реализацию до "лучших времён" (до того, пока не начнём переосмыслять стилизацию в библиотеке)

Детектить только тот blur, который происходит за пределы компонента. Это основная специализация компонента RenderLayer.

Крутая штука! Не понимал что конкретно она делает до этого момента (звучит как тема для тех. ревью internal компонентов, на тех. ревью также можно затронуть тему публичности некоторых internal компонентов/утилит, а также поговорить об эквивалентах этих компонентов/утилит на хуках, в общем с моей точки зрения эту тему можно растянуть на три тех. ревью)

@JackUait JackUait requested a review from zhzz June 3, 2022 12:24
@JackUait JackUait requested a review from zhzz June 3, 2022 14:30
@zhzz zhzz removed request for lossir and HelenaIsh June 17, 2022 12:00
@zhzz zhzz merged commit 350e0df into master Jun 24, 2022
@zhzz zhzz deleted the fix/password-input-focus branch June 24, 2022 10:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants