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'); + }); + }, +}); diff --git a/packages/layout-engine/layout-bridge/src/dom-mapping.ts b/packages/layout-engine/layout-bridge/src/dom-mapping.ts index 271f4b7934..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,6 +211,16 @@ export function clickToPositionDom(domContainer: HTMLElement, clientX: number, c return result; } + // 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; + } + const result = processFragment(fragmentEl, viewX, viewY); log('=== clickToPositionDom END ===', { result }); return result; @@ -223,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 edc8deff28..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. }; @@ -873,7 +940,32 @@ 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 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; + } + } + } + + if (!pageHit) { + pageHit = hitTestPage(layout, containerPoint, geometryHelper); + } + if (!pageHit) { logClickStage('warn', 'no-page', { point: containerPoint, @@ -881,15 +973,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 +997,24 @@ 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; - - 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, - }; - } + 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) { + fragmentHit = snapToNearestFragment(pageHit, blocks, measures, pageRelativePoint); } - - 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..943e3f4f0f 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, @@ -11,6 +11,7 @@ import { drawingLayout, drawingBlock, drawingMeasure, + buildTableFixtures, } from './mock-data'; describe('clickToPosition', () => { @@ -100,3 +101,251 @@ 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 { 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 = { + 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); + }); +}); 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..8535f4c809 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,98 @@ 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 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 span = container.querySelector('span') as HTMLElement; + + // 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]; + }; + + 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; + } + } + }); + }); + 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 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.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..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,15 +178,16 @@ 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) { - // 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 +195,35 @@ 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. + // 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) + 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 +251,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 = { @@ -287,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];