Overlay not closed when clicking another MenuTrigger

[madmanwithabike]

This is possibly related to #850, but using the @react-aria hooks only and is repeatable on every click, #850 seems to be fast click only and not reproducible at all in some cases.

🐛 Bug Report

Clicking between two (or more) buttons with useMenuTrigger and useOverlay, does not close the currently open menu.

🤔 Expected Behavior

Open menu should close and new menu should open.

😯 Current Behavior

This seems to only happen with useMenuTrigger, clicking from an open menu to a button using useButton closes the menu as expected.

Screenshot:

When clicking away repeatedly, the overlays are then closed one at a time in reverse order.

💁 Possible Solution

Possibly related to #850.

💻 Code Sample

Can be reproduced using the useMenuTrigger sample code: https://react-spectrum.adobe.com/react-aria/useMenuTrigger.html#example but duplicating the MenuButton:

<MenuButton label="Actions">
  <Item key="copy">Copy</Item>
  <Item key="copy">Copy</Item>
  <Item key="copy">Copy</Item>
</MenuButton>
<MenuButton label="Actions">
  <Item key="copy">Copy</Item>
  <Item key="copy">Copy</Item>
  <Item key="copy">Copy</Item>
</MenuButton>

🌍 Your Environment

Software	Version(s)
@react-aria/button	3.2.2
@react-aria/menu	3.1.2
@react-aria/overlays	3.4.0
Browser	Chrome + Firefox
Operating System	Ubuntu 18.04 + Windows 10

Thanks!

[LFDanLu]

Reproduced this locally, initial investigation points to this being an issue with how useOverlay adds new overlays to its visibleOverlays before calling onInteractOutside, resulting in onClose never being called for the first overlay when the second menu button is clicked.

[snowystinger]

Initial thoughts and recommendations

Problem statement: How to know which overlay should be removed, they can be siblings or nested.

Goals:
If siblings, the already open one should be closed and then the new one opened. Order of operations may not matter.
If nested, then already open should stay open and the new one should also open.

Data Structures options:

• [[modal1, nestedinmodal1], [sibling]] represents as nested structure, only one internal array, but there may be a transition period in which there are two (as shown)
• [[modal1, sibling], [nestedinmodal1]] represents as layers, only one element should be in a layer at a time, but there may be a transition period in which there are two (as shown)

• {
  root: [modal1, sibling],
  modal1: [nestedinmodal1],
  sibling: [],
  nestedinmodal1: []
  }
  

  or to take the layers a little further, it could be a doubly linked graph, usually it'd look like a doubly linked list, but there may be a brief period where it's a tree with branches (as shown)

How to tell if we're in the nested or sibling case

• through context
  • Overlay.tsx? then non rsp users will need to include a new element or use a new hook, they might forget
• all overlays always close, unless a param is passed in that puts it into a controlled state to keep it open
  • probably a breaking change, also, very gross
• look up dom tree
  • nothing to really search for exists right now given that they might or might not portal out and may not aria-hide the rest of the tree can't look at the e.target to see if it's in an overlay, the container(s) may not say that they are an overlay such as in useMenuTrigger example
  • we might be able to pass a prop (data attribute?) back from useOverlay that is part of the spread though so that we can search for that, this would end up inside the new dom tree (appended to body usually), so any new overlays may not be inside it, and there may not be a trigger or container element to pass a ref from from which we could start the search

Context is looking most promising, we'll just need to make it optional and those who opt out should get a warning or they won't get the functionality.
I think the dl tree is probably the best option for the data structure used, for example:
2 menus one with a nested menu which is open all the way. User clicks on the second menu, we can close the first sibling by going down the linked path and then close as we come back up. It's an easy way to see what the order of operations should be.

Other issues:
This change will need to be done after the re-adding of Pointer Events to useInteractOutside. Otherwise with mixed pointer/mouse events, there is a bug that onInteractOutside won't be called if the user clicks on anything using usePress.

[snowystinger]

Upon initial conversation with team, evaluated that all of the above are complex solutions. We'd like something simpler. To do so, we're going back to the original problem, two menu's open at the same time.
We tried fixing it with #925
This was reverted by #980
It looks like the issue causing us to revert this had something to do with closing/opening on mouse down or mouse up. Supposedly it was also possible to have to Menu's open at the same time, though we couldn't repro back when this was in, nor can I repro now. I've been unable to find any other mentions to the specific issues that were found.

Reverted 980, so it's as though 925 is in place again. Here's the build https://reactspectrum.blob.core.windows.net/reactspectrum/3dd87b7a864f3e9ad33a8e7b5caf1e4ba7396b44/storybook/index.html?path=/story/actiongroup--default
And here's the branch, not making it a PR yet as it's not ready for that. https://github.com/adobe/react-spectrum/tree/re-explore-double-open-menus
Didn't apply to combobox, relies on #1449

Kinds of things I've been testing with it un-reverted:

• Open/close menus, found one minor issue, click and hold, drag mouse off and back on, menu will toggle. This is probably solvable with a body listener in the trigger that detects if the mouse is canceled or up while it's off the button.
• Nested Popover's, open both, then click on the initial trigger, disappearing order seems better. it was initial followed by nested, now they close in the opposite order with this change)
• Clicking off a popover of any kind, now hides on down, it used to hide/close on up. Need to decide if we want to handle touch on up instead, should be ok for menus because they don't open on down on touch anyways.
• Rapidly clicking/touching, no issues observed
• clicking a disabled 2nd menu next to the first, no issues observed
• click outside of popover in an input, popover closes and input receives focus, no issues observed
• keyboard interactions, no issues observed
• Tooltips no change

Side notes for other consideration:
Menu's open on press start or touch up depending on input mode, but popover/dialogs always open on up.

[snowystinger]

closing as fixed by #1970