Skip to content

fix(selection): a selection edit may not delete a table cell boundary - #44

Merged
do-md merged 1 commit into
do-md:mainfrom
Systemofdumb:fix/table-cell-boundary
Sep 23, 2026
Merged

do-md merged 1 commit into
do-md:mainfrom
Systemofdumb:fix/table-cell-boundary

Conversation

@Systemofdumb

@Systemofdumb Systemofdumb commented Sep 16, 2026 •

Copy link
Copy Markdown
Contributor

Fixes the part of #42 that the triple-click takeover does not reach.

fcfbfed fixed the reported gesture, and I confirmed that on WebKit — a triple-click now selects "Test" with end = #text:4 and typing replaces the cell cleanly. But the corruption underneath it is still reachable from the keyboard: double-click a word in a cell, extend with Shift+Right or Shift+Down, type.

| Test | Test-2 |          typed over "Test" + the boundary
| --- | --- |         ->   | WrongTest-2 |
| Test-3 | Test-4 |        | Test-3 | Test-4 |

Row one is left with a single cell while row two still has two, so the document no longer round-trips as a grid, and undo cannot restore the structure. This is not a regression from your fix — it broke identically on v0.9.0 and v0.8.1; the gesture was one entry point to it.

Cause

A table is one top-level block whose cells are nested render blocks, so both endpoints of a cross-cell selection resolve to the same top-level index and land in applySelectEdit_'s single-block branch. That branch serializes the block and replaces everything between the two cursor markers — and in markdown that span contains the | between the cells.

Fix

The edit is clamped to the cell it starts in, via a small findEnclosingTableCell tree helper.

Scoped to cells deliberately: between paragraphs the same merge is the correct outcome, and that path is untouched. Clamping rather than editing every touched cell needs no multi-cell primitive — the failure it prevents is structural, while the cost is only that a second cell keeps text that was also highlighted. Document order decides which endpoint survives, so a right-to-left selection clamps identically.

If you would rather each touched cell were cleared, that is a bigger change and yours to choose; this one is aimed at making structure something an edit cannot destroy.

Verification

scripts/verify-table-cell-guard — 9 assertions against the real store: cross-cell replace, cross-row replace, cross-cell delete, a reversed pair, within-cell replace, and the cross-paragraph merge that must keep working.

With the clamp disabled, 5 of them fail, including the exact | WrongTest-2 | collapse — so it detects the regression rather than merely passing.

The harness sets the cursor pair directly because setSelection refuses a range straddling the separator (applied:false, reason:"invalid") — scaffolding offsets are not expressible as cursor coordinates. That is why only a DOM selection could reach this, and why it never surfaced headlessly.

Measured in WebKit against the built export as well:

dblclick + Shift+Right      broke=false  [["Wrong","Test-2"],["Test-3","Test-4"]]
dblclick + Shift+Right x2   broke=false  [["Wrong","Test-2"],["Test-3","Test-4"]]
dblclick + Shift+Down       broke=false  [["Wrong","Test-2"],["Test-3","Test-4"]]
triple click (#42)          cellCountChanged=false
Cmd+A in a table (#43)      cellsMissing=[none]  across all four document shapes

Existing suites unchanged: verify-block-format 188, verify-triple-click 23, verify-tab-store 15, verify-scroll-store 14, verify-tab-runtime 5, verify-ai-blocks 100, verify-ai-endpoint 29. App and kernel tsc clean.

Kernel change, so GPL-3.0. macOS/WebKit only here — no Windows or Linux machine.

@vercel

vercel Bot commented Sep 16, 2026

Copy link
Copy Markdown

@Systemofdumb is attempting to deploy a commit to the do-md's projects Team on Vercel.

A member of the Team first needs to authorize it.

do-md#42 was reported as a triple-click problem and fixed as one, but the
corruption underneath it is reachable from the keyboard too: double-click a
word in a cell, extend with Shift+Right or Shift+Down, type. The cell
separator goes with the replacement.

    | Test | Test-2 |          typed over "Test" + the boundary
    | --- | --- |         ->   | WrongTest-2 |
    | Test-3 | Test-4 |        | Test-3 | Test-4 |

Row one is left with a single cell while row two still has two, so the
document no longer round-trips as a grid — and undo cannot put the structure
back.

A table is ONE top-level block whose cells are nested render blocks, so both
endpoints of a cross-cell selection resolve to the same top-level index and
land in applySelectEdit_'s single-block branch. That branch serializes the
block and replaces everything between the two cursor markers; in markdown
that span contains the ` | ` between the cells.

The edit is now clamped to the cell it starts in. Scoped to cells
deliberately: between paragraphs the same merge is the CORRECT outcome, and
that path is untouched. Clamping rather than editing every touched cell
needs no multi-cell primitive — the failure it prevents is structural, while
the cost is only that a second cell keeps text that was also highlighted.

Document order decides which endpoint survives, so a selection made
right-to-left clamps identically.

Verified: new scripts/verify-table-cell-guard (9 assertions) drives the real
store — cross-cell replace, cross-row replace, cross-cell delete, a reversed
pair, within-cell replace, and the cross-paragraph merge that must keep
working. With the clamp disabled 5 of them fail, including the exact
`| WrongTest-2 |` collapse, so the harness detects the regression rather than
merely passing.

The cursor pair is set directly there because `setSelection` REFUSES a range
straddling the separator (applied:false, reason:"invalid") — scaffolding
offsets are not expressible as cursor coordinates, which is why only a DOM
selection could reach this and why it never showed up headlessly. Measured
in WebKit against the built export as well: dblclick+Shift+Right,
Shift+Right x2 and Shift+Down all keep the grid, while triple-click (do-md#42) and
Cmd+A in a table (do-md#43) stay fixed.

Existing suites unchanged: verify-block-format 188, verify-triple-click 23,
verify-tab-store 15, verify-scroll-store 14, verify-tab-runtime 5,
verify-ai-blocks 100, verify-ai-endpoint 29; app and kernel tsc clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@Systemofdumb
Systemofdumb force-pushed the fix/table-cell-boundary branch from 177b3f1 to c29c0c3 Compare September 16, 2026 18:56
@do-md
do-md merged commit 8a97e28 into do-md:main Sep 23, 2026
1 of 2 checks passed
@github-actions github-actions Bot locked and limited conversation to collaborators Sep 23, 2026
@do-md

do-md commented Sep 23, 2026

Copy link
Copy Markdown
Owner

Merged — thank you. The analysis was exactly right: the single-block branch cannot tell a cell wall from ordinary text once the table is serialized, and clamping at that boundary is the correct structural guarantee. The negative control in the harness (5 of 9 failing with the clamp disabled) made this easy to trust.

Two small adjustments applied on our side after the merge:

  • clampToOneTableCell_ now returns early unless the block is MarkdownType.Table, so the two subtree walks are not paid on every plain-paragraph selection edit (that branch is on the hot path of every single-block edit).
  • a.cell === b.cell became a uuid_ comparison — object identity happens to hold under the draft proxies today, but the uuid states the intent without depending on proxy caching.

On your open question — clearing every touched cell instead of clamping: agreed that it is the better end state, and also that it needs a real multi-cell edit primitive rather than more clamping. Tracked as a follow-up on our side; the clamp stays as the structural floor either way.

One note for anyone running scripts/verify-table-cell-guard locally: the harness resolves the kernel through .packages/@do-md/core/dist, so it tests stale code unless you npm run build there first — with an outdated dist the suite reports 5 failures that look like the bug, but are actually the pre-fix kernel.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants