fix: comment scroll after painter refactor (#2629)

Author: harbournick

packages/super-editor/src/editors/v1/core/presentation-editor/PresentationEditor.ts

@@ -121,6 +121,11 @@ import { isHeaderFooterPartId } from '../parts/adapters/header-footer-part-descr
 import type { PartChangedEvent } from '../parts/types.js';
 import { isInRegisteredSurface } from './utils/uiSurfaceRegistry.js';
 import { buildSemanticFootnoteBlocks } from './semantic-flow-footnotes.js';
+
+type ThreadAnchorScrollPlan = {
+  achievedClientY: number;
+  applyScroll: (behavior: ScrollBehavior) => void;
+};
 import { splitRunsAtDecorationBoundaries } from './layout/SplitRunsAtDecorationBoundaries.js';
 
 import type { ResolveRangeOutput, DocumentApi } from '@superdoc/document-api';
@@ -2449,6 +2454,18 @@ export class PresentationEditor extends EventEmitter {
     }
   }
 
+  /**
+   * Return the viewport Y coordinate this thread anchor can actually reach after
+   * scroll bounds clamp the requested target.
+   *
+   * @param threadId - Comment or tracked-change identifier
+   * @param targetClientY - Desired top position in client/viewport coordinates
+   * @returns The reachable client Y, or null when the thread cannot be resolved
+   */
+  getReachableThreadAnchorClientY(threadId: string, targetClientY: number): number | null {
+    return this.#buildThreadAnchorScrollPlan(threadId, targetClientY)?.achievedClientY ?? null;
+  }
+
   /**
    * Scroll a comment or tracked-change anchor so its top edge lands at the
    * requested viewport Y coordinate.
@@ -2464,35 +2481,80 @@ export class PresentationEditor extends EventEmitter {
     targetClientY: number,
     options: { behavior?: ScrollBehavior } = {},
   ): boolean {
-    if (!threadId || !Number.isFinite(targetClientY)) return false;
+    const scrollPlan = this.#buildThreadAnchorScrollPlan(threadId, targetClientY);
+    if (!scrollPlan) return false;
+
+    const behavior = options.behavior ?? 'auto';
+    scrollPlan.applyScroll(behavior);
+    return true;
+  }
+
+  #buildThreadAnchorScrollPlan(threadId: string, targetClientY: number): ThreadAnchorScrollPlan | null {
+    if (!threadId || !Number.isFinite(targetClientY)) return null;
 
     const threadPosition = this.#collectCommentPositions()[threadId];
-    if (!threadPosition) return false;
+    if (!threadPosition) return null;
 
     const selectionBounds = this.getSelectionBounds(threadPosition.start, threadPosition.end);
     const currentTop = selectionBounds?.bounds?.top;
-    if (!Number.isFinite(currentTop)) return false;
+    if (!Number.isFinite(currentTop)) return null;
 
-    const deltaY = currentTop - targetClientY;
-    if (Math.abs(deltaY) < 1) return true;
-
-    const behavior = options.behavior ?? 'auto';
+    const requestedScrollDelta = currentTop - targetClientY;
     const scrollTarget = this.#scrollContainer ?? this.#visibleHost;
 
     if (scrollTarget instanceof Window) {
-      const currentScrollY = scrollTarget.scrollY ?? scrollTarget.pageYOffset ?? 0;
-      scrollTarget.scrollTo({ top: currentScrollY + deltaY, behavior });
-      return true;
+      return this.#buildWindowThreadAnchorScrollPlan(scrollTarget, currentTop, requestedScrollDelta);
     }
 
     if (scrollTarget instanceof HTMLElement) {
-      const maxScrollTop = Math.max(0, scrollTarget.scrollHeight - scrollTarget.clientHeight);
-      const nextScrollTop = Math.max(0, Math.min(maxScrollTop, scrollTarget.scrollTop + deltaY));
-      scrollTarget.scrollTo({ top: nextScrollTop, behavior });
-      return true;
+      return this.#buildElementThreadAnchorScrollPlan(scrollTarget, currentTop, requestedScrollDelta);
     }
 
-    return false;
+    return null;
+  }
+
+  #buildWindowThreadAnchorScrollPlan(
+    scrollTarget: Window,
+    currentTop: number,
+    requestedScrollDelta: number,
+  ): ThreadAnchorScrollPlan {
+    const scrollRoot =
+      scrollTarget.document.scrollingElement ??
+      scrollTarget.document.documentElement ??
+      scrollTarget.document.body ??
+      null;
+    const currentScrollTop = scrollTarget.scrollY ?? scrollTarget.pageYOffset ?? scrollRoot?.scrollTop ?? 0;
+    const viewportHeight = scrollTarget.innerHeight ?? scrollRoot?.clientHeight ?? 0;
+    const maxScrollTop = Math.max(0, (scrollRoot?.scrollHeight ?? 0) - viewportHeight);
+    const nextScrollTop = Math.max(0, Math.min(maxScrollTop, currentScrollTop + requestedScrollDelta));
+    const appliedScrollDelta = nextScrollTop - currentScrollTop;
+
+    return {
+      achievedClientY: currentTop - appliedScrollDelta,
+      applyScroll: (behavior) => {
+        if (Math.abs(appliedScrollDelta) < 1) return;
+        scrollTarget.scrollTo({ top: nextScrollTop, behavior });
+      },
+    };
+  }
+
+  #buildElementThreadAnchorScrollPlan(
+    scrollTarget: HTMLElement,
+    currentTop: number,
+    requestedScrollDelta: number,
+  ): ThreadAnchorScrollPlan {
+    const currentScrollTop = scrollTarget.scrollTop;
+    const maxScrollTop = Math.max(0, scrollTarget.scrollHeight - scrollTarget.clientHeight);
+    const nextScrollTop = Math.max(0, Math.min(maxScrollTop, currentScrollTop + requestedScrollDelta));
+    const appliedScrollDelta = nextScrollTop - currentScrollTop;
+
+    return {
+      achievedClientY: currentTop - appliedScrollDelta,
+      applyScroll: (behavior) => {
+        if (Math.abs(appliedScrollDelta) < 1) return;
+        scrollTarget.scrollTo({ top: nextScrollTop, behavior });
+      },
+    };
   }
 
   /**

packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts

@@ -53,9 +53,23 @@ const AUTO_SCROLL_MAX_SPEED_PX = 24;
 /** Tolerance for detecting scrollability to handle sub-pixel rounding in browsers */
 const SCROLL_DETECTION_TOLERANCE_PX = 1;
 const COMMENT_HIGHLIGHT_SELECTOR = '.superdoc-comment-highlight';
