Popup: when multiple popups present, click on the other trigger doesn't close original popup

[helgamir]

Bug Report

This seems to be the same issue that was reported #1251 But it got back with new versions.

Steps

Create several popups with a click trigger.
Click first trigger --> first popup opens, then click second trigger --> the second opens, but the first stays open.

Expected Result

The first popup should be closed when the second trigger clicked.

Actual Result

The first popup stays open.

Version

0.82.0

Testcase

https://codesandbox.io/s/3rx96jzjr6

[welcome]

👋 Thanks for opening your first issue here! If you're reporting a 🐞 bug, please make sure you've completed all the fields in the issue template so we can best help.

We get a lot of issues on this repo, so please be patient and we will get back to you as soon as we can.

[lucmerceron]

Hello guys,

First, thanks for your amazing work !
I think I found the bug, can I submit a PR ?

[layershifter]

Yep, feel free to pickup any issue marked as help wanted 👍

[lucmerceron]

This line seems to be responsible. When adding the event to the second trigger, it is removing the targetHandler of the first one, hence removing its document listener (for a "click" in our case).
Is there a particular reason this is done this way ?

[layershifter]

Yep, it's hard to describe, but you can try to remove/move this line and check how it affects sidebar examples. The ideal solution, fix current bug and keep current behaviour of Sidebar examples. Not easy 🐱

[lucmerceron]

Indeed, it is harder to comprehend than expected.

From my investigation, removing and adding that handler on the second portal mount prevents the first one to receive the click event (the one that causes the second portal to mount).
A "naïve" analogy would be that removing & adding a click listener on a document click won't trigger the listener just added.

A lazy solution would be to avoid sharing an EventStack instance between every Portal components so the listeners management will not be mixed.
Sidebar, which actually suffers the same bug, could take advantage of that solution too.

Example of the solution

[lucmerceron]

The thing is that components like Sidebar expect the system to work as described above. So if we try to do it smarter by actually managing each handler in the eventPool without removing & adding the eventListener to the DOM, the Sidebar (an other) component will add the event that will be triggered directly (hence closing directly the newly visible sidebar).

I think the eventStack -> eventTarget -> eventPool -> eventSet system is flawed for these cases. It would work if each component added/removed themselves their own callbacks to the document.

What do you guys think ?

[honzajerabek]

Hi there, good job investigating! I discovered this bug with Dropdown component - https://codesandbox.io/s/m5x3k8y10p which brought me here, will help if needed :)

[lucmerceron]

Hello guys, is there a plan / direction to solve this bug ? I could work on it if we agree on a solution.

[dmitrykrylov]

@lucmerceron I would be grateful if anybody could solve it

[layershifter]

@lucmerceron I'm currently busy with my paid work.

EventStack was moved to the separate repo. The solution is reorder and change event subscribing, more details are there #3124 (comment). The help is much appricated.

It seems that we will have issues with Sidebar, but it's better to resolve the current problem.

[semireg]

I think I've been experiencing this bug in my app. The JSX looks something like:

      <Dropdown key="outlineKey" text="Outline" item closeOnBlur direction="left">
        <Dropdown.Menu>
          <Dropdown.Item

What I find interesting is that the first dropdown is closed OK. It's the second dropdown that doesn't close. But only if the second dropdown opening action caused another dropdown to close.

Perhaps this little tidbit will help with debugging. I can also test any solutions against my codebase to verify a fix. Good luck, this one doesn't seem easy!

[ivanjiang5628]

This issue is very critical to our application.

Can anyone help?

Thanks

[dmitrykrylov]

@ivanjiang5628 make popup controlled

[layershifter]

It's the high priority issue to me, I will work on it at the current week. The next release should contain the proper fix.

[ivanjiang5628]

@ivanjiang5628 make popup controlled

@dmitriyshmatov Thanks for replying

I can solve this problem by adding "force update" and "force update onChange function" states to the parent component and then passing them to the child components (Popups) as props, but it will significantly slow down the rendering speed of the application due to multiple renderings.

@layershifter Thank you

[layershifter]

Manually checked the new release, the issue is fixed in [email protected].