Reach-ui: [MenuButton] Long menu causes window to scroll and click on item

[raunofreiberg]

🐛 Bug report

Current Behavior

If the items of MenuButton exceed the window height, the window becomes scrollable. Then, opening the menu would scroll the window, and sometimes click on the option that falls under the cursor

Expected behavior

MenuButton should not click on the option? Idk. Is scrolling expected?

Reproducible example

https://codesandbox.io/s/long-thunder-ilwv4?file=/src/App.js

[raunofreiberg]

Here's a quick gif of what happens when I tried to use MenuButton with a long list of options. The Menu closes instantly after being opened:

---

I threw a max-height with an overflow-y: auto on there, because I don't see a use case for such a massive menu, and this fixed it. But I think this is a bug nonetheless.

[chaance]

This is a known issue, but I'm not sure if we should deal with it directly. I think you would definitely want to use an overflow, but you might also want to implement a custom scroll button with the position prop in MenuPopover. My opinion is that this is best left for individuals to implement.

If you were to run into any issues or barriers in customization, then we'd need to examine the API a bit to see how we could work around that.

[raunofreiberg]

I don't think we should deal with the scrolling issue. That I agree, should be up to the developer. Although, I find it a bit odd that the "open menu" click would then also select an item in the menu, and close it as a result. Could we do something about that?

[chaance]

I'll take a look. It's probably similar to the issue I recently patched in Listbox, so maybe a good abstraction would be helpful here.

[smeijer]

Can this be reopened? It's still an issue in the latest version.

[smeijer]

@chaance, I suggest adding Status: Consideration to the exemptLabels :sweat_smile:

https://github.com/reach/reach-ui/blob/51e163dcfc2b3654e5344b719398daa3775cab63/.github/stale.yml#L6-L12

[thiniulian]

Hello!
I also have this issue, exactly as described by the @raunofreiberg.
The scrolling is annoying but we can live with it (some pointers on how to disable/go around it would be much appreciated).
The auto click in turn is truly unwanted, and it's not necessarily happening on long menus, it's enough if you have a resized window and the height of the dropdown is larger than the window height.

[indiesquidge]

@thiniulian the auto click is still an issue you're saying? I'm not able to reproduce it. (I used this sandbox from #692 with the default <MenuList> since @raunofreiberg's reproducible example breaks on a TypeError for me.)

Related conversation around the auto-scroll behavior: https://github.com/reach/reach-ui/pull/692

[DispatchCommit]

@indiesquidge I can reproduce the bug with your code sandbox link.

I came up with a workaround by adding a timeout threshold requirement to disable event propagation within a short period, but because of how reach-ui handles it's event flow, it's possible to trigger the onSelect function on just mouseUp when your mouseDown starts on a menu-button

This causes issues when animating a menulist and it initially appears under the cursor as 1 click can both open the popover menu and trigger an action in that menu. In our case, it caused erroneous deletions.

example:

I suppose the solution to this is to just animate differently, which can be done, but this still seems like broken behavior when the first example for animations is a slide down animation.

but here's an example of how you would have to animate it to prevent this weird mouse event issue that makes 1 click seem like 2 clicks:

there's more information available in the issue linked above: lbryio/lbry-desktop#5416

edit: code workaround solution

        const [mouseClickCompleted, setMouseClickCompleted] = useState(false);

        // . . .

            <div className="comment__menu">
              <Menu>
                <MenuButton onMouseDown={() => {
                  setMouseClickCompleted(false);
                  setTimeout(() => setMouseClickCompleted(true), 200)
                }}>
                  <Icon
                    size={18}
                    className={mouseIsHovering ? 'comment__menu-icon--hovering' : 'comment__menu-icon'}
                    icon={ICONS.MORE_VERTICAL}
                  />
                </MenuButton>
                <MenuList className="menu__list--comments">
                  {commentIsMine ? (
                    <>
                      <MenuItem
                        className="comment__menu-option menu__link"
                        onMouseUp={e => !mouseClickCompleted && e.preventDefault()}
                        onSelect={handleEditComment}
                      >
                        <Icon aria-hidden icon={ICONS.EDIT} />
                        {__('Edit')}
                      </MenuItem>
                      <MenuItem
                        className="comment__menu-option menu__link"
                        onMouseUp={e => !mouseClickCompleted && e.preventDefault()}
                        onSelect={handleDeleteComment}
                      >
                        <Icon aria-hidden icon={ICONS.DELETE} />
                        {__('Delete')}
                      </MenuItem>
                    </>

[DispatchCommit]

Compare the behavior seen and demonstrated above to a UI framework like vuetify: https://vuetifyjs.com/en/components/menus/#use-in-components

Notice how vuetify's menus can render under the cursor and don't incorrectly trigger a function in the popup menu? That's how I would expect the behavior to be.

It does not seem correct to me that a mouse down event can trigger an action, and then moments later, the mouse up event can also trigger an action. 2 clicks for 1 mouse click is a poor user experience.