MBS-13516: Upgrade from popperjs to floating-ui #3207
Conversation
mwiencek
force-pushed
the
mbs-13516
branch
3 times, most recently
from
e6d2718
to
1b6080e
Compare
There was a problem hiding this comment.
A quick look through the changes didn't find anything obviously wrong (except ButtonPopver in the second commit's message), but admittedly I also didn't look too deep into how the library works and what all the new things do. That said, I did some manual testing and I see nothing worse than before (and one improvement - the Add new entity dialogs no longer extend the page after the footer with whitespace). Only issue I found while testing was already present in prod (MBS-13517).
Which of our Selenium tests (if any) cover these dialogs and popovers?
|
Fixed some issues here related to setting which element has initial focus in the popover. Using the |
|
Is it possible to add tests for focus management? |
We actually already have tests for initial focus in My previous comment was misleading, since focus was working in this branch before (which is why you didn't notice any issues), but it broke for the ArtistCreditEditor (which is where |
|
In that case I'd suggest putting this on beta already - if anything else is not working as intended I bet our chances of finding it are higher with users checking it :) |
The `tabbable` library seems to be more robust, and making use of it means less code for us to maintain. It's also a dependency of `floating-ui`, the successor to `popperjs`, which means we can avoid duplicating code that achieves the same purpose (if we upgrade to `floating-ui`). The only downside is that `tabbable` is likely "much" slower, since it uses a more reliable method of determining whether an element is visible (causing layout reflow). Its API also returns every tabbable element inside the container as an array. The previous code just tried to find the next tabbable element using the current active element as a cursor: much less work! But an advantage of `tabbable` is that it respects `tabIndex` ordering, unlike the previous code. (This is likely why it needs to build the entire list of elements as an array: in order to sort them by `tabIndex`, which is unrelated to the document order.) And as a final note, any performance impact is unlikely to be noticed by the user.
This can help locating errors more quickly (particularly if the stack trace references line numbers in the compiled build).
Our version of popperjs is obsolete and (I believe) no longer maintained. It was renamed to floating-ui in a major version release a long time ago, and unfortunately came with a host of API changes. And unfortunately for us, it's now written in TypeScript instead of Flow, and does not provide any Flow types. I had to convert a subset of their TS types by hand in flow-typed/npm/@floating-ui/react_v0.26.x.js. One upside is that the new version has a much larger scope and allows us to remove a lot of code; the size of this diff is misleading, since most of the additions are from the ported Flow types rather than actual executable code. Here are a few notable instances where I was able to delete code: * Our `useReturnFocus` hook was no longer needed, as this functionality is now built-in. * Likewise for `useOutsideClickEffect`, although it's still needed by our `Autocomplete2` component. * Much of the code in Dialog.js and focusManagement.js was redundant. I removed the former entirely, as it was no longer necessary to share any code between the `ButtonPopover` and `Modal` components. Relatedly, I removed the `Popover` component because it wasn't used outside of `ButtonPopver`. Things should mostly look and work the same, though there is some visual difference in how our modals are displayed: they fill up the whole screen vertically now (see the "Add a new $entity" dialogs for reference, which is currently the only use of Modals). The popover arrows are also now rendered as SVGs instead of with CSS.
Problem
MBS-13516
Our version of popperjs is obsolete and (I believe) no longer maintained. It was renamed to floating-ui in a major version release a long time ago, and unfortunately came with a host of API changes. And unfortunately for us, it's now written in TypeScript instead of Flow, and does not provide any Flow types.
Solution
I had to convert a subset of their TS types by hand in flow-typed/npm/@floating-ui/react_v0.26.x.js.
One upside is that the new version has a much larger scope and allows us to remove a lot of code; the size of this diff is misleading, since most of the additions are from the ported Flow types rather than actual executable code.
Here are a few notable instances where I was able to delete code:
Our
useReturnFocushook was no longer needed, as this functionality is now built-in.Likewise for
useOutsideClickEffect, although it's still needed by ourAutocomplete2component.Much of the code in Dialog.js and focusManagement.js was redundant. I removed the former entirely, as it was no longer necessary to share any code between the
ButtonPopoverandModalcomponents. Relatedly, I removed thePopovercomponent because it wasn't used outside ofButtonPopver.Things should mostly look and work the same, though there is some visual difference in how our modals are displayed: they fill up the whole screen vertically now (see the "Add a new $entity" dialogs for reference, which is currently the only use of Modals). The popover arrows are also now rendered as SVGs instead of with CSS.
Testing
Only manual testing. I would suggest testing with the "Add relationship" and "Add a new $entity" dialogs in the relationship editor, and making sure key events like tab and escape are handled correctly.