+const TRACK_CHANGE_SELECTOR = '[data-track-change-id]';
+const COMMENT_THREAD_HIT_TOLERANCE_PX = 3;
+const COMMENT_THREAD_HIT_SAMPLE_OFFSETS: ReadonlyArray<readonly [number, number]> = [
+  [0, 0],
+  [-COMMENT_THREAD_HIT_TOLERANCE_PX, 0],
+  [COMMENT_THREAD_HIT_TOLERANCE_PX, 0],
+  [0, -COMMENT_THREAD_HIT_TOLERANCE_PX],
+  [0, COMMENT_THREAD_HIT_TOLERANCE_PX],
+];
 
 const clamp = (value: number, min: number, max: number): number => Math.max(min, Math.min(max, value));
 
+type CommentThreadHit = {
+  isAmbiguous: boolean;
+  threadId: string | null;
+};
+
 /**
  * Block IDs for footnote content use prefix "footnote-{id}-" (see FootnotesBuilder).
  * Semantic footnote blocks use the {@link isSemanticFootnoteBlockId} helper from
@@ -83,6 +97,104 @@ function getCommentHighlightThreadIds(target: EventTarget | null): string[] {
     .filter(Boolean);
 }
 
+function resolveTrackChangeThreadId(target: EventTarget | null): string | null {
+  if (!(target instanceof Element)) {
+    return null;
+  }
+
+  const trackedChangeElement = target.closest(TRACK_CHANGE_SELECTOR);
+  const threadId = trackedChangeElement?.getAttribute('data-track-change-id')?.trim();
+
+  return threadId ? threadId : null;
+}
+
+function resolveCommentThreadHit(target: EventTarget | null): CommentThreadHit {
+  const threadIds = getCommentHighlightThreadIds(target);
+  if (threadIds.length > 1) {
+    return {
+      isAmbiguous: true,
+      threadId: null,
+    };
+  }
+
+  if (threadIds.length === 1) {
+    return {
+      isAmbiguous: false,
+      threadId: threadIds[0],
+    };
+  }
+
+  return {
+    isAmbiguous: false,
+    threadId: resolveTrackChangeThreadId(target),
+  };
+}
+
+function collectElementsNearPointerTarget(target: EventTarget | null, clientX: number, clientY: number): Element[] {
+  const candidates: Element[] = [];
+  const seen = new Set<Element>();
+  const ownerDocument = target instanceof Element ? target.ownerDocument : document;
+  const ownerWindow = ownerDocument.defaultView;
+
+  const addCandidate = (candidate: Element | null): void => {
+    if (!candidate || seen.has(candidate)) {
+      return;
+    }
+    seen.add(candidate);
+    candidates.push(candidate);
+  };
+
+  if (target instanceof Element) {
+    addCandidate(target);
+  }
+
+  if (typeof ownerDocument.elementsFromPoint !== 'function' || !ownerWindow) {
+    return candidates;
+  }
+
+  const maxX = Math.max(ownerWindow.innerWidth - 1, 0);
+  const maxY = Math.max(ownerWindow.innerHeight - 1, 0);
+
+  for (const [offsetX, offsetY] of COMMENT_THREAD_HIT_SAMPLE_OFFSETS) {
+    const sampleX = clamp(clientX + offsetX, 0, maxX);
+    const sampleY = clamp(clientY + offsetY, 0, maxY);
+    const elements = ownerDocument.elementsFromPoint(sampleX, sampleY);
+
+    for (const element of elements) {
+      addCandidate(element);
+    }
+  }
+
+  return candidates;
+}
+
+function resolveCommentThreadIdNearPointer(
+  target: EventTarget | null,
+  clientX: number,
+  clientY: number,
+): string | null {
+  const directHit = resolveCommentThreadHit(target);
+  if (directHit.isAmbiguous || directHit.threadId) {
+    return directHit.threadId;
+  }
+
+  // Painter output can split one visible annotation into adjacent runs. Sampling
+  // a few nearby points keeps narrow gaps from falling through to generic caret
+  // placement while still refusing ambiguous overlapping highlights.
+  const nearbyElements = collectElementsNearPointerTarget(target, clientX, clientY);
+  for (const element of nearbyElements) {
+    const hit = resolveCommentThreadHit(element);
+    if (hit.isAmbiguous) {
+      return null;
+    }
+    if (hit.threadId) {
+      return hit.threadId;
+    }
+  }
+
+  return null;
+}
+
 function getActiveCommentThreadId(editor: Editor): string | null {
   const pluginState = CommentsPluginKey.getState(editor.state) as { activeThreadId?: unknown } | null;
   const activeThreadId = pluginState?.activeThreadId;
@@ -94,18 +206,17 @@ function getActiveCommentThreadId(editor: Editor): string | null {
   return activeThreadId;
 }
 
-function shouldIgnoreRepeatClickOnActiveComment(target: EventTarget | null, activeThreadId: string | null): boolean {
+function shouldIgnoreRepeatClickOnActiveComment(
+  target: EventTarget | null,
+  clientX: number,
+  clientY: number,
+  activeThreadId: string | null,
+): boolean {
   if (!activeThreadId) {
     return false;
   }
 
-  const clickedThreadIds = getCommentHighlightThreadIds(target);
-
-  if (clickedThreadIds.length !== 1) {
-    return false;
-  }
-
-  return clickedThreadIds[0] === activeThreadId;
+  return resolveCommentThreadIdNearPointer(target, clientX, clientY) === activeThreadId;
 }
 
 // =============================================================================
@@ -924,6 +1035,10 @@ export class EditorInputManager {
     }
 
     const editor = this.#deps.getEditor();
+    if (this.#handleSingleCommentHighlightClick(event, target, editor)) {
+      return;
+    }
+
     if (this.#handleRepeatClickOnActiveComment(event, target, editor)) {
       return;
     }
@@ -2098,17 +2213,43 @@ export class EditorInputManager {
   #handleRepeatClickOnActiveComment(event: PointerEvent, target: HTMLElement | null, editor: Editor): boolean {
     const activeThreadId = getActiveCommentThreadId(editor);
 
-    if (!shouldIgnoreRepeatClickOnActiveComment(target, activeThreadId)) {
+    if (!shouldIgnoreRepeatClickOnActiveComment(target, event.clientX, event.clientY, activeThreadId)) {
       return false;
     }
 
     event.preventDefault();
-    this.#focusEditor();
     editor.emit?.('commentsUpdate', {
       type: comments_module_events.SELECTED,
       activeCommentId: activeThreadId,
     });
 
     return true;
   }
+
+  #handleSingleCommentHighlightClick(event: PointerEvent, target: HTMLElement | null, editor: Editor): boolean {
+    const clickedThreadId = resolveCommentThreadIdNearPointer(target, event.clientX, event.clientY);
+    if (!clickedThreadId) {
+      return false;
+    }
+
+    const activeThreadId = getActiveCommentThreadId(editor);
+    if (clickedThreadId === activeThreadId) {
+      return false;
+    }
+
+    event.preventDefault();
+
+    const didSetCursor = editor.commands?.setCursorById?.(clickedThreadId, {
+      activeCommentId: clickedThreadId,
+    });
+
+    if (!didSetCursor) {
+      editor.emit?.('commentsUpdate', {
+        type: comments_module_events.SELECTED,
+        activeCommentId: clickedThreadId,
+      });
+    }
+
+    return true;
+  }
 }