Menu Button Popover is rendered on incorrect position
jeanpan opened this issue · 1 comments
🐛 Bug report
Menu Button Popover is rendered on incorrect position because there are different DOMRect returned.
Current Behavior
With React 17, it's not obvious to see the issue, but if we set the breakpoint, we can clearly see the popover position is not correct.
https://user-images.githubusercontent.com/1307026/178013404-e4b5bdb1-772e-4f91-9543-8c2e34de1582.mp4
From the screen recording, you can see the DOMRect top got updated many times.
Here is the codesandbox to reproduce the issue.
However, with React 18, it's obvious to see the issue because of concurrency supported in React 18. I know currently reach-ui doesn't support React 18 yet, but it's not just an issue for React 18 only.
https://user-images.githubusercontent.com/1307026/178014673-7a6f99ba-a229-4c00-9abc-14f7d61588b6.mov
Here is codesandbox reproducible example for React 18.
Expected behavior
The position of popover should be consistent when menu button is clicked.
Reproducible example
Check above Current Behavior section.
Suggested solution(s)
The issue is caused by targetRect doesn't update after page scrolled. The first targetRect is from the component got mounted which is not correct. The observer for targetRef is only called when the popover is shown, but targetRef should be observed to update targetRect even when the popover is hidden.
I am thinking to update const targetRect = useRect(targetRef, { observe: !props.hidden }); to const targetRect = useRect(targetRef, { observe: true });, but not sure if I miss anything to consider (e.g. the case of popover not used for menu button?). Please let me know if it's a valid fix and I'd love to prepare a PR. 😃.
Your environment
| Software | Name(s) | Version |
|---|---|---|
| Reach Package | @reach/menu-button, @reach/popover | 0.17.0 |
| React | 17.0.0 and 18.0.0 | |
| Browser | Chrome | Version 103.0.5060.53 |
| Assistive tech | ||
| Node | ||
| npm/yarn | ||
| Operating System | macOS Monterey | 12.3.1 |