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

[Menu] Clicking outside a menu blocks click from bubbling up #11243

Open
simoami opened this issue May 4, 2018 · 15 comments
Open

[Menu] Clicking outside a menu blocks click from bubbling up #11243

simoami opened this issue May 4, 2018 · 15 comments
Labels
component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module! new feature New feature or request

Comments

@simoami
Copy link
Contributor

simoami commented May 4, 2018

Current Behavior

There a fundamental issue with the menu dismissal. When presented with multiple buttons or select fields that show a menu, it take 2 clicks to dismiss the existing menu and show the next one.

material-ui

Steps to Reproduce

In the demo below, try opening the first menu by clicking on the first button, then the second menu by clicking on the second button. Notice that an extra click is needed to dismiss the first menu then another one to show the second menu.

https://codesandbox.io/s/9lp94v86zo

Expected Behavior

Good UX guidelines recommend to reduce the number of clicks needed to perform an action. As opposed to modals and dialogs, menus are not expected to hijack the click away events.

The issue is that the background click to dismiss a menu, has a e.preventDefault() which prevents it from bubbling up to the other element.

In other frameworks, you can click on an other element, and it will have double effect: close up the first menu, and show the new one in one click action. For reference:

Bootstrap

http://getbootstrap.com/2.3.2/components.html#buttonDropdowns

bootstrap

Semantic-UI

semantic-ui

Sencha ExtJS

http://examples.sencha.com/extjs/6.5.3/examples/kitchensink/?modern#buttons-split

extjs6-2

UIKit

https://getuikit.com/v2/docs/button.html

Your Environment

Tech Version
Material-UI v1.0.0-beta.44
React 16
browser Chrome 65
etc
@oliviertassinari
Copy link
Member

oliviertassinari commented May 5, 2018

@simoami The current Menu behavior was designed on purpose. It's following the menu behavior on mobile, try Android for instance. I do think there is room for the two behaviors. It's a tradeoff. On mobile, the cost of wrongly interacting with an element is much higher. It might trigger a screen transition, it can slow users down.
Another important point to notice is the scroll blocking behavior.

We have a related issue: #9893. I see two possible paths going forward. 1. Either we add some options to the Popover or 2. we build a new component on top of the Modal. It's prioritized for the post v1 roadmap.

@oliviertassinari oliviertassinari added the new feature New feature or request label May 5, 2018
@oliviertassinari oliviertassinari added this to the post v1.0.0 milestone May 5, 2018
@simoami
Copy link
Contributor Author

simoami commented May 5, 2018

Thanks @oliviertassinari

oh yes, I just noticed the scroll block too. While this solves for mobile, it is not a desired behavior for menus on desktop. Btw, I took a look at #9893 and I feel that the dropdown issue is only one instance of a side effect of the current menu behavior and that a fix shouldn't be limited to dropdown components. Another common scenario is a list with secondary actions with menus attached to them.

In terms of what you're suggesting, I'm open to either option, as long as the default behavior is not altered. I'm thinking that a configurable option is the easiest one. Angular's material project has a hasBackdrop option, but they don't have it implemented correctly. We could implement it properly to alter the event and scroll lock behavior at the same time.

hasBackdrop: true|false (defaults to true) When disabled, click away events are accessible to parent nodes and page scrolling is allowed.

Fyi, Google's own cloud platform (which is material design) has the menus behave the right way:

google cloud

@oliviertassinari
Copy link
Member

oliviertassinari commented May 5, 2018

Fyi, Google's own cloud platform (which is material design) has the menus behave the right way:

IMHO, it's wrong. It's not the behavior a native select has: https://jsfiddle.net/q1pmh7oo/.

@simoami
Copy link
Contributor Author

simoami commented May 5, 2018

I see what you mean 👍 Like I said we can keep the default behavior intact and offer more customization. I think different use cases call for different behavior. I personally think one less click is better ux if it doesn't have a side effect.

@fangeugene
Copy link

The current Menu behavior was designed on purpose. It's following the menu behavior on mobile, try Android for instance.

