[MenuButton] Long menu causes window to scroll and click on item

raunofreiberg commented 4 years ago

🐛 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 commented 4 years ago

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:

gif

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

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

smeijer commented 4 years ago

@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 commented 3 years ago

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 commented 3 years ago

@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 commented 3 years ago

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

reach-ui-bug

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:

reach-ui-bug-lbry

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:

better-dropdown

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 commented 3 years ago

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.