diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index e9a4976365..dd246b3648 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -582,6 +582,28 @@ const sumLineHeights = (measure: ParagraphMeasure, fromLine: number, toLine: num * @param geometryHelper - Optional PageGeometryHelper for accurate Y calculations (recommended) * @returns Array of selection rectangles in container space */ +/** + * 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, + }); +} + export function selectionToRects( layout: Layout, blocks: FlowBlock[], @@ -623,7 +645,19 @@ export function selectionToRects( if (range.pmStart == null || range.pmEnd == null) return; const sliceFrom = Math.max(range.pmStart, from); const sliceTo = Math.min(range.pmEnd, to); - if (sliceFrom >= sliceTo) return; + if (sliceFrom >= sliceTo) { + // SD-3328: blank line spanned by the selection — see pushEmptyLineSelectionBand. + const emptyLineOffset = + lineHeightBeforeIndex(measure, index) - lineHeightBeforeIndex(measure, fragment.fromLine); + pushEmptyLineSelectionBand(rects, { + x: fragment.x, + y: fragment.y + emptyLineOffset + pageTopY, + width: fragment.width, + height: line.lineHeight, + pageIndex, + }); + return; + } // Convert PM positions to character offsets properly // (accounts for gaps in PM positions between runs) @@ -963,7 +997,26 @@ export function selectionToRects( if (range.pmStart == null || range.pmEnd == null) return; const sliceFrom = Math.max(range.pmStart, from); const sliceTo = Math.min(range.pmEnd, to); - if (sliceFrom >= sliceTo) return; + if (sliceFrom >= sliceTo) { + // SD-3328: blank line spanned by the selection — see pushEmptyLineSelectionBand. + const emptyLineOffset = + lineHeightBeforeIndex(info.measure, index) - lineHeightBeforeIndex(info.measure, info.startLine); + pushEmptyLineSelectionBand(rects, { + x: fragment.x + contentOffsetX + cellX + padding.left, + y: + fragment.y + + contentOffsetY + + rowOffset + + blockTopCursor + + effectiveSpacingBeforePx + + emptyLineOffset + + pageTopY, + width: cellMeasure.width - padding.left - padding.right, + height: line.lineHeight, + pageIndex, + }); + return; + } const charOffsetFrom = pmPosToCharOffset(info.block, line, sliceFrom); const charOffsetTo = pmPosToCharOffset(info.block, line, sliceTo); diff --git a/packages/layout-engine/layout-bridge/src/position-hit.ts b/packages/layout-engine/layout-bridge/src/position-hit.ts index 3ed345e8ea..283ee60362 100644 --- a/packages/layout-engine/layout-bridge/src/position-hit.ts +++ b/packages/layout-engine/layout-bridge/src/position-hit.ts @@ -981,11 +981,18 @@ export function clickToPositionGeometry( } } - // Fallback: return first position in the cell + // Fallback: return first position in the cell. + // SD-3328: an EMPTY paragraph (blank line / spacer between bullets) has no runs, + // so `firstRun` is undefined and the old code fell through to `return null`. A null + // hit aborts the drag (EditorInputManager #handleDragSelectionAt early-return) and + // freezes/collapses the in-progress selection while the pointer is over the blank + // line. Derive the paragraph's own PM start from its attrs so an empty cell paragraph + // always resolves to a valid forward position inside the cell, never null. const firstRun = cellBlock.runs?.[0]; - if (firstRun && firstRun.pmStart != null) { + const fallbackPos = firstRun?.pmStart ?? blockPmRangeFromAttrs(cellBlock).pmStart; + if (fallbackPos != null) { return { - pos: firstRun.pmStart, + pos: fallbackPos, layoutEpoch, blockId: tableHit.fragment.blockId, pageIndex, diff --git a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts index 020f11385d..ac24b4a63e 100644 --- a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts +++ b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts @@ -6,6 +6,7 @@ import type { Measure, Line, ParaFragment, + ParagraphAttrs, TableBlock, TableMeasure, TableFragment, @@ -143,6 +144,68 @@ describe('clickToPosition', () => { expect(result?.column).toBe(1); }); + it('resolves an EMPTY cell paragraph to its PM position instead of null (SD-3328)', () => { + // An empty paragraph (blank line / spacer) inside a table cell has no runs and no + // measured lines, so findLineIndexAtY returns null. Before the fix the cell fallback + // (`cellBlock.runs[0].pmStart`) was undefined and the hit resolved to null — which + // aborts an in-progress drag and collapses the selection. The hit must resolve to the + // empty paragraph's own PM position (from its attrs) so dragging across a blank line + // keeps extending the selection. + const emptyCellPara: FlowBlock = { + kind: 'paragraph', + id: 'empty-cell-para', + runs: [], + attrs: { pmStart: 100, pmEnd: 101 } as unknown as ParagraphAttrs, + }; + + const tableBlock: TableBlock = { + kind: 'table', + id: 'empty-cell-table', + rows: [{ id: 'row-0', cells: [{ id: 'cell-0-0', blocks: [emptyCellPara] }] }], + }; + + const emptyCellMeasure: Measure = { kind: 'paragraph', lines: [], totalHeight: 20 }; + + const tableMeasure: TableMeasure = { + kind: 'table', + rows: [ + { + cells: [ + { blocks: [emptyCellMeasure], paragraph: emptyCellMeasure, width: 320, height: 28, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }, + ], + height: 28, + }, + ], + columnWidths: [320], + totalWidth: 320, + totalHeight: 28, + }; + + const tableFragment: TableFragment = { + kind: 'table', + blockId: 'empty-cell-table', + fromRow: 0, + toRow: 1, + x: 30, + y: 40, + width: 320, + height: 28, + pmStart: 100, + pmEnd: 101, + }; + + const layout: Layout = { + pageSize: { w: 600, h: 800 }, + pages: [{ number: 1, margins: { top: 0, right: 0, bottom: 0, left: 0 }, fragments: [tableFragment] }], + }; + + const result = clickToPosition(layout, [tableBlock], [tableMeasure], { x: 120, y: 54 }); + + expect(result).not.toBeNull(); + expect(result?.pos).toBe(100); + expect(result?.blockId).toBe('empty-cell-table'); + }); + it('falls back to visual x when a table fragment has no columnIndex', () => { // Legacy fragments without columnIndex should still resolve a column via fragment.x. const cellParagraph: FlowBlock = { diff --git a/packages/layout-engine/layout-bridge/test/mock-data.ts b/packages/layout-engine/layout-bridge/test/mock-data.ts index d6dfde3fb3..5537dbb739 100644 --- a/packages/layout-engine/layout-bridge/test/mock-data.ts +++ b/packages/layout-engine/layout-bridge/test/mock-data.ts @@ -275,6 +275,101 @@ export const tableLayout: Layout = { ], }; +// Table cell with an EMPTY paragraph between two text paragraphs (SD-3328). +// PM layout: p1 "Table text" [2,12), empty para inside pos 14, p3 "More text" [16,26). +// A selection from 2..26 passes through all three lines; the empty line is a +// zero-width slice (pmStart === pmEnd === 14) that the rect builder used to skip. +const tableEmptyParaLineP1 = { fromRun: 0, fromChar: 0, toRun: 0, toChar: 10, width: 80, ascent: 10, descent: 4, lineHeight: TABLE_CELL_LINE_HEIGHT } as const; +const tableEmptyParaLineEmpty = { fromRun: 0, fromChar: 0, toRun: 0, toChar: 0, width: 0, ascent: 10, descent: 4, lineHeight: TABLE_CELL_LINE_HEIGHT } as const; +const tableEmptyParaLineP3 = { fromRun: 0, fromChar: 0, toRun: 0, toChar: 9, width: 70, ascent: 10, descent: 4, lineHeight: TABLE_CELL_LINE_HEIGHT } as const; + +export const tableEmptyParaBlock: FlowBlock = { + kind: 'table', + id: 'table-empty-para', + rows: [ + { + id: 'row-0', + cells: [ + { + id: 'cell-0', + attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } }, + blocks: [ + { kind: 'paragraph', id: 'p1', runs: [{ text: 'Table text', fontFamily: 'Arial', fontSize: 14, pmStart: 2, pmEnd: 12 }] }, + { kind: 'paragraph', id: 'p-empty', runs: [{ text: '', fontFamily: 'Arial', fontSize: 14, pmStart: 14, pmEnd: 14 }] }, + { kind: 'paragraph', id: 'p3', runs: [{ text: 'More text', fontFamily: 'Arial', fontSize: 14, pmStart: 16, pmEnd: 26 }] }, + ], + }, + ], + }, + ], +}; + +export const tableEmptyParaMeasure: Measure = { + kind: 'table', + rows: [ + { + height: TABLE_CELL_LINE_HEIGHT * 3 + 4, + cells: [ + { + width: 120, + height: TABLE_CELL_LINE_HEIGHT * 3 + 4, + gridColumnStart: 0, + blocks: [ + { kind: 'paragraph', lines: [tableEmptyParaLineP1], totalHeight: TABLE_CELL_LINE_HEIGHT }, + { kind: 'paragraph', lines: [tableEmptyParaLineEmpty], totalHeight: TABLE_CELL_LINE_HEIGHT }, + { kind: 'paragraph', lines: [tableEmptyParaLineP3], totalHeight: TABLE_CELL_LINE_HEIGHT }, + ], + }, + ], + }, + ], + columnWidths: [120], + totalWidth: 120, + totalHeight: TABLE_CELL_LINE_HEIGHT * 3 + 4, +}; + +export const tableEmptyParaLayout: Layout = { + pageSize: { w: 400, h: 500 }, + pages: [ + { + number: 1, + fragments: [ + { kind: 'table' as const, blockId: 'table-empty-para', fromRow: 0, toRow: 1, x: 30, y: 60, width: 120, height: TABLE_CELL_LINE_HEIGHT * 3 + 4 }, + ], + }, + ], +}; + +// Body paragraphs with an EMPTY paragraph between two text paragraphs (SD-3328). +// p1 "First line" [1,11), empty paragraph inside pos 13, p3 "Third line" [15,25). +// A selection 1..25 passes through all three; the empty line is a zero-width slice +// that the body rect builder used to skip, leaving a gap in the highlight band. +export const bodyEmptyParaBlocks: FlowBlock[] = [ + { kind: 'paragraph', id: 'body-p1', runs: [{ text: 'First line', fontFamily: 'Arial', fontSize: 16, pmStart: 1, pmEnd: 11 }] }, + { kind: 'paragraph', id: 'body-empty', runs: [{ text: '', fontFamily: 'Arial', fontSize: 16, pmStart: 13, pmEnd: 13 }] }, + { kind: 'paragraph', id: 'body-p3', runs: [{ text: 'Third line', fontFamily: 'Arial', fontSize: 16, pmStart: 15, pmEnd: 25 }] }, +]; + +export const bodyEmptyParaMeasures: Measure[] = [ + { kind: 'paragraph', lines: [{ fromRun: 0, fromChar: 0, toRun: 0, toChar: 10, width: 80, ascent: 12, descent: 4, lineHeight: 20 }], totalHeight: 20 }, + { kind: 'paragraph', lines: [{ fromRun: 0, fromChar: 0, toRun: 0, toChar: 0, width: 0, ascent: 12, descent: 4, lineHeight: 20 }], totalHeight: 20 }, + { kind: 'paragraph', lines: [{ fromRun: 0, fromChar: 0, toRun: 0, toChar: 10, width: 80, ascent: 12, descent: 4, lineHeight: 20 }], totalHeight: 20 }, +]; + +export const bodyEmptyParaLayout: Layout = { + pageSize: { w: 400, h: 500 }, + pages: [ + { + number: 1, + fragments: [ + { kind: 'para', blockId: 'body-p1', fromLine: 0, toLine: 1, x: 30, y: 40, width: 300, pmStart: 1, pmEnd: 11 }, + { kind: 'para', blockId: 'body-empty', fromLine: 0, toLine: 1, x: 30, y: 60, width: 300, pmStart: 13, pmEnd: 13 }, + { kind: 'para', blockId: 'body-p3', fromLine: 0, toLine: 1, x: 30, y: 80, width: 300, pmStart: 15, pmEnd: 25 }, + ], + }, + ], +}; + // Table cell spacing.before — selectionToRects tests (effective spacing, absorption, partial row) export const TABLE_SPACING_BEFORE = 12; export const TABLE_SPACING_FRAGMENT_Y = 50; diff --git a/packages/layout-engine/layout-bridge/test/selectionToRects.test.ts b/packages/layout-engine/layout-bridge/test/selectionToRects.test.ts index cfce735914..0d90b212dd 100644 --- a/packages/layout-engine/layout-bridge/test/selectionToRects.test.ts +++ b/packages/layout-engine/layout-bridge/test/selectionToRects.test.ts @@ -17,6 +17,12 @@ import { tableLayout, tableBlock, tableMeasure, + tableEmptyParaLayout, + tableEmptyParaBlock, + tableEmptyParaMeasure, + bodyEmptyParaLayout, + bodyEmptyParaBlocks, + bodyEmptyParaMeasures, tableSpacingBeforeBlock, tableSpacingBeforeMeasure, tableSpacingBeforeLayout, @@ -74,6 +80,37 @@ describe('selectionToRects', () => { expect(rects[0].x).toBeGreaterThan(tableLayout.pages[0].fragments[0].x); }); + it('highlights an empty paragraph row spanned by a table-cell selection (SD-3328)', () => { + // Selection 2..26 passes through p1 -> empty paragraph -> p3. The empty line is a + // zero-width slice; before the fix the builder skipped it, leaving a gap in the + // highlight band (the reported "selection highlight disappears over empty space"). + const rects = selectionToRects(tableEmptyParaLayout, [tableEmptyParaBlock], [tableEmptyParaMeasure], 2, 26); + + // One rect per line: text, empty, text — the empty row must NOT be dropped. + expect(rects).toHaveLength(3); + + // The middle (empty) row sits vertically between the two text rows and is visible. + const sorted = [...rects].sort((a, b) => a.y - b.y); + const emptyRect = sorted[1]; + expect(emptyRect.y).toBeGreaterThan(sorted[0].y); + expect(emptyRect.y).toBeLessThan(sorted[2].y); + expect(emptyRect.width).toBeGreaterThan(1); + }); + + it('highlights a blank line between body paragraphs spanned by a selection (SD-3328)', () => { + // Selection 1..25 passes through p1 -> empty paragraph -> p3 in body text. + // The blank line must be highlighted just like inside a table cell. + const rects = selectionToRects(bodyEmptyParaLayout, bodyEmptyParaBlocks, bodyEmptyParaMeasures, 1, 25); + + expect(rects).toHaveLength(3); + + const sorted = [...rects].sort((a, b) => a.y - b.y); + const emptyRect = sorted[1]; + expect(emptyRect.y).toBeGreaterThan(sorted[0].y); + expect(emptyRect.y).toBeLessThan(sorted[2].y); + expect(emptyRect.width).toBeGreaterThan(1); + }); + it('accounts for visual-only prefix runs when mapping PM selections to X coordinates', () => { const blockWithoutMarker: FlowBlock = { kind: 'paragraph', diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts index b22efb1fd4..db8d3c3072 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts @@ -31,12 +31,13 @@ import { getFirstTextPosition as getFirstTextPositionFromHelper, registerPointerClick as registerPointerClickFromHelper, } from '../input/ClickSelectionUtilities.js'; -import { calculateExtendedSelection } from '../selection/SelectionHelpers.js'; +import { calculateExtendedSelection, selectionCollapsesAcrossTableCells } from '../selection/SelectionHelpers.js'; import { shouldUseCellSelection as shouldUseCellSelectionFromHelper, getCellPosFromTableHit as getCellPosFromTableHitFromHelper, getTablePosFromHit as getTablePosFromHitFromHelper, hitTestTable as hitTestTableFromHelper, + resolveCrossCellSelection, } from '../tables/TableSelectionUtilities.js'; import { debugLog } from '../selection/SelectionDebug.js'; import { DOM_CLASS_NAMES, buildAnnotationSelector, DRAGGABLE_SELECTOR } from '@superdoc/dom-contract'; @@ -2592,12 +2593,44 @@ export class EditorInputManager { return; } - // Text selection mode const anchor = this.#dragAnchor!; const head = hit.pos; + // SD-3328: Cross-cell selection from the resolved PM positions. The geometry trigger above + // (#hitTestTable) can miss empty cell paragraphs and similar spots, leaving #cellAnchor unset, + // so a drag across cells falls to the text path — where prosemirror-tables collapses it + // (forward) or the guard freezes it (backward). Deriving the cells from the resolved positions + // is reliable: when the anchor and head land in different cells of the same table, select the + // cell range directly, the way Word and Google Docs do, regardless of the flaky geometry hit. + if (!useActiveSurfaceHitTest) { + const crossCell = resolveCrossCellSelection(editor.state.doc, anchor, head); + if (crossCell) { + try { + const tr = editor.state.tr.setSelection( + CellSelection.create(editor.state.doc, crossCell.anchorCellPos, crossCell.headCellPos), + ); + editor.view?.dispatch(tr); + this.#callbacks.scheduleSelectionUpdate?.(); + return; + } catch (error) { + // Fall through to text selection if the cell range cannot be built. + console.warn('[SELECTION] Failed to create cross-cell CellSelection during drag:', error); + } + } + } + + // Text selection mode const { selAnchor, selHead } = this.#calculateExtendedSelection(anchor, head, this.#dragExtensionMode); + // SD-3328: When dragging a body selection into (or through) a table, prosemirror-tables' + // normalization collapses a TextSelection whose head lands at the start of a cell block + // (an empty cell paragraph is always at parentOffset 0), rewriting e.g. [44, 2026] to + // [44, 49]. Detect that frame and keep the last good selection instead of dispatching a + // doomed one — the selection resumes extending as soon as the head moves into cell text. + if (selectionCollapsesAcrossTableCells(editor.state.doc, selAnchor, selHead)) { + return; + } + try { const tr = editor.state.tr.setSelection(TextSelection.create(editor.state.doc, selAnchor, selHead)); editor.view?.dispatch(tr); diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/selection/CellSelectionOverlay.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/CellSelectionOverlay.ts index 6d24921c2e..d84f85c5b2 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/selection/CellSelectionOverlay.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/CellSelectionOverlay.ts @@ -120,15 +120,28 @@ export function renderCellSelectionOverlay({ | undefined; } if (!tableBlock) { - const expectedBlockId = `${tableStart}-table`; - tableBlock = blocks.find((block) => block.kind === 'table' && block.id === expectedBlockId) as - | TableBlock - | undefined; - } - if (!tableBlock) { - const tableBlocks = blocks.filter((block) => block.kind === 'table') as TableBlock[]; - if (tableBlocks.length === 1) { - tableBlock = tableBlocks[0]; + // SD-3328: Map the selection's table to its layout block by document order. Table block IDs + // are sequential ("26-table"), not derived from the PM position, so matching a guessed + // `${tableStart}-table` id never works and the single-table fallback can't disambiguate a + // document with several tables. Counting tables up to the selection's table is reliable and + // mirrors how cell positions are resolved elsewhere (getCellPosFromTableHit). This lets a + // cell selection render even when no geometry cell-anchor was captured — e.g. a drag that + // started on an empty cell paragraph, where the geometry hit-test misses. + 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]; } } if (!tableBlock) { diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/selection/SelectionHelpers.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/SelectionHelpers.ts index 9f8ad0ce3d..332f7cc7b3 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/selection/SelectionHelpers.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/SelectionHelpers.ts @@ -1,3 +1,4 @@ +import type { Node as ProseMirrorNode } from 'prosemirror-model'; import type { FlowBlock } from '@superdoc/contracts'; import { findParagraphBoundaries, findWordBoundaries } from '@superdoc/layout-bridge'; @@ -166,3 +167,52 @@ export function calculateExtendedSelection( // Fallback to character mode (no extension) if boundaries not found or mode is 'char' return { selAnchor: anchor, selHead: head }; } + +/** + * Detects when extending a text selection from `anchor` to `head` would be collapsed by + * prosemirror-tables' selection normalization. + * + * prosemirror-tables (`normalizeSelection` -> `isTextSelectionAcrossCells`) rewrites a + * TextSelection to the anchor block's own bounds when the two endpoints resolve to different + * table cells AND the head sits at the very start of its block (`parentOffset === 0`). An + * empty paragraph inside a cell only ever has `parentOffset === 0`, so dragging a body + * selection into (or through) an empty cell paragraph collapses the whole selection back to + * the run at the anchor — e.g. `[44, 2026]` becomes `[44, 49]`. (SD-3328.) + * + * Returns true when extending to `head` would trigger that collapse, so a drag handler can + * keep the last good selection for that frame instead of dispatching a doomed one. Mirrors + * the upstream condition exactly; the regression test pins it against real editor behavior. + * + * @param doc - The current ProseMirror document. + * @param anchor - The selection anchor position. + * @param head - The prospective selection head position. + * @returns True if dispatching `TextSelection(anchor, head)` would be normalized to a collapse. + */ +export function selectionCollapsesAcrossTableCells(doc: ProseMirrorNode, anchor: number, head: number): boolean { + if (anchor === head) return false; + + // Fail open: if the document shape is unexpected or a position is out of range, never block + // the selection — extending it is the safe default. This also keeps the guard a no-op for + // callers that pass a minimal doc stub. + try { + const size = doc.content.size; + if (anchor < 0 || head < 0 || anchor > size || head > size) return false; + + const $from = doc.resolve(Math.min(anchor, head)); + const $to = doc.resolve(Math.max(anchor, head)); + + const cellAncestor = (pos: typeof $from): ProseMirrorNode | null => { + for (let depth = pos.depth; depth > 0; depth--) { + const role = pos.node(depth).type.spec.tableRole; + if (role === 'cell' || role === 'header_cell') return pos.node(depth); + } + return null; + }; + + // Mirrors prosemirror-tables `isTextSelectionAcrossCells`: endpoints in different cells + // (or one outside the table entirely) with the head at the start of its block. + return cellAncestor($from) !== cellAncestor($to) && $to.parentOffset === 0; + } catch { + return false; + } +} diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/selection/table-cell-selection-collapse.test.js b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/table-cell-selection-collapse.test.js new file mode 100644 index 0000000000..80db3fdc9d --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/selection/table-cell-selection-collapse.test.js @@ -0,0 +1,125 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { TextSelection } from 'prosemirror-state'; + +import { initTestEditor } from '@tests/helpers/helpers.js'; + +import { selectionCollapsesAcrossTableCells } from './SelectionHelpers.js'; + +/** + * SD-3328: Dragging a body selection into (or through) a table collapses the selection when + * the head lands at the start of a cell block. prosemirror-tables' `normalizeSelection` + * rewrites such a TextSelection to the anchor block's own bounds. An empty paragraph in a + * cell is always at `parentOffset === 0`, so it always triggers the collapse, while text in a + * cell (parentOffset > 0) does not. `selectionCollapsesAcrossTableCells` detects exactly the + * frames that would collapse so the drag handler can preserve the last good selection. + * + * These tests pin the detector against the REAL table plugin: the helper must return `true` + * exactly when a dispatched selection actually collapses. + */ +const DOC = { + type: 'doc', + content: [ + { + type: 'paragraph', + content: [{ type: 'run', content: [{ type: 'text', text: 'Body paragraph before the table' }] }], + }, + { + type: 'table', + attrs: { tableProperties: {}, grid: [{ col: 1500 }] }, + content: [ + { + type: 'tableRow', + content: [ + { + type: 'tableCell', + attrs: { colspan: 1, rowspan: 1, colwidth: [150] }, + content: [ + { + type: 'paragraph', + content: [{ type: 'run', content: [{ type: 'text', text: 'Cell paragraph text' }] }], + }, + // Empty paragraph inside the cell — the position that triggers the collapse. + { type: 'paragraph' }, + ], + }, + ], + }, + ], + }, + ], +}; + +describe('selectionCollapsesAcrossTableCells (SD-3328)', () => { + let editor; + + beforeEach(() => { + ({ editor } = initTestEditor({ loadFromSchema: true, content: DOC })); + }); + + /** Resolve representative positions in the built document. */ + function positions() { + const doc = editor.state.doc; + let bodyPos = null; + let cellTextPos = null; + let emptyCellParaPos = null; + + doc.descendants((node, pos) => { + if (node.isText && bodyPos === null && node.text.includes('Body paragraph')) { + bodyPos = pos + 5; // mid body text + } + if (node.isText && cellTextPos === null && node.text.includes('Cell paragraph')) { + cellTextPos = pos + 5; // mid cell text + } + }); + + doc.descendants((node, pos) => { + if (emptyCellParaPos !== null) return false; + if (node.type.name === 'paragraph' && node.content.size === 0) { + const $pos = doc.resolve(pos); + let inTable = false; + for (let depth = $pos.depth; depth > 0; depth--) { + if ($pos.node(depth).type.name === 'table') inTable = true; + } + if (inTable) emptyCellParaPos = pos + 1; // inside the empty cell paragraph + } + }); + + return { bodyPos, cellTextPos, emptyCellParaPos }; + } + + /** Whether dispatching TextSelection(anchor -> target) actually keeps the head at target. */ + 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; + } + + it('flags the body -> empty-cell-paragraph drag but not the body -> cell-text drag', () => { + const { bodyPos, cellTextPos, emptyCellParaPos } = positions(); + expect(bodyPos).not.toBeNull(); + expect(cellTextPos).not.toBeNull(); + expect(emptyCellParaPos).not.toBeNull(); + + const doc = editor.state.doc; + expect(selectionCollapsesAcrossTableCells(doc, bodyPos, emptyCellParaPos)).toBe(true); + expect(selectionCollapsesAcrossTableCells(doc, bodyPos, cellTextPos)).toBe(false); + expect(selectionCollapsesAcrossTableCells(doc, bodyPos, bodyPos)).toBe(false); // collapsed selection + }); + + it('matches the real table-plugin normalization: flags exactly the selections that collapse', () => { + const { bodyPos, cellTextPos, emptyCellParaPos } = positions(); + const doc = editor.state.doc; + + // Ground truth from the real prosemirror-tables plugin (runs in initTestEditor): + // body -> empty cell paragraph collapses (head does NOT survive), body -> cell text survives. + expect(dispatchKeepsHead(bodyPos, emptyCellParaPos)).toBe(false); + expect(dispatchKeepsHead(bodyPos, cellTextPos)).toBe(true); + + // The detector must agree with that ground truth for every endpoint. + for (const target of [emptyCellParaPos, cellTextPos]) { + const flagged = selectionCollapsesAcrossTableCells(doc, bodyPos, target); + const collapses = !dispatchKeepsHead(bodyPos, target); + expect(flagged).toBe(collapses); + } + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/tables/TableSelectionUtilities.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/tables/TableSelectionUtilities.ts index 107975a9d8..35e47e4dcc 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/tables/TableSelectionUtilities.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/tables/TableSelectionUtilities.ts @@ -229,6 +229,72 @@ export function getTablePosFromHit( return tablePos; } +/** + * Resolves the table cell that encloses a ProseMirror position. + * + * Walks the ancestors of `pos` to find the nearest `tableCell`/`tableHeader` and its enclosing + * `table`. Returns positions that point *at* the cell and table nodes (i.e. the position directly + * before each), which is the convention `CellSelection.create` expects. + * + * @param doc - The ProseMirror document. + * @param pos - A document position. + * @returns `{ cellPos, tablePos }` if `pos` is inside a table cell, otherwise null. + */ +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; + 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 }; +} + /** * Determines whether cell selection mode should be used based on the current drag state and position. * diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/tables/cross-cell-selection.test.js b/packages/super-editor/src/editors/v1/core/presentation-editor/tables/cross-cell-selection.test.js new file mode 100644 index 0000000000..9d21b094c4 --- /dev/null +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/tables/cross-cell-selection.test.js @@ -0,0 +1,134 @@ +import { beforeEach, describe, expect, it } from 'vitest'; +import { CellSelection } from 'prosemirror-tables'; + +import { initTestEditor } from '@tests/helpers/helpers.js'; + +import { resolveCellContext, resolveCrossCellSelection } from './TableSelectionUtilities.js'; + +/** + * SD-3328 (Option 2): dragging a selection across table cells should produce a CellSelection, + * not a text selection that prosemirror-tables collapses. The drag handler decides this from the + * resolved PM positions via resolveCrossCellSelection, which must hold across direction and only + * fire for genuine cross-cell drags within one table. + */ +const makeTable = (emptyLastCell) => ({ + type: 'table', + attrs: { tableProperties: {}, grid: [{ col: 1500 }, { col: 1500 }] }, + content: [ + { + type: 'tableRow', + content: [cell('A1'), cell('B1')], + }, + { + type: 'tableRow', + content: [cell('A2'), emptyLastCell ? emptyCell() : cell('B2')], + }, + ], +}); +const cell = (text) => ({ + type: 'tableCell', + attrs: { colspan: 1, rowspan: 1, colwidth: [150] }, + content: [{ type: 'paragraph', content: [{ type: 'run', content: [{ type: 'text', text }] }] }], +}); +const emptyCell = () => ({ + type: 'tableCell', + attrs: { colspan: 1, rowspan: 1, colwidth: [150] }, + content: [{ type: 'paragraph' }], +}); +const DOC = { + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'run', content: [{ type: 'text', text: 'Body paragraph' }] }] }, + makeTable(true), // table 1, last cell empty (the user's collapse trigger) + { type: 'paragraph', content: [{ type: 'run', content: [{ type: 'text', text: 'Between tables' }] }] }, + makeTable(false), // table 2 + ], +}; + +describe('cross-cell selection resolution (SD-3328)', () => { + let editor; + + beforeEach(() => { + ({ editor } = initTestEditor({ loadFromSchema: true, content: DOC })); + }); + + /** Positions inside representative nodes of the built document. */ + function positions() { + const doc = editor.state.doc; + const tableCellPositions = []; // one position inside each cell, in document order + let bodyInside = null; + let emptyCellInside = null; + + doc.descendants((node, pos) => { + if (node.isText && bodyInside === null && node.text.includes('Body paragraph')) { + bodyInside = pos + 1; + } + if (node.type.name === 'tableCell') { + tableCellPositions.push(pos + 1); // inside the cell + if (emptyCellInside === null && node.content.size > 0 && node.firstChild?.content.size === 0) { + emptyCellInside = pos + 1; // inside the empty cell's empty paragraph + } + } + }); + + return { bodyInside, tableCellPositions, emptyCellInside }; + } + + it('resolves a cell context inside a cell and nothing in the body', () => { + const { bodyInside, tableCellPositions } = positions(); + const doc = editor.state.doc; + + expect(resolveCellContext(doc, bodyInside)).toBeNull(); + + const cellCtx = resolveCellContext(doc, tableCellPositions[0]); + expect(cellCtx).not.toBeNull(); + // The cell position points at the cell node (its nodeAfter is the cell). + expect(doc.resolve(cellCtx.cellPos).nodeAfter?.type.name).toBe('tableCell'); + expect(doc.resolve(cellCtx.tablePos).nodeAfter?.type.name).toBe('table'); + }); + + it('returns cell positions for a cross-cell drag within one table, in either direction', () => { + const { tableCellPositions } = positions(); + const doc = editor.state.doc; + const [a1, b1] = tableCellPositions; // first two cells of table 1 + + const forward = resolveCrossCellSelection(doc, a1, b1); + const backward = resolveCrossCellSelection(doc, b1, a1); + expect(forward).not.toBeNull(); + expect(backward).not.toBeNull(); + // Direction is preserved: anchor stays the anchor. + expect(forward.anchorCellPos).toBe(backward.headCellPos); + expect(forward.headCellPos).toBe(backward.anchorCellPos); + + // The resulting positions build a real CellSelection spanning two cells. + const sel = CellSelection.create(doc, forward.anchorCellPos, forward.headCellPos); + let cellCount = 0; + sel.forEachCell(() => (cellCount += 1)); + expect(cellCount).toBe(2); + }); + + it('treats a drag that ends on an empty cell paragraph as cross-cell (the reported case)', () => { + const { tableCellPositions, emptyCellInside } = positions(); + const doc = editor.state.doc; + expect(emptyCellInside).not.toBeNull(); + + // Backward: anchor on the empty cell paragraph, head in an earlier cell of the same table. + const result = resolveCrossCellSelection(doc, emptyCellInside, tableCellPositions[0]); + expect(result).not.toBeNull(); + expect(() => CellSelection.create(doc, result.anchorCellPos, result.headCellPos)).not.toThrow(); + }); + + it('returns null when it is not a cross-cell drag', () => { + const { bodyInside, tableCellPositions } = positions(); + const doc = editor.state.doc; + + // Same cell -> text selection. + expect(resolveCrossCellSelection(doc, tableCellPositions[0], tableCellPositions[0] + 1)).toBeNull(); + // Body to cell -> not both in cells. + expect(resolveCrossCellSelection(doc, bodyInside, tableCellPositions[0])).toBeNull(); + // Different tables -> null (last cells belong to table 1 vs table 2). + const firstTableCell = tableCellPositions[0]; + const secondTableCell = tableCellPositions[tableCellPositions.length - 1]; + expect(resolveCrossCellSelection(doc, firstTableCell, secondTableCell)).toBeNull(); + }); +}); diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/DomSelectionGeometry.test.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/DomSelectionGeometry.test.ts index 242d283583..a7c541ab80 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/DomSelectionGeometry.test.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/DomSelectionGeometry.test.ts @@ -573,6 +573,68 @@ describe('computeSelectionRectsFromDom', () => { document.createRange = originalCreateRange; }); + // SD-3328: A multi-line selection used to render one DOM Range spanning every line and + // trust `range.getClientRects()`. Some Chrome builds return incomplete rects for a range + // crossing the absolutely-positioned `.superdoc-line` boxes — interior lines collapse to + // a few stray slivers while the first/last lines render fully — and `intersectsNode` does + // not flag it. Interior lines must instead be covered by the line element's own box. + it('covers interior lines with their full-width box when the spanning range returns slivers', () => { + painterHost.innerHTML = ` +
+
+ line one xx +
+
+ line two xx +
+
+ line three +
+
+ `; + + const layout = createMockLayout([{ pmStart: 1, pmEnd: 33 }]); + domPositionIndex.rebuild(painterHost); + + const pageEl = painterHost.querySelector('.superdoc-page') as HTMLElement; + pageEl.getBoundingClientRect = vi.fn(() => createRect(0, 0, 612, 792)); + + const lines = Array.from(painterHost.querySelectorAll('.superdoc-line')) as HTMLElement[]; + // The interior line (line two) reports a full-width box. If the fix regresses and the + // interior line is taken from the spanning range instead, it would be a sliver. + lines[1]!.getBoundingClientRect = vi.fn(() => createRect(10, 40, 580, 16)); + + // The spanning range simulates the broken browser: only a narrow sliver everywhere. + // `intersectsNode` returns true so the legacy missing-entries fallback would NOT fire — + // only the new "spans multiple lines" path rescues the interior line. + const mockRange = { + setStart: vi.fn(), + setEnd: vi.fn(), + setStartBefore: vi.fn(), + setStartAfter: vi.fn(), + setEndBefore: vi.fn(), + setEndAfter: vi.fn(), + intersectsNode: vi.fn(() => true), + // Sliver rects sit on the first line's row (y=20), away from the interior line + // (y=40), mirroring the broken browser output for the boundary lines. + getClientRects: vi.fn(() => [createRect(10, 20, 4, 16)]), + } as unknown as Range; + + const originalCreateRange = document.createRange; + document.createRange = vi.fn(() => mockRange); + + const options = createOptions(layout); + const rects = computeSelectionRectsFromDom(options, 2, 32); + + document.createRange = originalCreateRange; + + expect(rects).not.toBe(null); + // The interior line must be covered by a rect at its full width, not a 4px sliver. + const interiorRect = rects!.find((r) => Math.abs(r.y - 40) < 1 && r.width > 100); + expect(interiorRect).toBeDefined(); + expect(interiorRect!.width).toBeCloseTo(580, 0); + }); + it('sets range boundaries across descendant text nodes inside one PM-mapped span', () => { painterHost.innerHTML = `
diff --git a/packages/super-editor/src/editors/v1/dom-observer/DomSelectionGeometry.ts b/packages/super-editor/src/editors/v1/dom-observer/DomSelectionGeometry.ts index 65bfbc69bc..51371c300a 100644 --- a/packages/super-editor/src/editors/v1/dom-observer/DomSelectionGeometry.ts +++ b/packages/super-editor/src/editors/v1/dom-observer/DomSelectionGeometry.ts @@ -280,16 +280,25 @@ export function computeSelectionRectsFromDom( } } } - if (missingEntries && missingEntries.length > 0) { + // SD-3328: A single DOM Range spanning multiple visual lines is unreliable here + // because each `.superdoc-line` is absolutely positioned, and some Chrome builds + // return incomplete `getClientRects()` for a range crossing those positioned boxes + // (interior lines collapse to a few stray sliver rects while first/last lines render + // fully). The `intersectsNode` guard does not catch this — the entries still report + // as intersected. So whenever the slice spans more than one line we compute rects + // per line instead, which keeps each measuring range inside a single positioned box. + const spansMultipleLines = countDistinctLines(pageEntries) > 1; + if (spansMultipleLines || (missingEntries && missingEntries.length > 0)) { if (isVerbose) { debugLog( 'verbose', - `DOM selection rects: range missing entries ${JSON.stringify({ + `DOM selection rects: switching to per-line rects ${JSON.stringify({ pageIndex, sliceFrom, sliceTo, - missingCount: missingEntries.length, - missingPreview: missingEntries.slice(0, 20).map(entryDebugInfo), + spansMultipleLines, + missingCount: missingEntries?.length ?? 0, + missingPreview: (missingEntries ?? []).slice(0, 20).map(entryDebugInfo), })}`, ); } @@ -410,7 +419,7 @@ function collectClientRectsByLine( } } - for (const [, lineEntries] of lineMap) { + for (const [lineEl, lineEntries] of lineMap) { lineEntries.sort((a, b) => (a.pmStart - b.pmStart !== 0 ? a.pmStart - b.pmStart : a.pmEnd - b.pmEnd)); const linePmStart = lineEntries[0]?.pmStart ?? Infinity; const linePmEnd = lineEntries[lineEntries.length - 1]?.pmEnd ?? -Infinity; @@ -420,6 +429,21 @@ function collectClientRectsByLine( const lineTo = Math.min(sliceTo, linePmEnd); if (lineFrom >= lineTo) continue; + // SD-3328: For a strictly interior line (the selection both starts on an earlier line + // and ends on a later one), use the line element's own box instead of a measuring range. + // The element box is reliable across browsers and yields the full-width highlight a + // normal text selection shows for interior lines, including trailing whitespace. The + // first and last lines of the selection are left to the precise range below so they + // respect the selection start/end offset and any first-line indent. + const lineIsInterior = sliceFrom < linePmStart && linePmEnd < sliceTo && lineEl.isConnected; + if (lineIsInterior) { + const boxRect = lineEl.getBoundingClientRect(); + if (boxRect.width > 0 && boxRect.height > 0) { + rects.push(boxRect); + continue; + } + } + const startEntry = lineEntries.find((entry) => lineFrom >= entry.pmStart && lineFrom <= entry.pmEnd) ?? lineEntries[0]!; const endEntry = @@ -454,6 +478,29 @@ function collectClientRectsByLine( return rects; } +/** + * Counts how many distinct `.superdoc-line` elements the given entries belong to. + * + * Used to decide whether a selection slice spans more than one visual line. Entries + * without a `.superdoc-line` ancestor (loose entries) are counted once collectively so + * a slice made up only of loose entries still reports at least one line. + * + * @internal + */ +function countDistinctLines(entries: DomPositionIndexEntry[]): number { + const lines = new Set(); + let hasLoose = false; + for (const entry of entries) { + const lineEl = entry.el.closest('.superdoc-line') as HTMLElement | null; + if (lineEl) { + lines.add(lineEl); + } else { + hasLoose = true; + } + } + return lines.size + (hasLoose ? 1 : 0); +} + /** * Sets the start boundary of a DOM Range based on a ProseMirror position. *