fix(popover): dropdown does not flicker on firefox

[liamdebeasi]

resolves #9925

Background

The usePopper hook uses useLayoutEffect. Whenever useLayoutEffect runs, the popover is either created or destroyed and then re-created. This hook has no dependencies, so it runs any time its parent component re-renders.

This triggers a quirk in Firefox on hover since the popover is destroyed and re-created on each hover (since hovering causes a state change via Downshift). More info about the issue can be found here.

---

The Fix

To fix this, I updated usePopper to accept a list of dependencies that are then passed to useLayoutEffect. This ensures that useLayoutEffect only runs when the deps change. This is important for popovers that are added to the DOM on load but do not have any content (and thus have a dimension of 0x0). Since the content has a dimension of 0x0, Popper cannot position the popover correctly. Once the popover is opened, the content is added, and we need to re-run the hook so Popper can reposition the popover correctly.

---

Testing

The following components should be tested since they use usePopper:

• SymbolsPicker
• ItemPopup
• StoreHeading
• Select
• PressTip
• Dropdown

---

Other Info

This fix should also provide a slight performance boost since DIM no longer needs to destroy and re-create the popover as often as it used to.

[bhollis]

Awesome, glad to see a fix for this.

Longer term I'd love to see us do #8374 (Switch to Floating UI) or even swap to a CSS anchor positioning polyfill (though the one polyfill I've seen so far is not great).

[bhollis]

My original idea behind having the layout effect fire on every update was that I don't know what (if anything) has changed about the contents, so I don't know when exactly to re-position. This is made especially complicated by the fact that popper seems to have some bugs around its lifecycle (which afaik are fixed in floating-ui).

Testing around a bit, I can see that uses of Dropdown appear broken with this change - for example, the menu you can access from the three dots in the search bar.

[liamdebeasi]

Thanks for testing! I'll go back and take another look. It's likely that I missed a case I was not considering before.

[liamdebeasi]

Ok I made some adjustments. The big change here is usePopper now accepts a dependency list. For the particular broken popovers you noted, both the isOpen state and the items list are listed as dependencies in addition to all the other deps in usePopper. This should resolve the problem you noted.

I opted to not touch the other usePopper implementations because they currently do not exhibit the reported bug.

[bhollis]

Also if you're on the Discord, ping me and I'll add the contributor role for you.

[liamdebeasi]

React expects that all reactive values be listed as dependencies: https://react.dev/reference/react/useEffect#specifying-reactive-dependencies

[bhollis]

Fair, though in practice many of these are immutable. Can't hurt, though.

What doesn't work is including deps directly like this - hooks' dependencies list won't recursively evaluate equality. So you probably want:

Suggested change

	contents,
	reference,
	arrowClassName,
	menuClassName,
	boundarySelector,
	placement,
	offset,
	fixed,
	padding,
	deps,
	contents,
	reference,
	arrowClassName,
	menuClassName,
	boundarySelector,
	placement,
	offset,
	fixed,
	padding,
	...deps,

[liamdebeasi]

Ah yeah, so that causes a React warning since React can't statically analyze the dependencies that way. We can add a comment to ignore the rule, but wasn't sure if you'd be comfortable with that approach.

---

I can also remove the other refs if they are immutable, though that will cause React to log another warning since it expects them to be included as dependencies. Similarly, we can add a comment to ignore the rule if you'd like.

[bhollis]

Probably the best would be to silence the warning about having a dynamic dependency list, but still include all the other dependencies.

[liamdebeasi]

All set!