From b48cd5609bce899b38a095d6e6acaf1248f9c36c Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Sat, 7 Feb 2026 22:59:00 -0300 Subject: [PATCH 1/5] fix(layout-bridge): defer table fragment click mapping to geometry fallback When clicking on table cell padding or borders, elementsFromPoint returns the table fragment but not a specific line element. Previously, processFragment would search all lines across all cells using only Y coordinate matching, causing clicks to land in the wrong column. Now when a table fragment is found without a line in the hit chain, we return null to let the geometry fallback (hitTestTableFragment) handle cell resolution correctly using accumulated row/column dimensions. --- .../layout-bridge/src/dom-mapping.ts | 9 +++ .../layout-bridge/test/dom-mapping.test.ts | 75 +++++++++++++++++++ 2 files changed, 84 insertions(+) diff --git a/packages/layout-engine/layout-bridge/src/dom-mapping.ts b/packages/layout-engine/layout-bridge/src/dom-mapping.ts index 271f4b7934..e413a0008a 100644 --- a/packages/layout-engine/layout-bridge/src/dom-mapping.ts +++ b/packages/layout-engine/layout-bridge/src/dom-mapping.ts @@ -210,6 +210,15 @@ export function clickToPositionDom(domContainer: HTMLElement, clientX: number, c return result; } + // For table fragments without a direct line hit, return null to let the geometry + // fallback (hitTestTableFragment) handle cell lookup. processFragment searches all + // lines across all cells using only Y matching, which picks the wrong column when + // multiple cells share the same row height. + if (fragmentEl.classList.contains('superdoc-table-fragment')) { + log('Table fragment without line in hit chain, deferring to geometry fallback'); + return null; + } + const result = processFragment(fragmentEl, viewX, viewY); log('=== clickToPositionDom END ===', { result }); return result; diff --git a/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts b/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts index 4a0b480ce7..3345166c8c 100644 --- a/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts +++ b/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts @@ -322,6 +322,81 @@ describe('DOM-based click-to-position mapping', () => { expect(result === 5 || result === 15).toBe(true); }); + describe('table fragment fallback', () => { + it('returns null for table fragments without a line in the hit chain', () => { + // When clicking on a table fragment (e.g., cell padding or border) and + // elementsFromPoint doesn't include a line element, clickToPositionDom + // should return null to defer to the geometry fallback (hitTestTableFragment) + // which correctly handles column resolution. + container.innerHTML = ` +
+
+
+
+ Cell 1 text +
+
+
+
+ Cell 2 text +
+
+
+
+ `; + + // In a real browser, clicking on cell padding/borders returns the table fragment + // in elementsFromPoint but NOT the line (due to overflow:hidden clipping). + // JSDOM doesn't have elementsFromPoint, so we polyfill it to simulate + // the real browser behavior where the hit chain includes the table fragment + // but not the line elements. + const page = container.querySelector('.superdoc-page') as HTMLElement; + const tableFragment = container.querySelector('.superdoc-table-fragment') as HTMLElement; + + const originalElementsFromPoint = (document as any).elementsFromPoint; + (document as any).elementsFromPoint = () => { + // Simulate hit chain: table fragment → page → container (no line element) + return [tableFragment, page, container, document.body, document.documentElement]; + }; + + try { + const rect = tableFragment.getBoundingClientRect(); + const result = clickToPositionDom(container, rect.left + 1, rect.top + 1); + expect(result).toBeNull(); + } finally { + if (originalElementsFromPoint) { + (document as any).elementsFromPoint = originalElementsFromPoint; + } else { + delete (document as any).elementsFromPoint; + } + } + }); + + it('returns position when table fragment line IS in the hit chain', () => { + // When a line element IS directly hit (e.g., clicking directly on text), + // the function should use the hitChainLine path and return a valid position. + container.innerHTML = ` +
+
+
+ Cell text +
+
+
+ `; + + const line = container.querySelector('.superdoc-line') as HTMLElement; + const lineRect = line.getBoundingClientRect(); + + // Click directly on the line — elementsFromPoint should include the line + const result = clickToPositionDom(container, lineRect.left + 5, lineRect.top + 5); + + // Should return a valid position from the line + expect(result).toBeGreaterThanOrEqual(5); + expect(result).toBeLessThanOrEqual(15); + }); + }); + describe('inline SDT wrapper exclusion', () => { it('excludes inline SDT wrapper elements from click-to-position mapping', () => { // Inline SDT wrappers have PM positions for selection highlighting but should not From e98c1e1ed7eeb0cb6187323e4a2c1b49792533f3 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Sat, 7 Feb 2026 23:35:58 -0300 Subject: [PATCH 2/5] fix(painter-dom): scope table resize segments to fragment row range When a table splits across pages, each fragment's data-table-boundaries attribute contained segment heights for the entire table rather than just the rows rendered in that fragment. This caused resize handles to overflow beyond the fragment into the page gap and neighboring content. Scope the segment computation to iterate only over the rows actually rendered in each fragment (repeated headers + body rows from fromRow to toRow), matching the existing row rendering logic. Handle partial row heights for mid-row splits. --- .../dom/src/table/renderTableFragment.test.ts | 161 ++++++++++++++++++ .../dom/src/table/renderTableFragment.ts | 50 ++++-- 2 files changed, 198 insertions(+), 13 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts index f8169e4a1b..f5f8fe7c9d 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts @@ -1165,5 +1165,166 @@ describe('renderTableFragment', () => { expect(typeof seg.h).toBe('number'); }); }); + + it('should scope segments to fragment row range for split tables', () => { + // A 3-row table split across two pages: + // Fragment 1 (page 1): rows 0-1, height 60 + // Fragment 2 (page 2): row 2, height 30 + // Each fragment should only have segments matching its own rows. + const block: TableBlock = { + kind: 'table', + id: 'test-table-split' as BlockId, + rows: [ + { + id: 'row-0' as BlockId, + cells: [ + { id: 'cell-0-0' as BlockId, paragraph: { kind: 'paragraph', id: 'p-0-0' as BlockId, runs: [] } }, + { id: 'cell-0-1' as BlockId, paragraph: { kind: 'paragraph', id: 'p-0-1' as BlockId, runs: [] } }, + ], + }, + { + id: 'row-1' as BlockId, + cells: [ + { id: 'cell-1-0' as BlockId, paragraph: { kind: 'paragraph', id: 'p-1-0' as BlockId, runs: [] } }, + { id: 'cell-1-1' as BlockId, paragraph: { kind: 'paragraph', id: 'p-1-1' as BlockId, runs: [] } }, + ], + }, + { + id: 'row-2' as BlockId, + cells: [ + { id: 'cell-2-0' as BlockId, paragraph: { kind: 'paragraph', id: 'p-2-0' as BlockId, runs: [] } }, + { id: 'cell-2-1' as BlockId, paragraph: { kind: 'paragraph', id: 'p-2-1' as BlockId, runs: [] } }, + ], + }, + ], + }; + + const measure: TableMeasure = { + kind: 'table', + rows: [ + { + cells: [ + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 0, + colSpan: 1, + }, + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 1, + colSpan: 1, + }, + ], + height: 30, + }, + { + cells: [ + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 0, + colSpan: 1, + }, + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 1, + colSpan: 1, + }, + ], + height: 30, + }, + { + cells: [ + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 0, + colSpan: 1, + }, + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 30 }, + width: 100, + height: 30, + gridColumnStart: 1, + colSpan: 1, + }, + ], + height: 30, + }, + ], + columnWidths: [100, 100], + totalWidth: 200, + totalHeight: 90, + }; + + const columnBoundaries: TableColumnBoundary[] = [ + { index: 0, x: 0, width: 100, minWidth: 25, resizable: true }, + { index: 1, x: 100, width: 100, minWidth: 25, resizable: true }, + ]; + + const renderDeps = { + doc, + context, + blockLookup, + renderLine: (_block: ParagraphBlock, _line: unknown, _ctx: unknown, _lineIndex: number, _isLastLine: boolean) => + doc.createElement('div'), + applyFragmentFrame: () => {}, + applySdtDataset: () => {}, + applyStyles: () => {}, + }; + + // Fragment 1: rows 0-1 (height = 60) + const fragment1: TableFragment = { + kind: 'table', + blockId: 'test-table-split' as BlockId, + fromRow: 0, + toRow: 2, + x: 0, + y: 0, + width: 200, + height: 60, + continuesOnNext: true, + metadata: { columnBoundaries, coordinateSystem: 'fragment' }, + }; + + blockLookup.set(fragment1.blockId, { block, measure }); + const el1 = renderTableFragment({ ...renderDeps, fragment: fragment1 }); + const parsed1 = JSON.parse(el1.getAttribute('data-table-boundaries')!); + + // Fragment 1 has 2 rows of height 30 each → segment height should be 60 + expect(parsed1.segments[1]).toHaveLength(1); + expect(parsed1.segments[1][0].h).toBe(60); + expect(parsed1.segments[1][0].y).toBe(0); + + // Fragment 2: row 2 only (height = 30) + const fragment2: TableFragment = { + kind: 'table', + blockId: 'test-table-split' as BlockId, + fromRow: 2, + toRow: 3, + x: 0, + y: 0, + width: 200, + height: 30, + continuesFromPrev: true, + metadata: { columnBoundaries, coordinateSystem: 'fragment' }, + }; + + const el2 = renderTableFragment({ ...renderDeps, fragment: fragment2 }); + const parsed2 = JSON.parse(el2.getAttribute('data-table-boundaries')!); + + // Fragment 2 has 1 row of height 30 → segment height should be 30 + expect(parsed2.segments[1]).toHaveLength(1); + expect(parsed2.segments[1][0].h).toBe(30); + expect(parsed2.segments[1][0].y).toBe(0); + }); }); }); diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts index 55c64f4c6b..6a9f8b0f5c 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts @@ -182,10 +182,11 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement // Add metadata for interactive table resizing if (fragment.metadata?.columnBoundaries) { - // Build row-aware boundary segments - // For each grid column boundary, track which row ranges have actual cell edges there + // Build row-aware boundary segments scoped to THIS fragment's rows. + // When a table splits across pages, each fragment only renders a subset of rows + // (repeated headers + body rows from fromRow to toRow). Segments must match + // exactly the rendered rows so resize handles don't overflow the fragment. const columnCount = measure.columnWidths.length; - const rowCount = block.rows.length; // boundarySegments[colIndex] = array of {fromRow, toRow, y, height} segments where this boundary exists const boundarySegments: Array> = []; @@ -193,10 +194,33 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement boundarySegments.push([]); } - // For each row, determine which grid columns have cell boundaries + // Build the list of rows actually rendered in this fragment, matching the + // rendering order: repeated headers first, then body rows. + const renderedRows: Array<{ rowIndex: number; height: number }> = []; + + // Repeated header rows (only on continuation fragments) + if (fragment.repeatHeaderCount && fragment.repeatHeaderCount > 0) { + for (let r = 0; r < fragment.repeatHeaderCount; r++) { + const rowMeasure = measure.rows[r]; + if (!rowMeasure) break; + renderedRows.push({ rowIndex: r, height: rowMeasure.height }); + } + } + + // Body rows (fromRow to toRow), with partial row height for mid-row splits + for (let r = fragment.fromRow; r < fragment.toRow; r++) { + const rowMeasure = measure.rows[r]; + if (!rowMeasure) break; + const isPartialRow = fragment.partialRow && fragment.partialRow.rowIndex === r; + const actualHeight = isPartialRow ? fragment.partialRow!.partialHeight : rowMeasure.height; + renderedRows.push({ rowIndex: r, height: actualHeight }); + } + + // For each rendered row, determine which grid columns have cell boundaries // A boundary exists at column X if there's a cell that ENDS at column X (gridColumnStart + colSpan = X) let rowY = 0; - for (let rowIndex = 0; rowIndex < rowCount; rowIndex++) { + for (let i = 0; i < renderedRows.length; i++) { + const { rowIndex, height } = renderedRows[i]; const rowMeasure = measure.rows[rowIndex]; if (!rowMeasure) continue; @@ -224,22 +248,22 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement const segments = boundarySegments[boundaryCol]; const lastSegment = segments[segments.length - 1]; - // If the last segment ends at this row, extend it - if (lastSegment && lastSegment.toRow === rowIndex) { - lastSegment.toRow = rowIndex + 1; - lastSegment.height += rowMeasure.height; + // If the last segment ends at the previous rendered row, extend it + if (lastSegment && i > 0 && lastSegment.toRow === i) { + lastSegment.toRow = i + 1; + lastSegment.height += height; } else { // Start a new segment segments.push({ - fromRow: rowIndex, - toRow: rowIndex + 1, + fromRow: i, + toRow: i + 1, y: rowY, - height: rowMeasure.height, + height, }); } } - rowY += rowMeasure.height; + rowY += height; } const metadata = { From 18e60c6990b575c9061ac8edcdd068da42dc990e Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Sun, 8 Feb 2026 09:18:11 -0300 Subject: [PATCH 3/5] fix(layout-bridge): fix table cell click on page 2+ by using DOM-based page detection normalizeClientPoint adjusts Y by the page's DOM offset, making containerPoint page-relative. On page 1 this is ~0, but on page 2+ the geometry fallback double-subtracted pageTopY, causing hitTestPage to find the wrong page and table cell hit testing to fail. Fix: when DOM info is available, determine the page from elementsFromPoint and treat containerPoint as already page-relative. Also clean up debug logging from prior iteration. --- .../layout-engine/layout-bridge/src/index.ts | 171 ++++++---- .../test/clickToPosition.test.ts | 302 +++++++++++++++++- 2 files changed, 413 insertions(+), 60 deletions(-) diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index edc8deff28..79f62bef35 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -873,7 +873,38 @@ export function clickToPosition( // Fallback to geometry-based mapping logClickStage('log', 'geometry-attempt', { trying: 'geometry-based mapping' }); - const pageHit = hitTestPage(layout, containerPoint, geometryHelper); + + // When normalizeClientPoint produces containerPoint, it adjusts Y by the page's DOM + // offset, making containerPoint page-relative rather than container-space. On page 1 + // the offset is ~0 so it doesn't matter, but on page 2+ this causes hitTestPage to + // find the wrong page and pageRelativePoint to be doubly subtracted. + // + // Fix: when DOM info is available, determine the page from elementsFromPoint (same + // technique normalizeClientPoint uses) and treat containerPoint as already page-relative. + let pageHit: PageHit | null = null; + let isContainerPointPageRelative = false; + + if (domContainer != null && clientX != null && clientY != null) { + const doc = domContainer.ownerDocument ?? (typeof document !== 'undefined' ? document : null); + if (doc && typeof doc.elementsFromPoint === 'function') { + const hitChain = doc.elementsFromPoint(clientX, clientY); + const pageEl = Array.isArray(hitChain) + ? (hitChain.find((el) => (el as HTMLElement)?.classList?.contains('superdoc-page')) as HTMLElement | null) + : null; + if (pageEl) { + const domPageIndex = Number(pageEl.dataset.pageIndex ?? 'NaN'); + if (Number.isFinite(domPageIndex) && domPageIndex >= 0 && domPageIndex < layout.pages.length) { + pageHit = { pageIndex: domPageIndex, page: layout.pages[domPageIndex] }; + isContainerPointPageRelative = true; + } + } + } + } + + if (!pageHit) { + pageHit = hitTestPage(layout, containerPoint, geometryHelper); + } + if (!pageHit) { logClickStage('warn', 'no-page', { point: containerPoint, @@ -881,15 +912,22 @@ export function clickToPosition( return null; } - // Account for gaps between pages when calculating page-relative Y - // Calculate cumulative Y offset to this page - const pageTopY = geometryHelper - ? geometryHelper.getPageTop(pageHit.pageIndex) - : calculatePageTopFallback(layout, pageHit.pageIndex); - const pageRelativePoint: Point = { - x: containerPoint.x, - y: containerPoint.y - pageTopY, - }; + // Calculate page-relative point + let pageRelativePoint: Point; + if (isContainerPointPageRelative) { + // containerPoint is already page-relative (normalizeClientPoint adjusted Y by page offset) + pageRelativePoint = containerPoint; + } else { + // containerPoint is in container-space, subtract page top to get page-relative + const pageTopY = geometryHelper + ? geometryHelper.getPageTop(pageHit.pageIndex) + : calculatePageTopFallback(layout, pageHit.pageIndex); + pageRelativePoint = { + x: containerPoint.x, + y: containerPoint.y - pageTopY, + }; + } + logClickStage('log', 'page-hit', { pageIndex: pageHit.pageIndex, pageRelativePoint, @@ -898,61 +936,76 @@ export function clickToPosition( let fragmentHit = hitTestFragment(layout, pageHit, blocks, measures, pageRelativePoint); // If no fragment was hit (e.g., whitespace), snap to nearest hit-testable fragment on the page. + // But skip snap-to-nearest when the click is within a table fragment — otherwise the snap + // picks a nearby paragraph and returns its position, preventing clicks in empty table cell + // space (below text lines) from reaching hitTestTableFragment below. if (!fragmentHit) { - const page = pageHit.page; - const fragments = page.fragments.filter( - (f: Fragment | undefined): f is Fragment => f != null && typeof f === 'object', - ); - let nearestHit: FragmentHit | null = null; - let nearestDist = Infinity; - - for (const frag of fragments) { - const isPara = frag.kind === 'para'; - const isAtomic = isAtomicFragment(frag); - if (!isPara && !isAtomic) continue; - - const blockIndex = findBlockIndexByFragmentId(blocks, frag.blockId); - if (blockIndex === -1) continue; - const block = blocks[blockIndex]; - const measure = measures[blockIndex]; - if (!block || !measure) continue; + const tableFragments = pageHit.page.fragments.filter((f) => f.kind === 'table'); + const isWithinTableFragment = tableFragments.some((f) => { + const tf = f as TableFragment; + return ( + pageRelativePoint.x >= tf.x && + pageRelativePoint.x <= tf.x + tf.width && + pageRelativePoint.y >= tf.y && + pageRelativePoint.y <= tf.y + tf.height + ); + }); + if (!isWithinTableFragment) { + const page = pageHit.page; + const fragments = page.fragments.filter( + (f: Fragment | undefined): f is Fragment => f != null && typeof f === 'object', + ); + let nearestHit: FragmentHit | null = null; + let nearestDist = Infinity; + + for (const frag of fragments) { + const isPara = frag.kind === 'para'; + const isAtomic = isAtomicFragment(frag); + if (!isPara && !isAtomic) continue; + + const blockIndex = findBlockIndexByFragmentId(blocks, frag.blockId); + if (blockIndex === -1) continue; + const block = blocks[blockIndex]; + const measure = measures[blockIndex]; + if (!block || !measure) continue; + + let fragHeight = 0; + if (isAtomic) { + fragHeight = frag.height; + } else if (isPara && block.kind === 'paragraph' && measure.kind === 'paragraph') { + fragHeight = measure.lines + .slice(frag.fromLine, frag.toLine) + .reduce((sum: number, line: Line) => sum + line.lineHeight, 0); + } else { + continue; + } - let fragHeight = 0; - if (isAtomic) { - fragHeight = frag.height; - } else if (isPara && block.kind === 'paragraph' && measure.kind === 'paragraph') { - fragHeight = measure.lines - .slice(frag.fromLine, frag.toLine) - .reduce((sum: number, line: Line) => sum + line.lineHeight, 0); - } else { - continue; - } + const top = frag.y; + const bottom = frag.y + fragHeight; + let dist: number; + if (pageRelativePoint.y < top) { + dist = top - pageRelativePoint.y; + } else if (pageRelativePoint.y > bottom) { + dist = pageRelativePoint.y - bottom; + } else { + dist = 0; + } - const top = frag.y; - const bottom = frag.y + fragHeight; - let dist: number; - if (pageRelativePoint.y < top) { - dist = top - pageRelativePoint.y; - } else if (pageRelativePoint.y > bottom) { - dist = pageRelativePoint.y - bottom; - } else { - dist = 0; + if (dist < nearestDist) { + nearestDist = dist; + const pageY = Math.max(0, Math.min(pageRelativePoint.y - top, fragHeight)); + nearestHit = { + fragment: frag, + block, + measure, + pageIndex: pageHit.pageIndex, + pageY, + }; + } } - if (dist < nearestDist) { - nearestDist = dist; - const pageY = Math.max(0, Math.min(pageRelativePoint.y - top, fragHeight)); - nearestHit = { - fragment: frag, - block, - measure, - pageIndex: pageHit.pageIndex, - pageY, - }; - } + fragmentHit = nearestHit; } - - fragmentHit = nearestHit; } if (fragmentHit) { diff --git a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts index 9de2840e91..903d4b36ed 100644 --- a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts +++ b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect } from 'vitest'; import { clickToPosition, hitTestPage } from '../src/index.ts'; -import type { Layout } from '@superdoc/contracts'; +import type { FlowBlock, Layout, Measure } from '@superdoc/contracts'; import { simpleLayout, blocks, @@ -100,3 +100,303 @@ describe('hitTestPage with pageGap', () => { expect(result?.pageIndex).toBe(1); }); }); + +describe('clickToPosition: table cell empty space', () => { + // Table with tall cells (80px) but small text (18px line height). + // Clicking in the empty space below the text line should still resolve + // to a position in the table cell, NOT snap to a nearby paragraph. + const tableCellPara = { + kind: 'paragraph' as const, + id: 'table-cell-para', + runs: [{ text: 'Cell text', fontFamily: 'Arial', fontSize: 14, pmStart: 50, pmEnd: 59 }], + }; + + const tableBlock: FlowBlock = { + kind: 'table', + id: 'table-block', + rows: [ + { + id: 'row-0', + cells: [ + { + id: 'cell-0', + blocks: [tableCellPara], + attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } }, + }, + ], + }, + ], + }; + + const tableMeasure: Measure = { + kind: 'table', + rows: [ + { + height: 80, + cells: [ + { + width: 200, + height: 80, + gridColumnStart: 0, + blocks: [ + { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 9, + width: 70, + ascent: 10, + descent: 4, + lineHeight: 18, + }, + ], + totalHeight: 18, + }, + ], + }, + ], + }, + ], + columnWidths: [200], + totalWidth: 200, + totalHeight: 80, + }; + + // Paragraph above the table (snap-to-nearest candidate) + const paraBlock: FlowBlock = { + kind: 'paragraph', + id: 'para-above', + runs: [{ text: 'Above text', fontFamily: 'Arial', fontSize: 14, pmStart: 1, pmEnd: 11 }], + }; + + const paraMeasure: Measure = { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 10, + width: 80, + ascent: 10, + descent: 4, + lineHeight: 20, + }, + ], + totalHeight: 20, + }; + + // Layout: paragraph at y=30 (height=20), table at y=70 (height=80) + const layout: Layout = { + pageSize: { w: 400, h: 500 }, + pages: [ + { + number: 1, + fragments: [ + { + kind: 'para', + blockId: 'para-above', + fromLine: 0, + toLine: 1, + x: 30, + y: 30, + width: 300, + pmStart: 1, + pmEnd: 11, + }, + { + kind: 'table', + blockId: 'table-block', + fromRow: 0, + toRow: 1, + x: 30, + y: 70, + width: 200, + height: 80, + }, + ], + }, + ], + }; + + const allBlocks = [paraBlock, tableBlock]; + const allMeasures = [paraMeasure, tableMeasure]; + + it('resolves to table cell position when clicking below text in cell', () => { + // Click at (50, 130) — inside the table fragment (y=70 to y=150) + // but well below the text line which ends around y=70+2(padding)+18(line)=90 + // localY within table = 130-70 = 60, well below the 18px text line + const result = clickToPosition(layout, allBlocks, allMeasures, { x: 50, y: 130 }); + + expect(result).not.toBeNull(); + // Should resolve to a position within the table cell's PM range (50-59) + expect(result!.pos).toBeGreaterThanOrEqual(50); + expect(result!.pos).toBeLessThanOrEqual(59); + expect(result!.blockId).toBe('table-block'); + }); + + it('does not snap to nearby paragraph when clicking empty table cell space', () => { + // Click at (50, 140) — inside table fragment, far below text + const result = clickToPosition(layout, allBlocks, allMeasures, { x: 50, y: 140 }); + + expect(result).not.toBeNull(); + // Must NOT resolve to the paragraph above (PM range 1-11) + expect(result!.pos).toBeGreaterThanOrEqual(50); + expect(result!.blockId).toBe('table-block'); + }); +}); + +describe('clickToPosition: table cell on page 2 (multi-page)', () => { + // Table on page 2 with empty space below text line. + // Tests the geometry path with container-space coordinates on page 2+. + const tableCellPara = { + kind: 'paragraph' as const, + id: 'page2-cell-para', + runs: [{ text: 'Page 2 text', fontFamily: 'Arial', fontSize: 14, pmStart: 100, pmEnd: 111 }], + }; + + const tableBlock: FlowBlock = { + kind: 'table', + id: 'page2-table', + rows: [ + { + id: 'row-0', + cells: [ + { + id: 'cell-0', + blocks: [tableCellPara], + attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } }, + }, + ], + }, + ], + }; + + const tableMeasure: Measure = { + kind: 'table', + rows: [ + { + height: 80, + cells: [ + { + width: 200, + height: 80, + gridColumnStart: 0, + blocks: [ + { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 11, + width: 80, + ascent: 10, + descent: 4, + lineHeight: 18, + }, + ], + totalHeight: 18, + }, + ], + }, + ], + }, + ], + columnWidths: [200], + totalWidth: 200, + totalHeight: 80, + }; + + // Page 1 paragraph filler, page 2 has the table + const page1Para: FlowBlock = { + kind: 'paragraph', + id: 'page1-para', + runs: [{ text: 'Page 1 content', fontFamily: 'Arial', fontSize: 14, pmStart: 1, pmEnd: 15 }], + }; + + const page1Measure: Measure = { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 14, + width: 100, + ascent: 10, + descent: 4, + lineHeight: 20, + }, + ], + totalHeight: 20, + }; + + // Two-page layout: page 1 has a paragraph, page 2 has a table + const layout: Layout = { + pageSize: { w: 400, h: 500 }, + pages: [ + { + number: 1, + fragments: [ + { + kind: 'para', + blockId: 'page1-para', + fromLine: 0, + toLine: 1, + x: 30, + y: 30, + width: 300, + pmStart: 1, + pmEnd: 15, + }, + ], + }, + { + number: 2, + fragments: [ + { + kind: 'table', + blockId: 'page2-table', + fromRow: 0, + toRow: 1, + x: 30, + y: 50, + width: 200, + height: 80, + }, + ], + }, + ], + }; + + const allBlocks = [page1Para, tableBlock]; + const allMeasures = [page1Measure, tableMeasure]; + + it('resolves to table cell on page 2 with container-space coordinates', () => { + // Page 2 starts at y=500. Table is at y=50 within page 2 = container y=550. + // Click at y=590, which is 90 within page 2, inside table (50 to 130), below text. + const result = clickToPosition(layout, allBlocks, allMeasures, { x: 50, y: 590 }); + + expect(result).not.toBeNull(); + expect(result!.pos).toBeGreaterThanOrEqual(100); + expect(result!.pos).toBeLessThanOrEqual(111); + expect(result!.blockId).toBe('page2-table'); + expect(result!.pageIndex).toBe(1); + }); + + it('resolves to table cell on page 2 when clicking below text line', () => { + // Click at y=610, which is 110 within page 2, inside table (50 to 130), far below 18px text + const result = clickToPosition(layout, allBlocks, allMeasures, { x: 50, y: 610 }); + + expect(result).not.toBeNull(); + expect(result!.pos).toBeGreaterThanOrEqual(100); + expect(result!.pos).toBeLessThanOrEqual(111); + expect(result!.blockId).toBe('page2-table'); + expect(result!.pageIndex).toBe(1); + }); +}); From 5b3523ea6085c2a2413acfb0378470ca681a7aa2 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 11 Feb 2026 16:07:52 -0300 Subject: [PATCH 4/5] refactor(layout-bridge): address PR review feedback - Add TABLE_FRAGMENT to DOM_CLASS_NAMES constant, use across packages - Export and reuse findPageElement in clickToPosition instead of duplicating - Extract snapToNearestFragment helper from clickToPosition - Add buildTableFixtures shared test builder in mock-data - Polyfill elementsFromPoint in table hit chain test for proper coverage - Add cross-reference comments between metadata and rendering iterations --- .../layout-bridge/src/dom-mapping.ts | 14 +- .../layout-engine/layout-bridge/src/index.ts | 165 +++++++++--------- .../test/clickToPosition.test.ts | 67 +------ .../layout-bridge/test/dom-mapping.test.ts | 29 ++- .../layout-bridge/test/mock-data.ts | 88 ++++++++++ .../painters/dom/src/constants.ts | 6 + .../dom/src/table/renderTableFragment.ts | 11 +- 7 files changed, 228 insertions(+), 152 deletions(-) diff --git a/packages/layout-engine/layout-bridge/src/dom-mapping.ts b/packages/layout-engine/layout-bridge/src/dom-mapping.ts index e413a0008a..3ba3493082 100644 --- a/packages/layout-engine/layout-bridge/src/dom-mapping.ts +++ b/packages/layout-engine/layout-bridge/src/dom-mapping.ts @@ -28,6 +28,7 @@ const CLASS_NAMES = { page: DOM_CLASS_NAMES.PAGE, fragment: DOM_CLASS_NAMES.FRAGMENT, line: DOM_CLASS_NAMES.LINE, + tableFragment: DOM_CLASS_NAMES.TABLE_FRAGMENT, } as const; /** @@ -210,11 +211,12 @@ export function clickToPositionDom(domContainer: HTMLElement, clientX: number, c return result; } - // For table fragments without a direct line hit, return null to let the geometry - // fallback (hitTestTableFragment) handle cell lookup. processFragment searches all - // lines across all cells using only Y matching, which picks the wrong column when - // multiple cells share the same row height. - if (fragmentEl.classList.contains('superdoc-table-fragment')) { + // For table fragments without a direct line hit, return null so the caller + // (clickToPosition in index.ts) falls back to geometry-based hit testing via + // hitTestTableFragment, which correctly resolves the cell by column. processFragment + // would search all lines across all cells using only Y matching, picking the wrong + // column when multiple cells share the same row height. + if (fragmentEl.classList.contains(CLASS_NAMES.tableFragment)) { log('Table fragment without line in hit chain, deferring to geometry fallback'); return null; } @@ -232,7 +234,7 @@ export function clickToPositionDom(domContainer: HTMLElement, clientX: number, c * @param clientY - Y coordinate in viewport space * @returns The page element, or null if not found */ -function findPageElement(domContainer: HTMLElement, clientX: number, clientY: number): HTMLElement | null { +export function findPageElement(domContainer: HTMLElement, clientX: number, clientY: number): HTMLElement | null { // Check if the container itself is a page element if (domContainer.classList?.contains?.(CLASS_NAMES.page)) { return domContainer; diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index 79f62bef35..f9c681381f 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -15,7 +15,7 @@ import type { } from '@superdoc/contracts'; import { computeLinePmRange as computeLinePmRangeUnified } from '@superdoc/contracts'; import { charOffsetToPm, findCharacterAtX, measureCharacterX } from './text-measurement.js'; -import { clickToPositionDom } from './dom-mapping.js'; +import { clickToPositionDom, findPageElement } from './dom-mapping.js'; import { isListItem, getWordLayoutConfig, @@ -56,7 +56,7 @@ export type { HeaderFooterLayoutResult, IncrementalLayoutResult } from './increm export { computeDisplayPageNumber, type DisplayPageInfo } from '@superdoc/layout-engine'; export { remeasureParagraph } from './remeasure'; export { measureCharacterX } from './text-measurement'; -export { clickToPositionDom } from './dom-mapping'; +export { clickToPositionDom, findPageElement } from './dom-mapping'; export { isListItem, getWordLayoutConfig, calculateTextStartIndent, extractParagraphIndent } from './list-indent-utils'; export type { TextIndentCalculationParams } from './list-indent-utils'; export { LayoutVersionManager } from './layout-version-manager'; @@ -222,6 +222,73 @@ const isAtomicFragment = (fragment: Fragment): fragment is AtomicFragment => { return fragment.kind === 'drawing' || fragment.kind === 'image'; }; +/** + * Finds the nearest paragraph or atomic fragment to a point on a page. + * + * When a click lands in whitespace (no fragment hit), this snaps to the closest + * fragment by vertical distance. Used as a fallback when hitTestFragment misses. + */ +function snapToNearestFragment( + pageHit: PageHit, + blocks: FlowBlock[], + measures: Measure[], + pageRelativePoint: Point, +): FragmentHit | null { + const fragments = pageHit.page.fragments.filter( + (f: Fragment | undefined): f is Fragment => f != null && typeof f === 'object', + ); + let nearestHit: FragmentHit | null = null; + let nearestDist = Infinity; + + for (const frag of fragments) { + const isPara = frag.kind === 'para'; + const isAtomic = isAtomicFragment(frag); + if (!isPara && !isAtomic) continue; + + const blockIndex = findBlockIndexByFragmentId(blocks, frag.blockId); + if (blockIndex === -1) continue; + const block = blocks[blockIndex]; + const measure = measures[blockIndex]; + if (!block || !measure) continue; + + let fragHeight = 0; + if (isAtomic) { + fragHeight = frag.height; + } else if (isPara && block.kind === 'paragraph' && measure.kind === 'paragraph') { + fragHeight = measure.lines + .slice(frag.fromLine, frag.toLine) + .reduce((sum: number, line: Line) => sum + line.lineHeight, 0); + } else { + continue; + } + + const top = frag.y; + const bottom = frag.y + fragHeight; + let dist: number; + if (pageRelativePoint.y < top) { + dist = top - pageRelativePoint.y; + } else if (pageRelativePoint.y > bottom) { + dist = pageRelativePoint.y - bottom; + } else { + dist = 0; + } + + if (dist < nearestDist) { + nearestDist = dist; + const pageY = Math.max(0, Math.min(pageRelativePoint.y - top, fragHeight)); + nearestHit = { + fragment: frag, + block, + measure, + pageIndex: pageHit.pageIndex, + pageY, + }; + } + } + + return nearestHit; +} + const logClickStage = (_level: 'log' | 'warn' | 'error', _stage: string, _payload: Record) => { // No-op in production. Enable for debugging click-to-position mapping. }; @@ -885,18 +952,12 @@ export function clickToPosition( let isContainerPointPageRelative = false; if (domContainer != null && clientX != null && clientY != null) { - const doc = domContainer.ownerDocument ?? (typeof document !== 'undefined' ? document : null); - if (doc && typeof doc.elementsFromPoint === 'function') { - const hitChain = doc.elementsFromPoint(clientX, clientY); - const pageEl = Array.isArray(hitChain) - ? (hitChain.find((el) => (el as HTMLElement)?.classList?.contains('superdoc-page')) as HTMLElement | null) - : null; - if (pageEl) { - const domPageIndex = Number(pageEl.dataset.pageIndex ?? 'NaN'); - if (Number.isFinite(domPageIndex) && domPageIndex >= 0 && domPageIndex < layout.pages.length) { - pageHit = { pageIndex: domPageIndex, page: layout.pages[domPageIndex] }; - isContainerPointPageRelative = true; - } + const pageEl = findPageElement(domContainer, clientX, clientY); + if (pageEl) { + const domPageIndex = Number(pageEl.dataset.pageIndex ?? 'NaN'); + if (Number.isFinite(domPageIndex) && domPageIndex >= 0 && domPageIndex < layout.pages.length) { + pageHit = { pageIndex: domPageIndex, page: layout.pages[domPageIndex] }; + isContainerPointPageRelative = true; } } } @@ -940,71 +1001,19 @@ export function clickToPosition( // picks a nearby paragraph and returns its position, preventing clicks in empty table cell // space (below text lines) from reaching hitTestTableFragment below. if (!fragmentHit) { - const tableFragments = pageHit.page.fragments.filter((f) => f.kind === 'table'); - const isWithinTableFragment = tableFragments.some((f) => { - const tf = f as TableFragment; - return ( - pageRelativePoint.x >= tf.x && - pageRelativePoint.x <= tf.x + tf.width && - pageRelativePoint.y >= tf.y && - pageRelativePoint.y <= tf.y + tf.height - ); - }); + const isWithinTableFragment = pageHit.page.fragments + .filter((f) => f.kind === 'table') + .some((f) => { + const tf = f as TableFragment; + return ( + pageRelativePoint.x >= tf.x && + pageRelativePoint.x <= tf.x + tf.width && + pageRelativePoint.y >= tf.y && + pageRelativePoint.y <= tf.y + tf.height + ); + }); if (!isWithinTableFragment) { - const page = pageHit.page; - const fragments = page.fragments.filter( - (f: Fragment | undefined): f is Fragment => f != null && typeof f === 'object', - ); - let nearestHit: FragmentHit | null = null; - let nearestDist = Infinity; - - for (const frag of fragments) { - const isPara = frag.kind === 'para'; - const isAtomic = isAtomicFragment(frag); - if (!isPara && !isAtomic) continue; - - const blockIndex = findBlockIndexByFragmentId(blocks, frag.blockId); - if (blockIndex === -1) continue; - const block = blocks[blockIndex]; - const measure = measures[blockIndex]; - if (!block || !measure) continue; - - let fragHeight = 0; - if (isAtomic) { - fragHeight = frag.height; - } else if (isPara && block.kind === 'paragraph' && measure.kind === 'paragraph') { - fragHeight = measure.lines - .slice(frag.fromLine, frag.toLine) - .reduce((sum: number, line: Line) => sum + line.lineHeight, 0); - } else { - continue; - } - - const top = frag.y; - const bottom = frag.y + fragHeight; - let dist: number; - if (pageRelativePoint.y < top) { - dist = top - pageRelativePoint.y; - } else if (pageRelativePoint.y > bottom) { - dist = pageRelativePoint.y - bottom; - } else { - dist = 0; - } - - if (dist < nearestDist) { - nearestDist = dist; - const pageY = Math.max(0, Math.min(pageRelativePoint.y - top, fragHeight)); - nearestHit = { - fragment: frag, - block, - measure, - pageIndex: pageHit.pageIndex, - pageY, - }; - } - } - - fragmentHit = nearestHit; + fragmentHit = snapToNearestFragment(pageHit, blocks, measures, pageRelativePoint); } } diff --git a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts index 903d4b36ed..943e3f4f0f 100644 --- a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts +++ b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts @@ -11,6 +11,7 @@ import { drawingLayout, drawingBlock, drawingMeasure, + buildTableFixtures, } from './mock-data'; describe('clickToPosition', () => { @@ -105,65 +106,13 @@ describe('clickToPosition: table cell empty space', () => { // Table with tall cells (80px) but small text (18px line height). // Clicking in the empty space below the text line should still resolve // to a position in the table cell, NOT snap to a nearby paragraph. - const tableCellPara = { - kind: 'paragraph' as const, - id: 'table-cell-para', - runs: [{ text: 'Cell text', fontFamily: 'Arial', fontSize: 14, pmStart: 50, pmEnd: 59 }], - }; - - const tableBlock: FlowBlock = { - kind: 'table', - id: 'table-block', - rows: [ - { - id: 'row-0', - cells: [ - { - id: 'cell-0', - blocks: [tableCellPara], - attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } }, - }, - ], - }, - ], - }; - - const tableMeasure: Measure = { - kind: 'table', - rows: [ - { - height: 80, - cells: [ - { - width: 200, - height: 80, - gridColumnStart: 0, - blocks: [ - { - kind: 'paragraph', - lines: [ - { - fromRun: 0, - fromChar: 0, - toRun: 0, - toChar: 9, - width: 70, - ascent: 10, - descent: 4, - lineHeight: 18, - }, - ], - totalHeight: 18, - }, - ], - }, - ], - }, - ], - columnWidths: [200], - totalWidth: 200, - totalHeight: 80, - }; + const { block: tableBlock, measure: tableMeasure } = buildTableFixtures({ + cellWidth: 200, + cellHeight: 80, + lineHeight: 18, + pmStart: 50, + pmEnd: 59, + }); // Paragraph above the table (snap-to-nearest candidate) const paraBlock: FlowBlock = { diff --git a/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts b/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts index 3345166c8c..8535f4c809 100644 --- a/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts +++ b/packages/layout-engine/layout-bridge/test/dom-mapping.test.ts @@ -385,15 +385,32 @@ describe('DOM-based click-to-position mapping', () => { `; + const page = container.querySelector('.superdoc-page') as HTMLElement; + const tableFragment = container.querySelector('.superdoc-table-fragment') as HTMLElement; const line = container.querySelector('.superdoc-line') as HTMLElement; - const lineRect = line.getBoundingClientRect(); + const span = container.querySelector('span') as HTMLElement; - // Click directly on the line — elementsFromPoint should include the line - const result = clickToPositionDom(container, lineRect.left + 5, lineRect.top + 5); + // Polyfill elementsFromPoint to simulate a real browser hit chain that + // includes the line element (clicking directly on text inside a table cell). + const originalElementsFromPoint = (document as any).elementsFromPoint; + (document as any).elementsFromPoint = () => { + return [span, line, tableFragment, page, container, document.body, document.documentElement]; + }; - // Should return a valid position from the line - expect(result).toBeGreaterThanOrEqual(5); - expect(result).toBeLessThanOrEqual(15); + try { + const lineRect = line.getBoundingClientRect(); + const result = clickToPositionDom(container, lineRect.left + 5, lineRect.top + 5); + + // Should return a valid position from the line via the hitChainLine path + expect(result).toBeGreaterThanOrEqual(5); + expect(result).toBeLessThanOrEqual(15); + } finally { + if (originalElementsFromPoint) { + (document as any).elementsFromPoint = originalElementsFromPoint; + } else { + delete (document as any).elementsFromPoint; + } + } }); }); diff --git a/packages/layout-engine/layout-bridge/test/mock-data.ts b/packages/layout-engine/layout-bridge/test/mock-data.ts index 04f0089783..792c358f93 100644 --- a/packages/layout-engine/layout-bridge/test/mock-data.ts +++ b/packages/layout-engine/layout-bridge/test/mock-data.ts @@ -272,3 +272,91 @@ export const tableLayout: Layout = { }, ], }; + +/** + * Builds table test fixtures with customizable dimensions. + * Reduces duplication between clickToPosition and dom-mapping table tests. + */ +export function buildTableFixtures( + opts: { + cellWidth?: number; + cellHeight?: number; + lineHeight?: number; + pmStart?: number; + pmEnd?: number; + text?: string; + blockId?: string; + } = {}, +): { block: FlowBlock; measure: Measure } { + const { + cellWidth = 200, + cellHeight = 80, + lineHeight = 18, + pmStart = 50, + pmEnd = 59, + text = 'Cell text', + blockId = 'table-block', + } = opts; + + const block: FlowBlock = { + kind: 'table', + id: blockId, + rows: [ + { + id: 'row-0', + cells: [ + { + id: 'cell-0', + blocks: [ + { + kind: 'paragraph' as const, + id: `${blockId}-para`, + runs: [{ text, fontFamily: 'Arial', fontSize: 14, pmStart, pmEnd }], + }, + ], + attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } }, + }, + ], + }, + ], + }; + + const measure: Measure = { + kind: 'table', + rows: [ + { + height: cellHeight, + cells: [ + { + width: cellWidth, + height: cellHeight, + gridColumnStart: 0, + blocks: [ + { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: text.length, + width: 70, + ascent: 10, + descent: 4, + lineHeight, + }, + ], + totalHeight: lineHeight, + }, + ], + }, + ], + }, + ], + columnWidths: [cellWidth], + totalWidth: cellWidth, + totalHeight: cellHeight, + }; + + return { block, measure }; +} diff --git a/packages/layout-engine/painters/dom/src/constants.ts b/packages/layout-engine/painters/dom/src/constants.ts index b5adb992ec..35312791cc 100644 --- a/packages/layout-engine/painters/dom/src/constants.ts +++ b/packages/layout-engine/painters/dom/src/constants.ts @@ -51,6 +51,12 @@ export const DOM_CLASS_NAMES = { */ BLOCK_SDT: 'superdoc-structured-content-block', + /** + * Class name for table fragment containers. + * Applied to table fragments for resize overlay targeting and click mapping. + */ + TABLE_FRAGMENT: 'superdoc-table-fragment', + /** * Class name for document section containers. */ diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts index 6a9f8b0f5c..9f3a8383c9 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts @@ -9,6 +9,7 @@ import type { TableMeasure, } from '@superdoc/contracts'; import { CLASS_NAMES, fragmentStyles } from '../styles.js'; +import { DOM_CLASS_NAMES } from '../constants.js'; import type { FragmentRenderContext, BlockLookup } from '../renderer.js'; import { renderTableRow } from './renderTableRow.js'; import { applySdtContainerStyling, type SdtBoundaryOptions } from '../utils/sdt-helpers.js'; @@ -177,8 +178,8 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement // Apply SDT container styling (document sections, structured content blocks) applySdtContainerStyling(doc, container, block.attrs?.sdt, block.attrs?.containerSdt, sdtBoundary); - // Add table-specific class for resize overlay targeting - container.classList.add('superdoc-table-fragment'); + // Add table-specific class for resize overlay targeting and click mapping + container.classList.add(DOM_CLASS_NAMES.TABLE_FRAGMENT); // Add metadata for interactive table resizing if (fragment.metadata?.columnBoundaries) { @@ -196,6 +197,8 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement // Build the list of rows actually rendered in this fragment, matching the // rendering order: repeated headers first, then body rows. + // NOTE: This header-then-body iteration must stay in sync with the rendering + // loop below (~line 315) which uses the same order to render row elements. const renderedRows: Array<{ rowIndex: number; height: number }> = []; // Repeated header rows (only on continuation fragments) @@ -311,7 +314,9 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement let y = 0; - // If this is a continuation fragment with repeated headers, render headers first + // If this is a continuation fragment with repeated headers, render headers first. + // NOTE: This header-then-body iteration must stay in sync with the metadata + // segment builder above (~line 199) which uses the same order. if (fragment.repeatHeaderCount && fragment.repeatHeaderCount > 0) { for (let r = 0; r < fragment.repeatHeaderCount; r += 1) { const rowMeasure = measure.rows[r]; From 99c6fe80e3aa35ad155f241786b52bd6c908ad0b Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 11 Feb 2026 16:22:14 -0300 Subject: [PATCH 5/5] test(visual): add visual test for table cell click positioning (SD-1788) Verifies that clicking in different table cells correctly places the cursor in the targeted cell, not in nearby paragraphs or wrong columns. --- .../table-cell-click-positioning.ts | 119 ++++++++++++++++++ 1 file changed, 119 insertions(+) create mode 100644 devtools/visual-testing/tests/interactions/stories/basic-commands/table-cell-click-positioning.ts diff --git a/devtools/visual-testing/tests/interactions/stories/basic-commands/table-cell-click-positioning.ts b/devtools/visual-testing/tests/interactions/stories/basic-commands/table-cell-click-positioning.ts new file mode 100644 index 0000000000..bef3b121ea --- /dev/null +++ b/devtools/visual-testing/tests/interactions/stories/basic-commands/table-cell-click-positioning.ts @@ -0,0 +1,119 @@ +import { defineStory } from '@superdoc-testing/helpers'; + +const WAIT_MS = 400; +const WAIT_LONG_MS = 600; + +/** + * SD-1788: Cursor cannot be reliably placed in table cell + * + * Bug: Clicking in empty space inside a table cell (below the text line) + * would snap the cursor to a nearby paragraph outside the table instead + * of placing it in the correct cell. This happened because: + * + * 1. DOM-based click mapping (clickToPositionDom) searched all lines across + * all cells using only Y matching, picking the wrong column + * 2. The snap-to-nearest fallback chose a nearby paragraph over the table cell + * + * This test verifies: + * 1. Clicking inside a table cell places cursor in that cell + * 2. Clicking in different cells moves cursor to the correct cell + * 3. Clicking below text in a tall cell still resolves to that cell + * 4. Table works correctly on page 2+ (multi-page coordinate mapping) + */ +export default defineStory({ + name: 'table-cell-click-positioning', + description: 'Test that clicks in table cells map to correct cursor positions', + tickets: ['SD-1788'], + startDocument: null, + layout: true, + hideCaret: false, + hideSelection: false, + + async run(page, helpers): Promise { + const { step, focus, type, press, clickAt, waitForStable, milestone, executeCommand } = helpers; + + // Step 1: Create content above the table (snap-to-nearest candidate) + await step('Type paragraph above table', async () => { + await focus(); + await type('Paragraph above the table'); + await waitForStable(WAIT_MS); + await press('Enter'); + await press('Enter'); + await waitForStable(WAIT_MS); + await milestone('paragraph-above', 'Paragraph above where table will be inserted'); + }); + + // Step 2: Insert a table + await step('Insert table', async () => { + await executeCommand('insertTable', { rows: 3, cols: 3, withHeaderRow: false }); + await waitForStable(WAIT_LONG_MS); + await milestone('table-inserted', 'Inserted 3x3 table'); + }); + + // Step 3: Type content in first cell + await step('Type in first cell', async () => { + await type('Cell A1'); + await waitForStable(WAIT_MS); + await milestone('cell-a1-text', 'Typed in first cell (A1)'); + }); + + // Step 4: Navigate to second cell and type + await step('Type in second cell', async () => { + await press('Tab'); + await waitForStable(WAIT_MS); + await type('Cell B1'); + await waitForStable(WAIT_MS); + await milestone('cell-b1-text', 'Typed in second cell (B1)'); + }); + + // Step 5: Navigate to third cell and type + await step('Type in third cell', async () => { + await press('Tab'); + await waitForStable(WAIT_MS); + await type('Cell C1'); + await waitForStable(WAIT_MS); + await milestone('cell-c1-text', 'Typed in third cell (C1)'); + }); + + // Step 6: Navigate to second row, first cell + await step('Type in second row', async () => { + await press('Tab'); + await waitForStable(WAIT_MS); + await type('Cell A2'); + await waitForStable(WAIT_MS); + await milestone('cell-a2-text', 'Typed in row 2 first cell (A2)'); + }); + + // Step 7: Click back into cell A1 — the key test + // This tests that clicking in a table cell with content places the cursor + // in that cell, not in the paragraph above. + // Table starts at ~y=127, rows are ~17px each: + // Row 1: y≈127-144, Row 2: y≈144-161, Row 3: y≈161-178 + // Use short verification text "!" to avoid wrapping which shifts row heights. + await step('Click into first cell (A1)', async () => { + await clickAt(100, 135); + await waitForStable(WAIT_MS); + await type('!'); + await waitForStable(WAIT_MS); + await milestone('verify-click-a1', 'Clicked and typed "!" in A1 — should appear in first cell'); + }); + + // Step 8: Click into cell B1 (middle column, same row) + await step('Click into middle cell (B1)', async () => { + await clickAt(280, 135); + await waitForStable(WAIT_MS); + await type('!'); + await waitForStable(WAIT_MS); + await milestone('verify-click-b1', 'Clicked and typed "!" in B1 — should appear in middle cell'); + }); + + // Step 9: Click into cell A2 (second row) + await step('Click into second row cell (A2)', async () => { + await clickAt(100, 175); + await waitForStable(WAIT_MS); + await type('!'); + await waitForStable(WAIT_MS); + await milestone('verify-click-a2', 'Clicked and typed "!" in A2 — should appear in second row'); + }); + }, +});