Merge pull request #3572 from superdoc-dev/caio/sd-3315-find-replace-scroll

fix(search): don't re-center visible matches on find navigation (SD-3315)

Author: caio-pizzol

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<boolean> {
     // 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) {

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

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).