fix(Popup): displaying SVG icons

[JackUait]

Проблема

Если передать SVG иконку напрямую в Hint - подсказка на иконке отображаться не будет

Код для воспроизведения:

<Hint text="Редактирование">
  <svg width="16" height="16" viewBox="0 0 16 16">
    <path
      fillRule="evenodd"
      d="M13 12V12.998H8V12H13ZM3 13V11L9 4.99999L11 6.99999L5 13H3ZM13 5L11.5 6.5L9.5 4.5L11 3L13 5Z"
      clipRule="evenodd"
    />
  </svg>
</Hint>

При этом если обернуть тег <svg /> в любой другой тег (кроме <svg />) - проблема уйдёт

Решение

Решение оказалось достаточно тривиальным (оно занимает всего одну строчку), а вот его поиск был не так тривиален

Окончательное решение:

Проблема была в том, что утилита isHTMLElement, которая у нас используется в Popup и getRootNode (на парковке она не используется) проверяла только инстансы от HTMLElement. Но SVG иконки живут в другой спецификации, и имеют собственный инстанс - SVGElement, в связи с чем, вся логика, основывающаяся на проверке isHTMLElement - проходила мимо SVG, либо неправильно отрабатывала:

1. В getRootNode на 58-ой строчке есть проверка:

   return isHTMLElement(rootNode) ? rootNode : null;

   которая для SVG возвращала null, хотя должна - rootNode
2. В Popup на 338-ой и 348-ой строчках задаются и очищаются слушатели событий, которые завязаны на проверке isHTMLElement
3. Всё ещё в Popup на 573-ей строчке, возвращается расположение попапа, которое также завязано на проверке isHTMLElement

Есть и другие кейсы, в которых старое определение HTML-элемента ломает приложение (вплоть до его краша), но они не относятся к этой задаче

Из-за того, что различные инстансы для HTMLElement и SVGElement обусловлены различными спецификациями, а также из-за того что HTML-элементы и SVG-иконки неразрывно связаны - было решено расширить проверку в изначальной функции isHTMLElement до:

return el instanceof HTMLElement || el instanceof SVGElement

Поиски решения:

Как было сказано выше - если обернуть тег <svg /> в любой другой тег, то проблема уйдёт, но на месте старой проблемы появится новая, в виде мёртвой зоны, в которой не будет отрабатывать хинт, либо в виде увеличившейся зоны хинта (вероятно зависит от иконки, я сильно не погружался, так как расцениваю этот вариант как хак)

На скриншоте: тег <svg /> был обёрнут в <span />, из-за чего его нижняя граница увеличилась на 0.5 пикселей, возможен также случай, когда нижняя граница увеличивается и съезжает вниз, образуя мёртвую зону

Также были и другие варианты, но для истории, пожалуй, важен только этот

P.S. Изначально для задачи были написаны интеграционные тесты (и отрефакторены старые), но по итогу оказалось, что RTL при клике с фиксом и без - рендерит текст хинта, из-за чего переключился на скриншотные тесты

Ссылки

IF-608

Чек-лист перед запросом ревью

• Добавлены тесты на все изменения
  • unit-тесты для логики (гайд)
  • скриншоты для верстки и кросс-браузерности (гайд)
  • нерелевантно
• Добавлена (обновлена) документация
  • пропы, публичные методы и примеры использования в styleguidist (гайд)
  • jsdoc для утилит и хелперов (гайд)
  • комментарии для неочевидных мест в коде (гайд)
  • прочие инструкции (README.md), (contributing.md)
  • нерелевантно
• Изменения корректно типизированы
  • без использования any (#2856)
  • нерелевантно
• Все тесты и линтеры на CI проходят
• В коде нет лишних изменений
• Заголовок PR кратко и доступно отражает суть изменений (он попадет в changelog)

[lossir]

Так делать не стоит. Название функции и возвращаемый тип не отражает действительность. Это тоже нам может аукнуться.

Для этих узлов есть общая спецификация - Element (HTMLElement, SVGElement)

Надо добавить новую функцию isElement() и явно использовать вместо isHTMLElement, где это уместно.

---

Пока писал тесты для getRootNode тоже наткнулся на проблему с svg. Я тоже добавлю isElement() =)

[JackUait]

Так делать не стоит. Название функции и возвращаемый тип не отражает действительность. Это тоже нам может аукнуться.

Почему название оставил прежним, я описал в секции "решение", но думаю, чтобы полностью исключить путаницу - можно назвать функцию isHTMLOrSVGElement

А вот про возвращаемый тип я как-то я упустил объяснение.. В общем, суть такая, что сейчас мы не можем заменить возвращаемый тип, так как это уже "аукнулось" нам. Сейчас для изменения возвращаемого типа нужно вносить в код ломающие изменения. Плюс, с типом Element это будет сделать гораздо сложнее, чем с типом HTMLElement | SVGElement, так как Element это общий тип и в нём отсутствуют многие специфичные для HTMLElement и SVGElement сущности

Для этих узлов есть общая спецификация - Element (HTMLElement, SVGElement)

