Reach-ui: Menu List / Popover does not consistently close when clicking between Menu Buttons

[impulse]

🐛 Bug report

Current Behavior

Expected behavior

Dropdown of the previously clicked menu item should always close.

Reproducible example

https://codesandbox.io/s/inspiring-goodall-02zhd?file=/src/index.js

Your environment

| Software | Name(s) | Version |
| ---------------- | ------- | ------- |
| Reach Package | @reach/menu-button | 0.10.1 |
| React | react, react-dom | 16.13.1 |
| Browser | Chromium | 81.0.4044.113 |
| Browser | Firefox | 75.0 |
| Assistive tech | - | - |
| Node | nodejs | v13.12.0 |
| npm/yarn | yarn | 1.22.4 |
| Operating System | macOS | 10.15.4 |

[raunofreiberg]

I took a quick look at this and it seems like buttonClickedRef is causing this problem. Specifically, in this function here:

https://github.com/reach/reach-ui/blob/25840c3d15278e316cc74eaecf096fa01377f361/packages/menu-button/src/index.tsx#L857-L874

The else branch is never reached _sometimes_ when clicking between two menus. I tried removing this logic and the bug was resolved.

---

There is a comment about using this ref:

https://github.com/reach/reach-ui/blob/25840c3d15278e316cc74eaecf096fa01377f361/packages/menu-button/src/index.tsx#L110-L114

Yet, I don't understand why ref is even necessary if the listener checks for isExpanded before doing anything.

cc @chancestrickland

[raunofreiberg]

Okay. Digging more into this. I think what happens is that buttonClickedRef is set to true during onMouseDown, but it is never set to false when closing the menu since the else branch in the global listener does not run.

I think the correct fix here would be to set buttonClickedRef to false during onMouseUp in MenuButton. Although, I'm still not certain tracking this state in a ref is even necessary in the first place.

[chaance]

That's a great head start @raunofreiberg, thanks for digging in here! We'll likely use a similar state machine as listbox eventually but for now a simple fix will be great. I'll take a look this afternoon.

[raunofreiberg]

@chancestrickland shall we do something about this? Folks are starting to patch @reach/menu-button to fix this issue. I can open a PR if you'd like 🙂

[johnwcallahan]

Hi @chancestrickland and @raunofreiberg, any update on this? The patch we were using no longer works after upgrading from 0.9.1 to 0.11.2. I spent a little time in the code, but can't quite work out what's going wrong. Thanks in advance!

[jescowuester]

I'm having the same issue with the Listbox component (after 0.12). Should I open a new issue for that?