Skip to content

Commit ba03e76

Browse files
tupizztupizz
authored
fix(editor): selection highlight flickers when dragging across mark boundaries (SD-2024) (#2205)
* fix(editor): selection highlight flickers when dragging across mark boundaries (SD-2024) DomPositionIndex.findEntriesInRange used half-open [start, end) semantics. When a selection range fell exactly on a run boundary (the 2-position gap between adjacent text spans with different marks), neither adjacent entry matched — producing zero DOM rects and clearing the selection overlay for one frame before the next pointer event restored it. Add a `boundaryInclusive` option that switches to closed [start, end] semantics, and use it in DomSelectionGeometry so entries touching the boundary are always found. Also add a safety net in PresentationEditor that preserves the last overlay when a non-empty selection yields no rects. * test(editor): add integration tests for selection across mark boundaries (SD-2024) Add two computeSelectionRectsFromDom tests that exercise the exact edge case: selection range falls on the structural gap between two differently-marked runs, verifying the boundaryInclusive path produces non-empty rects. * fix(editor): address PR review — JSDoc correction, debug log for zero-rect path - Revert findElementsInRange JSDoc @param to back to "exclusive" (method does not pass boundaryInclusive) - Add debugLog warn when zero-rect early return fires so stale overlay cases are diagnosable * fix(editor): scope zero-rect overlay preservation to active drag only Narrows the safety net to isDragging so non-drag zero-rect cases (virtualized/disconnected DOM) still clear the overlay correctly. * fix(layout): improve click position handling with DOM-based page detection Refactor clickToPosition to utilize DOM-based page detection via elementsFromPoint, enhancing accuracy in multi-page layouts. This change ensures that containerPoint is treated as page-relative when applicable, addressing issues with virtualization and layout gaps. Additionally, clamp head position in EditorInputManager to prevent selection from entering isolating nodes like tables during drag operations. * fix(editor): use outermost isolating boundary for drag selection clamping (SD-2024) Fix backward drag clamping to resolve to the table boundary instead of an inner cell boundary. Also add behavioral tests for DOM-based page coordinate resolution and selection clamping at isolating nodes.
1 parent 0b3e2b4 commit ba03e76

9 files changed

Lines changed: 780 additions & 35 deletions

File tree

‎packages/layout-engine/layout-bridge/src/index.ts‎

Lines changed: 22 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -957,23 +957,27 @@ export function clickToPosition(
957957
// Fallback to geometry-based mapping
958958
logClickStage('log', 'geometry-attempt', { trying: 'geometry-based mapping' });
959959

960-
// When normalizeClientPoint produces containerPoint, it adjusts Y by the page's DOM
961-
// offset, making containerPoint page-relative rather than container-space. On page 1
962-
// the offset is ~0 so it doesn't matter, but on page 2+ this causes hitTestPage to
963-
// find the wrong page and pageRelativePoint to be doubly subtracted.
964-
//
965-
// Fix: when DOM info is available, determine the page from elementsFromPoint (same
966-
// technique normalizeClientPoint uses) and treat containerPoint as already page-relative.
960+
// Use DOM-based page detection when available. elementsFromPoint accurately identifies
961+
// the page element under the pointer, even in edge cases where the geometry-based
962+
// hitTestPage may return the wrong page (e.g., due to virtualization or gaps).
967963
let pageHit: PageHit | null = null;
968-
let isContainerPointPageRelative = false;
964+
let domPageRelativeY: number | undefined;
969965

970966
if (domContainer != null && clientX != null && clientY != null) {
971967
const pageEl = findPageElement(domContainer, clientX, clientY);
972968
if (pageEl) {
973969
const domPageIndex = Number(pageEl.dataset.pageIndex ?? 'NaN');
974970
if (Number.isFinite(domPageIndex) && domPageIndex >= 0 && domPageIndex < layout.pages.length) {
975971
pageHit = { pageIndex: domPageIndex, page: layout.pages[domPageIndex] };
976-
isContainerPointPageRelative = true;
972+
// Compute page-relative Y directly from the page element's DOM position.
973+
// containerPoint.y is in container-space (global layout Y) and cannot be used
974+
// as page-relative Y — subtracting geometry page-top may not match the actual
975+
// DOM page position due to viewport padding, margins, or virtualization offsets.
976+
const pageRect = pageEl.getBoundingClientRect();
977+
const layoutPageHeight = pageHit.page.size?.h ?? layout.pageSize.h;
978+
const domPageHeight = pageRect.height;
979+
const effectiveZoom = domPageHeight > 0 && layoutPageHeight > 0 ? domPageHeight / layoutPageHeight : 1;
980+
domPageRelativeY = (clientY - pageRect.top) / effectiveZoom;
977981
}
978982
}
979983
}
@@ -989,21 +993,15 @@ export function clickToPosition(
989993
return null;
990994
}
991995

992-
// Calculate page-relative point
993-
let pageRelativePoint: Point;
994-
if (isContainerPointPageRelative) {
995-
// containerPoint is already page-relative (normalizeClientPoint adjusted Y by page offset)
996-
pageRelativePoint = containerPoint;
997-
} else {
998-
// containerPoint is in container-space, subtract page top to get page-relative
999-
const pageTopY = geometryHelper
1000-
? geometryHelper.getPageTop(pageHit.pageIndex)
1001-
: calculatePageTopFallback(layout, pageHit.pageIndex);
1002-
pageRelativePoint = {
1003-
x: containerPoint.x,
1004-
y: containerPoint.y - pageTopY,
1005-
};
1006-
}
996+
// Calculate page-relative point. Prefer DOM-derived Y when available (accurate
997+
// regardless of viewport offsets), fall back to geometry subtraction.
998+
const pageTopY = geometryHelper
999+
? geometryHelper.getPageTop(pageHit.pageIndex)
1000+
: calculatePageTopFallback(layout, pageHit.pageIndex);
1001+
const pageRelativePoint: Point = {
1002+
x: containerPoint.x,
1003+
y: domPageRelativeY ?? containerPoint.y - pageTopY,
1004+
};
10071005

10081006
logClickStage('log', 'page-hit', {
10091007
pageIndex: pageHit.pageIndex,

‎packages/layout-engine/layout-bridge/test/clickToPosition-dom-page.test.ts‎

Lines changed: 301 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,301 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*
4+
* Tests for DOM-based page-relative coordinate resolution in clickToPosition.
5+
*
6+
* When the geometry fallback is used (DOM mapping returns null for fragment/line),
7+
* clickToPosition must compute page-relative Y from the actual DOM page element
8+
* position, not from geometry-based page-top calculations. This is critical on
9+
* page 2+ where container-space Y diverges from page-relative Y.
10+
*
11+
* Scenario (SD-2024): A paragraph sits just above a table on page 2. Clicking on
12+
* the paragraph's left margin (where no fragment/line is found by elementsFromPoint)
13+
* triggers the geometry fallback. If container-space Y is used as page-relative Y,
14+
* the click resolves to a position far below the paragraph — inside or past the
15+
* table. The fix computes page-relative Y directly from the page element's
16+
* bounding rect.
17+
*/
18+
import { describe, it, expect, beforeEach, afterEach } from 'vitest';
19+
import { clickToPosition } from '../src/index.ts';
20+
import type { Layout, FlowBlock, Measure } from '@superdoc/contracts';
21+
22+
/**
23+
* Helper: temporarily mock document.elementsFromPoint for the duration of `run()`.
24+
*/
25+
function withMockedElementsFromPoint(elements: Element[], run: () => void): void {
26+
const originalElementsFromPoint = document.elementsFromPoint;
27+
document.elementsFromPoint = (_x: number, _y: number) => elements;
28+
29+
try {
30+
run();
31+
} finally {
32+
document.elementsFromPoint = originalElementsFromPoint;
33+
}
34+
}
35+
36+
describe('clickToPosition: DOM-based page-relative Y on page 2+ (SD-2024)', () => {
37+
let container: HTMLElement;
38+
39+
// Layout: page 1 has a filler paragraph, page 2 has a paragraph followed by a table.
40+
// The paragraph on page 2 is at y=40 (page-relative), table at y=120.
41+
const paraBlock: FlowBlock = {
42+
kind: 'paragraph',
43+
id: 'page2-para',
44+
runs: [{ text: 'In witness to the commitment', fontFamily: 'Arial', fontSize: 11, pmStart: 100, pmEnd: 128 }],
45+
};
46+
47+
const tableBlock: FlowBlock = {
48+
kind: 'table',
49+
id: 'page2-table',
50+
rows: [
51+
{
52+
id: 'row-0',
53+
cells: [
54+
{
55+
id: 'cell-0',
56+
blocks: [
57+
{
58+
kind: 'paragraph',
59+
id: 'cell-para',
60+
runs: [{ text: 'Signature', fontFamily: 'Arial', fontSize: 11, pmStart: 130, pmEnd: 139 }],
61+
},
62+
],
63+
attrs: { padding: { top: 2, bottom: 2, left: 4, right: 4 } },
64+
},
65+
],
66+
},
67+
],
68+
};
69+
70+
const paraMeasure: Measure = {
71+
kind: 'paragraph',
72+
lines: [
73+
{
74+
fromRun: 0,
75+
fromChar: 0,
76+
toRun: 0,
77+
toChar: 28,
78+
width: 300,
79+
ascent: 10,
80+
descent: 4,
81+
lineHeight: 16,
82+
},
83+
],
84+
totalHeight: 16,
85+
};
86+
87+
const tableMeasure: Measure = {
88+
kind: 'table',
89+
rows: [
90+
{
91+
height: 40,
92+
cells: [
93+
{
94+
width: 200,
95+
height: 40,
96+
gridColumnStart: 0,
97+
blocks: [
98+
{
99+
kind: 'paragraph',
100+
lines: [
101+
{
102+
fromRun: 0,
103+
fromChar: 0,
104+
toRun: 0,
105+
toChar: 9,
106+
width: 80,
107+
ascent: 10,
108+
descent: 4,
109+
lineHeight: 16,
110+
},
111+
],
112+
totalHeight: 16,
113+
},
114+
],
115+
},
116+
],
117+
},
118+
],
119+
columnWidths: [200],
120+
totalWidth: 200,
121+
totalHeight: 40,
122+
};
123+
124+
const page1Para: FlowBlock = {
125+
kind: 'paragraph',
126+
id: 'page1-para',
127+
runs: [{ text: 'Page 1 content here', fontFamily: 'Arial', fontSize: 11, pmStart: 1, pmEnd: 20 }],
128+
};
129+
130+
const page1Measure: Measure = {
131+
kind: 'paragraph',
132+
lines: [
133+
{
134+
fromRun: 0,
135+
fromChar: 0,
136+
toRun: 0,
137+
toChar: 19,
138+
width: 150,
139+
ascent: 10,
140+
descent: 4,
141+
lineHeight: 16,
142+
},
143+
],
144+
totalHeight: 16,
145+
};
146+
147+
// Page 2 paragraph at y=40, table at y=120 (page-relative)
148+
const layout: Layout = {
149+
pageSize: { w: 600, h: 800 },
150+
pageGap: 24,
151+
pages: [
152+
{
153+
number: 1,
154+
fragments: [
155+
{
156+
kind: 'para',
157+
blockId: 'page1-para',
158+
fromLine: 0,
159+
toLine: 1,
160+
x: 72,
161+
y: 40,
162+
width: 456,
163+
pmStart: 1,
164+
pmEnd: 20,
165+
},
166+
],
167+
},
168+
{
169+
number: 2,
170+
fragments: [
171+
{
172+
kind: 'para',
173+
blockId: 'page2-para',
174+
fromLine: 0,
175+
toLine: 1,
176+
x: 72,
177+
y: 40,
178+
width: 456,
179+
pmStart: 100,
180+
pmEnd: 128,
181+
},
182+
{
183+
kind: 'table',
184+
blockId: 'page2-table',
185+
fromRow: 0,
186+
toRow: 1,
187+
x: 72,
188+
y: 120,
189+
width: 200,
190+
height: 40,
191+
},
192+
],
193+
},
194+
],
195+
};
196+
197+
const allBlocks = [page1Para, paraBlock, tableBlock];
198+
const allMeasures = [page1Measure, paraMeasure, tableMeasure];
199+
200+
beforeEach(() => {
201+
container = document.createElement('div');
202+
container.style.position = 'absolute';
203+
container.style.left = '0px';
204+
container.style.top = '0px';
205+
document.body.appendChild(container);
206+
});
207+
208+
afterEach(() => {
209+
document.body.removeChild(container);
210+
});
211+
212+
it('resolves click on page 2 paragraph to paragraph position, not table', () => {
213+
// Set up DOM with two page elements.
214+
// Page 1 is at DOM top=0, height=800.
215+
// Page 2 is at DOM top=824 (800 + 24 gap), height=800.
216+
container.innerHTML = `
217+
<div class="superdoc-page" data-page-index="0"
218+
style="position:absolute; top:0px; left:0px; width:600px; height:800px;"></div>
219+
<div class="superdoc-page" data-page-index="1"
220+
style="position:absolute; top:824px; left:0px; width:600px; height:800px;"></div>
221+
`;
222+
223+
const page2El = container.querySelectorAll('.superdoc-page')[1] as HTMLElement;
224+
225+
// Mock elementsFromPoint to return only the page element (no fragment/line found).
226+
// This simulates clicking on the left margin of the paragraph where no fragment
227+
// DOM element is under the pointer.
228+
withMockedElementsFromPoint([page2El, container], () => {
229+
// clientY = 864 → page 2 top (824) + 40 = paragraph Y on page 2
230+
// container-space Y = 864 (same as clientY at zoom=1 with no offset)
231+
// Page-relative Y should be 864 - 824 = 40 (where the paragraph is)
232+
const result = clickToPosition(
233+
layout,
234+
allBlocks,
235+
allMeasures,
236+
{ x: 72, y: 864 }, // container-space point
237+
container, // DOM container
238+
72, // clientX
239+
864, // clientY
240+
);
241+
242+
expect(result).not.toBeNull();
243+
// Should resolve to the paragraph (PM 100-128), NOT the table (PM 130-139)
244+
expect(result!.pos).toBeGreaterThanOrEqual(100);
245+
expect(result!.pos).toBeLessThanOrEqual(128);
246+
expect(result!.blockId).toBe('page2-para');
247+
expect(result!.pageIndex).toBe(1);
248+
});
249+
});
250+
251+
it('falls back to geometry when no DOM container is provided', () => {
252+
// Without DOM container, uses pure geometry path.
253+
// container-space Y = 864, geometry page 2 top = 824 (800 + 24 gap)
254+
// page-relative Y = 864 - 824 = 40 → should hit the paragraph
255+
const result = clickToPosition(
256+
layout,
257+
allBlocks,
258+
allMeasures,
259+
{ x: 72, y: 864 }, // container-space point
260+
// No DOM container — geometry fallback
261+
);
262+
263+
expect(result).not.toBeNull();
264+
expect(result!.pos).toBeGreaterThanOrEqual(100);
265+
expect(result!.pos).toBeLessThanOrEqual(128);
266+
expect(result!.blockId).toBe('page2-para');
267+
expect(result!.pageIndex).toBe(1);
268+
});
269+
270+
it('DOM-based Y handles zoom correctly', () => {
271+
// At 2x zoom, DOM page height is 1600px but layout height is 800.
272+
// Page 2 DOM top = (800*2) + 24*2 = 1648
273+
// clientY = 1728 → page-relative = (1728 - 1648) / 2 = 40
274+
container.innerHTML = `
275+
<div class="superdoc-page" data-page-index="0"
276+
style="position:absolute; top:0px; left:0px; width:1200px; height:1600px;"></div>
277+
<div class="superdoc-page" data-page-index="1"
278+
style="position:absolute; top:1648px; left:0px; width:1200px; height:1600px;"></div>
279+
`;
280+
281+
const page2El = container.querySelectorAll('.superdoc-page')[1] as HTMLElement;
282+
283+
withMockedElementsFromPoint([page2El, container], () => {
284+
const result = clickToPosition(
285+
layout,
286+
allBlocks,
287+
allMeasures,
288+
{ x: 72, y: 864 }, // container-space (layout coordinates, not zoomed)
289+
container,
290+
144, // clientX at 2x
291+
1728, // clientY at 2x: page2 DOM top (1648) + 40*2 = 1728
292+
);
293+
294+
expect(result).not.toBeNull();
295+
// Should still resolve to the paragraph
296+
expect(result!.pos).toBeGreaterThanOrEqual(100);
297+
expect(result!.pos).toBeLessThanOrEqual(128);
298+
expect(result!.blockId).toBe('page2-para');
299+
});
300+
});
301+
});

‎packages/super-editor/src/core/presentation-editor/PresentationEditor.ts‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3984,6 +3984,19 @@ export class PresentationEditor extends EventEmitter {
39843984
return;
39853985
}
39863986

3987+
// When dragging across mark boundaries, the selection can briefly land in the
3988+
// 2-position structural gap between adjacent runs, producing zero DOM rects for
3989+
// one frame. Preserve the last overlay only during active drag to prevent flicker.
3990+
// Outside drag (scroll, programmatic changes), zero rects means the DOM is stale
3991+
// or virtualized — clearing the overlay is the safer default.
3992+
if (domRects.length === 0 && from !== to && this.#editorInputManager?.isDragging) {
3993+
debugLog('warn', '[drawSelection] zero rects for non-collapsed selection — preserving last overlay', {
3994+
from,
3995+
to,
3996+
});
3997+
return;
3998+
}
3999+
39874000
try {
39884001
this.#localSelectionLayer.innerHTML = '';
39894002
const isFieldAnnotationSelection =

0 commit comments

Comments
 (0)