material-components / material-components-web

Modular and customizable Material Design UI components for the web
https://material.io/develop/web
MIT License
17.15k stars 2.15k forks source link

[MDCMenuSurface] Unable to toggle MenuSurface closed due to 'greedy' body click handler #6188

Open dabrowne opened 4 years ago

dabrowne commented 4 years ago

Bug report

I've been trying to create an app bar with a button to toggle a menu, a bit like this:

image

I tried to achieve this by implementing a 'naive' click handler on my toggle button:

toggle.addEventListener('click', () => menu.open = !menu.open);

This works to open the menu, however once the menu is open and the toggle is clicked to close it, the menu closes and then rapidly reopens again. I've done some digging and tracked it down to the way that MDCMenuSurface listens for clicks on body to close the menu when the user clicks away:

https://github.com/material-components/material-components-web/blob/7313fc1a7d6dd4fec142d6392f97c6847d46519a/packages/mdc-menu-surface/component.ts#L70-L75

Thanks to the {capture: true} on body, this handler will always be triggered first and close the menu. Then when the event continues on down to my 'naive' toggle handler, it ends up reopening the menu.

I've created a 'dirty' workaround, which stops the event from propagating any further down the DOM so that my handler isn't run, which is really not ideal:

toggle.addEventListener('click', () => {
  menu.open = !menu.open
  // Stop the click event from reaching the toggle element so that the toggle click handler
  // isn't run. The menu will be closed first by the handler registered in the MCDMenuSurface component
  document.body.addEventListener(
    'click', e => e.stopPropagation(),
    {capture: true, once: true}
  );
});

Here is my demo showing both behaviours: https://codepen.io/dabrowne/pen/zYrWzxd

There needs to be an easier way to achieve what I would assume is quite common functionality. Have I missed something? Am I doing it wrong?

Your Environment:

Software Version(s)
MDC Web 7.0.0
Browser Firefox
Operating System Windows 10, Ubuntu 18

Possible solution

It would be great if we could remove the {capture: true} so that MCDMenuSurface isn't so greedy with it's event capture, but there's an existing comment there rationalizing it to fix a race condition (I tried tracing the code but couldn't find the race myself).

Even better would be if we could register the 'toggle' element with the MDCMenu or MDCMenuSurface component, and the component itself can be in charge of open/close on element click.

EstebanG23 commented 4 years ago

Hi there, thanks for raising this issue. I was able to reproduce with the steps provided. Adding to backlog.

sb8244 commented 3 years ago

Thanks for the workaround. This was causing me issues and there's no apparent workaround that I found (other than the very clever handler in this issue)

sb8244 commented 3 years ago

One bug I noticed with the workaround: It opens the menu, but then the first click is ignored because the click never made it to the body. I have no clue what would cause this, but it goes away when I remove the handler.

I changed it to the following (with a reference to my button element in scope):

        document.body.addEventListener('click', (e) => {
          if (buttonEl.contains(e.target)) {
            e.stopPropagation()
          }
        }, { capture: true, once: true })
lborgman commented 2 years ago

I added if (buttonEl !== e.target) return. The first click is then not ignored, but I am not sure why.

jjstanton commented 2 years ago

@EstebanG23 Any chance this issue can get escalated?

samstride commented 2 years ago

This issue is still present in v 14.0.0. The workaround mentioned by @sb8244 works in v14.0.0.