Skip to content

SD-1830 - use fragment.lines for click position in multi-column layouts #1963

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
mattConnHarbour wants to merge 1 commit into
base: main
Choose a base branch
from
Open

SD-1830 - use fragment.lines for click position in multi-column layouts #1963

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 43 additions & 10 deletions packages/layout-engine/layout-bridge/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Copy link
Contributor

@caio-pizzol caio-pizzol Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: the renderer already handles this in one line (renderer.ts:2034):

  const lines = fragment.lines ?? measure.lines.slice(fragment.fromLine, fragment.toLine);

this would replace the 3 let declarations + if/else block, and findLineIndexAtY can just take (lines, pageY, 0, lines.length).

the slice cost is negligible for a click handler. also removes the need for the reference equality check at line 991.

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,
Expand All @@ -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) {
Copy link
Contributor

@caio-pizzol caio-pizzol Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

minor: using reference equality (lines === fragment.lines) for branch detection is fragile -- if someone copies the array, this silently breaks

a const isRemeasured boolean flag set at the branch point would be more explicit.

low risk but either way worth double checking

lineIndex = fragment.fromLine + lineIndex;
}

// Guard against undefined line (defensive check)
Copy link
Contributor

@caio-pizzol caio-pizzol Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

findLineIndexAtY already returns null when lines[i] is undefined (line 2127-2128), and if it returns a non-null index, every line in the range was verified to exist during iteration.

this guard can never trigger -- removing it avoids making a reader wonder "wait, can this actually be undefined?"

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
Expand Down Expand Up @@ -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);
Expand Down Expand Up @@ -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;
};

Expand Down
242 changes: 241 additions & 1 deletion packages/layout-engine/layout-bridge/test/clickToPosition.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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,
Expand Down Expand Up @@ -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);
Copy link
Contributor

@caio-pizzol caio-pizzol Feb 9, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the range assertions (1-12) accept any position in the paragraph -- they prove the crash is fixed but wouldn't catch a regression where clicking line 2 ("world") maps to a line 1 position.

tightening to expect(result?.pos).toBeGreaterThanOrEqual(7) for the second-line test would verify the correct line was hit.

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');
});
});
Loading