primereact icon indicating copy to clipboard operation
primereact copied to clipboard

DataTable: Fix and Simplify overlay handling in cell edit mode

Open gnawjaren opened this issue 9 months ago • 8 comments

Defect Fixes

Simplify overlay handling in cell edit mode

This PR simplifies the current overlay handling logic in cell edit mode by introducing a declarative approach. Instead of relying on OverlayService, selfClick, and setTimeout workarounds, it checks for a data-pr-is-overlay attribute on clicked elements (or their parents) to determine whether an outside click should close the cell editor.

Benefits: • Greatly reduces complexity and improves maintainability • More robust and easier to debug • Enables support for custom or third-party overlay editors, including full interaction, as long as they include the data-pr-is-overlay attribute on their container • Makes the logic more declarative and extensible

I tested this in various parts of our project with good results.

Let me know if this approach is acceptable – I’d be happy to adjust the implementation if needed. Fix #7405 Fix #7158 Fix #8080

gnawjaren avatar Apr 30 '25 12:04 gnawjaren

It appears that all code related to overlayService and the selfClickRef is only used in this specific context. I would update this PR to remove that logic—unless this general approach is not acceptable. Otherwise, I’ll push the remaining changes shortly.

gnawjaren avatar May 01 '25 15:05 gnawjaren

@gnawjaren may as well push all your changes.

melloware avatar May 01 '25 15:05 melloware

Conflicts now. Is this PR still needed?

melloware avatar May 06 '25 11:05 melloware

That depends on whether this alternative approach is considered desirable or an improvement over the current fix.

gnawjaren avatar May 06 '25 16:05 gnawjaren

OK i will leave it pending review. But you should fix the merge conflicts.

melloware avatar May 06 '25 18:05 melloware

@gnawjaren can you fix the merge conflicts? I have also linked this PR to that new ticket.

melloware avatar Jun 10 '25 15:06 melloware

I’ve rebased the branch and resolved the conflicts. I also re-tested the changes in our project. The main question now is whether this is the best place to add the attributes – or if it would make sense to include them in other components as well. Currently, only Dropdown and Calendar are affected, but extending it to more components should be straightforward.

gnawjaren avatar Jun 11 '25 22:06 gnawjaren

@gnawjaren yes please i think so this does simplify and clean up the handling.

melloware avatar Jun 11 '25 22:06 melloware

@gnawjaren could you please fix the merge conflicts once again?

Let's merge this long awaiting change to the next release :)

howkymike avatar Jul 01 '25 18:07 howkymike

@melloware Is there anything more we can do here or can we merge it?

howkymike avatar Jul 09 '25 09:07 howkymike

I don’t think this is the right solution. I believe DomHandler.getParents will affect the table’s performance. Also, OverlayService is an exported eventbus, so users can integrate a select or similar component from a different library using this OverlayService.on('overlay-click')

mertsincan avatar Aug 14 '25 14:08 mertsincan

I understand your concerns about performance. The primary motivation for this change is to achieve fully predictable event ordering. With the current OverlayService approach, we need to rely on setTimeout, which can cause race conditions and subtle bugs — especially when browser and custom events are mixed.

getParents only runs on document clicks and only when an overlay is open, so in practice the performance impact should be negligible.

The improved third-party overlay support is more of a side benefit — the main goal here is removing the setTimeout dependency to make the behavior more robust and maintainable.

Ultimately, I believe this trade-off is worth it, but of course we can revert if you decide otherwise.

gnawjaren avatar Aug 15 '25 12:08 gnawjaren