reach / reach-ui

The Accessible Foundation for React Apps and Design Systems
https://reacttraining.com/reach-ui/
MIT License
5.98k stars 560 forks source link

MenuLists in portals cause page to scroll with React 18 #956

Open Emilios1995 opened 2 years ago

Emilios1995 commented 2 years ago

🐛 Bug report

Current Behavior

When rendering menu buttons that open menu lists on slightly long pages, opening a menu after scrolling down a bit often causes the page to scroll farther down. This only happens when rendering the menu lists inside portals, and when using React 18.

This screen recording shows the behavior happening in the reproducible example.

Expected behavior

The menu should open without any change in the page's scroll position.

Reproducible example

Example in Code Sandbox

Your environment

Software Name(s) Version
Reach Package menu-button 0.17.0
React react 18.2.0
Browser Chrome 104.0.5112.79
Browser Firefox 103.0.1
Assistive tech - -
Node - -
npm/yarn - -
Operating System MacOS 12.4
gfohl commented 2 years ago

I have experienced this too

jeanpan commented 2 years ago

I believe I fixed this issue in this PR. You can also check my issue here. Hope we can get the patch released soon.

mena234 commented 2 years ago

I have the same issue.

spirosikmd commented 2 years ago

I have this issue with or without using the portal prop in the MenuList.

One way to fix this issue before the team releases the patch is to use a function to render the Menu's children and only render the MenuList when isExpanded is true.

<Menu>
  {({ isExpanded }) => (
    <>
      <MenuButton>Actions</MenuButton>
      {isExpanded ? (
        <MenuList portal>
          <MenuItem onSelect={() => alert("Download")}>Download</MenuItem>
          <MenuItem onSelect={() => alert("Copy")}>Create a Copy</MenuItem>
          <MenuItem onSelect={() => alert("Delete")}>Delete</MenuItem>
          <MenuLink as="a" href="https://reacttraining.com/workshops/">
            Attend a Workshop
          </MenuLink>
        </MenuList>
      ) : null}
  </>
)}
</Menu>
gfohl commented 2 years ago

@spirosikmd your fix worked, the patch helped, but still caused some scroll issues the first time it was opened. Thank you very much

spirosikmd commented 2 years ago

The only potential issue I see is that in development, I see a warning from the reach-rect package when I open the menu: You need to place the ref. I am unsure whether this causes any issues. The menus behave as expected as far as I can see.

JaceHensley commented 1 year ago

I'm trying the suggestion above in a custom select component that uses Popover and DescendantProvider to wire up downshift. The problem I'm seeing is that until the menu is opened no descendants are registered, so I cannot grab the selected item's value to display. Has anyone worked around an issue like that? Or another workaround to the root issue?