diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts index 72ce1888b0..60dfe2a0b3 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts @@ -481,6 +481,16 @@ export class PresentationEditor extends EventEmitter { * this unset so they don't fight the user's scroll position. */ #shouldScrollSelectionIntoView = false; + /** + * SD-3315: while a search-owned scrollToPosition({ suppressSelectionSyncScroll: true }) is in + * flight (set before its sync scroll, cleared in its RAF re-assert), selection-sync must NOT + * scroll the viewport. Find navigation owns the scroll for that window; the spurious + * selectionUpdate fired by the find-input focus restore (which reverts the editor selection to + * its pre-search caret) would otherwise yank the viewport to that stale caret, producing a + * jump/flash on every navigation. The selection overlay still renders during the window; only + * #scrollActiveEndIntoView is skipped. + */ + #suppressSelectionScrollUntilRaf = false; /** PM position for transient drag/drop insertion preview, rendered even while editor focus is elsewhere. */ #dragDropIndicatorPos: number | null = null; #epochMapper = new EpochPositionMapper(); @@ -3501,6 +3511,26 @@ export class PresentationEditor extends EventEmitter { return null; } + /** + * Whether an element is fully within the vertical bounds of the active scroll container. + * Used by scrollToPosition's `ifNeeded` mode (SD-3315) to avoid moving the viewport for a + * target that is already visible. Measures with getBoundingClientRect because inline match + * spans report clientHeight 0. Vertical-only: search navigation is a block-axis concern. + */ + #isElementFullyVisibleInScrollContainer(el: Element): boolean { + const rect = el.getBoundingClientRect(); + const viewport = + this.#scrollContainer instanceof Window + ? { top: 0, bottom: this.#scrollContainer.innerHeight } + : this.#scrollContainer instanceof Element + ? this.#scrollContainer.getBoundingClientRect() + : this.#visibleHost?.ownerDocument?.defaultView + ? { top: 0, bottom: this.#visibleHost.ownerDocument.defaultView.innerHeight } + : null; + if (!viewport) return false; + return rect.top >= viewport.top && rect.bottom <= viewport.bottom; + } + /** * Scroll the visible host so a given document position is brought into view. * @@ -3512,11 +3542,22 @@ export class PresentationEditor extends EventEmitter { * @param options - Scrolling options * @param options.block - Alignment within the viewport ('start' | 'center' | 'end' | 'nearest') * @param options.behavior - Scroll behavior ('auto' | 'smooth') + * @param options.ifNeeded - When true, skip movement if the target is already fully visible + * (downgrades to 'nearest'); off-screen targets still use `block`. Used by search navigation. + * @param options.suppressSelectionSyncScroll - When true, selection-sync auto-scroll is + * suppressed until this scroll's RAF re-assert runs, so it cannot fight this intentional + * scroll. Used by search navigation, whose find-input focus restore otherwise scrolls the + * viewport to a reverted/stale caret. * @returns True if the position could be mapped and scrolling was applied */ scrollToPosition( pos: number, - options: { block?: 'start' | 'center' | 'end' | 'nearest'; behavior?: ScrollBehavior } = {}, + options: { + block?: 'start' | 'center' | 'end' | 'nearest'; + behavior?: ScrollBehavior; + ifNeeded?: boolean; + suppressSelectionSyncScroll?: boolean; + } = {}, ): boolean { // Cancel any pending focus-scroll RAF so this intentional scroll is not undone // by the wrapOffscreenEditorFocus safety net (e.g. search navigation after focus). @@ -3534,7 +3575,9 @@ export class PresentationEditor extends EventEmitter { const clampedPos = Math.max(0, Math.min(pos, doc.content.size)); const behavior = options.behavior ?? 'auto'; - const block = options.block ?? 'center'; + // SD-3315: the caller's requested landing. In ifNeeded mode an already-visible match + // downgrades this to 'nearest' (computed per-target below) so it does not re-center. + const requestedBlock = options.block ?? 'center'; // Use a DOM marker + scrollIntoView so the browser finds the correct scroll container // (window, parent overflow container, etc.) without us guessing. @@ -3561,6 +3604,21 @@ export class PresentationEditor extends EventEmitter { // Find the specific element containing this position for precise centering const targetEl = this.#findElementAtPosition(pageEl, clampedPos); const elToScroll = targetEl ?? pageEl; + + // SD-3315: "scroll only if needed" mode for search navigation. When the caller + // opts in and we resolved the precise target element (the match span, not the + // page-div fallback), and that element is already fully inside the scroll + // container, downgrade the scroll to 'nearest' — a no-op for a fully-visible + // element — so next/previous does not re-center an already-visible match (the + // ~50px jump). We deliberately do NOT early-return: the scrollIntoView + RAF + // re-assert below also override the hidden editor's selection-sync scroll + // (dispatched .scrollIntoView()), which otherwise jumps the viewport to the + // hidden editor's geometry. A null targetEl (page fallback) or an off-screen / + // partially-clipped match keeps the requested block (center). + const block = + options.ifNeeded && targetEl && this.#isElementFullyVisibleInScrollContainer(targetEl) + ? 'nearest' + : requestedBlock; elToScroll.scrollIntoView({ block, inline: 'nearest', behavior }); // AIDEV-NOTE: SD-3045. Search nav (and any other caller of // scrollToPosition) places the viewport intentionally — usually @@ -3586,9 +3644,16 @@ export class PresentationEditor extends EventEmitter { // and is cheap. const win = this.#visibleHost.ownerDocument?.defaultView; if (win) { + // SD-3315: own the scroll until the RAF re-assert. The find-input focus restore fires + // a selectionUpdate that reverts the editor selection and would selection-sync-scroll + // the viewport to that stale caret before this RAF runs. Suppress that here and + // release after re-asserting, so normal selection scroll resumes next frame. Paired + // with the RAF below (set inside `if (win)` so it is always cleared). + if (options.suppressSelectionSyncScroll) this.#suppressSelectionScrollUntilRaf = true; win.requestAnimationFrame(() => { elToScroll.scrollIntoView({ block, inline: 'nearest', behavior }); this.#shouldScrollSelectionIntoView = false; + this.#suppressSelectionScrollUntilRaf = false; }); } return true; @@ -3764,11 +3829,19 @@ export class PresentationEditor extends EventEmitter { * @param options - Scrolling options * @param options.block - Alignment within the viewport ('start' | 'center' | 'end' | 'nearest') * @param options.behavior - Scroll behavior ('auto' | 'smooth') + * @param options.ifNeeded - When true, skip movement if the target is already fully visible + * (downgrades to 'nearest'); off-screen targets still use `block`. Used by search navigation. + * @param options.suppressSelectionSyncScroll - Forwarded to scrollToPosition; see there. * @returns Promise resolving to true if scrolling succeeded, false otherwise */ async scrollToPositionAsync( pos: number, - options: { block?: 'start' | 'center' | 'end' | 'nearest'; behavior?: ScrollBehavior } = {}, + options: { + block?: 'start' | 'center' | 'end' | 'nearest'; + behavior?: ScrollBehavior; + ifNeeded?: boolean; + suppressSelectionSyncScroll?: boolean; + } = {}, ): Promise { // Fast path: try sync scroll first (works if page already mounted) if (this.scrollToPosition(pos, options)) { @@ -3811,8 +3884,12 @@ export class PresentationEditor extends EventEmitter { return false; } - // Retry now that page is mounted - return this.scrollToPosition(pos, options); + // Retry now that page is mounted. Reaching this path means the target was on an unmounted + // (off-screen) page at call time, and #scrollPageIntoView above only scrolled the page into + // view — not the specific match, which can now sit at a viewport edge. Force ifNeeded:false so + // the match centers, instead of letting the now-edge-visible match downgrade to 'nearest' and + // skip centering (SD-3315 review). suppressSelectionSyncScroll is preserved via the spread. + return this.scrollToPosition(pos, { ...options, ifNeeded: false }); } /** @@ -3916,7 +3993,9 @@ export class PresentationEditor extends EventEmitter { async focusContentControl( entityId: string, options: { block?: 'start' | 'center' | 'end' | 'nearest'; behavior?: ScrollBehavior } = {}, - ): Promise<{ success: true } | { success: false; reason: 'not-ready' | 'invalid-id' | 'not-found' | 'not-reachable' }> { + ): Promise< + { success: true } | { success: false; reason: 'not-ready' | 'invalid-id' | 'not-found' | 'not-reachable' } + > { const editor = this.#editor; if (!editor) return { success: false, reason: 'not-ready' }; if (typeof entityId !== 'string' || entityId.length === 0) return { success: false, reason: 'invalid-id' }; @@ -7331,6 +7410,12 @@ export class PresentationEditor extends EventEmitter { * page into view to trigger mount; the next selection update handles precise scroll. */ #scrollActiveEndIntoView(pageIndex: number): void { + // SD-3315: a search-owned scroll is in flight (find next/previous). Do not let selection-sync + // scroll the viewport to the (reverted/stale) caret — the search scroll and its RAF re-assert + // own positioning for this window. The selection overlay still renders in #updateSelection; + // only this scroll is skipped. Cleared on the search scroll's RAF, so normal keyboard/pointer + // selection scroll resumes the next frame. + if (this.#suppressSelectionScrollUntilRaf) return; // Check if the target page is mounted before trusting rendered element positions. const pageIsMounted = !!this.#painterHost.querySelector(`[data-page-index="${pageIndex}"]`); if (!pageIsMounted) { diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/PresentationEditor.scrollToPosition.test.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/PresentationEditor.scrollToPosition.test.ts index 76c3e7b0f3..5083952f7a 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/PresentationEditor.scrollToPosition.test.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/PresentationEditor.scrollToPosition.test.ts @@ -649,6 +649,51 @@ describe('PresentationEditor - scrollToPosition', () => { expect(result).toBe(true); }); + it('centers a virtualized match after mount even with ifNeeded (SD-3315)', async () => { + // Reaching the async/mount path means the target was off-screen at call time, so it should + // center once mounted. The post-mount retry forces ifNeeded:false; without that, a match + // that ends up edge-visible after the page scrolls into view would downgrade to 'nearest' + // and skip centering. Pin innerHeight so the span counts as "visible" (where the old + // forwarded ifNeeded:true would have downgraded). + const originalInnerHeight = window.innerHeight; + Object.defineProperty(window, 'innerHeight', { value: 800, configurable: true }); + try { + editor = new PresentationEditor({ element: container, documentId: 'test-doc' }); + await vi.waitFor(() => expect(mockIncrementalLayout).toHaveBeenCalled()); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const pagesHost = container.querySelector('.presentation-editor__pages') as HTMLElement; + const span = document.createElement('span'); + span.dataset.pmStart = '140'; + span.dataset.pmEnd = '160'; + span.scrollIntoView = vi.fn(); + span.getBoundingClientRect = vi.fn( + () => ({ top: 100, bottom: 120, left: 0, right: 0, width: 0, height: 20 }) as DOMRect, + ); + // Mount page index 1 (covers pos 150) after the async call starts. + setTimeout(() => { + const mockPage = document.createElement('div'); + mockPage.setAttribute('data-page-index', '1'); + mockPage.scrollIntoView = vi.fn(); + mockPage.appendChild(span); + pagesHost.appendChild(mockPage); + }, 100); + + const result = await editor.scrollToPositionAsync(150, { + block: 'center', + ifNeeded: true, + suppressSelectionSyncScroll: true, + }); + + expect(result).toBe(true); + // Post-mount retry must center the now-visible match, not downgrade to 'nearest'. + expect(span.scrollIntoView).toHaveBeenCalledWith({ block: 'center', inline: 'nearest', behavior: 'auto' }); + expect(span.scrollIntoView).not.toHaveBeenCalledWith(expect.objectContaining({ block: 'nearest' })); + } finally { + Object.defineProperty(window, 'innerHeight', { value: originalInnerHeight, configurable: true }); + } + }); + it('should return false and warn when page fails to mount within timeout', async () => { const consoleWarnSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); @@ -672,4 +717,107 @@ describe('PresentationEditor - scrollToPosition', () => { consoleWarnSpy.mockRestore(); }); }); + + // SD-3315: "scroll only if needed" mode for search navigation. When the precise match + // element is already fully visible, scrolling is downgraded to 'nearest' (a no-op for a + // fully-visible element) so next/previous does not re-center an already-visible match. + // Off-screen matches keep the requested 'center'. The visibility check measures the match + // element against the scroll container; with no scrollable ancestor the container is the + // window, so we pin innerHeight for determinism. + describe('scrollToPosition — ifNeeded (SD-3315)', () => { + let originalInnerHeight: number; + + beforeEach(() => { + originalInnerHeight = window.innerHeight; + Object.defineProperty(window, 'innerHeight', { value: 800, configurable: true }); + }); + + afterEach(() => { + Object.defineProperty(window, 'innerHeight', { value: originalInnerHeight, configurable: true }); + }); + + /** Build a mounted page (index 0) with a match span at pos 50 whose rect we control. */ + async function setupMatchSpan(rect: { top: number; bottom: number }) { + editor = new PresentationEditor({ element: container, documentId: 'test-doc' }); + await vi.waitFor(() => expect(mockIncrementalLayout).toHaveBeenCalled()); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const pagesHost = container.querySelector('.presentation-editor__pages') as HTMLElement; + const mockPage = document.createElement('div'); + mockPage.setAttribute('data-page-index', '0'); + mockPage.scrollIntoView = vi.fn(); + + const span = document.createElement('span'); + span.dataset.pmStart = '45'; + span.dataset.pmEnd = '55'; + span.scrollIntoView = vi.fn(); + span.getBoundingClientRect = vi.fn( + () => + ({ + top: rect.top, + bottom: rect.bottom, + left: 0, + right: 0, + width: 0, + height: rect.bottom - rect.top, + }) as DOMRect, + ); + mockPage.appendChild(span); + pagesHost.appendChild(mockPage); + return { span, mockPage }; + } + + it('downgrades to nearest (no re-center) when the match is already fully visible', async () => { + const { span } = await setupMatchSpan({ top: 100, bottom: 120 }); // inside 0..800 + + const result = editor.scrollToPosition(50, { block: 'center', ifNeeded: true }); + + expect(result).toBe(true); + expect(span.scrollIntoView).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest', behavior: 'auto' }); + expect(span.scrollIntoView).not.toHaveBeenCalledWith(expect.objectContaining({ block: 'center' })); + }); + + it('centers when the match is off-screen, even in ifNeeded mode', async () => { + const { span } = await setupMatchSpan({ top: 1000, bottom: 1020 }); // below 800 → off-screen + + const result = editor.scrollToPosition(50, { block: 'center', ifNeeded: true }); + + expect(result).toBe(true); + expect(span.scrollIntoView).toHaveBeenCalledWith({ block: 'center', inline: 'nearest', behavior: 'auto' }); + }); + + it('centers when the match is partially clipped at the bottom edge', async () => { + const { span } = await setupMatchSpan({ top: 790, bottom: 815 }); // top in view, bottom past 800 + + editor.scrollToPosition(50, { block: 'center', ifNeeded: true }); + + expect(span.scrollIntoView).toHaveBeenCalledWith({ block: 'center', inline: 'nearest', behavior: 'auto' }); + }); + + it('does not change default behavior: a fully visible match still centers without ifNeeded', async () => { + const { span } = await setupMatchSpan({ top: 100, bottom: 120 }); + + editor.scrollToPosition(50, { block: 'center' }); + + expect(span.scrollIntoView).toHaveBeenCalledWith({ block: 'center', inline: 'nearest', behavior: 'auto' }); + }); + + it('centers via the page fallback (no precise match element) even in ifNeeded mode', async () => { + editor = new PresentationEditor({ element: container, documentId: 'test-doc' }); + await vi.waitFor(() => expect(mockIncrementalLayout).toHaveBeenCalled()); + await new Promise((resolve) => setTimeout(resolve, 50)); + + const pagesHost = container.querySelector('.presentation-editor__pages') as HTMLElement; + const mockPage = document.createElement('div'); + mockPage.setAttribute('data-page-index', '0'); + mockPage.scrollIntoView = vi.fn(); + // No span with pm data → #findElementAtPosition returns null → page fallback, not measurable. + pagesHost.appendChild(mockPage); + + const result = editor.scrollToPosition(50, { block: 'center', ifNeeded: true }); + + expect(result).toBe(true); + expect(mockPage.scrollIntoView).toHaveBeenCalledWith({ block: 'center', inline: 'nearest', behavior: 'auto' }); + }); + }); }); diff --git a/packages/super-editor/src/editors/v1/extensions/search/search.js b/packages/super-editor/src/editors/v1/extensions/search/search.js index 5b733a2cc2..8679aa08e3 100644 --- a/packages/super-editor/src/editors/v1/extensions/search/search.js +++ b/packages/super-editor/src/editors/v1/extensions/search/search.js @@ -577,13 +577,22 @@ export const Search = Extension.create({ if (dispatch) dispatch(tr); const presentationEditor = editor.presentationEditor; + // SD-3315: per-match navigation uses "scroll only if needed" — an already-visible + // match must not be re-centered (the ~50px jump). When the match is off-screen or + // partially clipped, scrollToPosition falls back to its normal block:'center' landing. + // suppressSelectionSyncScroll makes search nav own the scroll across the find-input + // focus-restore cycle: that focus blur fires a selectionUpdate which reverts the editor + // selection to its pre-search caret, and selection-sync would otherwise scroll the + // viewport to that stale caret — a jump/flash on every navigation. goToFirstMatch keeps + // plain centering for the initial jump. + const scrollOpts = { block: 'center', ifNeeded: true, suppressSelectionSyncScroll: true }; // Try sync scroll first — returns true when the page is mounted and in body mode. - const scrolled = presentationEditor?.scrollToPosition?.(from, { block: 'center' }) ?? false; + const scrolled = presentationEditor?.scrollToPosition?.(from, scrollOpts) ?? false; if (!scrolled) { // Async version handles virtualized (un-mounted) pages; fire-and-forget // because it will scroll once the target page mounts. - Promise.resolve(presentationEditor?.scrollToPositionAsync?.(from, { block: 'center' })).catch(() => {}); + Promise.resolve(presentationEditor?.scrollToPositionAsync?.(from, scrollOpts)).catch(() => {}); // DOM fallback for non-presentation contexts or when presentation // scroll cannot run (e.g. header/footer mode, no layout). diff --git a/packages/super-editor/src/editors/v1/extensions/search/search.scrollBehavior.test.js b/packages/super-editor/src/editors/v1/extensions/search/search.scrollBehavior.test.js index 49f52d6379..62680bd1bf 100644 --- a/packages/super-editor/src/editors/v1/extensions/search/search.scrollBehavior.test.js +++ b/packages/super-editor/src/editors/v1/extensions/search/search.scrollBehavior.test.js @@ -117,7 +117,14 @@ describe('goToSearchResult — scroll behavior', () => { const result = goToSearchResultFactory(match)(ctx); expect(result).toBe(true); - expect(scrollToPosition).toHaveBeenCalledWith(10, { block: 'center' }); + // SD-3315: per-match navigation opts into "scroll only if needed" and owns the scroll across + // the find-input focus restore (suppressSelectionSyncScroll), so selection-sync can't yank + // the viewport to the reverted caret. + expect(scrollToPosition).toHaveBeenCalledWith(10, { + block: 'center', + ifNeeded: true, + suppressSelectionSyncScroll: true, + }); // Async and DOM fallback should NOT fire when sync succeeds expect(scrollToPositionAsync).not.toHaveBeenCalled(); expect(ctx._domNode.scrollIntoView).not.toHaveBeenCalled(); @@ -136,7 +143,11 @@ describe('goToSearchResult — scroll behavior', () => { expect(result).toBe(true); expect(scrollToPosition).toHaveBeenCalled(); // Async should fire for virtualized pages - expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { block: 'center' }); + expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { + block: 'center', + ifNeeded: true, + suppressSelectionSyncScroll: true, + }); // DOM fallback should also fire expect(ctx._domNode.scrollIntoView).toHaveBeenCalledWith({ block: 'center', @@ -168,7 +179,11 @@ describe('goToSearchResult — scroll behavior', () => { const result = goToSearchResultFactory(match)(ctx); expect(result).toBe(true); - expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { block: 'center' }); + expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { + block: 'center', + ifNeeded: true, + suppressSelectionSyncScroll: true, + }); // Give the microtask queue a tick so the .catch() runs — no unhandled rejection await new Promise((r) => setTimeout(r, 0)); }); @@ -184,7 +199,11 @@ describe('goToSearchResult — scroll behavior', () => { expect(result).toBe(true); // scrollToPosition is undefined, so sync returns false (via ?. → undefined → ?? false) - expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { block: 'center' }); + expect(scrollToPositionAsync).toHaveBeenCalledWith(10, { + block: 'center', + ifNeeded: true, + suppressSelectionSyncScroll: true, + }); // DOM fallback should also fire since sync didn't succeed expect(ctx._domNode.scrollIntoView).toHaveBeenCalled(); });