diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index 9543df1be3..81d811a310 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -1037,7 +1037,11 @@ export function clickToPosition( const { fragment, block, measure, pageIndex, pageY } = fragmentHit; // Handle paragraph fragments if (fragment.kind === 'para' && measure.kind === 'paragraph' && block.kind === 'paragraph') { - const lineIndex = findLineIndexAtY(measure, pageY, fragment.fromLine, fragment.toLine); + // Use fragment-specific lines when available (remeasured for column width), + // otherwise slice from measure.lines for this fragment's range. + const lines = fragment.lines ?? measure.lines.slice(fragment.fromLine, fragment.toLine); + + const lineIndex = findLineIndexAtY(lines, pageY, 0, lines.length); if (lineIndex == null) { logClickStage('warn', 'no-line', { blockId: fragment.blockId, @@ -1046,7 +1050,8 @@ export function clickToPosition( }); return null; } - const line = measure.lines[lineIndex]; + + const line = lines[lineIndex]; const isRTL = isRtlBlock(block); // Type guard: Validate indent structure and ensure numeric values @@ -1155,7 +1160,7 @@ export function clickToPosition( const { cellBlock, cellMeasure, localX, localY, pageIndex } = tableHit; // Find the line at the local Y position within the cell paragraph - const lineIndex = findLineIndexAtY(cellMeasure, localY, 0, cellMeasure.lines.length); + const lineIndex = findLineIndexAtY(cellMeasure.lines, localY, 0, cellMeasure.lines.length); if (lineIndex != null) { const line = cellMeasure.lines[lineIndex]; const isRTL = isRtlBlock(cellBlock); @@ -2142,13 +2147,13 @@ const determineColumn = (layout: Layout, fragmentX: number): number => { }; /** - * Finds the line index at a given Y offset within a paragraph measure. + * Finds the line index at a given Y offset within a set of lines. * * This function searches within a specified range of lines to determine which line * contains the given Y coordinate. It validates bounds to prevent out-of-bounds * access in case of corrupted layout data. * - * @param measure - The paragraph measure containing line data + * @param lines - The array of lines to search through * @param offsetY - The Y offset in pixels to search for * @param fromLine - The starting line index (inclusive) * @param toLine - The ending line index (exclusive) @@ -2156,29 +2161,29 @@ const determineColumn = (layout: Layout, fragmentX: number): number => { * * @throws Never throws - returns null for invalid inputs */ -const findLineIndexAtY = (measure: Measure, offsetY: number, fromLine: number, toLine: number): number | null => { - if (measure.kind !== 'paragraph') return null; +const findLineIndexAtY = (lines: Line[], offsetY: number, fromLine: number, toLine: number): number | null => { + if (!lines || lines.length === 0) return null; // Validate bounds to prevent out-of-bounds access - const lineCount = measure.lines.length; + const lineCount = lines.length; if (fromLine < 0 || toLine > lineCount || fromLine >= toLine) { return null; } let cursor = 0; - // Only search within the fragment's line range + // Only search within the specified line range for (let i = fromLine; i < toLine; i += 1) { - const line = measure.lines[i]; + const line = lines[i]; // Guard against undefined lines (defensive check for corrupted data) if (!line) return null; const next = cursor + line.lineHeight; if (offsetY >= cursor && offsetY < next) { - return i; // Return absolute line index within measure + return i; // Return line index within the array } cursor = next; } - // If beyond all lines, return the last line in the fragment + // If beyond all lines, return the last line in the range return toLine - 1; }; diff --git a/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts b/packages/layout-engine/layout-bridge/test/clickToPosition.test.ts index 41c6503fa3..8e9bd017fe 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, hitTestTableFragment } from '../src/index.ts'; -import type { FlowBlock, Layout, Measure } from '@superdoc/contracts'; +import type { Layout, FlowBlock, Measure, Line, ParaFragment } from '@superdoc/contracts'; import { simpleLayout, blocks, @@ -107,6 +107,246 @@ describe('hitTestPage with pageGap', () => { }); }); +describe('clickToPosition with fragment.lines', () => { + // Tests for multi-column documents where fragments have remeasured lines + // that differ from measure.lines. + // + // Example scenario - paragraph "Hello world" in a two-column layout: + // + // Original measure (full page width): Remeasured for column width: + // ┌────────────────────────────────┐ ┌──────────────┐ + // │ Hello world │ │ Hello │ ← line 0 + // └────────────────────────────────┘ │ world │ ← line 1 + // (1 line) └──────────────┘ + // (2 lines) + // + // measure.lines = [line0] fragment.lines = [line0, line1] + // + // The bug: using measure.lines with fragment.fromLine/toLine indices + // caused out-of-bounds access when the fragment had more lines than measure. + + // ───────────────────────────────────────────────────────────────────────────── + // REMEASURED LINES + // ───────────────────────────────────────────────────────────────────────────── + // These represent the line breaks after remeasuring at column width. + // The paragraph "Hello world" wraps into two lines: + // + // remeasuredLine1: "Hello " (run 0, chars 0-5) + // remeasuredLine2: "world" (run 0 char 5 → run 1 char 5) + // + // ┌──────────────┐ + // │ H e l l o │ ← remeasuredLine1 (y: 0-20) + // │ w o r l d │ ← remeasuredLine2 (y: 20-40) + // └──────────────┘ + // + const remeasuredLine1: Line = { + fromRun: 0, + fromChar: 0, + toRun: 0, + toChar: 5, // "Hello" (5 chars, space trimmed) + width: 100, + ascent: 12, + descent: 4, + lineHeight: 20, + }; + + const remeasuredLine2: Line = { + fromRun: 0, + fromChar: 5, // continues from end of line 1 + toRun: 1, + toChar: 5, // "world" (5 chars) + width: 100, + ascent: 12, + descent: 4, + lineHeight: 20, + }; + + // ───────────────────────────────────────────────────────────────────────────── + // FLOW BLOCK (ProseMirror content) + // ───────────────────────────────────────────────────────────────────────────── + // The source paragraph content with two runs: + // + // run 0: "Hello " (pmStart: 1, pmEnd: 7) + // run 1: "world" (pmStart: 7, pmEnd: 12) + // + // PM positions: 1 2 3 4 5 6 7 8 9 10 11 12 + // Characters: H e l l o w o r l d + // └─── run 0 ───┘ └─── run 1 ───┘ + // + const twoColumnBlock: FlowBlock = { + kind: 'paragraph', + id: 'two-column-para', + runs: [ + { text: 'Hello ', fontFamily: 'Arial', fontSize: 16, pmStart: 1, pmEnd: 7 }, + { text: 'world', fontFamily: 'Arial', fontSize: 16, pmStart: 7, pmEnd: 12 }, + ], + }; + + // ───────────────────────────────────────────────────────────────────────────── + // ORIGINAL MEASURE (full page width) + // ───────────────────────────────────────────────────────────────────────────── + // When measured at full page width, the entire paragraph fits on one line: + // + // ┌────────────────────────────────────────┐ + // │ H e l l o w o r l d │ ← single line (y: 0-20) + // └────────────────────────────────────────┘ + // + // measure.lines.length = 1 + // + const originalMeasure: Measure = { + kind: 'paragraph', + lines: [ + { + fromRun: 0, + fromChar: 0, + toRun: 1, + toChar: 5, // entire paragraph: "Hello world" + width: 200, + ascent: 12, + descent: 4, + lineHeight: 20, + }, + ], + totalHeight: 20, + }; + + // ───────────────────────────────────────────────────────────────────────────── + // FRAGMENT (positioned on page, with remeasured lines) + // ───────────────────────────────────────────────────────────────────────────── + // This fragment is placed in column 2 of a two-column layout. + // It contains `lines` array with the remeasured line breaks. + // + // Page layout (600px wide): + // + // x=0 x=290 x=310 x=600 + // ┌──────────┐ ┌──────────┐ + // │ Column 1 │ │ Column 2 │ + // │ │ │┌────────┐│ + // │ │ ││ Hello ││ ← fragment at (300, 40) + // │ │ ││ world ││ + // │ │ │└────────┘│ + // └──────────┘ └──────────┘ + // + // THE BUG: fragment.fromLine=0, fragment.toLine=2 are indices into + // fragment.lines (length 2), but the old code used these to access + // measure.lines (length 1), causing measure.lines[1] → undefined + // + const fragmentWithRemeasuredLines: ParaFragment = { + kind: 'para', + blockId: 'two-column-para', + fromLine: 0, // index into fragment.lines (NOT measure.lines) + toLine: 2, // would be out-of-bounds for measure.lines! + x: 300, // positioned in column 2 + y: 40, + width: 150, + pmStart: 1, + pmEnd: 12, + lines: [remeasuredLine1, remeasuredLine2], // the remeasured lines for this fragment + }; + + const twoColumnLayout: Layout = { + pageSize: { w: 600, h: 800 }, + columns: { count: 2, gap: 20 }, + pages: [ + { + number: 1, + fragments: [fragmentWithRemeasuredLines], + }, + ], + }; + + it('uses fragment.lines when available instead of measure.lines', () => { + // ─────────────────────────────────────────────────────────────────────── + // Click in the first line of the fragment: + // + // Click point: (350, 50) + // + // Fragment at (300, 40): + // y=40 ┌──────────────┐ + // │ Hello ← * │ click y=50 hits line 1 (y: 40-60) + // y=60 │ world │ + // y=80 └──────────────┘ + // x=350 + // + // Without the fix: TypeError because measure.lines[1] is undefined + // With the fix: uses fragment.lines to find line, returns valid position + // ─────────────────────────────────────────────────────────────────────── + const result = clickToPosition(twoColumnLayout, [twoColumnBlock], [originalMeasure], { x: 350, y: 50 }); + + expect(result).not.toBeNull(); + expect(result?.blockId).toBe('two-column-para'); + expect(result?.pos).toBeGreaterThanOrEqual(1); + expect(result?.pos).toBeLessThanOrEqual(12); + }); + + it('correctly maps click position in second line of fragment with remeasured lines', () => { + // ─────────────────────────────────────────────────────────────────────── + // Click in the second line of the fragment: + // + // Click point: (350, 65) + // + // Fragment at (300, 40): + // y=40 ┌──────────────┐ + // │ Hello │ + // y=60 │ world ← * │ click y=65 hits line 2 (y: 60-80) + // y=80 └──────────────┘ + // x=350 + // + // This tests that we correctly index into fragment.lines[1] ("world") + // ─────────────────────────────────────────────────────────────────────── + const result = clickToPosition(twoColumnLayout, [twoColumnBlock], [originalMeasure], { x: 350, y: 65 }); + + expect(result).not.toBeNull(); + expect(result?.blockId).toBe('two-column-para'); + // The click should map to a position in the second line's range ("world" starts at position 7) + expect(result?.pos).toBeGreaterThanOrEqual(7); + expect(result?.pos).toBeLessThanOrEqual(12); + }); + + it('handles fragment without lines array (uses measure.lines)', () => { + // ─────────────────────────────────────────────────────────────────────── + // Fallback test: fragment WITHOUT remeasured lines + // + // When fragment.lines is absent, we fall back to measure.lines. + // This is the common case for single-column layouts. + // + // Fragment at (30, 40), width=200 (full width, no remeasure): + // y=40 ┌────────────────────────────────┐ + // │ Hello world ← * │ click y=50 hits line 1 + // y=60 └────────────────────────────────┘ + // x=100 + // + // ─────────────────────────────────────────────────────────────────────── + const fragmentWithoutLines: ParaFragment = { + kind: 'para', + blockId: 'two-column-para', + fromLine: 0, + toLine: 1, + x: 30, + y: 40, + width: 200, + pmStart: 1, + pmEnd: 12, + // No `lines` property - should fall back to measure.lines + }; + + const layoutWithoutFragmentLines: Layout = { + pageSize: { w: 400, h: 500 }, + pages: [ + { + number: 1, + fragments: [fragmentWithoutLines], + }, + ], + }; + + const result = clickToPosition(layoutWithoutFragmentLines, [twoColumnBlock], [originalMeasure], { x: 100, y: 50 }); + + expect(result).not.toBeNull(); + expect(result?.blockId).toBe('two-column-para'); + }); +}); + describe('hitTestTableFragment with rowspan (SD-1626 / IT-22)', () => { // Table is at x:30, y:60, width:300, height:48 // Row 0: y:60-84 (height 24) - has 3 cells diff --git a/tests/behavior/tests/selection/fixtures/two-column-simple.docx b/tests/behavior/tests/selection/fixtures/two-column-simple.docx new file mode 100644 index 0000000000..cd7b9abbf9 Binary files /dev/null and b/tests/behavior/tests/selection/fixtures/two-column-simple.docx differ diff --git a/tests/behavior/tests/selection/multi-column-click-positioning.spec.ts b/tests/behavior/tests/selection/multi-column-click-positioning.spec.ts new file mode 100644 index 0000000000..ff1580b496 --- /dev/null +++ b/tests/behavior/tests/selection/multi-column-click-positioning.spec.ts @@ -0,0 +1,91 @@ +import fs from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { test, expect } from '../../fixtures/superdoc.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const DOC_PATH = path.resolve(__dirname, 'fixtures/two-column-simple.docx'); + +test.skip(!fs.existsSync(DOC_PATH), 'Two-column fixture not available'); + +test.use({ config: { toolbar: 'none', showCaret: true, showSelection: true } }); + +test('clicking in second column does not crash (SD-1830 / IT-407)', async ({ superdoc }) => { + await superdoc.loadDocument(DOC_PATH); + await superdoc.waitForStable(); + + // Get the first page bounding box to compute column positions + const page = superdoc.page.locator('.superdoc-page').first(); + const pageBox = await page.boundingBox(); + if (!pageBox) throw new Error('Page not visible'); + + // Click near the top of the second column (right half of page). + // This is exactly where the customer reported the crash (IT-407): + // "the first paragraph at the top of the second column" + const secondColumnX = pageBox.x + pageBox.width * 0.75; + const topOfColumnY = pageBox.y + 40; + + // Without the fix this throws: TypeError: can't access property "fromRun", line is undefined + await superdoc.page.mouse.click(secondColumnX, topOfColumnY); + await superdoc.waitForStable(); + + // Cursor should be at a valid position (not crashed, not zero) + const sel = await superdoc.getSelection(); + expect(sel.from).toBeGreaterThan(0); +}); + +test('clicking in both columns places cursor at different positions', async ({ superdoc }) => { + await superdoc.loadDocument(DOC_PATH); + await superdoc.waitForStable(); + + const page = superdoc.page.locator('.superdoc-page').first(); + const pageBox = await page.boundingBox(); + if (!pageBox) throw new Error('Page not visible'); + + // Click well into the text area of the first column (left quarter, middle of page height) + const firstColumnX = pageBox.x + pageBox.width * 0.25; + const clickY = pageBox.y + pageBox.height * 0.5; + + await superdoc.page.mouse.click(firstColumnX, clickY); + await superdoc.waitForStable(); + const selLeft = await superdoc.getSelection(); + + // Click at the same Y in the second column (right quarter) + const secondColumnX = pageBox.x + pageBox.width * 0.75; + + await superdoc.page.mouse.click(secondColumnX, clickY); + await superdoc.waitForStable(); + const selRight = await superdoc.getSelection(); + + // Both should be valid positions + expect(selLeft.from).toBeGreaterThan(0); + expect(selRight.from).toBeGreaterThan(0); + + // Cursor should land at different positions — text flows left column then right column, + // so the right column position should be further into the document. + expect(selRight.from).not.toBe(selLeft.from); +}); + +test('typing after clicking in second column inserts text (SD-1830)', async ({ superdoc }) => { + await superdoc.loadDocument(DOC_PATH); + await superdoc.waitForStable(); + + const page = superdoc.page.locator('.superdoc-page').first(); + const pageBox = await page.boundingBox(); + if (!pageBox) throw new Error('Page not visible'); + + // Click in the second column + const secondColumnX = pageBox.x + pageBox.width * 0.75; + const topOfColumnY = pageBox.y + 40; + + await superdoc.page.mouse.click(secondColumnX, topOfColumnY); + await superdoc.waitForStable(); + + // Type a unique marker to prove the cursor is functional + await superdoc.type('MARKER'); + await superdoc.waitForStable(); + + // The marker should appear in the document + const text = await superdoc.getTextContent(); + expect(text).toContain('MARKER'); +});