fix(viewer): keep Add-comment popover attached to highlighted text on scroll

packages/viewer/e2e/comments.spec.ts

@@ -148,6 +148,34 @@ test.describe("Inline comments", () => {
     await expect(commentButton).toBeVisible();
   });
 
+  test("popover follows the highlighted text on scroll", async ({ page }) => {
+    // Shrink the viewport so the homepage overflows and the window scrolls.
+    await page.setViewportSize({ width: 1400, height: 400 });
+    await page.goto("/");
+    await page.getByRole("article").waitFor();
+
+    await selectText(page, "Navigation sidebar");
+
+    const commentButton = page.getByRole("button", { name: "Add comment" });
+    await expect(commentButton).toBeVisible();
+
+    const before = await commentButton.boundingBox();
+    expect(before).not.toBeNull();
+
+    const scrollDelta = 50;
+    await page.evaluate((y) => window.scrollBy(0, y), scrollDelta);
+
+    await expect
+      .poll(async () => (await commentButton.boundingBox())?.y, { timeout: 1000 })
+      .not.toBe(before!.y);
+
+    const after = await commentButton.boundingBox();
+    expect(after).not.toBeNull();
+    // Popover sits in viewport-fixed coords, so a downward scroll of N pushes
+    // the highlighted text — and therefore the popover — up by N.
+    expect(after!.y).toBeCloseTo(before!.y - scrollDelta, 0);
+  });
+
   test("clicking the selected text dismisses the comment popover", async ({ page }) => {
     await page.goto("/");
     await page.getByRole("article").waitFor();

packages/viewer/src/components/PageContent.svelte

@@ -8,6 +8,7 @@
   import Button from "$lib/ui/primitives/Button.svelte";
   import Popover from "$lib/ui/primitives/Popover.svelte";
   import { useElementSize } from "$lib/ui/hooks/useElementSize.svelte";
+  import { useAnchorOffset } from "$lib/ui/hooks/useAnchorOffset.svelte";
   import PageComments from "./comments/PageComments.svelte";
 
   const ctx = getRwContext();
@@ -18,24 +19,35 @@
 
   const articleSize = useElementSize(() => articleRef ?? null);
 
-  // Comment selection state
-  let selectionRect: { x: number; y: number } | null = $state(null);
+  // The popover renders in `position: fixed` coords, so its anchor must
+  // re-measure on scroll — hold the Range, not a one-shot rect.
+  let selectionRange: Range | null = $state.raw(null);
+  const selectionRect = useAnchorOffset(() => selectionRange);
 
   // Dismiss the popover when the selection collapses (e.g. user clicks on the
   // selected text). Blink runs the click-on-selection collapse as a default
   // action of `click`, so reading window.getSelection() inside `mouseup`
   // still returns the active range — handleMouseUp would re-pin the popover
   // at the same coords and only the highlight would disappear.
   $effect(() => {
-    if (!selectionRect) return;
+    if (!selectionRange) return;
     const handler = () => {
       const sel = window.getSelection();
-      if (!sel || sel.isCollapsed) selectionRect = null;
+      if (!sel || sel.isCollapsed) selectionRange = null;
     };
     document.addEventListener("selectionchange", handler);
     return () => document.removeEventListener("selectionchange", handler);
   });
 
+  // Drop any in-flight selection when the article content changes (live reload,
+  // navigation). The cached Range's start/end nodes get detached, so
+  // getBoundingClientRect() returns zeros and would briefly jump the popover to
+  // the top-left corner before anything else dismissed it.
+  $effect(() => {
+    void page.data;
+    selectionRange = null;
+  });
+
   // Delay skeleton appearance so fast page loads don't flash it.
   const SKELETON_DELAY_MS = 300;
   $effect(() => {
@@ -232,7 +244,7 @@
 
     // If no text selected, check if click landed on a comment highlight
     if (!selection || selection.isCollapsed) {
-      selectionRect = null;
+      selectionRange = null;
 
       // Toggle: click an inactive highlight to activate, click the active one to dismiss.
       const hitId = findCommentAtPoint(event);
@@ -242,12 +254,13 @@
 
     const range = selection.getRangeAt(0);
     if (!articleRef || !articleRef.contains(range.commonAncestorContainer)) {
-      selectionRect = null;
+      selectionRange = null;
       return;
     }
 
-    const rect = range.getBoundingClientRect();
-    selectionRect = { x: rect.left + rect.width / 2, y: rect.top };
+    // `Selection.getRangeAt(0)` returns a live Range that mutates when the
+    // user re-selects; clone so the captured range stays put.
+    selectionRange = range.cloneRange();
   }
 
   function handleMouseMove(event: MouseEvent) {
@@ -335,12 +348,12 @@
 
     comments.pending = { documentId: docId, selectors };
     comments.activeId = null;
-    selectionRect = null;
+    selectionRange = null;
     window.getSelection()?.removeAllRanges();
   }
 </script>
 
-{#if comments.enabled && selectionRect}
+{#if comments.enabled && selectionRect.measured}
   <!--
     Free-mode Popover anchored to the caret end of the current selection.
     `-translate-x-1/2 -translate-y-full` centers above the click point; the
@@ -349,8 +362,8 @@
   -->
   <Popover
     open
-    x={selectionRect.x}
-    y={selectionRect.y - 8}
+    x={selectionRect.left + selectionRect.width / 2}
+    y={selectionRect.top - 8}
     class="
       -translate-x-1/2 -translate-y-full rounded-lg border border-gray-200 bg-white shadow-lg
       dark:border-neutral-600 dark:bg-neutral-700

packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte

@@ -3,12 +3,15 @@
   lifecycle (mount / prop-change / unmount), so its internal $effect actually
   runs. Tests read the reactive values from data-* attributes rather than from
   a returned handle, since runes hooks can only be called from a component.
+
+  `el` is a slight misnomer when a Range is passed, but `target` and `anchor`
+  both collide with Svelte 5's mount options.
 -->
 <script lang="ts">
   import { useAnchorOffset } from "../useAnchorOffset.svelte";
 
   interface Props {
-    el: HTMLElement | null;
+    el: Element | Range | null;
   }
 
   let { el }: Props = $props();

packages/viewer/src/lib/ui/hooks/__fixtures__/resize-observer-mock.ts

@@ -32,11 +32,8 @@ export class MockResizeObserver {
   }
 }
 
-// Not redefining getBoundingClientRect on the prototype keeps other tests
-// (and any unrelated rects read from the document root) untouched.
-export function makeAnchor(rect: Partial<DOMRect>): HTMLElement {
-  const el = document.createElement("div");
-  const get = () => ({
+export function fakeRect(rect: Partial<DOMRect>): DOMRect {
+  return {
     top: 0,
     left: 0,
     width: 0,
@@ -47,7 +44,21 @@ export function makeAnchor(rect: Partial<DOMRect>): HTMLElement {
     y: 0,
     toJSON: () => ({}),
     ...rect,
-  });
-  el.getBoundingClientRect = () => get() as DOMRect;
+  } as DOMRect;
+}
+
+// Not redefining getBoundingClientRect on the prototype keeps other tests
+// (and any unrelated rects read from the document root) untouched.
+export function makeAnchor(rect: Partial<DOMRect> | (() => Partial<DOMRect>)): HTMLElement {
+  const get = typeof rect === "function" ? rect : () => rect;
+  const el = document.createElement("div");
+  el.getBoundingClientRect = () => fakeRect(get());
   return el;
 }
+
+export function makeRange(rect: Partial<DOMRect> | (() => Partial<DOMRect>)): Range {
+  const get = typeof rect === "function" ? rect : () => rect;
+  const range = document.createRange();
+  range.getBoundingClientRect = () => fakeRect(get());
+  return range;
+}

packages/viewer/src/lib/ui/hooks/observeTarget.svelte.ts

@@ -0,0 +1,55 @@
+/**
+ * Run `onMeasure(target)` once on subscription and again whenever the target
+ * resizes. Re-subscribes automatically when the getter starts returning a
+ * different target, and tears down the observer (and any window listeners)
+ * on cleanup.
+ *
+ * `target` can be an `Element` (observed via `ResizeObserver`) or a `Range`
+ * (no resize observer — Ranges aren't observable, so re-measurement relies
+ * solely on `trackWindow`).
+ *
+ * `trackWindow`: also re-measure on window scroll (capture phase, so ancestor
+ * scroll containers count) and window resize. Use this for hooks that report
+ * viewport-relative coordinates; skip it for size-only hooks, since size is
+ * scroll-invariant and listening would only produce redundant work.
+ *
+ * `onLost`: called whenever the getter returns `null`, so consumers can roll
+ * any "anchor disappeared" state in without spawning a second `$effect` that
+ * re-subscribes to the same getter.
+ *
+ * Implementation detail of `useElementSize` / `useAnchorOffset` — not
+ * exported outside the hooks/ layer.
+ */
+export function observeTarget<T extends Element | Range>(
+  getTarget: () => T | null,
+  onMeasure: (target: T) => void,
+  { trackWindow = false, onLost }: { trackWindow?: boolean; onLost?: () => void } = {},
+): void {
+  $effect(() => {
+    const target = getTarget();
+    if (!target) {
+      onLost?.();
+      return;
+    }
+
+    const measure = () => onMeasure(target);
+
+    measure();
+    let observer: ResizeObserver | null = null;
+    if (target instanceof Element) {
+      observer = new ResizeObserver(measure);
+      observer.observe(target);
+    }
+    if (trackWindow) {
+      window.addEventListener("scroll", measure, { capture: true, passive: true });
+      window.addEventListener("resize", measure);
+    }
+    return () => {
+      observer?.disconnect();
+      if (trackWindow) {
+        window.removeEventListener("scroll", measure, { capture: true });
+        window.removeEventListener("resize", measure);
+      }
+    };
+  });
+}

packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts

@@ -1,18 +1,20 @@
-import { observeElement } from "./observeElement.svelte";
+import { observeTarget } from "./observeTarget.svelte";
 
 /**
- * Track the viewport-relative bounding rect of an element across resizes and
- * ancestor scrolls.
+ * Track the viewport-relative bounding rect of a target across resizes and
+ * ancestor scrolls. The target may be an `Element` (observed via
+ * `ResizeObserver` for size changes) or a `Range` (re-measured only on
+ * window scroll/resize, since Ranges aren't observable).
  *
  * Returns a reactive object whose `top`/`left`/`width`/`height` values update
- * whenever the element resizes, the window resizes, or any scroll container
+ * whenever the target resizes, the window resizes, or any scroll container
  * scrolls. Consumers read the fields inside `$derived` (or directly in markup)
  * to drive positioning — typically for popover panels that need to sit beside
- * an external anchor.
+ * an external anchor or a selected text passage.
  *
  * The getter form lets the anchor target switch at runtime (e.g. when a parent
  * mounts a different trigger); the internal `$effect` re-subscribes whenever
- * the getter returns a different element.
+ * the getter returns a different target.
  *
  * Scroll tracking uses a capture-phase listener on `window`, which catches
  * scrolls from any ancestor scroll container without walking the DOM — this
@@ -25,28 +27,36 @@ export interface AnchorOffset {
   readonly width: number;
   readonly height: number;
   /**
-   * False until the first `getBoundingClientRect` read completes. Consumers
-   * use this to suppress a panel's initial paint at (0,0) while waiting for
-   * the anchor's coordinates — the classic Radix/Floating UI "positioning"
-   * state. True in all later states, including after the anchor changes.
+   * False until the first `getBoundingClientRect` read completes, and again
+   * whenever the target getter returns `null`. Consumers use this to suppress
+   * a panel's initial paint at (0,0) while waiting for coordinates — the
+   * classic Radix/Floating UI "positioning" state — and to hide the panel
+   * once the anchor disappears.
    */
   readonly measured: boolean;
 }
 
-export function useAnchorOffset(getEl: () => HTMLElement | null): AnchorOffset {
+export function useAnchorOffset<T extends Element | Range>(
+  getTarget: () => T | null,
+): AnchorOffset {
   const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false });
 
-  observeElement(
-    getEl,
-    (el) => {
-      const r = el.getBoundingClientRect();
+  observeTarget(
+    getTarget,
+    (target) => {
+      const r = target.getBoundingClientRect();
       rect.top = r.top;
       rect.left = r.left;
       rect.width = r.width;
       rect.height = r.height;
       rect.measured = true;
     },
-    { trackWindow: true },
+    {
+      trackWindow: true,
+      onLost: () => {
+        rect.measured = false;
+      },
+    },
   );
 
   return {