mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.35k stars 32.13k forks source link

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

Open simoami opened 6 years ago

simoami commented 6 years ago

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

@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.

simoami commented 6 years ago

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

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

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

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

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

oliviertassinari commented 4 years ago

Related to #17353.

simo-eskalera commented 4 years ago

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

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

@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 https://github.com/mui-org/material-ui/issues/17636#issuecomment-536693407.

dmitriy-komarov commented 2 years ago

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

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 commented 1 year ago

Any update on this?

Harishan15 commented 1 year ago

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!