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

useFixOverscrollBehavior not working for Loadout Popup #9886

Closed
robojumper opened this issue Sep 19, 2023 · 0 comments · Fixed by #9924
Closed

useFixOverscrollBehavior not working for Loadout Popup #9886

robojumper opened this issue Sep 19, 2023 · 0 comments · Fixed by #9924
Assignees
Labels

Comments

@robojumper
Copy link
Member

#9852 made the Loadout Popup overflow: hidden with a call to useFixOverscrollBehavior (that internally uses useResizeObserver) to restore overflow: auto in case the popup actually does overflow. In practice, the Loadout Popup would remain unscrollable and nev changed it to overflow: auto in 4aa708f. According to comments in #9852 this would regress overscroll behavior in Safari however:

Safari seems very sensitive about what overflow values an element has to start with - starting with auto and setting to hidden doesn't work, but starting from hidden and setting to auto does.

So we should fix whatever makes useFixOverscrollBehavior not work correctly here:

useFixOverscrollBehavior(menuRef);

and then revert 4aa708f.

My hunch is that useResizeObserver can't deal with the ref target changing in subsequent renders:

https://github.com/jaredLunde/react-hook/blob/3c813dab5b21e26f2c85e733314ca5c063c6bfce/packages/resize-observer/src/index.tsx#L24-L50

Since StoreHeading first renders (and calls the hook) with a closed popup, the hook doesn't have a target element to subscribe, and when it does finally open and StoreHeading re-renders, none of the dependencies of useResizeObserver's layoutEffect change (since they're only ref objects or globals), so the subscription of the ResizeObserver never happens with the correct element.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants