useDisplayOrder: clears incorrect

[Manizuca]

Describe the bug

The useDisplayOrder hooks clears out an incorrect id when unmounting, so very simple Dialogs configurations throw "Unexpected: global esc key listener with priority [300, x] already exists." errors", even if not using the 'closeOnEscape' option (if they are not destroyed in the reverse order they become visible).

primereact/components/lib/hooks/useDisplayOrder.js

Line 22 in d5625fe

delete groupToDisplayedElements[group][newDisplayOrder];

Here, the information is deleted taking the generated displayOrder as an index, but that will always be off by 1.

In the attached example, te following happens in this order:

• Dialog "one" is created visible, is asigned displayOrder 1.
• Dialog "two" becomes visible, is asigned displayOrder 2.
• Dialog "one" is hidden. Because of this bug, the information for Dialog "two" is removed, as the value of groupToDisplayedElements[group] is [dialogOne, dialogTwo], and deleted the value with index 1.
• Dialog "three" becomes visible, is asigned displayOrder 2.

Finally, because there are two visible Dialogs with displayOrder 2, the application crashes.

The fix should be pretty trivial, just delete the item with index newDisplayOrder - 1. Also, i think the global Esc handler should not be added if closeOnEscape is false.

Reproducer

https://stackblitz.com/edit/vitejs-vite-e88fmg

PrimeReact version

10.5.1

React version

17.x

Language

ES6

Build / Runtime

Create React App (CRA)

Browser(s)

No response

Steps to reproduce the behavior

Just load the page, and look for the "Unexpected: global esc key listener with priority [300, 2] already exists." error.

Expected behavior

Should not crash.

[melloware]

Thanks for the report. I improved closeOnEscape logic on a few components including this one.

[Manizuca]

Thanks @melloware , this fixes the problem for me. I think the useDisplayOrder code should be fixed too, since it could still happen when closeOnEscape is true.

[melloware]

well what is weird is I can't reproduce your error with the showcase. Now showcase is Next.Js not sure if that makes a difference or not. Want to suggest a PR?

[Manizuca]

Changing the closeOnEscape value to true con the reproducer throws the error for me for that case, doesn't it happen for you?

I think it should work by changing
delete groupToDisplayedElements[group][newDisplayOrder];
to
delete groupToDisplayedElements[group][newDisplayOrder - 1];

I could create a PR later today if you want.

[melloware]

let me try that. And no in using the showcase i can't get it to error!

[melloware]

@Manizuca can you check out my new PR it refactors how that is done...

1. Removed the unnecessary check if (!(group in groupToDisplayedElements)) since we're checking if groupToDisplayedElements[group] exists directly.
2. Simplified the calculation of newDisplayOrder by using the push method, which returns the new length of the array.
3. Removed the deletion of elements by index and replaced it with finding the index of the element using indexOf and then removing it using splice.
4. Removed the cleanup logic to reduce the length of the array since it seems unnecessary considering we are setting setDisplayOrder(undefined).

This version should be cleaner and more concise while maintaining the same functionality.