@oliviertassinari, you're right that the behavior on Android is for the menu to dismiss when tapping outside a menu. However, a key difference with the MUI implementation is that it closes onClick, whereas on Android it closes onTouchStart. The issue is that drag events don't fire onClick, so if a user initiates a drag (for example to scroll), the menu doesn't dismiss, leaving the user confused.

Here's an illustration, just to be clear.

Currently:

  1. User opens menu
  2. User tries to scroll the background, but nothing happens. (repeat until user realizes what is going on)
  3. User clicks on background, dismissing the menu.
  4. User tries to scroll again, and succeeds.

Desired (and how it behaves on native Android):

  1. User opens menu
  2. User tries to scroll the background, dismissing the menu.
  3. User tries to scroll again, and succeeds.

@oliviertassinari oliviertassinari removed this from the post v1.0.0 milestone Jul 17, 2018
@oliviertassinari
Copy link
Member

@simoami What do you think of moving the discussion to #9893?
@fangeugene Interesting concern.

@oliviertassinari oliviertassinari added priority: important This change can make a difference component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module! labels Mar 13, 2019
@oliviertassinari
Copy link
Member

Related to #17353.

@simo-eskalera
Copy link

@oliviertassinari Sorry, I missed your last comment.
I'm ok moving the convo to the other issue. I just want to point out that this is not specific to the dropdown component. Take for example a table view with each row having the options icon. And with the clickaway layer, it also takes two clicks to open the menu for a different row. The common issue here and with the dropdown component is the menu clickaway layer that hijacks clicks.

@eps1lon
Copy link
Member

eps1lon commented Oct 28, 2019

I would question how we use the Popover for Menu or Select in the first place currently. These are pretty disruptive since they mess with page layout (see scroll bars) and the clickaway listener prevents any other clicks. As outlined in this issue you usually don't want them to have the same status as a something like a dialog (don't hide rest of the UI and don't intercept clicks).

Using the Popper and then closing the listboxes when it looses focus would be more like the platform handles these things. We only need to consider the edge case when we click the trigger again when the listbox is displayed. Native FocusEvent has relatedTarget but that is incorrectly polyfilled in react.

@oliviertassinari
Copy link
Member

oliviertassinari commented Oct 28, 2019

