If 1701 masked input fixes #3390
Conversation
*backward compatible
…MaskedInput-fixes
…MaskedInput-fixes
…MaskedInput-fixes
…MaskedInput-fixes
| inputRef.current && | ||
| (inputRef.current.selectAll = () => { | ||
| setTimeout(() => { | ||
| inputRef.current?.setSelectionRange(0, 999); |
There was a problem hiding this comment.
- Мы здесь явно патчим инпут и переопределяем selectAll. Выглядит сомнительно, но на вскидку не могу предложить вариант лучше.
- Магическая константа. Маловероятно, но что если текст в инпуте окажется больше? Может есть смысл использовать Number.MAX_SAFE_INTEGER?
| } | ||
| } | ||
|
|
||
| function handleMouseUp() { |
There was a problem hiding this comment.
Обсуждали голосом handleMouseUp и handleKeyUp. Они нужны потому что в imask есть рассинхрон между изменением выделенного значения и реальным значением в итоге. Текст всегда дописывается в начало.
На мой взгляд лучше оставлять максимально нативное поведение и кастомное поведение этих обработчиков в будущем может выстрелить в каком-то другом месте. Появится баг и его будет очень сложно исправить сохраняя текущее поведение.
И второй момент:
Нужно в конце вызывать onMouseUp, onKeyUp из пропов. Иначе у нас будут в контракте обработчики, которые не работают.
| expect(input).toHaveValue('+7 890 123 45 '); | ||
| }); | ||
|
|
||
| it('prefix on focus', () => { |
There was a problem hiding this comment.
А что не так с этими тестами? Я их добавлял потому что встречал баги с этим поведением.
Я бы наоборот еще добавил тестов, например, на то что при передаче значения не по маске у нас не вызывается onValueChange. Как это работало раньше. Или явно зафиксировать тестом новое поведение, если оно изменилось
| children: React.ReactElement; | ||
| }; | ||
|
|
||
| export type MaskedShadows = [string, string]; |
There was a problem hiding this comment.
MaskedShadows не используется
| spanRef.current.shadowRoot && | ||
| (spanRef.current.shadowRoot.innerHTML = ` | ||
| <style> * { font: ${style.font}; } </style> | ||
| ${val} |
There was a problem hiding this comment.
На сколько безопасно писать в spanRef.innerHtml?
Это потенциальная уязвимость. В маске может оказаться html со скриптом.
Может лучше писать val через .textContent?
| } | ||
| const { selectionStart, selectionEnd } = imaskRef.current.element; | ||
|
|
||
| const nearest = imaskRef.current.maskRef.masked.nearestInputPos(999, 'LEFT'); |
There was a problem hiding this comment.
Может быть тут тоже лучше использовать Number.MAX_SAFE_INTEGER
Баги от лица пользователя описаны в задаче
IF-17011. Всего их 6.Здесь описаны технические проблемы, которые вызывали эти баги.
Проблема 1
Старый код, отвечающий за подсветку маски.
Плюс его в поддержке IE11 и старых версий других браузеров.
Минусы - сложность; влияние на вёрстку; специфичное тестирование.
Проблема 2
Использование
setTimeoutдля обработки событияonUnexpectedInput.Решение 1
После отказа от поддержки IE11, эту логику получилось сделать "проще", хоть и многословнее.
Новый код минимально влияет на вёрстку, благодаря чему
MaskedInputтестируется как обычныйInput.Всё работает с помощью градиента и стилей
background-clip2 и-webkit-text-fill-color3.Также символ подчеркивания снова реализовали через специальный шрифт, как это было в старых версиях пакета.
Но теперь шрифт лежит на сервере статики, что позволило избежать сложностей, которые он создавал раньше4.
Решение 2
setTimeoutудалось заменить на синхронный код.Ссылки
Песочница с новыми стялими
https://codepen.io/lossir/pen/ExrewOx
Чек-лист перед запросом ревью
Добавлены тесты на все изменения
⬜ unit-тесты для логики
⬜ скриншоты для верстки и кросс-браузерности
⬜ нерелевантно
Добавлена (обновлена) документация
⬜ styleguidist для пропов и примеров использования компонентов
⬜ jsdoc для утилит и хелперов
⬜ комментарии для неочевидных мест в коде
⬜ прочие инструкции (
README.md,contributing.mdи др.)⬜ нерелевантно
Изменения корректно типизированы
⬜ без использования
any(см. PR2856)⬜ нерелевантно
Прочее
⬜ все тесты и линтеры на CI проходят
⬜ в коде нет лишних изменений
⬜ заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)
Footnotes
https://yt.skbkontur.ru/issue/IF-1701 ↩
https://caniuse.com/background-clip-text ↩
https://caniuse.com/mdn-css_properties_-webkit-text-fill-color ↩
https://github.com/skbkontur/retail-ui/issues/2181 ↩