fix(selection): table cell and multiline selection rendering (SD-3328)#3688
fix(selection): table cell and multiline selection rendering (SD-3328)#3688tupizz wants to merge 6 commits into
Conversation
…sition (SD-3328) An empty paragraph (blank line / spacer between bullets) in a table cell has no runs and no measured lines, so the table hit-test fell through to a null result. A null hit aborts an in-progress drag (EditorInputManager #handleDragSelectionAt early-return), freezing/collapsing the selection while the pointer is over the blank line. Derive the paragraph's PM start from its attrs so an empty cell paragraph always resolves to a valid position, never null. Also emit a content-width highlight band for blank lines on the selectionToRects geometry path (fallback parity). Note: the live selection overlay uses the DOM Range path (DomSelectionGeometry.getClientRects), so the blank-line band and the interior-line render race on that path are addressed separately.
A multi-line selection built one DOM Range spanning every line and trusted range.getClientRects(). Each .superdoc-line is absolutely positioned, and some Chrome builds return incomplete rects for a range crossing those positioned boxes: interior lines collapse to a few stray slivers while the first and last lines render fully, leaving the middle of the selection visually unhighlighted. intersectsNode does not flag it because the entries still report as intersected. Compute rects per line when a selection spans more than one line. Interior lines use the line element's own bounding box (reliable across browsers and the full-width highlight normal selection shows); the first and last lines keep the precise range so they respect the selection offset and first-line indent. Single-line selections are unchanged.
Dragging a body text selection into (or through) a table collapsed the whole selection. prosemirror-tables' normalizeSelection rewrites a TextSelection to the anchor block's own bounds when its endpoints resolve to different cells and the head sits at the start of its block (parentOffset 0). An empty paragraph inside a cell is always at parentOffset 0, so dragging the head onto one rewrote e.g. [44, 2026] to [44, 49] on dispatch, collapsing the selection to the first run. Add selectionCollapsesAcrossTableCells to detect that exact frame (mirroring the upstream condition) and skip dispatching the doomed selection in the drag handler, preserving the last good selection. The selection resumes extending as soon as the head moves into cell text. The detector fails open on any unexpected document shape so it can never block a normal selection.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 88450fadc3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Dragging a selection from one table cell into another (notably backward, or
starting on an empty cell paragraph) broke: prosemirror-tables will not let a
text selection span cells, so it collapsed the selection, and the empty-para
guard then froze it. Cross-cell drags should be a cell-range selection, the way
Word and Google Docs behave.
Detect cross-cell drags from the resolved PM positions (reliable) rather than the
geometry hit-test (which misses empty cell paragraphs and leaves the cell anchor
unset). When the anchor and head land in different cells of the same table,
dispatch a CellSelection; within a single cell stays a text selection, and a
body-into-table drag keeps the empty-paragraph guard.
Also fix the cell-selection overlay so it renders without a geometry cell anchor:
it now locates the table block by document order instead of guessing an id like
`${tableStart}-table`, which never matched the real sequential ids.
Adds unit tests for the cross-cell resolvers (both directions, the empty-cell
case, and the same-cell / body-to-cell / different-table null cases).
…disappears-over-empty-space
…disappears-over-empty-space
| const docNode = $anchorCell.node(0); | ||
| let selectedTableIndex = -1; | ||
| let seenTables = 0; | ||
| docNode.descendants((node, pos) => { | ||
| if (node.type.name !== 'table') return true; | ||
| if (pos === tableStart) { | ||
| selectedTableIndex = seenTables; | ||
| return false; | ||
| } | ||
| seenTables += 1; | ||
| return true; | ||
| }); | ||
| if (selectedTableIndex !== -1) { | ||
| const tableBlocks = blocks.filter((block) => block.kind === 'table') as TableBlock[]; | ||
| tableBlock = tableBlocks[selectedTableIndex]; |
There was a problem hiding this comment.
the descendants walk counts every table including nested ones (returning true descends into the node's children), but blocks.filter(kind === 'table') only holds top-level tables — nested tables live under their parent cell's blocks. so a nested table earlier in the doc desyncs selectedTableIndex and you index the wrong block, or get undefined and the overlay silently disappears. returning false after counting a table would keep both counts top-level.
also, this is the third copy of the "Nth table in doc order ↔ Nth table block" mapping (getCellPosFromTableHit and getTablePosFromHit do the inverse) — might be worth extracting one shared helper in TableSelectionUtilities, which would also be a natural single place for the nested-table fix. and since the existing overlay tests always pass a matching cellAnchorTableBlockId, this fallback path never runs in any test — a two-table doc with cellAnchorTableBlockId: null would cover it.
| /** | ||
| * SD-3328: an empty paragraph / blank line that the selection passes through is a | ||
| * zero-width slice (`pmStart === pmEnd`). `findLinesIntersectingRange` only yields | ||
| * such a line when `from < pos < to`, so it is genuinely spanned and must be | ||
| * highlighted. Emit a content-width band so the selection highlight stays | ||
| * continuous across the blank line — the same as selecting any text. Without this | ||
| * the band shows a gap and the highlight appears to "disappear" while a drag | ||
| * crosses a blank line (reported in body paragraphs and inside table cells). | ||
| */ | ||
| function pushEmptyLineSelectionBand( | ||
| rects: Rect[], | ||
| opts: { x: number; y: number; width: number; height: number; pageIndex: number }, | ||
| ): void { | ||
| rects.push({ | ||
| x: opts.x, | ||
| y: opts.y, | ||
| width: Math.max(1, opts.width), | ||
| height: opts.height, | ||
| pageIndex: opts.pageIndex, | ||
| }); | ||
| } | ||
|
|
There was a problem hiding this comment.
pushEmptyLineSelectionBand got inserted between the selectionToRects jsdoc and its signature, so the @param block is now orphaned from the exported function. moving the helper (and its doc) above the selectionToRects doc block fixes it.
while you're there: the helper only clamps width and pushes, but the emptyLineOffset math — the part most likely to drift — is copy-pasted at both call sites. consider moving that into the helper (pass measure/index/startLine), or just inlining the push.
| export function resolveCellContext( | ||
| doc: ProseMirrorNode | null, | ||
| pos: number, | ||
| ): { cellPos: number; tablePos: number } | null { | ||
| if (!doc || !Number.isFinite(pos) || pos < 0 || pos > doc.content.size) return null; | ||
|
|
||
| let $pos: ReturnType<ProseMirrorNode['resolve']>; | ||
| try { | ||
| $pos = doc.resolve(pos); | ||
| } catch { | ||
| return null; | ||
| } | ||
|
|
||
| let cellDepth = -1; | ||
| let tableDepth = -1; | ||
| for (let depth = $pos.depth; depth > 0; depth--) { | ||
| const role = $pos.node(depth).type.spec.tableRole; | ||
| if (cellDepth === -1 && (role === 'cell' || role === 'header_cell')) { | ||
| cellDepth = depth; | ||
| } | ||
| if (cellDepth !== -1 && role === 'table') { | ||
| tableDepth = depth; | ||
| break; | ||
| } | ||
| } | ||
| if (cellDepth === -1 || tableDepth === -1) return null; | ||
|
|
||
| return { cellPos: $pos.before(cellDepth), tablePos: $pos.before(tableDepth) }; | ||
| } | ||
|
|
||
| /** | ||
| * Determines whether a drag from `anchorPos` to `headPos` should be a cross-cell selection. | ||
| * | ||
| * SD-3328: The geometry trigger (`hitTestTable`) can miss empty cell paragraphs, leaving the cell | ||
| * anchor unset, so a drag across cells falls back to a plain text selection — which prosemirror- | ||
| * tables then collapses. Deriving the cells from the resolved PM positions is reliable. When both | ||
| * endpoints resolve to *different* cells of the *same* table, the caller should build a | ||
| * `CellSelection` (the cell-range highlight Word and Docs show) instead of a text selection. | ||
| * | ||
| * @returns Cell positions for `CellSelection.create`, or null when this is not a cross-cell drag | ||
| * (one endpoint outside any table, both in the same cell, or in different tables). | ||
| */ | ||
| export function resolveCrossCellSelection( | ||
| doc: ProseMirrorNode | null, | ||
| anchorPos: number, | ||
| headPos: number, | ||
| ): { anchorCellPos: number; headCellPos: number } | null { | ||
| const anchor = resolveCellContext(doc, anchorPos); | ||
| const head = resolveCellContext(doc, headPos); | ||
| if (!anchor || !head) return null; | ||
| if (anchor.tablePos !== head.tablePos) return null; // different tables | ||
| if (anchor.cellPos === head.cellPos) return null; // same cell -> text selection | ||
| return { anchorCellPos: anchor.cellPos, headCellPos: head.cellPos }; | ||
| } | ||
|
|
There was a problem hiding this comment.
resolveCellContext is a hand-rolled version of cellAround($pos) from prosemirror-tables, and the same-table check duplicates its inSameTable — both are public exports, and cellAround/cellWrapping are already vendored in extensions/table/tableHelpers/. building on those would drop ~40 lines of traversal. same goes for the inline cellAncestor closure in SelectionHelpers.ts:204 (that one is cellWrapping). worth reusing, or was there a reason to avoid the prosemirror-tables helpers here?
| function dispatchKeepsHead(anchor, target) { | ||
| const doc = editor.state.doc; | ||
| const applied = editor.state.apply(editor.state.tr.setSelection(TextSelection.create(doc, anchor, target))); | ||
| return applied.selection.to === target; | ||
| } |
There was a problem hiding this comment.
dispatchKeepsHead compares selection.to to the target, which only holds for forward selections. all current cases are forward so this is fine today, but a backward-drag case added later would report a false collapse — a Math.max(anchor, target) or a short comment pinning the forward-only assumption would prevent that.
|
hey @tupizz! nice work here, the selections are looking a lot better! codex found this issue below and I can reproduce it in the browser. I can see that empty lines don't generate any selection rectangles and so you can't really tell that they are selected.
also, claude found a few more things that I added as inline comments. |
4 participants
What
Fixes SD-3328. The selection highlight disappeared over empty space inside table cells. While tracking that down I found two more bugs on the same selection rendering path, so this PR groups the three related fixes.
Demo
CleanShot.2026-06-09.at.12.43.07.mp4
Background
Presentation mode draws its own selection overlay. It does not use the browser selection. The overlay rects come from
computeSelectionRectsFromDom, which maps the ProseMirror selection to the painted DOM and readsgetClientRects().1. Dragging into an empty paragraph in a table collapsed the selection (the reported bug)
prosemirror-tables normalizes a text selection whose endpoints resolve to different cells when the head sits at the start of its block (
parentOffset === 0). An empty paragraph in a cell only ever has offset 0, so dragging a body selection onto one rewrote[44, 2026]to[44, 49]on dispatch. Text positions in a cell escape because their offset is greater than 0, which is why dragging through cell text worked but an empty paragraph killed it.Fix: a detector mirrors that exact condition. When a drag frame would collapse, the handler keeps the last good selection and resumes extending as soon as the head moves into cell text. The detector fails open on any unexpected document shape so it can never block a normal selection.
2. Interior lines of a multiline selection rendered blank
A multiline selection built one DOM Range across every line and trusted
getClientRects(). Each.superdoc-lineis absolutely positioned, and some Chrome builds return incomplete rects for a Range crossing those boxes: the first and last lines render, the interior lines collapse to a few slivers.intersectsNodedoes not catch it because the entries still report as intersected.Fix: when a selection spans more than one line, compute rects per line. Interior lines use the line element box, which is reliable across browsers and gives the full width highlight normal selection shows. The first and last lines keep the precise Range so they respect the selection offset and first line indent.
3. Empty cell paragraphs resolved to no hit position
The geometry hit test returned null for an empty cell paragraph (no runs), which aborted the active drag and froze the selection. Now it falls back to the paragraph PM start so every empty cell resolves to a valid position.
Testing