@eps1lon I share your feeling, I think that the current menu and select behavior is too disruptive for desktop (I think that it's much better suited for mobile). There is a related comment you might appreciate in #17636 (comment).

@oliviertassinari oliviertassinari removed the priority: important This change can make a difference label Dec 1, 2019
@oliviertassinari oliviertassinari changed the title Clicking outside a menu blocks click from bubbling up [Menu] Clicking outside a menu blocks click from bubbling up Dec 1, 2019
@dmitriy-komarov
Copy link

Understanding, this works by design for mobile platforms, but for desktops (browsers) there is still not friendly behaviour.

The case:
App renders tree of folders. User calls a context menu for one folder, then for another folder, etc.. Currently user always needs to mandatory close previous popover by left clicking outside of it to open another.

I checked menus in Windows. When I call context menus for different folders, I do not need to close menu before opening a new one.

There are two issues I see here:

  1. There is not event bubbling on desktops. I suppose there should be a flag (property) for this. Even for desktops. I ask to return to this question discussion.
  2. Popover can be closed by left click outside only. Right click opens default browser menu - this turns the case twice harder for user. Now he have to close two menus before opening a new one!

Maybe the item (1) is for discuss, but I guess item (2) should be fixed. At least by disabling browser context menu on right click. This behaviour can be changed adding onContextMenu property only, not by CSS. But the best if right click will close the menu as well left click does.

@Nichtmetall
Copy link

Hey man, i've had the same issue. So i searched on the world wide web and found something which is similar to your problem. It solves the problem in a different way than the 'vanilla' way of MUI.
The website I found promoted an 'npm' package which provides a Custom React Hook that keeps track of the local state for a single popup, and functions to connect trigger, toggle, and popover/menu/popper components to the state.

So I tested it. And it worked fine for me. In my case i wanted just a hover menu, but here you can find some other usefull templates for dropdowns. Also as in your case a normal dropdown menu.

In my case i created a react component:

import * as React from 'react';
import HoverMenu from 'material-ui-popup-state/HoverMenu';
import {MenuItem, Button, Typography, Divider} from '@mui/material';
import ArrowDropDownRoundedIcon from '@mui/icons-material/ArrowDropDownRounded';
import { usePopupState, bindHover, bindMenu, } from 'material-ui-popup-state/hooks';
import Link from 'next/link';

//options on items collection:
//'br' => sets divider
//'TEXT' => button with link

const DropDown = ({title, items = []}) => {
  const popupState = usePopupState({
    variant: 'popover',
    popupId: 'menu',
  })

   return (
    <React.Fragment>
      <Button variant="ghost" {...bindHover(popupState)} endIcon={ <ArrowDropDownRoundedIcon />}>{title}</Button>
      <HoverMenu disableScrollLock={true} {...bindMenu(popupState)} anchorOrigin={{ vertical: 'bottom', horizontal: 'left' }} transformOrigin={{ vertical: 'top', horizontal: 'left' }}>
        {items.map(item => {
            if(item == 'br'){
                return <Divider />
            }
            else {
                return <MenuItem onClick={popupState.close}><Link href={item}><Typography variant='button'>Team</Typography></Link></MenuItem>
            }
        })}
      </HoverMenu>
    </React.Fragment>
  )
}

export default DropDown;

I hope this helps you and others!

@iamharshad
Copy link

Any update on this?

@Harishan15
Copy link

Harishan15 commented Jul 26, 2023

Hi All,
@simoami @oliviertassinari @fangeugene @eps1lon @iamharshad

The problem was that in the project, when we opened a menu, we had to click once to close it before being able to open another menu. This double-click requirement was inconvenient.

To fix this issue, I made changes to the code as below:

  1. I added a property called disablePortal to the menu.
  2. I applied a specific style to the menu using sx property with zIndex: 'unset'.
<Menu
  PaperProps={{
    style: {
      maxHeight: mdMatches ? '500px' : '300px',
    },
  }}
  anchorOrigin={{
    vertical: 'bottom',
    horizontal: 'left',
  }}
  transformOrigin={{
    vertical: 'top',
    horizontal: 'left',
  }}
  sx={{
    zIndex:'unset',
    bgcolor: { xs: 'rgba(0,0,0,0.2)', md: 'unset' },
    backdropFilter: { xs: 'blur(2px)', md: 'unset' },
    ul: { py: '0px' },
    '.MuiPaper-root': {
      borderRadius: '10px',
      boxShadow: '2px 2px 18px rgb(0 0 0 / 20%)',
      border: ' 1px solid #ddd',
      width: { xs: '100%', md: 'fit-content' },
    },
  }}
  disablePortal
>
// List
</Menu>

These changes did the trick! Now, when you click to close an opened menu, you can immediately open another menu without having to click twice.

If anyone else is facing a similar problem, you can try implementing this solution. Let me know if it works for you too!

@pontovinte
Copy link

Just in case if someone else faces the same issue, I was able to solve with the following:

  1. Added a scroll handler:
    React.useEffect(() => {
        window.addEventListener('scroll', handleProfileMenuScroll);
        return () => window.removeEventListener('scroll', handleProfileMenuScroll);
    }, [])
  1. Then on handleProfileMenuScroll():
    const handleProfileMenuScroll = () => {
        setOpenMenu(false);
    }
  1. Added the option disableScrollLock to the <Menu> definition

When the user scroll down, it closes the menu (useful if you're building dynamic Menus on different cards for example). If you only have one Menu (on the top for example), then you can skip the scroll handler and use the disableScrollLock option.

I hope it helps.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: menu This is the name of the generic UI component, not the React module! component: select This is the name of the generic UI component, not the React module! new feature New feature or request
Projects
None yet
Development

No branches or pull requests

10 participants