Skip to content
This repository was archived by the owner on Jun 28, 2026. It is now read-only.

DataTable: Fix and Simplify overlay handling in cell edit mode #7951

Merged
melloware merged 3 commits into
primefaces:masterfrom
gnawjaren:datatable-celledit
Jul 9, 2025
Merged

DataTable: Fix and Simplify overlay handling in cell edit mode #7951
melloware merged 3 commits into
primefaces:masterfrom
gnawjaren:datatable-celledit

Conversation

@gnawjaren

@gnawjaren gnawjaren commented Apr 30, 2025

Copy link
Copy Markdown
Contributor

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 gnawjaren changed the title Improve Overlay Handling (Minimal) DataTable: improve Overlay handling in cell edit mode (Minimal) Apr 30, 2025
@melloware melloware added the Status: Pending Review Issue or pull request is being reviewed by Core Team label Apr 30, 2025
@gnawjaren

Copy link
Copy Markdown
Contributor Author

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.

@melloware

Copy link
Copy Markdown
Member

@gnawjaren may as well push all your changes.

@gnawjaren gnawjaren changed the title DataTable: improve Overlay handling in cell edit mode (Minimal) DataTable: Fix and Simplify overlay handling in cell edit mode May 1, 2025
@melloware

Copy link
Copy Markdown
Member

Conflicts now. Is this PR still needed?

@gnawjaren

Copy link
Copy Markdown
Contributor Author

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

@melloware

Copy link
Copy Markdown
Member

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

@melloware

Copy link
Copy Markdown
Member

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

@gnawjaren gnawjaren force-pushed the datatable-celledit branch from 5a6a89c to 453f589 Compare June 11, 2025 22:32
@gnawjaren

Copy link
Copy Markdown
Contributor Author

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.

@melloware

Copy link
Copy Markdown
Member

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

@howkymike

Copy link
Copy Markdown

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

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

@melloware melloware force-pushed the datatable-celledit branch from e227522 to a68155f Compare July 6, 2025 15:00
@howkymike

Copy link
Copy Markdown

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

@melloware melloware added Type: Bug Issue contains a defect related to a specific component. and removed Status: Pending Review Issue or pull request is being reviewed by Core Team labels Jul 9, 2025
@melloware melloware merged commit bbab313 into primefaces:master Jul 9, 2025
2 checks passed
@mertsincan

Copy link
Copy Markdown
Member

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')

@gnawjaren

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Type: Bug Issue contains a defect related to a specific component.

Projects

None yet

4 participants