Skip to content

Fix click propagation for non-editable cells with editors#4891

Merged
azmy60 merged 3 commits into
masterfrom
claude/clever-wozniak-LVkGj
Jun 10, 2026
Merged

Fix click propagation for non-editable cells with editors#4891
azmy60 merged 3 commits into
masterfrom
claude/clever-wozniak-LVkGj

Conversation

@rathboma

@rathboma rathboma commented Jun 4, 2026

Copy link
Copy Markdown
Collaborator

Summary

Fixed an issue where click events on non-editable cells were being stopped from propagating, preventing the cellClick callback from firing when a column had an editor defined but the cell itself was not editable.

Changes

  • Edit.js: Moved the e.stopPropagation() call to only execute after confirming the cell is actually editable. Previously, the propagation was stopped before checking editability, which swallowed clicks on non-editable cells and prevented them from reaching parent listeners.

Implementation Details

The fix ensures that:

  • Click events on non-editable cells now properly bubble up to parent listeners, allowing the table-level cellClick callback to fire
  • Click events on editable cells still have propagation stopped as intended
  • The change is minimal and surgical, only reordering when stopPropagation() is called relative to the allowEdit() check

This resolves issue #4421 where the cellClick event was not firing for cells in columns with editors defined but marked as non-editable.

https://claude.ai/code/session_01Nt4ggeWkoLbhrLcYwciwts

claude added 2 commits June 5, 2026 19:19
#4421)

Add a failing unit test demonstrating that when a column defines an editor
but the cell is not editable, the edit click handler calls e.stopPropagation()
before checking editability. This swallows the click so it never reaches the
table-level listener that powers the cellClick callback.
The edit click handler called e.stopPropagation() unconditionally whenever a
column had an editor defined, before checking whether the cell was actually
editable. When the cell was not editable the click was swallowed and never
reached the table-level listener that powers the cellClick callback, so it
never fired (regression since 5.6).

Move the stopPropagation() call inside the editability check so it only runs
when the cell will actually be edited, allowing clicks on non-editable cells
to propagate normally.
@rathboma rathboma force-pushed the claude/clever-wozniak-LVkGj branch from 5bfae87 to 8f21a88 Compare June 5, 2026 19:19

rathboma commented Jun 5, 2026

Copy link
Copy Markdown
Collaborator Author

Root cause of #4421 and why this behavior change is correct

Where the bug came from

This is an accidental regression introduced by the editTriggerEvent feature in 5.6.0, not a deliberate behavior change.

In 5.5.4 (working), bindEditor wired editing through two separate handlers — the click handler only focused the cell, and edit() was invoked from the focus handler:

element.addEventListener("click", function(e){
    if(!element.classList.contains("tabulator-editing")){
        element.focus({preventScroll: true});   // click ONLY focuses
    }
});
element.addEventListener("focus", function(e){
    if(!self.recursionBlock){
        self.edit(cell, e, false);               // edit() gets the FOCUS event
    }
});

So edit() — and its long-standing e.stopPropagation() (which dates back to the 2021 modular rewrite) — only ever ran against the focus event. Focus events don't bubble, so that stopPropagation() was harmless: the actual click always bubbled up to the table-level interaction listener and cellClick fired.

In 5.6.0, the editTriggerEvent option was added and the click handler was rewritten to call edit() directly with the click event:

if(this.options("editTriggerEvent") === "focus" || this.options("editTriggerEvent") === "click"){
    element.addEventListener("click", function(e){
        if(!element.classList.contains("tabulator-editing")){
            element.focus({preventScroll: true});
            self.edit(cell, e, false);           // edit() now gets the CLICK event
        }
    });
}

Now the same old e.stopPropagation() runs against a bubbling click event, before the editability check:

if(!cell.column.modules.edit.blocked){
    if(e){ e.stopPropagation(); }   // swallows the click...
    allowEdit = this.allowEdit(cell); // ...even when this is false
    ...
}

For a cell that has an editor defined but is not editable, the click is swallowed before it can reach the table-level listener, so cellClick (and any other click-driven handler) never fires. Nobody decided "non-editable editor cells should stop firing cellClick" — it fell out of passing a bubbling event where previously only a non-bubbling one was passed.

Why changing the behavior is correct

  • It restores the pre-5.6 / 5.5.x behavior that this issue reports as the regression, and makes a non-editable editor cell behave like any ordinary cell — consistent and least-surprising.
  • The fix is minimal and surgical: it only gates stopPropagation() behind the allowEdit || forceEdit check, so propagation is stopped exactly when the cell is actually being edited (which is the only case that needs it, to avoid the click leaking to other handlers while an editor opens). The editTriggerEvent feature is untouched.
  • Editable cells are unchangedstopPropagation() still runs for them.
  • No double-firing: the column-level cellClick is invoked directly inside edit() only on the editable branch; the non-editable case fires once, via normal bubbling.

One thing worth flagging

Restoring propagation means it isn't only cellClick that fires for these cells — all click-driven consumers do, as they did pre-5.6: the external cellClick event, click-triggered menus (clickMenu) and popups (clickPopup), SelectRange click handling, and rowClick. This matches 5.5.x and is consistent with non-editor cells, but anyone who came to rely on the 5.6+ "editor cells swallow clicks" behavior would notice the difference. The alternative — dispatching cellClick explicitly for non-editable cells instead of restoring propagation — would diverge from pre-5.6 behavior and add complexity, so the current approach seems preferable.

Covered by a regression test in test/unit/modules/Edit.spec.js.


Generated by Claude Code

Comment thread test/unit/modules/Edit.spec.js Outdated
const ancestorClickHandler = jest.fn();
const ancestor = document.createElement("div");
document.body.appendChild(ancestor);
ancestor.appendChild(cellElement);

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is correct. We should just listen to the cell click event from the table instance.

…4421)

Per review feedback, test the actual cellClick event from the table instance
instead of a stand-in ancestor listener. Using a fixed height with basic
vertical rendering forces rows to lay out in the DOM under jsdom, so a real
click bubbles to the table-level listener and both the column cellClick
callback and the external cellClick event are observed.
@azmy60 azmy60 merged commit da438c2 into master Jun 10, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants