Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Popup-menu closes when refresh is called during initial render #877

Open
marstamm opened this issue Mar 26, 2024 · 1 comment
Open

Popup-menu closes when refresh is called during initial render #877

marstamm opened this issue Mar 26, 2024 · 1 comment
Assignees
Labels
backlog Queued in backlog bug Something isn't working

Comments

@marstamm
Copy link
Contributor

Describe the Bug

In this Component, the popup menu will close directly when the promise is resolved instantly and the timeout is removed
https://github.com/bpmn-io/refactorings/blob/main/lib/popup-menu/RefactoringsPopupMenuProvider.js#L30-L32

Steps to Reproduce

Steps to reproduce the behavior:

  1. do this
  2. do that

If possible, try to build a test case that reproduces your problem.

Expected Behavior

A clear and concise description of what you expected to happen.

Environment

Please complete the following information:

  • Browser: [e.g. IE 11, Chrome 69]
  • OS: [e.g. Windows 7]
  • Library version: [e.g. 2.0.0]
@marstamm marstamm added bug Something isn't working in progress Currently worked on labels Mar 26, 2024
@marstamm marstamm self-assigned this Mar 26, 2024
@marstamm
Copy link
Contributor Author

Root Cause
We open the Menu using a click event, that originates outside of the Popup menu.

We register event listeners on the body, to check whether and outside click occurred, in an useEffect hook. Per default, the effect execution is delayed until after the browser renders the next frame.

Because of how Promises are scheduled when they are immediately resolved we call popupMenu.refresh() before the original click event bubbles to the body.
This forces a re-render of the Preact component, which forces execution of the delayed effects. The event listeners are created on the body, while the original click event is still pending.

When it bubbles up to the body, the check fails and closes the menu directly.

How to fix it
We discussed possible solutions in the Hour of Code. We agree that the use-case is valid and this is a bug in the core.

However, we were unable to find an elegant solution. The solution sketch we came up with is as follows:

  • Move the outside-click logic out of the Preact component. It uses css selectors only (no refs) anyway.
  • Ensure that the listener is registered/active only after the initial rendering has happened, e.g. using requestAnimationFrame.

Conclusion
As we have a workaround in refactorings, this is an edge-case and the solution is not elegant, I will move this to the backlog for now. If you want to experiment with it, I added the test-case I used to showcase it in the HOC here: https://github.com/bpmn-io/diagram-js/tree/877-experiement-popup-menu-event-handler

@marstamm marstamm added backlog Queued in backlog and removed in progress Currently worked on labels Mar 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backlog Queued in backlog bug Something isn't working
Projects
None yet
Development

No branches or pull requests

1 participant