From fd197bb78d87b7cb46d0f1e452a1d1cbab3dfcdd Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Wed, 29 Apr 2026 19:40:59 +0300 Subject: [PATCH 1/8] fix(viewer): keep Add-comment popover attached to highlighted text on scroll Previously the popover captured a viewport-fixed rect at mouseup, so any scroll detached it from the selection. Track the Range and re-measure on scroll/resize, mirroring how `observeElement` already does for element anchors. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/viewer/e2e/comments.spec.ts | 29 ++++++++++++ .../viewer/src/components/PageContent.svelte | 44 +++++++++++++++---- 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/packages/viewer/e2e/comments.spec.ts b/packages/viewer/e2e/comments.spec.ts index d06ce054..d851a77d 100644 --- a/packages/viewer/e2e/comments.spec.ts +++ b/packages/viewer/e2e/comments.spec.ts @@ -148,6 +148,35 @@ 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); + + // Re-measure runs on the scroll event; poll until the popover updates. + 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(); diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 07d5f3d6..0e5836b0 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -18,19 +18,45 @@ const articleSize = useElementSize(() => articleRef ?? null); - // Comment selection state + // Comment selection state. We hold the Range itself (not a snapshot rect) so + // we can re-measure on scroll/resize — the popover sits in `position: fixed` + // viewport coords and would otherwise detach from the highlighted text the + // moment anything scrolls. + let selectionRange: Range | null = $state.raw(null); let selectionRect: { x: number; y: number } | null = $state(null); + // Re-measure the range on scroll/resize so the popover stays attached. + // Capture-phase scroll catches ancestor scroll containers (e.g. when the + // viewer is embedded), matching how `observeElement` tracks element anchors. + $effect(() => { + const range = selectionRange; + if (!range) { + selectionRect = null; + return; + } + const measure = () => { + const rect = range.getBoundingClientRect(); + selectionRect = { x: rect.left + rect.width / 2, y: rect.top }; + }; + measure(); + window.addEventListener("scroll", measure, { capture: true, passive: true }); + window.addEventListener("resize", measure); + return () => { + window.removeEventListener("scroll", measure, { capture: true }); + window.removeEventListener("resize", measure); + }; + }); + // 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); @@ -232,7 +258,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 +268,14 @@ 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 }; + // Clone so the captured range is decoupled from the live Selection — the + // browser may reuse the same Range object for subsequent selections, which + // would defeat $state.raw identity tracking on a reassignment. + selectionRange = range.cloneRange(); } function handleMouseMove(event: MouseEvent) { @@ -335,7 +363,7 @@ comments.pending = { documentId: docId, selectors }; comments.activeId = null; - selectionRect = null; + selectionRange = null; window.getSelection()?.removeAllRanges(); } From defd06ba89c8ec41be1402f79d215efe784b2f97 Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Wed, 29 Apr 2026 19:52:35 +0300 Subject: [PATCH 2/8] fix(viewer): drop selection on page content change If live reload or navigation swapped the article DOM while a selection was active, the cached Range's start/end nodes got detached and getBoundingClientRect() returned zeros, briefly jumping the Add-comment popover to the top-left corner. Clear selectionRange when page.data identity changes so the popover dismisses cleanly. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/viewer/src/components/PageContent.svelte | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 0e5836b0..08215fe7 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -62,6 +62,15 @@ 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(() => { From dce6249fe1338f826fb0c7085728379148706b87 Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Thu, 30 Apr 2026 07:08:05 +0300 Subject: [PATCH 3/8] perf(viewer): skip re-measure write when selection rect is unchanged MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The scroll/resize handler reassigned `selectionRect` to a new object on every tick, even when x/y were identical — triggering Svelte to re-run the Popover's reactive bindings during scrolls. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/viewer/src/components/PageContent.svelte | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 08215fe7..9cb1ff92 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -36,7 +36,10 @@ } const measure = () => { const rect = range.getBoundingClientRect(); - selectionRect = { x: rect.left + rect.width / 2, y: rect.top }; + const x = rect.left + rect.width / 2; + const y = rect.top; + if (selectionRect && selectionRect.x === x && selectionRect.y === y) return; + selectionRect = { x, y }; }; measure(); window.addEventListener("scroll", measure, { capture: true, passive: true }); From 308087d631f00b0feeaa1579f0e8545e903239a3 Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Thu, 30 Apr 2026 07:21:02 +0300 Subject: [PATCH 4/8] refactor(viewer): extract useRangeRect hook for selection popover Replaces the bespoke scroll/resize listener wiring and equality guard in PageContent with a sibling to useAnchorOffset that tracks a Range's viewport rect. Per-field $state writes give the no-op-skip behavior implicitly, so the equality guard added in dce6249 is gone. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../viewer/src/components/PageContent.svelte | 40 +---- .../__fixtures__/RangeRectHarness.svelte | 26 ++++ .../src/lib/ui/hooks/useRangeRect.svelte.ts | 72 +++++++++ .../lib/ui/hooks/useRangeRect.test.svelte.ts | 146 ++++++++++++++++++ 4 files changed, 252 insertions(+), 32 deletions(-) create mode 100644 packages/viewer/src/lib/ui/hooks/__fixtures__/RangeRectHarness.svelte create mode 100644 packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts create mode 100644 packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 9cb1ff92..ff7776f9 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/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 { useRangeRect } from "$lib/ui/hooks/useRangeRect.svelte"; import PageComments from "./comments/PageComments.svelte"; const ctx = getRwContext(); @@ -19,36 +20,11 @@ const articleSize = useElementSize(() => articleRef ?? null); // Comment selection state. We hold the Range itself (not a snapshot rect) so - // we can re-measure on scroll/resize — the popover sits in `position: fixed` - // viewport coords and would otherwise detach from the highlighted text the - // moment anything scrolls. + // useRangeRect can re-measure on scroll/resize — the popover sits in + // `position: fixed` viewport coords and would otherwise detach from the + // highlighted text the moment anything scrolls. let selectionRange: Range | null = $state.raw(null); - let selectionRect: { x: number; y: number } | null = $state(null); - - // Re-measure the range on scroll/resize so the popover stays attached. - // Capture-phase scroll catches ancestor scroll containers (e.g. when the - // viewer is embedded), matching how `observeElement` tracks element anchors. - $effect(() => { - const range = selectionRange; - if (!range) { - selectionRect = null; - return; - } - const measure = () => { - const rect = range.getBoundingClientRect(); - const x = rect.left + rect.width / 2; - const y = rect.top; - if (selectionRect && selectionRect.x === x && selectionRect.y === y) return; - selectionRect = { x, y }; - }; - measure(); - window.addEventListener("scroll", measure, { capture: true, passive: true }); - window.addEventListener("resize", measure); - return () => { - window.removeEventListener("scroll", measure, { capture: true }); - window.removeEventListener("resize", measure); - }; - }); + const selectionRect = useRangeRect(() => 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 @@ -380,7 +356,7 @@ } -{#if comments.enabled && selectionRect} +{#if comments.enabled && selectionRect.measured} + import { useRangeRect } from "../useRangeRect.svelte"; + + interface Props { + range: Range | null; + } + + let { range }: Props = $props(); + + const rect = useRangeRect(() => range); + + +
diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts new file mode 100644 index 00000000..1240ac2b --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts @@ -0,0 +1,72 @@ +/** + * Track the viewport-relative bounding rect of a `Range` across scrolls and + * resizes. Mirrors `useAnchorOffset`, but for `Range` targets, which can't be + * fed to a `ResizeObserver` — re-measurement is driven by window scroll + * (capture phase, so ancestor scroll containers count) and window resize. The + * underlying `$state` already skips no-op writes, so callers don't need to + * guard against scroll frames where the rect is unchanged. + * + * The getter form lets the target Range switch at runtime (or go to `null` + * when the selection is dropped); the internal `$effect` re-subscribes on + * change and tears down listeners when the Range goes away. + */ +export interface RangeRect { + readonly top: number; + readonly left: number; + readonly width: number; + readonly height: number; + /** + * False until the first `getBoundingClientRect` read completes, and again + * whenever the target Range becomes `null`. Consumers gate rendering on this + * to avoid a flash at (0, 0) before the first measurement (or after the + * Range disappears). + */ + readonly measured: boolean; +} + +export function useRangeRect(getRange: () => Range | null): RangeRect { + const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false }); + + $effect(() => { + const range = getRange(); + if (!range) { + rect.measured = false; + return; + } + + const measure = () => { + const r = range.getBoundingClientRect(); + rect.top = r.top; + rect.left = r.left; + rect.width = r.width; + rect.height = r.height; + rect.measured = true; + }; + + measure(); + window.addEventListener("scroll", measure, { capture: true, passive: true }); + window.addEventListener("resize", measure); + return () => { + window.removeEventListener("scroll", measure, { capture: true }); + window.removeEventListener("resize", measure); + }; + }); + + return { + get top() { + return rect.top; + }, + get left() { + return rect.left; + }, + get width() { + return rect.width; + }, + get height() { + return rect.height; + }, + get measured() { + return rect.measured; + }, + }; +} diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts new file mode 100644 index 00000000..db3e62a0 --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts @@ -0,0 +1,146 @@ +import { describe, it, expect, vi } from "vitest"; +import { flushSync } from "svelte"; +import { render } from "@testing-library/svelte"; +import Harness from "./__fixtures__/RangeRectHarness.svelte"; + +function makeRange(rect: Partial): Range { + const full = { top: 0, left: 0, width: 0, height: 0, ...rect }; + const range = { + getBoundingClientRect: () => + ({ ...full, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, + }; + return range as unknown as Range; +} + +describe("useRangeRect", () => { + it("populates the rect fields from getBoundingClientRect on mount", () => { + const range = makeRange({ top: 10, left: 20, width: 100, height: 50 }); + + const { getByTestId } = render(Harness, { range }); + const out = getByTestId("range-rect"); + + expect(out.dataset.top).toBe("10"); + expect(out.dataset.left).toBe("20"); + expect(out.dataset.width).toBe("100"); + expect(out.dataset.height).toBe("50"); + expect(out.dataset.measured).toBe("true"); + }); + + it("leaves measured=false when the range is null", () => { + const { getByTestId } = render(Harness, { range: null }); + const out = getByTestId("range-rect"); + + expect(out.dataset.measured).toBe("false"); + }); + + it("re-measures when window scrolls", () => { + let current = { top: 100, left: 50, width: 100, height: 50 }; + const range = { + getBoundingClientRect: () => + ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, + } as unknown as Range; + + const { getByTestId } = render(Harness, { range }); + const out = getByTestId("range-rect"); + expect(out.dataset.top).toBe("100"); + + current = { top: 20, left: 50, width: 100, height: 50 }; + window.dispatchEvent(new Event("scroll")); + flushSync(); + + expect(out.dataset.top).toBe("20"); + }); + + it("re-measures on ancestor scroll-container scrolls (capture phase)", () => { + let current = { top: 100, left: 50, width: 100, height: 50 }; + const range = { + getBoundingClientRect: () => + ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, + } as unknown as Range; + const scroller = document.createElement("div"); + document.body.appendChild(scroller); + + try { + const { getByTestId } = render(Harness, { range }); + const out = getByTestId("range-rect"); + expect(out.dataset.top).toBe("100"); + + current = { top: 40, left: 50, width: 100, height: 50 }; + scroller.dispatchEvent(new Event("scroll", { bubbles: false })); + flushSync(); + + expect(out.dataset.top).toBe("40"); + } finally { + scroller.remove(); + } + }); + + it("re-measures when window resizes", () => { + let current = { top: 10, left: 10, width: 100, height: 50 }; + const range = { + getBoundingClientRect: () => + ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, + } as unknown as Range; + + const { getByTestId } = render(Harness, { range }); + const out = getByTestId("range-rect"); + expect(out.dataset.left).toBe("10"); + + current = { top: 10, left: 200, width: 100, height: 50 }; + window.dispatchEvent(new Event("resize")); + flushSync(); + + expect(out.dataset.left).toBe("200"); + }); + + it("removes window listeners when the range becomes null", async () => { + const range = makeRange({ top: 10, left: 10, width: 100, height: 50 }); + const removeSpy = vi.spyOn(window, "removeEventListener"); + + const { rerender, getByTestId } = render(Harness, { range }); + const out = getByTestId("range-rect"); + expect(out.dataset.measured).toBe("true"); + + await rerender({ range: null }); + + const removed = removeSpy.mock.calls + .filter(([type]) => type === "scroll" || type === "resize") + .map(([type]) => type); + expect(removed).toContain("scroll"); + expect(removed).toContain("resize"); + expect(out.dataset.measured).toBe("false"); + + removeSpy.mockRestore(); + }); + + it("removes window listeners on unmount", () => { + const range = makeRange({ top: 10, left: 10, width: 100, height: 50 }); + const removeSpy = vi.spyOn(window, "removeEventListener"); + + const { unmount } = render(Harness, { range }); + unmount(); + + const removed = removeSpy.mock.calls + .filter(([type]) => type === "scroll" || type === "resize") + .map(([type]) => type); + expect(removed).toContain("scroll"); + expect(removed).toContain("resize"); + + removeSpy.mockRestore(); + }); + + it("re-subscribes when the range prop changes", async () => { + const rangeA = makeRange({ top: 10, left: 0, width: 100, height: 20 }); + const rangeB = makeRange({ top: 500, left: 0, width: 30, height: 40 }); + + const { getByTestId, rerender } = render(Harness, { range: rangeA }); + const out = getByTestId("range-rect"); + expect(out.dataset.top).toBe("10"); + expect(out.dataset.width).toBe("100"); + + await rerender({ range: rangeB }); + + expect(out.dataset.top).toBe("500"); + expect(out.dataset.width).toBe("30"); + }); +}); From d17324bae215442763bcdf4f96f3ec0b82df2c93 Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Thu, 30 Apr 2026 07:53:28 +0300 Subject: [PATCH 5/8] refactor(viewer): share window-listener wiring between useRangeRect and observeElement MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Generalize observeElement to accept Element | Range — ResizeObserver attaches only when the target is an Element. useRangeRect now delegates its scroll/resize listeners through observeElement instead of re-implementing the capture-phase scroll + resize pair, removing a near-duplicate of useAnchorOffset's wiring. Also consolidate the makeRange test fixture into the shared __fixtures__/resize-observer-mock module alongside makeAnchor (sharing a fakeRect builder), trim a few change-narration comments, and fix the cloneRange comment in PageContent — the real reason to clone is that Selection.getRangeAt(0) returns a live Range, not anything about \$state.raw identity tracking. Co-Authored-By: Claude Opus 4.7 (1M context) --- packages/viewer/e2e/comments.spec.ts | 1 - .../viewer/src/components/PageContent.svelte | 12 +++--- .../__fixtures__/resize-observer-mock.ts | 24 ++++++++---- .../src/lib/ui/hooks/observeElement.svelte.ts | 33 +++++++++------- .../src/lib/ui/hooks/useRangeRect.svelte.ts | 39 +++++++------------ .../lib/ui/hooks/useRangeRect.test.svelte.ts | 31 ++++----------- 6 files changed, 63 insertions(+), 77 deletions(-) diff --git a/packages/viewer/e2e/comments.spec.ts b/packages/viewer/e2e/comments.spec.ts index d851a77d..5d0f058c 100644 --- a/packages/viewer/e2e/comments.spec.ts +++ b/packages/viewer/e2e/comments.spec.ts @@ -165,7 +165,6 @@ test.describe("Inline comments", () => { const scrollDelta = 50; await page.evaluate((y) => window.scrollBy(0, y), scrollDelta); - // Re-measure runs on the scroll event; poll until the popover updates. await expect .poll(async () => (await commentButton.boundingBox())?.y, { timeout: 1000 }) .not.toBe(before!.y); diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index ff7776f9..1c412806 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -19,10 +19,9 @@ const articleSize = useElementSize(() => articleRef ?? null); - // Comment selection state. We hold the Range itself (not a snapshot rect) so - // useRangeRect can re-measure on scroll/resize — the popover sits in - // `position: fixed` viewport coords and would otherwise detach from the - // highlighted text the moment anything scrolls. + // The popover renders in `position: fixed` coords, so its anchor must + // re-measure on scroll. Hold the Range itself, not a one-shot rect, and let + // useRangeRect track it. let selectionRange: Range | null = $state.raw(null); const selectionRect = useRangeRect(() => selectionRange); @@ -260,9 +259,8 @@ return; } - // Clone so the captured range is decoupled from the live Selection — the - // browser may reuse the same Range object for subsequent selections, which - // would defeat $state.raw identity tracking on a reassignment. + // `Selection.getRangeAt(0)` returns a live Range that mutates when the + // user re-selects; clone so the captured range stays put. selectionRange = range.cloneRange(); } diff --git a/packages/viewer/src/lib/ui/hooks/__fixtures__/resize-observer-mock.ts b/packages/viewer/src/lib/ui/hooks/__fixtures__/resize-observer-mock.ts index da1d7da9..1b8a4a1d 100644 --- a/packages/viewer/src/lib/ui/hooks/__fixtures__/resize-observer-mock.ts +++ b/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): HTMLElement { - const el = document.createElement("div"); - const get = () => ({ +function fakeRect(rect: Partial): DOMRect { + return { top: 0, left: 0, width: 0, @@ -47,7 +44,20 @@ export function makeAnchor(rect: Partial): 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): HTMLElement { + const el = document.createElement("div"); + el.getBoundingClientRect = () => fakeRect(rect); return el; } + +// Accepts a getter for tests that mutate the rect across re-measure events +// (window scroll, resize), since a Range can't be `observe()`-d. +export function makeRange(rect: Partial | (() => Partial)): Range { + const get = typeof rect === "function" ? rect : () => rect; + return { getBoundingClientRect: () => fakeRect(get()) } as unknown as Range; +} diff --git a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts index b1c541b3..cf66db64 100644 --- a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts @@ -1,37 +1,44 @@ /** - * Run `onMeasure(el)` once on subscription and again whenever the element + * Run `onMeasure(target)` once on subscription and again whenever the target * resizes. Re-subscribes automatically when the getter starts returning a - * different element, and tears down the observer (and any window listeners) + * 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. * - * Implementation detail of `useElementSize` / `useAnchorOffset` — not exported - * outside the hooks/ layer. + * Implementation detail of `useElementSize` / `useAnchorOffset` / + * `useRangeRect` — not exported outside the hooks/ layer. */ -export function observeElement( - getEl: () => HTMLElement | null, - onMeasure: (el: HTMLElement) => void, +export function observeElement( + getTarget: () => T | null, + onMeasure: (target: T) => void, { trackWindow = false }: { trackWindow?: boolean } = {}, ): void { $effect(() => { - const el = getEl(); - if (!el) return; + const target = getTarget(); + if (!target) return; - const measure = () => onMeasure(el); + const measure = () => onMeasure(target); measure(); - const observer = new ResizeObserver(measure); - observer.observe(el); + 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(); + observer?.disconnect(); if (trackWindow) { window.removeEventListener("scroll", measure, { capture: true }); window.removeEventListener("resize", measure); diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts index 1240ac2b..30f30feb 100644 --- a/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts @@ -1,14 +1,11 @@ +import { observeElement } from "./observeElement.svelte"; + /** * Track the viewport-relative bounding rect of a `Range` across scrolls and - * resizes. Mirrors `useAnchorOffset`, but for `Range` targets, which can't be - * fed to a `ResizeObserver` — re-measurement is driven by window scroll - * (capture phase, so ancestor scroll containers count) and window resize. The - * underlying `$state` already skips no-op writes, so callers don't need to - * guard against scroll frames where the rect is unchanged. - * - * The getter form lets the target Range switch at runtime (or go to `null` - * when the selection is dropped); the internal `$effect` re-subscribes on - * change and tears down listeners when the Range goes away. + * resizes. Sibling of `useAnchorOffset`, but for `Range` targets — Ranges + * can't be fed to a `ResizeObserver`, so re-measurement relies on the window + * scroll (capture phase) and resize listeners that `observeElement` sets up + * under `trackWindow`. */ export interface RangeRect { readonly top: number; @@ -28,29 +25,21 @@ export function useRangeRect(getRange: () => Range | null): RangeRect { const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false }); $effect(() => { - const range = getRange(); - if (!range) { - rect.measured = false; - return; - } + if (!getRange()) rect.measured = false; + }); - const measure = () => { + observeElement( + getRange, + (range) => { const r = range.getBoundingClientRect(); rect.top = r.top; rect.left = r.left; rect.width = r.width; rect.height = r.height; rect.measured = true; - }; - - measure(); - window.addEventListener("scroll", measure, { capture: true, passive: true }); - window.addEventListener("resize", measure); - return () => { - window.removeEventListener("scroll", measure, { capture: true }); - window.removeEventListener("resize", measure); - }; - }); + }, + { trackWindow: true }, + ); return { get top() { diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts index db3e62a0..459c1fc6 100644 --- a/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts @@ -2,15 +2,7 @@ import { describe, it, expect, vi } from "vitest"; import { flushSync } from "svelte"; import { render } from "@testing-library/svelte"; import Harness from "./__fixtures__/RangeRectHarness.svelte"; - -function makeRange(rect: Partial): Range { - const full = { top: 0, left: 0, width: 0, height: 0, ...rect }; - const range = { - getBoundingClientRect: () => - ({ ...full, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, - }; - return range as unknown as Range; -} +import { makeRange } from "./__fixtures__/resize-observer-mock"; describe("useRangeRect", () => { it("populates the rect fields from getBoundingClientRect on mount", () => { @@ -34,11 +26,8 @@ describe("useRangeRect", () => { }); it("re-measures when window scrolls", () => { - let current = { top: 100, left: 50, width: 100, height: 50 }; - const range = { - getBoundingClientRect: () => - ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, - } as unknown as Range; + let current: Partial = { top: 100, left: 50, width: 100, height: 50 }; + const range = makeRange(() => current); const { getByTestId } = render(Harness, { range }); const out = getByTestId("range-rect"); @@ -52,11 +41,8 @@ describe("useRangeRect", () => { }); it("re-measures on ancestor scroll-container scrolls (capture phase)", () => { - let current = { top: 100, left: 50, width: 100, height: 50 }; - const range = { - getBoundingClientRect: () => - ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, - } as unknown as Range; + let current: Partial = { top: 100, left: 50, width: 100, height: 50 }; + const range = makeRange(() => current); const scroller = document.createElement("div"); document.body.appendChild(scroller); @@ -76,11 +62,8 @@ describe("useRangeRect", () => { }); it("re-measures when window resizes", () => { - let current = { top: 10, left: 10, width: 100, height: 50 }; - const range = { - getBoundingClientRect: () => - ({ ...current, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect, - } as unknown as Range; + let current: Partial = { top: 10, left: 10, width: 100, height: 50 }; + const range = makeRange(() => current); const { getByTestId } = render(Harness, { range }); const out = getByTestId("range-rect"); From d413a40fb7a3fac2ac44408df9c65c2b681c4e2b Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Thu, 30 Apr 2026 08:22:04 +0300 Subject: [PATCH 6/8] refactor(viewer): fold useRangeRect into a generic useAnchorOffset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useRangeRect and useAnchorOffset had identical state shape, identical observeElement wiring, and identical getter return — only the target type differed. Generalize useAnchorOffset over `Element | Range` and delete the duplicate hook. Push the "measured = false when target becomes null" reset into useAnchorOffset itself, so popovers anchored to elements also hide cleanly when their anchor disappears. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../viewer/src/components/PageContent.svelte | 7 +- .../__fixtures__/AnchorOffsetHarness.svelte | 5 +- .../__fixtures__/RangeRectHarness.svelte | 26 ---- .../src/lib/ui/hooks/observeElement.svelte.ts | 4 +- .../lib/ui/hooks/useAnchorOffset.svelte.ts | 35 +++-- .../ui/hooks/useAnchorOffset.test.svelte.ts | 110 ++++++++++----- .../src/lib/ui/hooks/useRangeRect.svelte.ts | 61 --------- .../lib/ui/hooks/useRangeRect.test.svelte.ts | 129 ------------------ 8 files changed, 108 insertions(+), 269 deletions(-) delete mode 100644 packages/viewer/src/lib/ui/hooks/__fixtures__/RangeRectHarness.svelte delete mode 100644 packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts delete mode 100644 packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 1c412806..ee9cbc99 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -8,7 +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 { useRangeRect } from "$lib/ui/hooks/useRangeRect.svelte"; + import { useAnchorOffset } from "$lib/ui/hooks/useAnchorOffset.svelte"; import PageComments from "./comments/PageComments.svelte"; const ctx = getRwContext(); @@ -20,10 +20,9 @@ const articleSize = useElementSize(() => articleRef ?? null); // The popover renders in `position: fixed` coords, so its anchor must - // re-measure on scroll. Hold the Range itself, not a one-shot rect, and let - // useRangeRect track it. + // re-measure on scroll — hold the Range, not a one-shot rect. let selectionRange: Range | null = $state.raw(null); - const selectionRect = useRangeRect(() => selectionRange); + 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 diff --git a/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte b/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte index f2016274..19f2d87a 100644 --- a/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte +++ b/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` + collide with @testing-library/svelte's mount options. --> - -
diff --git a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts index cf66db64..7057bef9 100644 --- a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts @@ -13,8 +13,8 @@ * viewport-relative coordinates; skip it for size-only hooks, since size is * scroll-invariant and listening would only produce redundant work. * - * Implementation detail of `useElementSize` / `useAnchorOffset` / - * `useRangeRect` — not exported outside the hooks/ layer. + * Implementation detail of `useElementSize` / `useAnchorOffset` — not + * exported outside the hooks/ layer. */ export function observeElement( getTarget: () => T | null, diff --git a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts index 98697150..d4a40b21 100644 --- a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts @@ -1,18 +1,20 @@ import { observeElement } from "./observeElement.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,21 +27,28 @@ 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( + getTarget: () => T | null, +): AnchorOffset { const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false }); + $effect(() => { + if (!getTarget()) rect.measured = false; + }); + observeElement( - getEl, - (el) => { - const r = el.getBoundingClientRect(); + getTarget, + (target) => { + const r = target.getBoundingClientRect(); rect.top = r.top; rect.left = r.left; rect.width = r.width; diff --git a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.test.svelte.ts b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.test.svelte.ts index 709ba7e0..625cd7b2 100644 --- a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.test.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.test.svelte.ts @@ -2,7 +2,7 @@ import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; import { flushSync } from "svelte"; import { render } from "@testing-library/svelte"; import Harness from "./__fixtures__/AnchorOffsetHarness.svelte"; -import { MockResizeObserver, makeAnchor } from "./__fixtures__/resize-observer-mock"; +import { MockResizeObserver, makeAnchor, makeRange } from "./__fixtures__/resize-observer-mock"; describe("useAnchorOffset", () => { beforeEach(() => { @@ -15,9 +15,9 @@ describe("useAnchorOffset", () => { }); it("populates the rect fields from getBoundingClientRect on mount", () => { - const el = makeAnchor({ top: 10, left: 20, width: 100, height: 50 }); + const anchor = makeAnchor({ top: 10, left: 20, width: 100, height: 50 }); - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.top).toBe("10"); @@ -26,7 +26,7 @@ describe("useAnchorOffset", () => { expect(out.dataset.height).toBe("50"); }); - it("leaves the rect fields at zero when the element is null", () => { + it("leaves the rect fields at zero when the anchor is null", () => { const { getByTestId } = render(Harness, { el: null }); const out = getByTestId("anchor-offset"); @@ -39,34 +39,46 @@ describe("useAnchorOffset", () => { }); it("flips measured to true after the initial synchronous measurement", () => { - const el = makeAnchor({ top: 10, left: 20, width: 100, height: 50 }); + const anchor = makeAnchor({ top: 10, left: 20, width: 100, height: 50 }); - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.measured).toBe("true"); }); - it("keeps measured=true when the element switches to a new one", async () => { - const elA = makeAnchor({ top: 10, width: 100, height: 20 }); - const elB = makeAnchor({ top: 500, width: 30, height: 40 }); + it("flips measured back to false when the anchor becomes null after measure", async () => { + const anchor = makeAnchor({ top: 10, left: 20, width: 100, height: 50 }); - const { getByTestId, rerender } = render(Harness, { el: elA }); + const { getByTestId, rerender } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.measured).toBe("true"); - await rerender({ el: elB }); + await rerender({ el: null }); + + expect(out.dataset.measured).toBe("false"); + }); + + it("keeps measured=true when the anchor switches to a new one", async () => { + const anchorA = makeAnchor({ top: 10, width: 100, height: 20 }); + const anchorB = makeAnchor({ top: 500, width: 30, height: 40 }); + + const { getByTestId, rerender } = render(Harness, { el: anchorA }); + const out = getByTestId("anchor-offset"); + expect(out.dataset.measured).toBe("true"); + + await rerender({ el: anchorB }); expect(out.dataset.measured).toBe("true"); }); it("updates the rect fields when ResizeObserver fires", () => { let currentRect = { top: 0, left: 0, width: 100, height: 50 }; - const el = document.createElement("div"); - el.getBoundingClientRect = () => + const anchor = document.createElement("div"); + anchor.getBoundingClientRect = () => ({ ...currentRect, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect; - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.top).toBe("0"); expect(out.dataset.width).toBe("100"); @@ -82,9 +94,9 @@ describe("useAnchorOffset", () => { }); it("disconnects the observer on unmount", () => { - const el = makeAnchor({ width: 10, height: 10 }); + const anchor = makeAnchor({ width: 10, height: 10 }); - const { unmount } = render(Harness, { el }); + const { unmount } = render(Harness, { el: anchor }); const observer = MockResizeObserver.instances[0]; expect(observer.disconnected).toBe(false); @@ -94,11 +106,11 @@ describe("useAnchorOffset", () => { it("updates the rect fields when window scrolls", () => { let currentRect = { top: 100, left: 50, width: 100, height: 50 }; - const el = document.createElement("div"); - el.getBoundingClientRect = () => + const anchor = document.createElement("div"); + anchor.getBoundingClientRect = () => ({ ...currentRect, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect; - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.top).toBe("100"); @@ -112,14 +124,14 @@ describe("useAnchorOffset", () => { it("updates the rect fields when an ancestor scroll-container scrolls (capture phase)", () => { let currentRect = { top: 100, left: 50, width: 100, height: 50 }; const scroller = document.createElement("div"); - const el = document.createElement("div"); - scroller.appendChild(el); + const anchor = document.createElement("div"); + scroller.appendChild(anchor); document.body.appendChild(scroller); - el.getBoundingClientRect = () => + anchor.getBoundingClientRect = () => ({ ...currentRect, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect; try { - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.top).toBe("100"); @@ -135,11 +147,11 @@ describe("useAnchorOffset", () => { it("updates the rect fields when the window resizes", () => { let currentRect = { top: 10, left: 10, width: 100, height: 50 }; - const el = document.createElement("div"); - el.getBoundingClientRect = () => + const anchor = document.createElement("div"); + anchor.getBoundingClientRect = () => ({ ...currentRect, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect; - const { getByTestId } = render(Harness, { el }); + const { getByTestId } = render(Harness, { el: anchor }); const out = getByTestId("anchor-offset"); expect(out.dataset.left).toBe("10"); @@ -151,11 +163,11 @@ describe("useAnchorOffset", () => { }); it("removes window listeners on unmount", () => { - const el = makeAnchor({ top: 10, left: 10, width: 100, height: 50 }); + const anchor = makeAnchor({ top: 10, left: 10, width: 100, height: 50 }); const addSpy = vi.spyOn(window, "addEventListener"); const removeSpy = vi.spyOn(window, "removeEventListener"); - const { unmount } = render(Harness, { el }); + const { unmount } = render(Harness, { el: anchor }); const added = addSpy.mock.calls .filter(([type]) => type === "scroll" || type === "resize") .map(([type]) => type); @@ -173,22 +185,54 @@ describe("useAnchorOffset", () => { removeSpy.mockRestore(); }); - it("re-subscribes when the element prop changes", async () => { - const elA = makeAnchor({ top: 10, width: 100, height: 20 }); - const elB = makeAnchor({ top: 500, width: 30, height: 40 }); + it("re-subscribes when the anchor prop changes", async () => { + const anchorA = makeAnchor({ top: 10, width: 100, height: 20 }); + const anchorB = makeAnchor({ top: 500, width: 30, height: 40 }); - const { getByTestId, rerender } = render(Harness, { el: elA }); + const { getByTestId, rerender } = render(Harness, { el: anchorA }); const out = getByTestId("anchor-offset"); expect(out.dataset.top).toBe("10"); expect(out.dataset.width).toBe("100"); expect(MockResizeObserver.instances).toHaveLength(1); const firstObserver = MockResizeObserver.instances[0]; - await rerender({ el: elB }); + await rerender({ el: anchorB }); expect(firstObserver.disconnected).toBe(true); expect(MockResizeObserver.instances).toHaveLength(2); expect(out.dataset.top).toBe("500"); expect(out.dataset.width).toBe("30"); }); + + // Range targets share the same window scroll/resize plumbing as Element + // anchors — most behavior is covered above. These tests pin the + // Range-specific surface: no ResizeObserver subscription, and re-measure + // on window scroll still works. + describe("with a Range anchor", () => { + it("populates the rect fields without subscribing to ResizeObserver", () => { + const anchor = makeRange({ top: 10, left: 20, width: 100, height: 50 }); + + const { getByTestId } = render(Harness, { el: anchor }); + const out = getByTestId("anchor-offset"); + + expect(out.dataset.top).toBe("10"); + expect(out.dataset.measured).toBe("true"); + expect(MockResizeObserver.instances).toHaveLength(0); + }); + + it("re-measures when window scrolls", () => { + let current: Partial = { top: 100, left: 50, width: 100, height: 50 }; + const anchor = makeRange(() => current); + + const { getByTestId } = render(Harness, { el: anchor }); + const out = getByTestId("anchor-offset"); + expect(out.dataset.top).toBe("100"); + + current = { top: 20, left: 50, width: 100, height: 50 }; + window.dispatchEvent(new Event("scroll")); + flushSync(); + + expect(out.dataset.top).toBe("20"); + }); + }); }); diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts deleted file mode 100644 index 30f30feb..00000000 --- a/packages/viewer/src/lib/ui/hooks/useRangeRect.svelte.ts +++ /dev/null @@ -1,61 +0,0 @@ -import { observeElement } from "./observeElement.svelte"; - -/** - * Track the viewport-relative bounding rect of a `Range` across scrolls and - * resizes. Sibling of `useAnchorOffset`, but for `Range` targets — Ranges - * can't be fed to a `ResizeObserver`, so re-measurement relies on the window - * scroll (capture phase) and resize listeners that `observeElement` sets up - * under `trackWindow`. - */ -export interface RangeRect { - readonly top: number; - readonly left: number; - readonly width: number; - readonly height: number; - /** - * False until the first `getBoundingClientRect` read completes, and again - * whenever the target Range becomes `null`. Consumers gate rendering on this - * to avoid a flash at (0, 0) before the first measurement (or after the - * Range disappears). - */ - readonly measured: boolean; -} - -export function useRangeRect(getRange: () => Range | null): RangeRect { - const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false }); - - $effect(() => { - if (!getRange()) rect.measured = false; - }); - - observeElement( - getRange, - (range) => { - const r = range.getBoundingClientRect(); - rect.top = r.top; - rect.left = r.left; - rect.width = r.width; - rect.height = r.height; - rect.measured = true; - }, - { trackWindow: true }, - ); - - return { - get top() { - return rect.top; - }, - get left() { - return rect.left; - }, - get width() { - return rect.width; - }, - get height() { - return rect.height; - }, - get measured() { - return rect.measured; - }, - }; -} diff --git a/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts b/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts deleted file mode 100644 index 459c1fc6..00000000 --- a/packages/viewer/src/lib/ui/hooks/useRangeRect.test.svelte.ts +++ /dev/null @@ -1,129 +0,0 @@ -import { describe, it, expect, vi } from "vitest"; -import { flushSync } from "svelte"; -import { render } from "@testing-library/svelte"; -import Harness from "./__fixtures__/RangeRectHarness.svelte"; -import { makeRange } from "./__fixtures__/resize-observer-mock"; - -describe("useRangeRect", () => { - it("populates the rect fields from getBoundingClientRect on mount", () => { - const range = makeRange({ top: 10, left: 20, width: 100, height: 50 }); - - const { getByTestId } = render(Harness, { range }); - const out = getByTestId("range-rect"); - - expect(out.dataset.top).toBe("10"); - expect(out.dataset.left).toBe("20"); - expect(out.dataset.width).toBe("100"); - expect(out.dataset.height).toBe("50"); - expect(out.dataset.measured).toBe("true"); - }); - - it("leaves measured=false when the range is null", () => { - const { getByTestId } = render(Harness, { range: null }); - const out = getByTestId("range-rect"); - - expect(out.dataset.measured).toBe("false"); - }); - - it("re-measures when window scrolls", () => { - let current: Partial = { top: 100, left: 50, width: 100, height: 50 }; - const range = makeRange(() => current); - - const { getByTestId } = render(Harness, { range }); - const out = getByTestId("range-rect"); - expect(out.dataset.top).toBe("100"); - - current = { top: 20, left: 50, width: 100, height: 50 }; - window.dispatchEvent(new Event("scroll")); - flushSync(); - - expect(out.dataset.top).toBe("20"); - }); - - it("re-measures on ancestor scroll-container scrolls (capture phase)", () => { - let current: Partial = { top: 100, left: 50, width: 100, height: 50 }; - const range = makeRange(() => current); - const scroller = document.createElement("div"); - document.body.appendChild(scroller); - - try { - const { getByTestId } = render(Harness, { range }); - const out = getByTestId("range-rect"); - expect(out.dataset.top).toBe("100"); - - current = { top: 40, left: 50, width: 100, height: 50 }; - scroller.dispatchEvent(new Event("scroll", { bubbles: false })); - flushSync(); - - expect(out.dataset.top).toBe("40"); - } finally { - scroller.remove(); - } - }); - - it("re-measures when window resizes", () => { - let current: Partial = { top: 10, left: 10, width: 100, height: 50 }; - const range = makeRange(() => current); - - const { getByTestId } = render(Harness, { range }); - const out = getByTestId("range-rect"); - expect(out.dataset.left).toBe("10"); - - current = { top: 10, left: 200, width: 100, height: 50 }; - window.dispatchEvent(new Event("resize")); - flushSync(); - - expect(out.dataset.left).toBe("200"); - }); - - it("removes window listeners when the range becomes null", async () => { - const range = makeRange({ top: 10, left: 10, width: 100, height: 50 }); - const removeSpy = vi.spyOn(window, "removeEventListener"); - - const { rerender, getByTestId } = render(Harness, { range }); - const out = getByTestId("range-rect"); - expect(out.dataset.measured).toBe("true"); - - await rerender({ range: null }); - - const removed = removeSpy.mock.calls - .filter(([type]) => type === "scroll" || type === "resize") - .map(([type]) => type); - expect(removed).toContain("scroll"); - expect(removed).toContain("resize"); - expect(out.dataset.measured).toBe("false"); - - removeSpy.mockRestore(); - }); - - it("removes window listeners on unmount", () => { - const range = makeRange({ top: 10, left: 10, width: 100, height: 50 }); - const removeSpy = vi.spyOn(window, "removeEventListener"); - - const { unmount } = render(Harness, { range }); - unmount(); - - const removed = removeSpy.mock.calls - .filter(([type]) => type === "scroll" || type === "resize") - .map(([type]) => type); - expect(removed).toContain("scroll"); - expect(removed).toContain("resize"); - - removeSpy.mockRestore(); - }); - - it("re-subscribes when the range prop changes", async () => { - const rangeA = makeRange({ top: 10, left: 0, width: 100, height: 20 }); - const rangeB = makeRange({ top: 500, left: 0, width: 30, height: 40 }); - - const { getByTestId, rerender } = render(Harness, { range: rangeA }); - const out = getByTestId("range-rect"); - expect(out.dataset.top).toBe("10"); - expect(out.dataset.width).toBe("100"); - - await rerender({ range: rangeB }); - - expect(out.dataset.top).toBe("500"); - expect(out.dataset.width).toBe("30"); - }); -}); From 3f3f5c89d22653d418cb08ca02ef96d859033482 Mon Sep 17 00:00:00 2001 From: Mike Yumatov Date: Thu, 30 Apr 2026 08:37:03 +0300 Subject: [PATCH 7/8] refactor(viewer): rename observeElement and dedupe DOMRect test helpers `observeElement` now accepts `Element | Range`, so rename it to `observeTarget` to match the contract. Export `fakeRect` from the shared resize-observer mock and consume it in `overlay-testing`'s `mockRect`, removing the inlined DOMRect literal. Extend `makeAnchor` to accept a getter (mirroring `makeRange`) and collapse four hand-rolled `getBoundingClientRect` blocks in the hook tests. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../__fixtures__/AnchorOffsetHarness.svelte | 2 +- .../__fixtures__/resize-observer-mock.ts | 14 ++++++----- ...ment.svelte.ts => observeTarget.svelte.ts} | 2 +- .../lib/ui/hooks/useAnchorOffset.svelte.ts | 4 ++-- .../ui/hooks/useAnchorOffset.test.svelte.ts | 24 +++++++------------ .../src/lib/ui/hooks/useElementSize.svelte.ts | 4 ++-- .../__fixtures__/overlay-testing.ts | 16 +++---------- 7 files changed, 25 insertions(+), 41 deletions(-) rename packages/viewer/src/lib/ui/hooks/{observeElement.svelte.ts => observeTarget.svelte.ts} (96%) diff --git a/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte b/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte index 19f2d87a..9012eb6e 100644 --- a/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte +++ b/packages/viewer/src/lib/ui/hooks/__fixtures__/AnchorOffsetHarness.svelte @@ -5,7 +5,7 @@ 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` - collide with @testing-library/svelte's mount options. + both collide with Svelte 5's mount options. -->