From f018a4cee769832acc3ce666f9ed6f4d89cd9483 Mon Sep 17 00:00:00 2001 From: Matthew Connelly Date: Fri, 6 Feb 2026 20:53:23 -0500 Subject: [PATCH] fix: use fragment.lines for click position in multi-column layouts --- .../layout-engine/layout-bridge/src/index.ts | 53 +++- .../test/clickToPosition.test.ts | 242 +++++++++++++++++- 2 files changed, 284 insertions(+), 11 deletions(-) diff --git a/packages/layout-engine/layout-bridge/src/index.ts b/packages/layout-engine/layout-bridge/src/index.ts index edc8deff28..b682b91ce4 100644 --- a/packages/layout-engine/layout-bridge/src/index.ts +++ b/packages/layout-engine/layout-bridge/src/index.ts @@ -959,7 +959,24 @@ 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 fragments in multi-column layouts). + // When a paragraph is remeasured for column width, fragment.lines contains the actual + // rendered lines which may differ from measure.lines. + let lines: Line[]; + let fromLine: number; + let toLine: number; + + if (fragment.lines && fragment.lines.length > 0) { + lines = fragment.lines; + fromLine = 0; + toLine = fragment.lines.length; + } else { + lines = measure.lines; + fromLine = fragment.fromLine; + toLine = fragment.toLine; + } + + let lineIndex = findLineIndexAtY(lines, pageY, fromLine, toLine); if (lineIndex == null) { logClickStage('warn', 'no-line', { blockId: fragment.blockId, @@ -968,7 +985,23 @@ export function clickToPosition( }); return null; } - const line = measure.lines[lineIndex]; + + const line = lines[lineIndex]; + // Convert to absolute index when using fragment-local lines + if (lines === fragment.lines) { + lineIndex = fragment.fromLine + lineIndex; + } + + // Guard against undefined line (defensive check) + if (!line) { + logClickStage('warn', 'no-line', { + blockId: fragment.blockId, + pageIndex, + pageY, + reason: 'line is undefined after lookup', + }); + return null; + } const isRTL = isRtlBlock(block); // Type guard: Validate indent structure and ensure numeric values @@ -1077,7 +1110,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); @@ -2078,29 +2111,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 9de2840e91..4da0474c87 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 { Layout, FlowBlock, Measure, Line, ParaFragment } from '@superdoc/contracts'; import { simpleLayout, blocks, @@ -100,3 +100,243 @@ describe('hitTestPage with pageGap', () => { expect(result?.pageIndex).toBe(1); }); }); + +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 + expect(result?.pos).toBeGreaterThanOrEqual(1); + 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'); + }); +});