DataTable: Fix and Simplify overlay handling in cell edit mode
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
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 may as well push all your changes.
Conflicts now. Is this PR still needed?
That depends on whether this alternative approach is considered desirable or an improvement over the current fix.
OK i will leave it pending review. But you should fix the merge conflicts.
@gnawjaren can you fix the merge conflicts? I have also linked this PR to that new ticket.
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 yes please i think so this does simplify and clean up the handling.
@gnawjaren could you please fix the merge conflicts once again?
Let's merge this long awaiting change to the next release :)
@melloware Is there anything more we can do here or can we merge it?
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')
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.