И так как в идеальном мире возвращаемый тип у нас HTMLElement | SVGElement, то и проверка на instanceof Element нам не совсем подходит из-за того, что мы вроде как проверяем является ли переданный элемент SVG или HTML элементом, а по факту смотрим на того является ли элемент инстансом Element. То есть с точки зрения работы обе версии выдадут одинаковый результат, но версия с проверкой на HTMLElement + SVGElement логичнее выглядит и проще для понимания

---

Изменил название функции и добавил пояснение про возвращаемый тип

[lossir]

Если в возвращаемый тип не добавить SVGElement, то это потенциально ломающая штука.
Например, в компоненте Menu есть такой код:

retail-ui/packages/react-ui/internal/Menu/Menu.tsx

Line 196 in 772e647

this.scrollContainer.scrollTo(getRootNode(this.highlighted));

Который дёргает свойство offsetLeft, и ниже ещё и offsetTop

retail-ui/packages/react-ui/components/ScrollContainer/ScrollContainer.tsx

Lines 142 to 149 in 772e647

	public scrollTo(element: Nullable<HTMLElement>) {
	if (!element || !this.inner) {
	return;
	}
	this.inner.scrollLeft = element.offsetLeft;
	this.inner.scrollTop = getScrollYOffset(element, this.inner);
	}

Но для SVGElement эти свойства задикрикечены: offsetLeft, offsetTop

---

Интерфейс Element наследуют только HTMLElement, SVGElement и MathMLElement (по крайней мере других я не нашёл).

Все 3 типа элементов можно получить через законные рефки Реакта. Хоть у MathML сейчас ограниченная поддержка, но она только увеличивается.

Поддержка MathML

https://caniuse.com/?search=MathML

---

Поэтому, добавить отдельную функцию isElement мне кажется уместным решением. В описании можно указать про HTMLElement, SVGElement и MathMLElement. Надо будет везде явно указать типы и разрулить проблемы.

Я бы занялся этим в своем ПРе #2939, т.к. уже напоролся на все эти проблемы с типами.

Можем созвониться и обсудить.

[JackUait]

Что по итогу было сделано:

1. Заменил функцию isHTMLOrSVGElement на isElement

2. Везде, где надо было, заменил тип HTMLElement на Element

3. Чтобы задача не разрасталась и не задерживалась, пофиксил тип только в тех местах, которые мешали работе функции isElement. Для исправления остальных случаев завёл задачу IF-643

4. Привёл тип к HTMLElement || SVGElement там, где нужен был вызов нативных методов (таких как focus) из getRootNode

5. Поправил тип в FocusEventType, поправил и расширил тип в MouseEventType

6. Заменил обращение к offsetLeft, offsetTop, offsetWidth и остальных offset* на получение значения из getBoundingClientRect. Эти сущности в большинстве случаев возвращают одинаковые значения, за исключением случая, когда к элементу применяется стиль transform. Например, если к элементу с width: 100px будет применён стиль transform: scale(0.5);, то offsetWidth вернёт 100, а getBoundingClientRect вернёт 50. Это может поломать интерфейсы, которые уже завязались на этом баге, но кажется, что это крайне маловероятно + лучшая альтернатива getBoudingClientRect которую я нашёл это вот этот сниппет, но при его тестировании оказалось, что работает он крайне так себе и часто возвращает неправильные значения

   Также, getBoundingClientRect возвращает дробное значение, а offset* - целое, поэтому, чтобы избежать неожиданных багов, округлил к нижней границе значения получаемые из getBoundingClientRect

[JackUait]

Дополнение к пункту 6:
После написания прошлого комментария обнаружилась ещё одна проблема с getBoundingClientRect vs offset*: если использовать getBoundingClientRect в контейнерах со скроллом, то чтобы получить значение равное offsetLeft нужно произвести вычисление getDOMRect(element).right - getDOMRect(element).left

Ещё не углублялся в причины этого поведения, но думаю, что это лучше решать в рамках отдельной задачи, так как проблема может оказаться гораздо глубже и замедлить выкатку этого пулл-реквеста. Пока оставил в местах, где обнаружился баг - offset*, и завёл задачу IF-647, в рамках которой буду разбираться с этой проблемой

[lossir]

Ручное управление уже тестируется в юнит тесте. Ещё есть такая стори SetManualAndOpenedPropOnClick со скриншотами.

Думаю, здесь достаточно сразу рендерить открытый Хинт, для проверки работы с свг-шкой.

[JackUait]

Тогда мы не будем проверять пользовательский сценарий, а также не покроем все кейсы, из-за которых хинт может не отрендериться на свг-шке. В задаче хинт рендерился не только из-за того, что мы не могли найти его ноду, а также из-за того, что на него не передавались слушатели событий

Хотя, исходя из этой же логики текущая реализация не является корректной. Следовательно, единственный правильный вариант реализации этого теста - проверять отображается ли хинт при ховере

---

Обновил тест, а также заскипал скриншоты везде кроме хрома, чтобы сократить нагрузку. Думаю, что одного хрома хватит для тестирования логики