fix(Dialog): "esc" key close dialogs in wrong order

[avasuro]

Fix: #5019

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

2 Ignored Deployments

Name	Status	Preview	Comments	Updated (UTC)
primereact	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2023 3:40pm
primereact-v9	⬜️ Ignored (Inspect)	Visit Preview		Nov 29, 2023 3:40pm

[avasuro]

@melloware

in the Dialog.js we use the useOnEscapeKey to subscribe to the "esc" key press event.

And there's an issue that I can't resolve on my own. Inside the useOnEscapeKey hook on line 7 there is stopImmediatePropagation call. This call causes only one escape handler to be executed, specifically the first one that was assigned.

This limitation prevents me from calling the proper onClose handler in the Dialog component. It will work correctly only for the first dialog, but for the second and subsequent dialogs, the escape key handler (passed in useOnEscapeKey hook) won't be invoked at all.

I can't do any changes to the useOnEscapeKey hook since it's used across many components, and the stopImmediatePropagation is there for some reason, so if I remove it - it's highly likely that this change will introduce new bugs.

If you don't mind, I'd like to leave it to you and the core team to address this issue in the PR, as you have a better understanding of the situation with stopImmediatePropagation.

[melloware]

Good point let me look at that stopImmediate as that I assume was meant to eat the ESC key from bubbling up. But not sure why it needs to be stopped?

[melloware]

I think it should be preventDefault() and not StopImmediate WDYT?

[avasuro]

@melloware

I believe it uses the stopImmediatePropagation to ensure that once the "esc" key is handled, for example, by a modal, it prevents any other components, such as the sidebar, from also handling it.

Same behavior could be achieved with preventDefault (and I like this idea) - but in that case everywhere were useOnEscapeKey is used we need to put a check like if (e.defaultPrevented) return // do nothing.

[melloware]

Agreed its a better solution if you want to update. stopImmediate should rarely be used. It might be a holdover from the original code I looked through Git and it was there in 2018 but so much has changed since then.

[akshayantony55]

@melloware I think we need to add some code to identify the top most modal and return the execution from useOnEscapeKey -> handleEsc() if its not the top most modal.

if (event.key === 'Escape' && !isTopModal()) {
  return;
}

But Identyfying the top most modal will be some good amount of work to do!

[avasuro]

@akshayantony55 in this PR I already written the code that identifies the topmost dialog (it will be the last dialog in primeDialogParams variable ) - the problem is that handleEsc handler is not executed at all for any dialog except first one.

And I didin't tested, but I feel like if another component (SideBar?) that uses useOnEscapeKey will be used together with Dialog - then handleEsc will not be triggered for one of them (either SideBar or Dialog, depending which one has assigned esc handler first).

P.S.: or, if I understood you wrong and you propose to add isTopModal check directly into useOnEscapeKey - probably it's not the best idea, as this violates incapsulation and separation of concerns rule.

[akshayantony55]

@melloware I will spend some time on this during the weekend.

[avasuro]

I apologise, had busy week, so didn't had free time to look at the bug. I think I will be able to do that this weekend.

@akshayantony55 please, inform if you already started doing something and had some progress

[melloware]

Another PR for this issue was submitted: #5126 Please review and give thoughts?

[avasuro]

@melloware

This task more complicated than I expected. I've added a commit with a basic concept for a possible implementation.
I can't say that I like this solution pretty much, but for now it's the only one that I've found that seems to work.

Here's a rough overview:

• There's a set of "esc" key handlers, sorted by priority. When the "esc" key is pressed, we identify the handler with the highest priority and execute it. All other handlers are bypassed.
• Priority is composed of two parts: [component, internal priority]. For example, any handler from the Dialog component will run before any handler from the Sidebar component. Internal priority helps to determine which one of the opened dialogs should handle the "esc" key first if multiple dialogs are open.
• To help Dialog components identify the internal priority, I've created a separate hook called useDisplayOrder, which determines which Dialog is displayed on top of the others. This hook can be reused for other components as well.

Please note that this is a very rough concept, implemented for testing purposes. However, it has already resolved the issue with the Dialog component in the current implementation.

Please, share your thoughts, and tell should I continue in this way, and apply the same approach to other components with "esc" handling or, probably, something should be done differently (or, probably, as bug fixed for Dialog we should stop at this point and then see how this solution will work "in production" before being applied everywhere)

[melloware]

@avasuro I like where you are going with this. I agree we need some way of tracking across the different components if they are all subscribed to the document ESC key etc.

[avasuro]

@melloware

Ready for review. Please test thoroughly as I haven't done extensive testing - just a basic check to ensure the "esc" key functions properly in all updated components. I've conducted a few tests involving multiple components. For example, making sure that if both the sidebar and dialog are open together, the modal closes on the first "esc" key press, and the sidebar closes on the second.
Thank you.

[melloware]

I have also added PrimeTek on this review. This is great work.

[melloware]

@avasuro another spot to add to: #5361 CascadeSelect

[avasuro]

@melloware added

[melloware]

sorry @avasuro one more: #5366

[avasuro]

@melloware added

[melloware]

@avasuro i just merged some changes and there is a conflict as well as SpeedButton also just got close on esc! Appreciate you keeping up with these.

[avasuro]

@melloware merged

[melloware]

Great job @avasuro