diff --git a/CHANGELOG.md b/CHANGELOG.md index 939ca90d..448be565 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [Unreleased] + +### Fixed + +- Opening an inline-comment deep link (`#comment-`) no longer leaves the comment thread pinned in the wrong vertical position. The thread in the right-margin column (and the narrow-screen comment popover) could land hundreds of pixels above its highlighted passage and stay there when content above the passage reflowed *after* the thread was positioned — e.g. a web-font swap on first load, or a late-loading image or diagram. Threads now re-align whenever their highlighted passage moves, not only when the article is resized, so they track the highlight through any late layout shift. A normal click was never affected (it happens after the page has settled). + ## [0.1.27] - 2026-06-22 ### Added diff --git a/packages/viewer/src/components/PageContent.svelte b/packages/viewer/src/components/PageContent.svelte index 22f0def3..a92d0d7a 100644 --- a/packages/viewer/src/components/PageContent.svelte +++ b/packages/viewer/src/components/PageContent.svelte @@ -21,6 +21,7 @@ import IconButton from "$lib/ui/primitives/IconButton.svelte"; import Popover from "$lib/ui/primitives/Popover.svelte"; import { useElementSize } from "$lib/ui/hooks/useElementSize.svelte"; + import { useElementMove } from "$lib/ui/hooks/useElementMove.svelte"; import { useSelectionPopover } from "$lib/ui/hooks/useSelectionPopover.svelte"; import { useScrollIntoViewOnNav } from "$lib/ui/hooks/useScrollIntoViewOnNav.svelte"; import PageComments from "./comments/PageComments.svelte"; @@ -32,10 +33,37 @@ let articleRef: HTMLElement | undefined = $state(); let showSkeleton = $state(false); + // Bumped after a deep-link reveal to force one re-measure of the thread pin. + // On a cold load the thread's vertical offset is first computed while the page + // is still settling (a web-font subset reflowing the text above the highlight), + // and no observer catches that promptly: the article ResizeObserver misses the + // load burst, and observeMove — armed while the highlight is still off-screen — + // only falls back to its ~1s poll. By the time landOnComment reveals the + // target, the reflow has settled, so re-measuring then aligns it immediately. + let deepLinkSettleSeq = $state(0); + const docId = $derived(page.data ? documentIdFor(page.data.meta) : null); const articleSize = useElementSize(() => articleRef ?? null); + // Track the active inline highlight's position so the pinned thread re-aligns + // when content above it reflows (FOUT, late image/diagram load) — a move the + // article ResizeObserver can't see. Null when no inline thread is active or + // its passage isn't wrapped (resolved / orphaned), which is harmless. + const activeHighlightMove = useElementMove(() => { + // Read `items` so this re-evaluates (and observeMove re-subscribes to the + // fresh wrapper node) after a reconcile rebuilds the highlights — e.g. a + // live-reload replaces the article HTML, destroying the old wrapper while + // activeId/articleRef are unchanged. Mirrors the data-active effect below; + // querySelector alone is not reactive to that node swap. + void comments.items; + const id = comments.activeId; + if (!id || !articleRef) return null; + return articleRef.querySelector( + `rw-annotation[data-comment-id="${escapeId(id)}"]`, + ); + }); + // Text-selection state for the Add-comment popover. The hook owns the // captured Range, the article-relative anchor point, and dismiss-on-collapse. const selectionPopover = useSelectionPopover(() => articleRef ?? null, articleSize); @@ -144,6 +172,11 @@ if (comments.linkedId === id) return; // already landed (a racing tick won) if (revealCommentTarget(id, kind)) { comments.linkedId = id; + // Re-measure the pin now the target is revealed and the cold-load reflow + // has settled (see deepLinkSettleSeq). rAF so the scroll/layout flushes. + requestAnimationFrame(() => { + deepLinkSettleSeq++; + }); } }); } @@ -338,7 +371,15 @@ comments.activeLeft = null; return; } + // Recompute on article resize AND on the active highlight *moving* — the + // latter catches content above it reflowing (web-font swap, a late image / + // diagram load) which slides the highlight without changing the article's + // box, so the ResizeObserver alone would miss it. Also re-measure once right + // after a deep-link reveal, when the cold-load reflow has settled but no + // observer has fired yet (see deepLinkSettleSeq). void articleSize.version; + void activeHighlightMove.version; + void deepLinkSettleSeq; const anchor = getHighlightAnchor(activeId); comments.activeTop = anchor?.top ?? null; comments.activeLeft = anchor ? clampPopoverLeft(anchor.centerX, articleRef.clientWidth) : null; diff --git a/packages/viewer/src/lib/ui/hooks/__fixtures__/ElementMoveHarness.svelte b/packages/viewer/src/lib/ui/hooks/__fixtures__/ElementMoveHarness.svelte new file mode 100644 index 00000000..f19e90d4 --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/__fixtures__/ElementMoveHarness.svelte @@ -0,0 +1,18 @@ + + + +
diff --git a/packages/viewer/src/lib/ui/hooks/__fixtures__/intersection-observer-mock.ts b/packages/viewer/src/lib/ui/hooks/__fixtures__/intersection-observer-mock.ts new file mode 100644 index 00000000..3f0c0313 --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/__fixtures__/intersection-observer-mock.ts @@ -0,0 +1,47 @@ +// jsdom does not implement IntersectionObserver, and `observeMove` needs +// programmatic control over the callback to simulate the element moving. A +// minimal mock: store the callback + options, track observed elements, expose +// a `trigger(ratio)` helper. Mirrors the ResizeObserver mock in this folder. +export class MockIntersectionObserver { + static instances: MockIntersectionObserver[] = []; + callback: IntersectionObserverCallback; + options?: IntersectionObserverInit; + observed: Element[] = []; + disconnected = false; + + constructor(cb: IntersectionObserverCallback, options?: IntersectionObserverInit) { + this.callback = cb; + this.options = options; + MockIntersectionObserver.instances.push(this); + } + + observe(el: Element) { + this.observed.push(el); + } + + unobserve() {} + + disconnect() { + this.disconnected = true; + this.observed = []; + } + + takeRecords(): IntersectionObserverEntry[] { + return []; + } + + /** Simulate an intersection change at the given ratio (1 = unmoved). */ + trigger(ratio: number) { + this.callback( + this.observed.map( + (target) => ({ target, intersectionRatio: ratio }) as IntersectionObserverEntry, + ), + this as unknown as IntersectionObserver, + ); + } + + /** The most recently constructed (currently-armed) observer. */ + static get latest(): MockIntersectionObserver | undefined { + return MockIntersectionObserver.instances[MockIntersectionObserver.instances.length - 1]; + } +} diff --git a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts index b1c541b3..bd236ecf 100644 --- a/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/observeElement.svelte.ts @@ -9,6 +9,10 @@ * viewport-relative coordinates; skip it for size-only hooks, since size is * scroll-invariant and listening would only produce redundant work. * + * This observes the element's own box only. Position changes caused by content + * *above* it reflowing (which leave its box unchanged) are the job of + * `observeMove`; consumers that anchor to an element use both. + * * Implementation detail of `useElementSize` / `useAnchorOffset` — not exported * outside the hooks/ layer. */ diff --git a/packages/viewer/src/lib/ui/hooks/observeMove.svelte.ts b/packages/viewer/src/lib/ui/hooks/observeMove.svelte.ts new file mode 100644 index 00000000..ccda67f1 --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/observeMove.svelte.ts @@ -0,0 +1,102 @@ +/** + * Run `onMove(el)` whenever the element's *position* changes — even when its + * own box does not resize. Re-subscribes automatically when the getter starts + * returning a different element, and tears down on cleanup. + * + * This fills the gap a `ResizeObserver` cannot: when content *above* an element + * reflows (web-font swap / FOUT, a late-loading image or diagram, an expanding + * block), the element slides down without its own width/height changing — so a + * ResizeObserver on it (or on an ancestor whose box is unchanged) never fires. + * Positioning that is anchored to such an element (a comment thread pinned to + * its highlight) would otherwise go stale. + * + * Implemented with an `IntersectionObserver` whose `rootMargin` is computed to + * frame the element exactly, so the observed ratio drops the instant the + * element moves; the callback then re-measures and re-arms. This is the + * mechanism behind Floating UI's `autoUpdate({ layoutShift: true })`, ported + * here (MIT-licensed) so the viewer needs no extra dependency. + * + * Like a scroll listener it also fires on scroll (the element moves in the + * viewport); consumers whose computed value is scroll-invariant simply + * recompute the same value — cheap and idempotent. + * + * Implementation detail of the hooks/ layer — not exported outside it. + */ +export function observeMove( + getEl: () => HTMLElement | null, + onMove: (el: HTMLElement) => void, +): void { + $effect(() => { + const el = getEl(); + if (!el || typeof IntersectionObserver === "undefined") return; + + let alive = true; + let io: IntersectionObserver | null = null; + let timeoutId: ReturnType | undefined; + const root = el.ownerDocument.documentElement; + + function cleanup() { + if (timeoutId !== undefined) clearTimeout(timeoutId); + timeoutId = undefined; + io?.disconnect(); + io = null; + } + + function refresh(skip: boolean, threshold: number) { + cleanup(); + if (!alive || !el) return; + + const { left, top, width, height } = el.getBoundingClientRect(); + if (!skip) onMove(el); + // A zero-area (detached / display:none) element can't be framed; bail and + // wait for the next re-subscribe to re-arm once it has a box again. + if (!width || !height) return; + + // Negative insets frame the element exactly, so threshold=1 means "fully + // in view at this precise position". Any movement drops the ratio. + const rootMargin = + `${-Math.floor(top)}px ${-Math.floor(root.clientWidth - (left + width))}px ` + + `${-Math.floor(root.clientHeight - (top + height))}px ${-Math.floor(left)}px`; + + let isFirstUpdate = true; + const handle = (entries: IntersectionObserverEntry[]) => { + // A delivery already queued when the effect tore down must not re-arm a + // timer on the dead closure (cleanup has already run and won't clear it). + if (!alive) return; + // Use the most recent entry; a batched callback is not ordered latest-last. + const ratio = entries[entries.length - 1].intersectionRatio; + if (ratio !== threshold) { + if (!isFirstUpdate) { + refresh(false, 1); + } else if (ratio) { + // Re-arm at the actual ratio so the next genuine move retriggers. + refresh(false, ratio); + } else { + // Ratio 0 (e.g. scrolled fully off-screen): poll back in a beat. + timeoutId = setTimeout(() => refresh(false, 1e-7), 1000); + } + } + isFirstUpdate = false; + }; + + const options: IntersectionObserverInit = { + rootMargin, + threshold: Math.max(0, Math.min(1, threshold)) || 1, + }; + try { + // Observe relative to the document so an ancestor's reflow is caught. + io = new IntersectionObserver(handle, { ...options, root: el.ownerDocument }); + } catch { + io = new IntersectionObserver(handle, options); + } + io.observe(el); + } + + refresh(true, 1); + + return () => { + alive = false; + cleanup(); + }; + }); +} diff --git a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.test.ts b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.test.ts index 709ba7e0..e5f85812 100644 --- a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.test.ts +++ b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.test.ts @@ -3,11 +3,14 @@ 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 { MockIntersectionObserver } from "./__fixtures__/intersection-observer-mock"; describe("useAnchorOffset", () => { beforeEach(() => { MockResizeObserver.instances = []; + MockIntersectionObserver.instances = []; vi.stubGlobal("ResizeObserver", MockResizeObserver); + vi.stubGlobal("IntersectionObserver", MockIntersectionObserver); }); afterEach(() => { @@ -191,4 +194,25 @@ describe("useAnchorOffset", () => { expect(out.dataset.top).toBe("500"); expect(out.dataset.width).toBe("30"); }); + + it("updates the rect fields when the anchor moves (content above it reflows)", () => { + // The anchor's own box is unchanged (no ResizeObserver fire) — it just slid + // down because content above it grew. observeMove must catch this. + let currentRect = { top: 100, left: 50, width: 100, height: 50 }; + const el = document.createElement("div"); + el.getBoundingClientRect = () => + ({ ...currentRect, right: 0, bottom: 0, x: 0, y: 0, toJSON: () => ({}) }) as DOMRect; + + const { getByTestId } = render(Harness, { el }); + const out = getByTestId("anchor-offset"); + expect(out.dataset.top).toBe("100"); + expect(MockIntersectionObserver.instances.length).toBeGreaterThanOrEqual(1); + + // Anchor slid down to 600 with no size change; the move observer fires. + currentRect = { top: 600, left: 50, width: 100, height: 50 }; + MockIntersectionObserver.latest!.trigger(0.5); + flushSync(); + + expect(out.dataset.top).toBe("600"); + }); }); diff --git a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts index 98697150..8fd6e6ca 100644 --- a/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts +++ b/packages/viewer/src/lib/ui/hooks/useAnchorOffset.svelte.ts @@ -1,14 +1,17 @@ import { observeElement } from "./observeElement.svelte"; +import { observeMove } from "./observeMove.svelte"; /** - * Track the viewport-relative bounding rect of an element across resizes and - * ancestor scrolls. + * Track the viewport-relative bounding rect of an element across resizes, + * ancestor scrolls, and position changes. * * Returns a reactive object whose `top`/`left`/`width`/`height` values update - * whenever the element 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. + * whenever the element resizes, the window resizes, any scroll container + * scrolls, or the element *moves* (content above it reflows — web-font swap, a + * late image/diagram load — sliding the anchor without resizing it). 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. Together these triggers mirror Floating UI's `autoUpdate`. * * 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 @@ -36,18 +39,17 @@ export interface AnchorOffset { export function useAnchorOffset(getEl: () => HTMLElement | null): AnchorOffset { const rect = $state({ top: 0, left: 0, width: 0, height: 0, measured: false }); - observeElement( - getEl, - (el) => { - const r = el.getBoundingClientRect(); - rect.top = r.top; - rect.left = r.left; - rect.width = r.width; - rect.height = r.height; - rect.measured = true; - }, - { trackWindow: true }, - ); + const measure = (el: HTMLElement) => { + const r = el.getBoundingClientRect(); + rect.top = r.top; + rect.left = r.left; + rect.width = r.width; + rect.height = r.height; + rect.measured = true; + }; + + observeElement(getEl, measure, { trackWindow: true }); + observeMove(getEl, measure); return { get top() { diff --git a/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.test.ts b/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.test.ts new file mode 100644 index 00000000..5da6c560 --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.test.ts @@ -0,0 +1,130 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from "vitest"; +import { flushSync } from "svelte"; +import { render } from "@testing-library/svelte"; +import Harness from "./__fixtures__/ElementMoveHarness.svelte"; +import { MockIntersectionObserver } from "./__fixtures__/intersection-observer-mock"; +import { makeAnchor } from "./__fixtures__/resize-observer-mock"; + +describe("useElementMove", () => { + beforeEach(() => { + MockIntersectionObserver.instances = []; + vi.stubGlobal("IntersectionObserver", MockIntersectionObserver); + }); + + afterEach(() => { + vi.unstubAllGlobals(); + }); + + it("arms an IntersectionObserver on mount without bumping version", () => { + const el = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + + const { getByTestId } = render(Harness, { el }); + const out = getByTestId("element-move"); + + // First refresh measures and arms, but does NOT count as a move. + expect(out.dataset.version).toBe("0"); + expect(MockIntersectionObserver.instances).toHaveLength(1); + expect(MockIntersectionObserver.instances[0].observed).toContain(el); + }); + + it("bumps version and re-arms (observing the element again) when it moves", () => { + const el = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + + const { getByTestId } = render(Harness, { el }); + const out = getByTestId("element-move"); + expect(out.dataset.version).toBe("0"); + + // Ratio < threshold (1) on the first update => the element moved => onMove. + MockIntersectionObserver.instances[0].trigger(0.5); + flushSync(); + + expect(out.dataset.version).toBe("1"); + // Re-armed with a fresh observer (the old one was disconnected) that is + // actually re-observing the element — otherwise later moves wouldn't fire. + expect(MockIntersectionObserver.instances.length).toBeGreaterThanOrEqual(2); + expect(MockIntersectionObserver.instances[0].disconnected).toBe(true); + expect(MockIntersectionObserver.latest!.observed).toContain(el); + }); + + it("keeps bumping version on subsequent moves (second update re-arms at threshold 1)", () => { + const el = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + + const { getByTestId } = render(Harness, { el }); + const out = getByTestId("element-move"); + + MockIntersectionObserver.latest!.trigger(0.5); // first move + flushSync(); + expect(out.dataset.version).toBe("1"); + + MockIntersectionObserver.latest!.trigger(0.3); // moved again + flushSync(); + expect(out.dataset.version).toBe("2"); + expect(MockIntersectionObserver.latest!.observed).toContain(el); + }); + + it("polls back via a timer when the element moves fully off-screen (ratio 0)", () => { + vi.useFakeTimers(); + try { + const el = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + + const { getByTestId } = render(Harness, { el }); + const out = getByTestId("element-move"); + const armedCount = MockIntersectionObserver.instances.length; + + // Ratio 0 => off-screen: no immediate move, a re-arm is scheduled. + MockIntersectionObserver.latest!.trigger(0); + flushSync(); + expect(out.dataset.version).toBe("0"); + expect(MockIntersectionObserver.instances.length).toBe(armedCount); + + // After the poll interval it re-arms (a fresh observer re-observes el). + vi.advanceTimersByTime(1000); + flushSync(); + expect(MockIntersectionObserver.instances.length).toBeGreaterThan(armedCount); + expect(MockIntersectionObserver.latest!.observed).toContain(el); + } finally { + vi.useRealTimers(); + } + }); + + it("does not arm an observer for a zero-area element", () => { + const el = makeAnchor({ top: 0, left: 0, width: 0, height: 0 }); + + render(Harness, { el }); + + expect(MockIntersectionObserver.instances).toHaveLength(0); + }); + + it("leaves version at zero when the element is null", () => { + const { getByTestId } = render(Harness, { el: null }); + const out = getByTestId("element-move"); + + expect(out.dataset.version).toBe("0"); + expect(MockIntersectionObserver.instances).toHaveLength(0); + }); + + it("disconnects the observer on unmount", () => { + const el = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + + const { unmount } = render(Harness, { el }); + const observer = MockIntersectionObserver.instances[0]; + expect(observer.disconnected).toBe(false); + + unmount(); + expect(observer.disconnected).toBe(true); + }); + + it("re-subscribes when the element prop changes", async () => { + const elA = makeAnchor({ top: 100, left: 0, width: 200, height: 20 }); + const elB = makeAnchor({ top: 300, left: 0, width: 200, height: 20 }); + + const { rerender } = render(Harness, { el: elA }); + const first = MockIntersectionObserver.instances[0]; + expect(first.observed).toContain(elA); + + await rerender({ el: elB }); + + expect(first.disconnected).toBe(true); + expect(MockIntersectionObserver.latest!.observed).toContain(elB); + }); +}); diff --git a/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.ts b/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.ts new file mode 100644 index 00000000..d07e715b --- /dev/null +++ b/packages/viewer/src/lib/ui/hooks/useElementMove.svelte.ts @@ -0,0 +1,35 @@ +import { observeMove } from "./observeMove.svelte"; + +/** + * Track an element's *position changes* as a monotonic `version` counter. + * + * Returns a reactive object whose `version` increments whenever the element + * moves — including when content above it reflows (web-font swap, late image / + * diagram load, an expanding block) and the element slides without its own box + * resizing. Consumers subscribe via `void move.version` to recompute a + * position-derived value that a `ResizeObserver` would miss. + * + * Companion to `useElementSize` (which tracks size). The getter form lets the + * target switch at runtime; the internal effect re-subscribes when the getter + * returns a different element. + */ +export interface ElementMove { + readonly version: number; +} + +export function useElementMove(getEl: () => HTMLElement | null): ElementMove { + const state = $state({ version: 0 }); + // Plain `let`, not `$state`: incrementing inside the observer callback must + // not create a self-dependency (mirrors useElementSize). + let counter = 0; + + observeMove(getEl, () => { + state.version = ++counter; + }); + + return { + get version() { + return state.version; + }, + }; +}