From b54e80514d060269f67587879774c17152dcbba5 Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:02:41 +0000 Subject: [PATCH 01/11] UN-4505 [FEAT] Add the v1 DocumentViewer with toolbar, zoom, keys and scroll handle - DocumentViewer composes the loader, status screens, toolbar and page list: , with scrollToPage and scrollToHighlight on the ref. A scroll asked for while loading runs once the pages are laid out, and the active highlight is scrolled into view when it changes. - ViewerToolbar: previous/next, a page box (n / N, Enter to jump), zoom -/+, the zoom level, fit width and fit page, each a labelled button reachable with Tab. - Zoom: fit width by default, fitted from the viewer's size with a ResizeObserver; -/+ step through 25%-400% from wherever a fit left it. A zoom change keeps the same spot of the same page in view. - Keys while the pages have focus: PageUp/PageDown, Home/End, + and -; modified keys are left to the browser. - PdfPageList scrolls by the pages' laid-out offsets, which are exact before any page renders, and reports the page showing most of itself. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 190 +++++++++++++ .../document-viewer/DocumentViewer.tsx | 263 ++++++++++++++++++ .../document-viewer/PdfPageList.test.tsx | 136 ++++++++- .../patterns/document-viewer/PdfPageList.tsx | 146 +++++++++- .../document-viewer/ViewerToolbar.test.tsx | 100 +++++++ .../document-viewer/ViewerToolbar.tsx | 180 ++++++++++++ frontend/src/v1/hooks/useElementSize.ts | 40 +++ frontend/src/v1/hooks/usePageWindow.ts | 45 ++- frontend/src/v1/lib/pdf/keyboard.test.ts | 19 ++ frontend/src/v1/lib/pdf/keyboard.ts | 41 +++ frontend/src/v1/lib/pdf/zoom.test.ts | 54 ++++ frontend/src/v1/lib/pdf/zoom.ts | 63 +++++ 12 files changed, 1262 insertions(+), 15 deletions(-) create mode 100644 frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx create mode 100644 frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx create mode 100644 frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx create mode 100644 frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx create mode 100644 frontend/src/v1/hooks/useElementSize.ts create mode 100644 frontend/src/v1/lib/pdf/keyboard.test.ts create mode 100644 frontend/src/v1/lib/pdf/keyboard.ts create mode 100644 frontend/src/v1/lib/pdf/zoom.test.ts create mode 100644 frontend/src/v1/lib/pdf/zoom.ts diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx new file mode 100644 index 0000000000..b9ea9e4701 --- /dev/null +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -0,0 +1,190 @@ +import { act, fireEvent, render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import type { OpenedPdf, PdfSource } from "@v1/lib/pdf/types"; +import type { PDFDocumentProxy } from "pdfjs-dist"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; +import { createRef } from "react"; +import { DocumentViewer, type DocumentViewerHandle } from "./DocumentViewer"; + +const opening = vi.hoisted( + (): { + resolve: (value: OpenedPdf) => void; + reject: (error: unknown) => void; + } => ({ resolve: () => undefined, reject: () => undefined }), +); + +vi.mock("@v1/lib/pdf/pdfjs", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + openPdfDocument: () => { + const promise = new Promise((resolve, reject) => { + opening.resolve = resolve; + opening.reject = reject; + }); + return { promise, destroy: vi.fn() }; + }, + }; +}); + +let resize: ((width: number, height: number) => void) | null = null; + +class FakeResizeObserver { + constructor(callback: (entries: ResizeObserverEntry[]) => void) { + resize = (width, height) => + callback([{ contentRect: { width, height } } as ResizeObserverEntry]); + } + observe() { + // Sizes arrive only when a test calls resize(). + } + disconnect() { + // Nothing to release. + } +} + +/** Reports nothing, so no page renders: these tests are about the chrome. */ +class FakeIntersectionObserver { + observe() { + // Never reports an intersection. + } + unobserve() { + // Nothing to stop. + } + disconnect() { + // Nothing to release. + } +} + +const source: PdfSource = { kind: "url", url: "/invoice.pdf" }; +const document = { + numPages: 3, + getPage: vi.fn(), +} as unknown as PDFDocumentProxy; +// US Letter: 816 CSS px wide at 100%. +const pageSizes = Array.from({ length: 3 }, () => ({ + width: 612, + height: 792, +})); + +async function open() { + await act(async () => opening.resolve({ document, pageSizes })); + // The body is 864 px wide: 816 + the 48 px fit gutter. + act(() => resize?.(864, 600)); +} + +function zoomLabel() { + return screen.getByLabelText("Zoom level").textContent; +} + +beforeEach(() => { + vi.stubGlobal("ResizeObserver", FakeResizeObserver); + vi.stubGlobal("IntersectionObserver", FakeIntersectionObserver); +}); +afterEach(() => vi.unstubAllGlobals()); + +it("shows the loading state, then the toolbar once the document opens", async () => { + render(); + expect(screen.getByRole("status")).toBeTruthy(); + expect(screen.queryByRole("toolbar")).toBeNull(); + await open(); + expect(screen.getByRole("toolbar")).toBeTruthy(); + expect(screen.getByText("/ 3")).toBeTruthy(); +}); + +it("shows a failure with Retry", async () => { + render(); + await act(async () => opening.reject(new Error("Network error"))); + expect(screen.getByRole("alert").textContent).toContain("Network error"); + expect(screen.getByRole("button", { name: "Retry" })).toBeTruthy(); +}); + +it("fits the width by default, and zooms in steps from there", async () => { + render(); + await open(); + expect(zoomLabel()).toBe("100%"); + expect( + screen + .getByRole("button", { name: "Fit width" }) + .getAttribute("aria-pressed"), + ).toBe("true"); + + await userEvent.click(screen.getByRole("button", { name: "Zoom in" })); + expect(zoomLabel()).toBe("125%"); + expect( + screen + .getByRole("button", { name: "Fit width" }) + .getAttribute("aria-pressed"), + ).toBe("false"); +}); + +it("refits when the viewer is resized", async () => { + render(); + await open(); + act(() => resize?.(456, 600)); + expect(zoomLabel()).toBe("50%"); +}); + +it("zooms with + and − while the pages have focus, and leaves other keys alone", async () => { + render(); + await open(); + const pages = screen.getByRole("region", { name: "Document pages" }); + fireEvent.keyDown(pages, { key: "+" }); + expect(zoomLabel()).toBe("125%"); + fireEvent.keyDown(pages, { key: "-" }); + fireEvent.keyDown(pages, { key: "-" }); + expect(zoomLabel()).toBe("75%"); + fireEvent.keyDown(pages, { key: "+", ctrlKey: true }); + expect(zoomLabel()).toBe("75%"); +}); + +/** Pages 1000 px tall, 16 px apart, as a browser would lay them out. */ +function layOutPages() { + for (const page of window.document.querySelectorAll( + "[data-page-index]", + )) { + const index = Number(page.getAttribute("data-page-index")); + Object.defineProperties(page, { + offsetTop: { value: 16 + index * 1016 }, + offsetHeight: { value: 1000 }, + }); + } + return screen.getByRole("region", { name: "Document pages" }); +} + +it("pages with End and Home while the pages have focus", async () => { + render(); + await open(); + const pages = layOutPages(); + fireEvent.keyDown(pages, { key: "End" }); + expect(pages.scrollTop).toBe(2 * 1016); + fireEvent.keyDown(pages, { key: "Home" }); + expect(pages.scrollTop).toBe(0); +}); + +it("runs a scroll asked for while loading once the pages are laid out", async () => { + // Every page element reports its browser layout from the moment it mounts. + const original = Object.getOwnPropertyDescriptor( + HTMLElement.prototype, + "offsetTop", + ); + Object.defineProperty(HTMLElement.prototype, "offsetTop", { + configurable: true, + get(this: HTMLElement) { + const index = this.getAttribute("data-page-index"); + return index === null ? 0 : 16 + Number(index) * 1016; + }, + }); + try { + const ref = createRef(); + render(); + act(() => ref.current?.scrollToPage(2)); + await open(); + expect( + screen.getByRole("region", { name: "Document pages" }).scrollTop, + ).toBe(2 * 1016); + } finally { + if (original) { + Object.defineProperty(HTMLElement.prototype, "offsetTop", original); + } + } +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx new file mode 100644 index 0000000000..980f626f85 --- /dev/null +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -0,0 +1,263 @@ +import { useElementSize } from "@v1/hooks/useElementSize"; +import { usePdfDocument } from "@v1/hooks/usePdfDocument"; +import { cn } from "@v1/lib/cn"; +import type { Highlight } from "@v1/lib/pdf/highlights"; +import { viewerKeyAction } from "@v1/lib/pdf/keyboard"; +import type { PageSize, PdfSource } from "@v1/lib/pdf/types"; +import { + fitZoom, + MAX_ZOOM, + MIN_ZOOM, + type ZoomMode, + zoomIn, + zoomOut, +} from "@v1/lib/pdf/zoom"; +import { + type KeyboardEvent, + type Ref, + useCallback, + useEffect, + useImperativeHandle, + useMemo, + useRef, + useState, +} from "react"; +import { PdfPageList, type PdfPageListHandle } from "./PdfPageList"; +import { ViewerStatus } from "./ViewerStatus"; +import { ViewerToolbar } from "./ViewerToolbar"; + +export interface DocumentViewerHandle { + /** Scrolls a 0-based page to the top. Waits for the document if it is loading. */ + scrollToPage: (index: number) => void; + /** Centres a highlight. Waits for the document if it is loading. */ + scrollToHighlight: (id: string) => void; +} + +export interface DocumentViewerProps { + source: PdfSource | null; + highlights?: readonly Highlight[]; + /** Shown in the active tone and scrolled into view when it changes. */ + activeHighlightId?: string | null; + onHighlightClick?: (highlight: Highlight) => void; + /** A zoom level (1 is 100%) or a fit; fit width by default. */ + initialZoom?: number | Exclude; + className?: string; + ref?: Ref; +} + +type ScrollRequest = + | { kind: "page"; index: number } + | { kind: "highlight"; id: string }; + +interface ZoomState { + mode: ZoomMode; + /** The level in custom mode; ignored while fitting. */ + level: number; +} + +/** + * Room around a fitted page, in CSS px: the list's padding on both sides plus + * a vertical scrollbar, so fitting never causes a horizontal scrollbar. + */ +const FIT_GUTTER = 48; + +// Gives each opened document its own key, so a new one gets a fresh page list: +// back at the top, with no window or anchor left over from the last. +const documentKeys = new WeakMap(); +let nextDocumentKey = 0; +function documentKey(document: object): number { + let key = documentKeys.get(document); + if (key === undefined) { + key = nextDocumentKey++; + documentKeys.set(document, key); + } + return key; +} + +function widest(pageSizes: readonly PageSize[]): PageSize | undefined { + let best: PageSize | undefined; + for (const size of pageSizes) { + if (!best || size.width > best.width) { + best = size; + } + } + return best; +} + +function initialZoomState( + initial: DocumentViewerProps["initialZoom"], +): ZoomState { + if (typeof initial === "number") { + return { mode: "custom", level: initial }; + } + return { mode: initial ?? "fit-width", level: 1 }; +} + +/** + * A PDF with a toolbar: loads it, shows its states, and renders its pages with + * highlights. Keys while the pages have focus: PageUp/PageDown, Home/End, + * + and −. + */ +export function DocumentViewer({ + source, + highlights, + activeHighlightId, + onHighlightClick, + initialZoom, + className, + ref, +}: DocumentViewerProps) { + const { state, retry, submitPassword } = usePdfDocument(source); + const [zoomState, setZoomState] = useState(() => + initialZoomState(initialZoom), + ); + const [pageIndex, setPageIndex] = useState(0); + const [bodyRef, bodySize] = useElementSize(); + const listRef = useRef(null); + const pendingRef = useRef(null); + + const ready = state.status === "ready"; + const pageSizes = ready ? state.pageSizes : undefined; + const pageCount = pageSizes?.length ?? 0; + + const zoom = useMemo(() => { + if (zoomState.mode === "custom" || !pageSizes || !bodySize) { + return zoomState.level; + } + const page = + zoomState.mode === "fit-width" + ? widest(pageSizes) + : (pageSizes[pageIndex] ?? pageSizes[0]); + if (!page) { + return zoomState.level; + } + return fitZoom( + page, + bodySize.width, + zoomState.mode === "fit-page" ? bodySize.height : null, + FIT_GUTTER, + ); + }, [zoomState, pageSizes, bodySize, pageIndex]); + + const run = useCallback((request: ScrollRequest) => { + const list = listRef.current; + if (!list) { + pendingRef.current = request; + return; + } + if (request.kind === "page") { + list.scrollToPage(request.index); + } else { + list.scrollToHighlight(request.id); + } + }, []); + + useImperativeHandle( + ref, + () => ({ + scrollToPage: (index) => run({ kind: "page", index }), + scrollToHighlight: (id) => run({ kind: "highlight", id }), + }), + [run], + ); + + // Requests made while loading run once the pages are laid out. + useEffect(() => { + if (ready && pendingRef.current) { + const request = pendingRef.current; + pendingRef.current = null; + run(request); + } + }, [ready, run]); + + useEffect(() => { + if (activeHighlightId) { + run({ kind: "highlight", id: activeHighlightId }); + } + }, [activeHighlightId, run]); + + const goToPage = useCallback( + (index: number) => run({ kind: "page", index }), + [run], + ); + const zoomBy = useCallback( + (step: (level: number) => number) => + setZoomState({ mode: "custom", level: step(zoom) }), + [zoom], + ); + + const onKeyDown = (event: KeyboardEvent) => { + const action = viewerKeyAction(event); + if (action === null) { + return; + } + event.preventDefault(); + switch (action) { + case "previous-page": + goToPage(pageIndex - 1); + break; + case "next-page": + goToPage(pageIndex + 1); + break; + case "first-page": + goToPage(0); + break; + case "last-page": + goToPage(pageCount - 1); + break; + case "zoom-in": + zoomBy(zoomIn); + break; + case "zoom-out": + zoomBy(zoomOut); + break; + } + }; + + return ( +
+ {ready && ( + MIN_ZOOM} + onPageChange={goToPage} + onZoomIn={() => zoomBy(zoomIn)} + onZoomOut={() => zoomBy(zoomOut)} + onFitWidth={() => setZoomState({ mode: "fit-width", level: zoom })} + onFitPage={() => setZoomState({ mode: "fit-page", level: zoom })} + /> + )} +
+ {ready ? ( + + ) : ( + + )} +
+
+ ); +} diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index aad2ebad47..f55bc6feaf 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -1,7 +1,8 @@ import { act, render } from "@testing-library/react"; import type { PDFDocumentProxy } from "pdfjs-dist"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; -import { PdfPageList } from "./PdfPageList"; +import { createRef } from "react"; +import { PdfPageList, type PdfPageListHandle } from "./PdfPageList"; /** An IntersectionObserver whose "in view" set the test controls. */ class FakeIntersectionObserver { @@ -21,12 +22,20 @@ class FakeIntersectionObserver { disconnect() { this.targets.clear(); } + emit(entries: object[]) { + this.callback(entries as unknown as IntersectionObserverEntry[]); + } show(indexes: number[]) { const entries = [...this.targets].map((target) => ({ target, isIntersecting: indexes.includes( Number(target.getAttribute("data-page-index")), ), + intersectionRatio: indexes.includes( + Number(target.getAttribute("data-page-index")), + ) + ? 1 + : 0, })); this.callback(entries as unknown as IntersectionObserverEntry[]); } @@ -262,3 +271,128 @@ it("draws each highlight on its own page", () => { container.querySelector('[data-page-index="1"] [data-highlight-id]'), ).toBeNull(); }); + +/** + * Gives the list a layout, which happy-dom does not compute: pages 800 × 600 + * at zoom 1, 16 px apart after 16 px of padding, in a 500 × 400 viewport. + */ +function layOut(container: HTMLElement, zoom: () => number) { + const root = container.querySelector("section"); + if (!root) { + throw new Error("no scroll region"); + } + Object.defineProperty(root, "clientHeight", { value: 400 }); + Object.defineProperty(root, "clientWidth", { value: 500 }); + for (const page of container.querySelectorAll( + "[data-page-index]", + )) { + const index = Number(page.getAttribute("data-page-index")); + Object.defineProperties(page, { + offsetTop: { get: () => 16 + index * (800 * zoom() + 16) }, + offsetHeight: { get: () => 800 * zoom() }, + offsetWidth: { get: () => 600 * zoom() }, + offsetLeft: { get: () => 16 }, + }); + } + return root; +} + +it("scrolls a page to the top exactly, with no wait", () => { + const { document } = fakeDocument(10); + const ref = createRef(); + const { container } = render( + , + ); + const root = layOut(container, () => 1); + act(() => ref.current?.scrollToPage(3)); + expect(root.scrollTop).toBe(3 * 816); + act(() => ref.current?.scrollToPage(99)); + expect(root.scrollTop).toBe(9 * 816); +}); + +it("centres a highlight, across as well as down for a box", () => { + const { document } = fakeDocument(10); + const ref = createRef(); + const highlights = [ + { + id: "w", + page: 2, + x: 0.5, + y: 0.25, + w: 0.1, + h: 0.05, + variant: "box" as const, + tone: "default" as const, + }, + ]; + const { container } = render( + , + ); + const root = layOut(container, () => 1); + let found = false; + act(() => { + found = ref.current?.scrollToHighlight("w") ?? false; + }); + expect(found).toBe(true); + // Page 2 starts at 1632; the box's centre is 0.275 × 800 = 220 into it. + expect(root.scrollTop).toBe(1632 + 220 - 200); + // 0.55 × 600 = 330 across, from the page's left edge at 16. + expect(root.scrollLeft).toBe(16 + 330 - 250); + expect(ref.current?.scrollToHighlight("missing")).toBe(false); +}); + +it("keeps the same spot of the same page in view across a zoom change", () => { + const { document } = fakeDocument(10); + let zoom = 1; + const { container, rerender } = render( + , + ); + const root = layOut(container, () => zoom); + show([0]); + // Half way down page 0. + root.scrollTop = 400; + act(() => { + root.dispatchEvent(new Event("scroll")); + }); + zoom = 2; + rerender( + , + ); + expect(root.scrollTop).toBe(800); +}); + +it("reports the page showing most of itself", () => { + const { document } = fakeDocument(10); + const onCurrentPageChange = vi.fn(); + render( + , + ); + act(() => { + const observer = FakeIntersectionObserver.current; + const target = (index: number) => + [...(observer?.targets ?? [])].find( + (t) => t.getAttribute("data-page-index") === String(index), + ); + observer?.emit([ + { target: target(4), isIntersecting: true, intersectionRatio: 0.25 }, + { target: target(5), isIntersecting: true, intersectionRatio: 0.75 }, + ]); + }); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(5); +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index 0c595b65a0..0fcfe0c1ea 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -3,11 +3,28 @@ import { cn } from "@v1/lib/cn"; import { type Highlight, highlightsByPage } from "@v1/lib/pdf/highlights"; import type { PageSize } from "@v1/lib/pdf/types"; import type { PDFDocumentProxy } from "pdfjs-dist"; -import { type ReactNode, useMemo } from "react"; +import { + type KeyboardEvent, + type ReactNode, + type Ref, + useCallback, + useEffect, + useImperativeHandle, + useLayoutEffect, + useMemo, + useRef, +} from "react"; import { PdfPage } from "./PdfPage"; const NO_HIGHLIGHTS: readonly Highlight[] = []; +export interface PdfPageListHandle { + /** Scrolls a 0-based page to the top of the viewer. */ + scrollToPage: (index: number) => void; + /** Centres a highlight in the viewer. False when no highlight has that id. */ + scrollToHighlight: (id: string) => boolean; +} + export interface PdfPageListProps { document: PDFDocumentProxy; pageSizes: readonly PageSize[]; @@ -17,15 +34,36 @@ export interface PdfPageListProps { highlights?: readonly Highlight[]; activeHighlightId?: string | null; onHighlightClick?: (highlight: Highlight) => void; + /** Called with the 0-based page showing most of itself. */ + onCurrentPageChange?: (index: number) => void; + onKeyDown?: (event: KeyboardEvent) => void; /** Drawn over a page, above its highlights; receives the 0-based page index. */ renderPageOverlay?: (index: number) => ReactNode; className?: string; + ref?: Ref; +} + +/** Where the reader is, kept across a zoom change: a page and a share of it. */ +interface Anchor { + index: number; + fraction: number; +} + +function pageElement(root: HTMLElement, index: number): HTMLElement | null { + return root.querySelector(`[data-page-index="${index}"]`); +} + +/** A page's top in scroll coordinates, so that page 0 sits at 0. */ +function pageTop(root: HTMLElement, page: HTMLElement): number { + const first = pageElement(root, 0); + return page.offsetTop - (first?.offsetTop ?? 0); } /** * Every page of a document in one scrolling column. All pages are laid out at - * their real size from the start, so the scrollbar is right, but only the - * pages in view (± 2) hold a canvas. + * their real size from the start, so the scrollbar is right and scrolling to + * any page or highlight lands exactly without waiting for a render, but only + * the pages in view (± 2) hold a canvas. */ export function PdfPageList({ document, @@ -34,15 +72,107 @@ export function PdfPageList({ highlights = NO_HIGHLIGHTS, activeHighlightId, onHighlightClick, + onCurrentPageChange, + onKeyDown, renderPageOverlay, className, + ref, }: PdfPageListProps) { - const { scrollRef, observePage, rendered } = usePageWindow(pageSizes.length); + const { scrollRef, observePage, rendered, current } = usePageWindow( + pageSizes.length, + ); const byPage = useMemo(() => highlightsByPage(highlights), [highlights]); + const rootRef = useRef(null); + const anchorRef = useRef({ index: 0, fraction: 0 }); + const currentRef = useRef(current); + currentRef.current = current; + + const setRoot = useCallback( + (element: HTMLElement | null) => { + rootRef.current = element; + scrollRef(element); + }, + [scrollRef], + ); + + useEffect(() => { + onCurrentPageChange?.(current); + }, [current, onCurrentPageChange]); + + const recordAnchor = useCallback(() => { + const root = rootRef.current; + const page = root && pageElement(root, currentRef.current); + if (!root || !page || page.offsetHeight === 0) { + return; + } + anchorRef.current = { + index: currentRef.current, + fraction: (root.scrollTop - pageTop(root, page)) / page.offsetHeight, + }; + }, []); + + // A zoom change resizes every page; keep the same spot of the same page at + // the top. Runs after the pages' own layout effects have resized them. + const previousZoom = useRef(zoom); + useLayoutEffect(() => { + if (previousZoom.current === zoom) { + return; + } + previousZoom.current = zoom; + const root = rootRef.current; + const { index, fraction } = anchorRef.current; + const page = root && pageElement(root, index); + if (root && page) { + root.scrollTop = pageTop(root, page) + fraction * page.offsetHeight; + } + }, [zoom]); + + useImperativeHandle( + ref, + () => ({ + scrollToPage(index) { + const root = rootRef.current; + const clamped = Math.min(Math.max(0, index), pageSizes.length - 1); + const page = root && pageElement(root, clamped); + if (root && page) { + root.scrollTop = pageTop(root, page); + anchorRef.current = { index: clamped, fraction: 0 }; + } + }, + scrollToHighlight(id) { + const highlight = highlights.find((candidate) => candidate.id === id); + const root = rootRef.current; + const page = highlight && root && pageElement(root, highlight.page); + if (!highlight || !root || !page) { + return false; + } + const centreY = (highlight.y + highlight.h / 2) * page.offsetHeight; + root.scrollTop = pageTop(root, page) + centreY - root.clientHeight / 2; + if (highlight.variant === "box") { + const centreX = (highlight.x + highlight.w / 2) * page.offsetWidth; + root.scrollLeft = page.offsetLeft + centreX - root.clientWidth / 2; + } + return true; + }, + }), + [highlights, pageSizes.length], + ); + + // A scrollable region must be focusable for keyboard users (WCAG 2.1.1), + // and its focus is where the viewer's paging and zoom keys are read. return ( -
{/* w-max with min-w-full: centred while pages fit, scrollable both ways once a zoomed page is wider than the viewer. */} @@ -67,6 +197,6 @@ export function PdfPageList({ ))}
- + ); } diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx new file mode 100644 index 0000000000..420d48790b --- /dev/null +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx @@ -0,0 +1,100 @@ +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { expect, it, vi } from "vitest"; +import { ViewerToolbar, type ViewerToolbarProps } from "./ViewerToolbar"; + +function setup(overrides: Partial = {}) { + const props: ViewerToolbarProps = { + pageIndex: 0, + pageCount: 12, + zoom: 1.25, + zoomMode: "custom", + canZoomIn: true, + canZoomOut: true, + onPageChange: vi.fn(), + onZoomIn: vi.fn(), + onZoomOut: vi.fn(), + onFitWidth: vi.fn(), + onFitPage: vi.fn(), + ...overrides, + }; + render(); + return props; +} + +it("shows the page as n / N and the zoom as a percentage", () => { + setup({ pageIndex: 2 }); + expect(screen.getByLabelText("Page").value).toBe("3"); + expect(screen.getByText("/ 12")).toBeTruthy(); + expect(screen.getByLabelText("Zoom level").textContent).toBe("125%"); +}); + +it("pages back and forward, and stops at either end", async () => { + const first = setup(); + expect( + screen.getByRole("button", { name: "Previous page" }) + .disabled, + ).toBe(true); + await userEvent.click(screen.getByRole("button", { name: "Next page" })); + expect(first.onPageChange).toHaveBeenCalledWith(1); +}); + +it("disables Next on the last page", () => { + setup({ pageIndex: 11 }); + expect( + screen.getByRole("button", { name: "Next page" }) + .disabled, + ).toBe(true); +}); + +it("jumps to a typed page on Enter and restores a page out of range", async () => { + const props = setup(); + const box = screen.getByLabelText("Page"); + await userEvent.clear(box); + await userEvent.type(box, "5{Enter}"); + expect(props.onPageChange).toHaveBeenCalledWith(4); + + await userEvent.clear(box); + await userEvent.type(box, "99{Enter}"); + expect(props.onPageChange).toHaveBeenCalledTimes(1); + expect(box.value).toBe("1"); +}); + +it("zooms, fits, and shows the active fit as pressed", async () => { + const props = setup({ zoomMode: "fit-width", canZoomIn: false }); + expect( + screen.getByRole("button", { name: "Zoom in" }).disabled, + ).toBe(true); + await userEvent.click(screen.getByRole("button", { name: "Zoom out" })); + await userEvent.click(screen.getByRole("button", { name: "Fit page" })); + expect(props.onZoomOut).toHaveBeenCalled(); + expect(props.onFitPage).toHaveBeenCalled(); + expect( + screen + .getByRole("button", { name: "Fit width" }) + .getAttribute("aria-pressed"), + ).toBe("true"); +}); + +it("is reachable with Tab, control by control", async () => { + setup({ pageIndex: 3 }); + const order = [ + "Previous page", + "Page", + "Next page", + "Zoom out", + "Zoom in", + "Fit width", + "Fit page", + ]; + for (const name of order) { + await userEvent.tab(); + const focused = document.activeElement; + const label = + focused?.getAttribute("aria-label") ?? + (focused?.id + ? document.querySelector(`label[for="${focused.id}"]`)?.textContent + : null); + expect(label).toBe(name); + } +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx new file mode 100644 index 0000000000..da20056582 --- /dev/null +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx @@ -0,0 +1,180 @@ +import { cn } from "@v1/lib/cn"; +import { formatZoom, type ZoomMode } from "@v1/lib/pdf/zoom"; +import { + ChevronLeft, + ChevronRight, + type LucideIcon, + MoveHorizontal, + Scan, + ZoomIn, + ZoomOut, +} from "lucide-react"; +import { type FormEvent, useId, useState } from "react"; + +export interface ViewerToolbarProps { + /** 0-based. */ + pageIndex: number; + pageCount: number; + zoom: number; + zoomMode: ZoomMode; + canZoomIn: boolean; + canZoomOut: boolean; + onPageChange: (index: number) => void; + onZoomIn: () => void; + onZoomOut: () => void; + onFitWidth: () => void; + onFitPage: () => void; + className?: string; +} + +const iconButton = + "ui:inline-flex ui:size-8 ui:shrink-0 ui:items-center ui:justify-center ui:rounded-md ui:text-foreground ui:hover:bg-accent ui:disabled:pointer-events-none ui:disabled:opacity-50 ui:aria-pressed:bg-accent ui:focus-visible:focus-ring"; + +function ToolbarButton({ + icon: Icon, + label, + onClick, + disabled, + pressed, +}: { + icon: LucideIcon; + label: string; + onClick: () => void; + disabled?: boolean; + pressed?: boolean; +}) { + return ( + + ); +} + +/** The page box: shows the current page, and jumps to a typed one on Enter. */ +function PageInput({ + pageIndex, + pageCount, + onPageChange, +}: Pick) { + const [draft, setDraft] = useState(String(pageIndex + 1)); + const id = useId(); + const commit = (event: FormEvent) => { + event.preventDefault(); + const page = Number.parseInt(draft, 10); + if (Number.isInteger(page) && page >= 1 && page <= pageCount) { + onPageChange(page - 1); + } else { + setDraft(String(pageIndex + 1)); + } + }; + return ( +
+ + setDraft(event.target.value)} + onBlur={() => setDraft(String(pageIndex + 1))} + className="ui:w-10 ui:rounded-md ui:border ui:border-input ui:bg-background ui:px-1 ui:py-1 ui:text-center ui:text-foreground ui:focus-visible:focus-ring" + /> + / {pageCount} +
+ ); +} + +/** Paging and zoom controls for the document viewer. */ +export function ViewerToolbar({ + pageIndex, + pageCount, + zoom, + zoomMode, + canZoomIn, + canZoomOut, + onPageChange, + onZoomIn, + onZoomOut, + onFitWidth, + onFitPage, + className, +}: ViewerToolbarProps) { + return ( +
+
+ onPageChange(pageIndex - 1)} + /> + {/* Remounts when the page changes, so the box shows the new page. */} + + = pageCount - 1} + onClick={() => onPageChange(pageIndex + 1)} + /> +
+
+ + + {formatZoom(zoom)} + + + + +
+
+ ); +} diff --git a/frontend/src/v1/hooks/useElementSize.ts b/frontend/src/v1/hooks/useElementSize.ts new file mode 100644 index 0000000000..e2410696ac --- /dev/null +++ b/frontend/src/v1/hooks/useElementSize.ts @@ -0,0 +1,40 @@ +import { useCallback, useEffect, useState } from "react"; + +export interface ElementSize { + width: number; + height: number; +} + +/** + * An element's content-box size, kept current with a ResizeObserver. Returns a + * ref for the element and its size, null until it has been measured. + */ +export function useElementSize(): [ + (element: T | null) => void, + ElementSize | null, +] { + const [element, setElement] = useState(null); + const [size, setSize] = useState(null); + + useEffect(() => { + if (element === null) { + return; + } + const observer = new ResizeObserver(([entry]) => { + if (!entry) { + return; + } + const { width, height } = entry.contentRect; + setSize((previous) => + previous?.width === width && previous.height === height + ? previous + : { width, height }, + ); + }); + observer.observe(element); + return () => observer.disconnect(); + }, [element]); + + const ref = useCallback((next: T | null) => setElement(next), []); + return [ref, size]; +} diff --git a/frontend/src/v1/hooks/usePageWindow.ts b/frontend/src/v1/hooks/usePageWindow.ts index 3127d9d4ac..a45f365bde 100644 --- a/frontend/src/v1/hooks/usePageWindow.ts +++ b/frontend/src/v1/hooks/usePageWindow.ts @@ -13,6 +13,28 @@ export interface PageWindow { visible: ReadonlySet; /** 0-based pages that should hold a canvas: the visible ones ± overscan. */ rendered: ReadonlySet; + /** The 0-based page showing most of itself; the last one while none shows. */ + current: number; +} + +// Ratios are reported at these steps, which is enough to tell which page +// shows most without a callback on every scrolled pixel. +const THRESHOLDS = [0, 0.25, 0.5, 0.75, 1]; + +/** The page with the largest visible share; the lower index wins a tie. */ +function mostVisible(ratios: ReadonlyMap): number | null { + let best: number | null = null; + let bestRatio = -1; + for (const [index, ratio] of ratios) { + if ( + ratio > bestRatio || + (ratio === bestRatio && best !== null && index < best) + ) { + best = index; + bestRatio = ratio; + } + } + return best; } function sameMembers(a: ReadonlySet, b: ReadonlySet): boolean { @@ -43,6 +65,7 @@ export function usePageWindow( const [visible, setVisible] = useState>( () => new Set(), ); + const [current, setCurrent] = useState(0); const observerRef = useRef(null); const pages = useRef(new Set()); @@ -50,23 +73,27 @@ export function usePageWindow( if (root === null) { return; } - const inView = new Set(); + const ratios = new Map(); const observer = new IntersectionObserver( (entries) => { for (const entry of entries) { const index = pageIndexOf(entry.target); if (entry.isIntersecting) { - inView.add(index); + ratios.set(index, entry.intersectionRatio); } else { - inView.delete(index); + ratios.delete(index); } } - const next = new Set(inView); + const next = new Set(ratios.keys()); setVisible((previous) => sameMembers(previous, next) ? previous : next, ); + const top = mostVisible(ratios); + if (top !== null) { + setCurrent(top); + } }, - { root }, + { root, threshold: THRESHOLDS }, ); observerRef.current = observer; for (const page of pages.current) { @@ -95,5 +122,11 @@ export function usePageWindow( [visible, pageCount, overscan], ); - return { scrollRef: setRoot, observePage, visible, rendered }; + return { + scrollRef: setRoot, + observePage, + visible, + rendered, + current: Math.min(current, Math.max(0, pageCount - 1)), + }; } diff --git a/frontend/src/v1/lib/pdf/keyboard.test.ts b/frontend/src/v1/lib/pdf/keyboard.test.ts new file mode 100644 index 0000000000..f7ab917e0c --- /dev/null +++ b/frontend/src/v1/lib/pdf/keyboard.test.ts @@ -0,0 +1,19 @@ +import { expect, it } from "vitest"; +import { viewerKeyAction } from "./keyboard"; + +it("maps paging and zoom keys", () => { + expect(viewerKeyAction({ key: "PageUp" })).toBe("previous-page"); + expect(viewerKeyAction({ key: "PageDown" })).toBe("next-page"); + expect(viewerKeyAction({ key: "Home" })).toBe("first-page"); + expect(viewerKeyAction({ key: "End" })).toBe("last-page"); + expect(viewerKeyAction({ key: "+" })).toBe("zoom-in"); + expect(viewerKeyAction({ key: "=" })).toBe("zoom-in"); + expect(viewerKeyAction({ key: "-" })).toBe("zoom-out"); +}); + +it("leaves other keys and modified keys to the browser", () => { + expect(viewerKeyAction({ key: "ArrowDown" })).toBeNull(); + expect(viewerKeyAction({ key: "+", ctrlKey: true })).toBeNull(); + expect(viewerKeyAction({ key: "-", metaKey: true })).toBeNull(); + expect(viewerKeyAction({ key: "Home", altKey: true })).toBeNull(); +}); diff --git a/frontend/src/v1/lib/pdf/keyboard.ts b/frontend/src/v1/lib/pdf/keyboard.ts new file mode 100644 index 0000000000..f8033ca429 --- /dev/null +++ b/frontend/src/v1/lib/pdf/keyboard.ts @@ -0,0 +1,41 @@ +/** What a key does in the document viewer, when the viewer has focus. */ +export type ViewerKeyAction = + | "previous-page" + | "next-page" + | "first-page" + | "last-page" + | "zoom-in" + | "zoom-out"; + +/** + * Maps a key to a viewer action. Keys pressed with Ctrl, Meta or Alt are left + * to the browser, so its own zoom and shortcuts keep working. + */ +export function viewerKeyAction(event: { + key: string; + ctrlKey?: boolean; + metaKey?: boolean; + altKey?: boolean; +}): ViewerKeyAction | null { + if (event.ctrlKey || event.metaKey || event.altKey) { + return null; + } + switch (event.key) { + case "PageUp": + return "previous-page"; + case "PageDown": + return "next-page"; + case "Home": + return "first-page"; + case "End": + return "last-page"; + case "+": + case "=": + return "zoom-in"; + case "-": + case "_": + return "zoom-out"; + default: + return null; + } +} diff --git a/frontend/src/v1/lib/pdf/zoom.test.ts b/frontend/src/v1/lib/pdf/zoom.test.ts new file mode 100644 index 0000000000..36878094aa --- /dev/null +++ b/frontend/src/v1/lib/pdf/zoom.test.ts @@ -0,0 +1,54 @@ +import { describe, expect, it } from "vitest"; +import { + clampZoom, + fitZoom, + formatZoom, + MAX_ZOOM, + MIN_ZOOM, + zoomIn, + zoomOut, +} from "./zoom"; + +describe("zoomIn / zoomOut", () => { + it("steps through the levels", () => { + expect(zoomIn(1)).toBe(1.25); + expect(zoomOut(1)).toBe(0.75); + }); + + it("goes to the nearest level from a fitted zoom between levels", () => { + expect(zoomIn(1.1)).toBe(1.25); + expect(zoomOut(1.1)).toBe(1); + expect(zoomIn(0.9999)).toBe(1.25); + }); + + it("stops at the ends", () => { + expect(zoomIn(MAX_ZOOM)).toBe(MAX_ZOOM); + expect(zoomOut(MIN_ZOOM)).toBe(MIN_ZOOM); + }); +}); + +describe("fitZoom", () => { + const letter = { width: 612, height: 792 }; + + it("fits the page width inside the gutter", () => { + // 612 pt at 96/72 is 816 CSS px. + expect(fitZoom(letter, 816 + 32, null, 32)).toBeCloseTo(1); + expect(fitZoom(letter, 408 + 32, null, 32)).toBeCloseTo(0.5); + }); + + it("fits the whole page, limited by the tighter side", () => { + expect(fitZoom(letter, 2000, 1056 + 32, 32)).toBeCloseTo(1); + }); + + it("stays within the zoom range", () => { + expect(fitZoom(letter, 10, null, 32)).toBe(MIN_ZOOM); + expect(fitZoom(letter, 100_000, null, 32)).toBe(MAX_ZOOM); + }); +}); + +it("clamps and formats", () => { + expect(clampZoom(10)).toBe(MAX_ZOOM); + expect(clampZoom(Number.NaN)).toBe(1); + expect(formatZoom(1.25)).toBe("125%"); + expect(formatZoom(0.333)).toBe("33%"); +}); diff --git a/frontend/src/v1/lib/pdf/zoom.ts b/frontend/src/v1/lib/pdf/zoom.ts new file mode 100644 index 0000000000..bc23e9f553 --- /dev/null +++ b/frontend/src/v1/lib/pdf/zoom.ts @@ -0,0 +1,63 @@ +/* + * Zoom levels for the document viewer. Zoom 1 is 100%: a page at its printed + * size (see CSS_PIXELS_PER_POINT). + */ +import type { PageSize } from "./types"; +import { CSS_PIXELS_PER_POINT } from "./windowing"; + +/** The levels the − and + buttons step through. */ +export const ZOOM_STEPS = [0.25, 0.5, 0.75, 1, 1.25, 1.5, 2, 3, 4] as const; +export const MIN_ZOOM = ZOOM_STEPS[0]; +export const MAX_ZOOM = ZOOM_STEPS[ZOOM_STEPS.length - 1]; + +/** How the zoom is chosen: a fixed level, or fitted to the viewer. */ +export type ZoomMode = "custom" | "fit-width" | "fit-page"; + +// Two steps a hair apart (a fitted 0.9999) should not count as different. +const EPSILON = 1e-3; + +export function clampZoom(zoom: number): number { + if (!Number.isFinite(zoom)) { + return 1; + } + return Math.min(MAX_ZOOM, Math.max(MIN_ZOOM, zoom)); +} + +/** The next level above the current zoom, which may be between steps. */ +export function zoomIn(zoom: number): number { + return ZOOM_STEPS.find((step) => step > zoom + EPSILON) ?? MAX_ZOOM; +} + +/** The next level below the current zoom, which may be between steps. */ +export function zoomOut(zoom: number): number { + for (let index = ZOOM_STEPS.length - 1; index >= 0; index--) { + const step = ZOOM_STEPS[index]; + if (step !== undefined && step < zoom - EPSILON) { + return step; + } + } + return MIN_ZOOM; +} + +/** + * The zoom at which a page fills the given width, or the given box when a + * height is given too. `gutter` is the space kept around the page, in CSS px. + */ +export function fitZoom( + page: PageSize, + width: number, + height: number | null, + gutter: number, +): number { + const byWidth = (width - gutter) / (page.width * CSS_PIXELS_PER_POINT); + if (height === null) { + return clampZoom(byWidth); + } + const byHeight = (height - gutter) / (page.height * CSS_PIXELS_PER_POINT); + return clampZoom(Math.min(byWidth, byHeight)); +} + +/** The zoom as the toolbar shows it, e.g. "125%". */ +export function formatZoom(zoom: number): string { + return `${Math.round(zoom * 100)}%`; +} From 504a4b271e253231e667514d03d243a27c2b99e6 Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Thu, 29 Oct 2026 08:03:47 +0000 Subject: [PATCH 02/11] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- .../components/patterns/document-viewer/DocumentViewer.test.tsx | 2 +- .../v1/components/patterns/document-viewer/PdfPageList.test.tsx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index b9ea9e4701..94d985532a 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -2,8 +2,8 @@ import { act, fireEvent, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import type { OpenedPdf, PdfSource } from "@v1/lib/pdf/types"; import type { PDFDocumentProxy } from "pdfjs-dist"; -import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { createRef } from "react"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { DocumentViewer, type DocumentViewerHandle } from "./DocumentViewer"; const opening = vi.hoisted( diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index f55bc6feaf..1e0db570f0 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -1,7 +1,7 @@ import { act, render } from "@testing-library/react"; import type { PDFDocumentProxy } from "pdfjs-dist"; -import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { createRef } from "react"; +import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { PdfPageList, type PdfPageListHandle } from "./PdfPageList"; /** An IntersectionObserver whose "in view" set the test controls. */ From bc6150b8ca36040c90be28075a695341266473e1 Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Mon, 2 Nov 2026 06:26:18 +0000 Subject: [PATCH 03/11] UN-4505 [FEAT] Keep fit-page steady and the page draft while scrolling - Fit page fits every page instead of the current one. With mixed page sizes, following the current page rescaled the whole document each time scrolling crossed into a page of another size. - The page box keeps a half-typed number and its focus when the current page changes; it follows the current page only while not being edited. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 19 ++++++++++++ .../document-viewer/DocumentViewer.tsx | 30 +++++++++++-------- .../document-viewer/ViewerToolbar.test.tsx | 28 +++++++++++++++-- .../document-viewer/ViewerToolbar.tsx | 14 ++++----- 4 files changed, 69 insertions(+), 22 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index 94d985532a..efd6f32c9a 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -188,3 +188,22 @@ it("runs a scroll asked for while loading once the pages are laid out", async () } } }); + +it("fits every page in fit page, so a wider page later on sets the zoom", async () => { + render(); + await act(async () => + opening.resolve({ + document, + // A portrait page first, then a landscape one. + pageSizes: [ + { width: 612, height: 792 }, + { width: 792, height: 612 }, + ], + }), + ); + // Tall enough that only width limits: 816 px across, after the gutter. + act(() => resize?.(864, 2000)); + await userEvent.click(screen.getByRole("button", { name: "Fit page" })); + // The landscape page is 1056 px wide at 100%: 816 / 1056 ≈ 77%. + expect(zoomLabel()).toBe("77%"); +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index 980f626f85..8cff4e8e34 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -124,20 +124,24 @@ export function DocumentViewer({ if (zoomState.mode === "custom" || !pageSizes || !bodySize) { return zoomState.level; } - const page = - zoomState.mode === "fit-width" - ? widest(pageSizes) - : (pageSizes[pageIndex] ?? pageSizes[0]); - if (!page) { - return zoomState.level; + if (zoomState.mode === "fit-width") { + const page = widest(pageSizes); + return page + ? fitZoom(page, bodySize.width, null, FIT_GUTTER) + : zoomState.level; + } + // Fit page fits every page, not the current one: in a document that mixes + // sizes, a zoom that followed the current page would rescale the whole + // document each time scrolling crossed into a page of another size. + let level = Number.POSITIVE_INFINITY; + for (const page of pageSizes) { + level = Math.min( + level, + fitZoom(page, bodySize.width, bodySize.height, FIT_GUTTER), + ); } - return fitZoom( - page, - bodySize.width, - zoomState.mode === "fit-page" ? bodySize.height : null, - FIT_GUTTER, - ); - }, [zoomState, pageSizes, bodySize, pageIndex]); + return Number.isFinite(level) ? level : zoomState.level; + }, [zoomState, pageSizes, bodySize]); const run = useCallback((request: ScrollRequest) => { const list = listRef.current; diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx index 420d48790b..ea033da4d2 100644 --- a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx @@ -18,8 +18,12 @@ function setup(overrides: Partial = {}) { onFitPage: vi.fn(), ...overrides, }; - render(); - return props; + const { rerender } = render(); + return { + ...props, + rerender: (next: Partial) => + rerender(), + }; } it("shows the page as n / N and the zoom as a percentage", () => { @@ -98,3 +102,23 @@ it("is reachable with Tab, control by control", async () => { expect(label).toBe(name); } }); + +it("keeps a half-typed page and the focus when the current page changes", async () => { + const props = setup(); + const box = screen.getByLabelText("Page"); + await userEvent.clear(box); + await userEvent.type(box, "1"); + props.rerender({ pageIndex: 3 }); + const after = screen.getByLabelText("Page"); + expect(after).toBe(box); + expect(document.activeElement).toBe(box); + expect(box.value).toBe("1"); + await userEvent.type(box, "2{Enter}"); + expect(props.onPageChange).toHaveBeenCalledWith(11); +}); + +it("follows the current page while not being edited", () => { + const props = setup(); + props.rerender({ pageIndex: 6 }); + expect(screen.getByLabelText("Page").value).toBe("7"); +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx index da20056582..106ae84624 100644 --- a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx @@ -64,16 +64,17 @@ function PageInput({ pageCount, onPageChange, }: Pick) { - const [draft, setDraft] = useState(String(pageIndex + 1)); + // What the reader is typing, or null when they are not editing: then the + // box follows the current page, and a page change never discards a draft. + const [draft, setDraft] = useState(null); const id = useId(); const commit = (event: FormEvent) => { event.preventDefault(); - const page = Number.parseInt(draft, 10); + const page = Number.parseInt(draft ?? "", 10); if (Number.isInteger(page) && page >= 1 && page <= pageCount) { onPageChange(page - 1); - } else { - setDraft(String(pageIndex + 1)); } + setDraft(null); }; return (
setDraft(event.target.value)} - onBlur={() => setDraft(String(pageIndex + 1))} + onBlur={() => setDraft(null)} className="ui:w-10 ui:rounded-md ui:border ui:border-input ui:bg-background ui:px-1 ui:py-1 ui:text-center ui:text-foreground ui:focus-visible:focus-ring" /> / {pageCount} @@ -130,7 +131,6 @@ export function ViewerToolbar({ /> {/* Remounts when the page changes, so the box shows the new page. */} Date: Mon, 2 Nov 2026 06:28:44 +0000 Subject: [PATCH 04/11] UN-4505 [FEAT] Drop a stale comment about remounting the page box Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../src/v1/components/patterns/document-viewer/ViewerToolbar.tsx | 1 - 1 file changed, 1 deletion(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx index 106ae84624..9100d765c9 100644 --- a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.tsx @@ -129,7 +129,6 @@ export function ViewerToolbar({ disabled={pageIndex <= 0} onClick={() => onPageChange(pageIndex - 1)} /> - {/* Remounts when the page changes, so the box shows the new page. */} Date: Mon, 2 Nov 2026 06:46:03 +0000 Subject: [PATCH 05/11] UN-4505 Read the current page from the layout and tie waiting scrolls to their source The current page came from intersection ratios reported at quarter steps, so for a page more than four times the viewer's height it was arbitrary, and after a jump it lagged a frame: the toolbar showed the wrong page, PageDown could skip one, and the zoom anchor was recorded against the old page. Both now come from the laid-out page offsets, on scroll and straight after a jump. A scroll waiting for a document is dropped when another source is shown, and a highlight scroll whose highlight is not in the list yet waits and is tried again when the highlights change. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 135 ++++++++++++++++++ .../document-viewer/DocumentViewer.tsx | 32 ++++- .../document-viewer/PdfPageList.test.tsx | 108 ++++++++++++-- .../patterns/document-viewer/PdfPageList.tsx | 93 +++++++++--- .../document-viewer/ViewerToolbar.test.tsx | 6 + frontend/src/v1/hooks/usePageWindow.ts | 45 +----- 6 files changed, 343 insertions(+), 76 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index efd6f32c9a..fa1bb19887 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -207,3 +207,138 @@ it("fits every page in fit page, so a wider page later on sets the zoom", async // The landscape page is 1056 px wide at 100%: 816 / 1056 ≈ 77%. expect(zoomLabel()).toBe("77%"); }); + +it("pages forward and back with PageDown and PageUp, one page per press", async () => { + render(); + await open(); + const pages = layOutPages(); + fireEvent.keyDown(pages, { key: "PageDown" }); + fireEvent.keyDown(pages, { key: "PageDown" }); + expect(pages.scrollTop).toBe(2 * 1016); + expect(screen.getByLabelText("Page").value).toBe("3"); + fireEvent.keyDown(pages, { key: "PageUp" }); + expect(pages.scrollTop).toBe(1016); + expect(screen.getByLabelText("Page").value).toBe("2"); +}); + +/** + * Lays out every page element from the moment it mounts: 1000 px tall, 16 px + * apart. Returns a function that puts the prototype back. + */ +function layOutEveryPage() { + const originals = (["offsetTop", "offsetHeight"] as const).map( + (name) => + [ + name, + Object.getOwnPropertyDescriptor(HTMLElement.prototype, name), + ] as const, + ); + Object.defineProperties(HTMLElement.prototype, { + offsetTop: { + configurable: true, + get(this: HTMLElement) { + const index = this.getAttribute("data-page-index"); + return index === null ? 0 : 16 + Number(index) * 1016; + }, + }, + offsetHeight: { + configurable: true, + get(this: HTMLElement) { + return this.hasAttribute("data-page-index") ? 1000 : 0; + }, + }, + }); + return () => { + for (const [name, descriptor] of originals) { + if (descriptor) { + Object.defineProperty(HTMLElement.prototype, name, descriptor); + } + } + }; +} + +const onPage1 = { + id: "total", + page: 1, + x: 0.1, + y: 0.5, + w: 0.2, + h: 0, + variant: "box" as const, + tone: "default" as const, +}; +// Page 1 starts at 1016 and the highlight's centre is 500 px into it; the +// viewer reports no height, so centring adds nothing. +const onPage1Top = 1016 + 500; + +function pagesRegion() { + return screen.getByRole("region", { name: "Document pages" }); +} + +it("drops a scroll asked for one document when another is shown", async () => { + const restore = layOutEveryPage(); + try { + const ref = createRef(); + const { rerender } = render(); + act(() => ref.current?.scrollToPage(2)); + rerender( + , + ); + await open(); + expect(pagesRegion().scrollTop).toBe(0); + } finally { + restore(); + } +}); + +it("scrolls to the active highlight when it changes, and to one set while loading", async () => { + const restore = layOutEveryPage(); + try { + const { rerender } = render( + , + ); + await open(); + expect(pagesRegion().scrollTop).toBe(onPage1Top); + + pagesRegion().scrollTop = 0; + rerender( + , + ); + expect(pagesRegion().scrollTop).toBe(2 * 1016 + 500); + } finally { + restore(); + } +}); + +it("scrolls to an active highlight that arrives after its id", async () => { + const restore = layOutEveryPage(); + try { + const { rerender } = render( + , + ); + await open(); + expect(pagesRegion().scrollTop).toBe(0); + rerender( + , + ); + expect(pagesRegion().scrollTop).toBe(onPage1Top); + } finally { + restore(); + } +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index 8cff4e8e34..fbafd93442 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -3,6 +3,7 @@ import { usePdfDocument } from "@v1/hooks/usePdfDocument"; import { cn } from "@v1/lib/cn"; import type { Highlight } from "@v1/lib/pdf/highlights"; import { viewerKeyAction } from "@v1/lib/pdf/keyboard"; +import { pdfSourceKey } from "@v1/lib/pdf/pdfjs"; import type { PageSize, PdfSource } from "@v1/lib/pdf/types"; import { fitZoom, @@ -29,7 +30,10 @@ import { ViewerToolbar } from "./ViewerToolbar"; export interface DocumentViewerHandle { /** Scrolls a 0-based page to the top. Waits for the document if it is loading. */ scrollToPage: (index: number) => void; - /** Centres a highlight. Waits for the document if it is loading. */ + /** + * Centres a highlight. Waits for the document if it is loading, and for the + * highlight if it is not in `highlights` yet. + */ scrollToHighlight: (id: string) => void; } @@ -143,6 +147,9 @@ export function DocumentViewer({ return Number.isFinite(level) ? level : zoomState.level; }, [zoomState, pageSizes, bodySize]); + // A request runs now if it can, or waits in pendingRef: for the pages while + // loading, and for its highlight while that is not in the list yet. A newer + // request replaces a waiting one. const run = useCallback((request: ScrollRequest) => { const list = listRef.current; if (!list) { @@ -150,9 +157,10 @@ export function DocumentViewer({ return; } if (request.kind === "page") { + pendingRef.current = null; list.scrollToPage(request.index); } else { - list.scrollToHighlight(request.id); + pendingRef.current = list.scrollToHighlight(request.id) ? null : request; } }, []); @@ -165,18 +173,28 @@ export function DocumentViewer({ [run], ); - // Requests made while loading run once the pages are laid out. + // A waiting request belongs to the document it was made for: another + // source drops it rather than scrolling a document nobody asked about. + const sourceKey = pdfSourceKey(source); + // biome-ignore lint/correctness/useExhaustiveDependencies: runs on each new source + useEffect(() => { + pendingRef.current = null; + }, [sourceKey]); + + // A waiting request runs once the pages are laid out, and is tried again + // when the highlights change, in case its highlight has arrived. + // biome-ignore lint/correctness/useExhaustiveDependencies: retried on each new highlights list useEffect(() => { if (ready && pendingRef.current) { - const request = pendingRef.current; - pendingRef.current = null; - run(request); + run(pendingRef.current); } - }, [ready, run]); + }, [ready, highlights, run]); useEffect(() => { if (activeHighlightId) { run({ kind: "highlight", id: activeHighlightId }); + } else if (pendingRef.current?.kind === "highlight") { + pendingRef.current = null; } }, [activeHighlightId, run]); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index 32c6018820..eb3aa7123b 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -403,10 +403,17 @@ it("keeps the same spot of the same page in view across a zoom change", () => { expect(root.scrollTop).toBe(800); }); +function scrollTo(root: HTMLElement, top: number) { + root.scrollTop = top; + act(() => { + root.dispatchEvent(new Event("scroll")); + }); +} + it("reports the page showing most of itself", () => { const { document } = fakeDocument(10); const onCurrentPageChange = vi.fn(); - render( + const { container } = render( { onCurrentPageChange={onCurrentPageChange} />, ); + const root = layOut(container, () => 1); + // Page 4 spans 3264-4064 and page 5 4080-4880: 100 px of 4, 284 px of 5. + scrollTo(root, 3964); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(5); + // 300 px of 4, 84 px of 5. + scrollTo(root, 3764); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(4); +}); + +it("keeps the current page right for a page much taller than the viewer", () => { + const { document } = fakeDocument(10); + const onCurrentPageChange = vi.fn(); + const { container } = render( + , + ); + // At 400% a page is 3200 px in a 400 px viewer, so no intersection ratio + // ever reaches a quarter. Page 2 spans 6432-9632. + const root = layOut(container, () => 4); + scrollTo(root, 9332); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(2); + scrollTo(root, 9600); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(3); +}); + +it("reports the page jumped to without waiting for a scroll event", () => { + const { document } = fakeDocument(10); + const ref = createRef(); + const onCurrentPageChange = vi.fn(); + const { container } = render( + , + ); + layOut(container, () => 1); + act(() => ref.current?.scrollToPage(6)); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(6); +}); + +it("keeps the same spot of a later page across a zoom change", () => { + const { document } = fakeDocument(10); + let zoom = 1; + const { container, rerender } = render( + , + ); + const root = layOut(container, () => zoom); + // A quarter of the way down page 3, which starts at 3 × 816. + scrollTo(root, 3 * 816 + 200); + zoom = 2; + rerender( + , + ); + expect(root.scrollTop).toBe(3 * 1616 + 400); +}); + +it("zooms around the page jumped to, even after the jump's scroll event", () => { + const { document } = fakeDocument(200); + const ref = createRef(); + let zoom = 1; + const { container, rerender } = render( + , + ); + const root = layOut(container, () => zoom); + act(() => ref.current?.scrollToPage(100)); + // The browser fires the jump's scroll event before intersections update. act(() => { - const observer = FakeIntersectionObserver.current; - const target = (index: number) => - [...(observer?.targets ?? [])].find( - (t) => t.getAttribute("data-page-index") === String(index), - ); - observer?.emit([ - { target: target(4), isIntersecting: true, intersectionRatio: 0.25 }, - { target: target(5), isIntersecting: true, intersectionRatio: 0.75 }, - ]); + root.dispatchEvent(new Event("scroll")); }); - expect(onCurrentPageChange).toHaveBeenLastCalledWith(5); + zoom = 1.5; + rerender( + , + ); + expect(root.scrollTop).toBe(100 * (1200 + 16)); }); it("cleans up a page that arrives after it has left the window", async () => { diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index 0fcfe0c1ea..b8c0c27612 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -13,6 +13,7 @@ import { useLayoutEffect, useMemo, useRef, + useState, } from "react"; import { PdfPage } from "./PdfPage"; @@ -59,6 +60,53 @@ function pageTop(root: HTMLElement, page: HTMLElement): number { return page.offsetTop - (first?.offsetTop ?? 0); } +/** The last page whose top is at or above `y`, found by halving the range. */ +function pageAt(root: HTMLElement, pageCount: number, y: number): number { + let low = 0; + let high = pageCount - 1; + while (low < high) { + const middle = Math.ceil((low + high) / 2); + const page = pageElement(root, middle); + if (page && pageTop(root, page) <= y) { + low = middle; + } else { + high = middle - 1; + } + } + return low; +} + +/** + * The page with the most of itself in view; the lower index wins a tie. Read + * from the layout, so it is exact for pages of any height and right straight + * after a jump, unlike intersection ratios, which only arrive at thresholds and + * a frame later. + */ +function mostVisiblePage(root: HTMLElement, pageCount: number): number { + const top = root.scrollTop; + const bottom = top + root.clientHeight; + let best = pageAt(root, pageCount, top); + let bestShown = Number.NEGATIVE_INFINITY; + for (let index = best; index < pageCount; index++) { + const page = pageElement(root, index); + if (!page) { + break; + } + const pageStart = pageTop(root, page); + if (pageStart >= bottom) { + break; + } + const shown = + Math.min(pageStart + page.offsetHeight, bottom) - + Math.max(pageStart, top); + if (shown > bestShown) { + best = index; + bestShown = shown; + } + } + return best; +} + /** * Every page of a document in one scrolling column. All pages are laid out at * their real size from the start, so the scrollbar is right and scrolling to @@ -78,14 +126,12 @@ export function PdfPageList({ className, ref, }: PdfPageListProps) { - const { scrollRef, observePage, rendered, current } = usePageWindow( - pageSizes.length, - ); + const pageCount = pageSizes.length; + const { scrollRef, observePage, rendered } = usePageWindow(pageCount); const byPage = useMemo(() => highlightsByPage(highlights), [highlights]); const rootRef = useRef(null); const anchorRef = useRef({ index: 0, fraction: 0 }); - const currentRef = useRef(current); - currentRef.current = current; + const [current, setCurrent] = useState(0); const setRoot = useCallback( (element: HTMLElement | null) => { @@ -99,20 +145,27 @@ export function PdfPageList({ onCurrentPageChange?.(current); }, [current, onCurrentPageChange]); - const recordAnchor = useCallback(() => { + // Reads where the reader is from the layout: the page at the top, for the + // zoom anchor, and the page showing most, for the toolbar. Runs on every + // scroll and straight after a jump, so neither waits for a scroll event. + const sync = useCallback(() => { const root = rootRef.current; - const page = root && pageElement(root, currentRef.current); - if (!root || !page || page.offsetHeight === 0) { + if (!root || pageCount === 0) { return; } - anchorRef.current = { - index: currentRef.current, - fraction: (root.scrollTop - pageTop(root, page)) / page.offsetHeight, - }; - }, []); + const index = pageAt(root, pageCount, root.scrollTop); + const page = pageElement(root, index); + if (page && page.offsetHeight > 0) { + anchorRef.current = { + index, + fraction: (root.scrollTop - pageTop(root, page)) / page.offsetHeight, + }; + } + setCurrent(mostVisiblePage(root, pageCount)); + }, [pageCount]); // A zoom change resizes every page; keep the same spot of the same page at - // the top. Runs after the pages' own layout effects have resized them. + // the top. The pages took their new size from their style at commit. const previousZoom = useRef(zoom); useLayoutEffect(() => { if (previousZoom.current === zoom) { @@ -124,19 +177,20 @@ export function PdfPageList({ const page = root && pageElement(root, index); if (root && page) { root.scrollTop = pageTop(root, page) + fraction * page.offsetHeight; + sync(); } - }, [zoom]); + }, [zoom, sync]); useImperativeHandle( ref, () => ({ scrollToPage(index) { const root = rootRef.current; - const clamped = Math.min(Math.max(0, index), pageSizes.length - 1); + const clamped = Math.min(Math.max(0, index), pageCount - 1); const page = root && pageElement(root, clamped); if (root && page) { root.scrollTop = pageTop(root, page); - anchorRef.current = { index: clamped, fraction: 0 }; + sync(); } }, scrollToHighlight(id) { @@ -152,10 +206,11 @@ export function PdfPageList({ const centreX = (highlight.x + highlight.w / 2) * page.offsetWidth; root.scrollLeft = page.offsetLeft + centreX - root.clientWidth / 2; } + sync(); return true; }, }), - [highlights, pageSizes.length], + [highlights, pageCount, sync], ); // A scrollable region must be focusable for keyboard users (WCAG 2.1.1), @@ -167,7 +222,7 @@ export function PdfPageList({ // biome-ignore lint/a11y/noNoninteractiveTabindex: a scrollable region must be focusable tabIndex={0} aria-label="Document pages" - onScroll={recordAnchor} + onScroll={sync} onKeyDown={onKeyDown} className={cn( "ui:relative ui:h-full ui:overflow-auto ui:bg-muted ui:focus-visible:focus-ring", diff --git a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx index ea033da4d2..da3af729aa 100644 --- a/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/ViewerToolbar.test.tsx @@ -43,6 +43,12 @@ it("pages back and forward, and stops at either end", async () => { expect(first.onPageChange).toHaveBeenCalledWith(1); }); +it("pages back from the middle of the document", async () => { + const props = setup({ pageIndex: 3 }); + await userEvent.click(screen.getByRole("button", { name: "Previous page" })); + expect(props.onPageChange).toHaveBeenCalledWith(2); +}); + it("disables Next on the last page", () => { setup({ pageIndex: 11 }); expect( diff --git a/frontend/src/v1/hooks/usePageWindow.ts b/frontend/src/v1/hooks/usePageWindow.ts index a45f365bde..3127d9d4ac 100644 --- a/frontend/src/v1/hooks/usePageWindow.ts +++ b/frontend/src/v1/hooks/usePageWindow.ts @@ -13,28 +13,6 @@ export interface PageWindow { visible: ReadonlySet; /** 0-based pages that should hold a canvas: the visible ones ± overscan. */ rendered: ReadonlySet; - /** The 0-based page showing most of itself; the last one while none shows. */ - current: number; -} - -// Ratios are reported at these steps, which is enough to tell which page -// shows most without a callback on every scrolled pixel. -const THRESHOLDS = [0, 0.25, 0.5, 0.75, 1]; - -/** The page with the largest visible share; the lower index wins a tie. */ -function mostVisible(ratios: ReadonlyMap): number | null { - let best: number | null = null; - let bestRatio = -1; - for (const [index, ratio] of ratios) { - if ( - ratio > bestRatio || - (ratio === bestRatio && best !== null && index < best) - ) { - best = index; - bestRatio = ratio; - } - } - return best; } function sameMembers(a: ReadonlySet, b: ReadonlySet): boolean { @@ -65,7 +43,6 @@ export function usePageWindow( const [visible, setVisible] = useState>( () => new Set(), ); - const [current, setCurrent] = useState(0); const observerRef = useRef(null); const pages = useRef(new Set()); @@ -73,27 +50,23 @@ export function usePageWindow( if (root === null) { return; } - const ratios = new Map(); + const inView = new Set(); const observer = new IntersectionObserver( (entries) => { for (const entry of entries) { const index = pageIndexOf(entry.target); if (entry.isIntersecting) { - ratios.set(index, entry.intersectionRatio); + inView.add(index); } else { - ratios.delete(index); + inView.delete(index); } } - const next = new Set(ratios.keys()); + const next = new Set(inView); setVisible((previous) => sameMembers(previous, next) ? previous : next, ); - const top = mostVisible(ratios); - if (top !== null) { - setCurrent(top); - } }, - { root, threshold: THRESHOLDS }, + { root }, ); observerRef.current = observer; for (const page of pages.current) { @@ -122,11 +95,5 @@ export function usePageWindow( [visible, pageCount, overscan], ); - return { - scrollRef: setRoot, - observePage, - visible, - rendered, - current: Math.min(current, Math.max(0, pageCount - 1)), - }; + return { scrollRef: setRoot, observePage, visible, rendered }; } From dc3a33c212270c0c63aa5c7640eef59b2aef3172 Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Mon, 2 Nov 2026 06:54:44 +0000 Subject: [PATCH 06/11] UN-4505 Fix the end-of-document page and keep waiting scrolls made at commit Round two of the review: - the last page is current once the end is reached, so Next and PageDown no longer stick when a short last page or two whole pages show - the page in view is measured in the content's own coordinates, with one query per scroll, and again when the viewer changes height - a waiting scroll is stamped with its source and dropped only at flush time, so one made in a parent's layout effect on mount survives - the active highlight is re-armed for each new source, and clearing it drops only a request it made Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 69 ++++++++++-- .../document-viewer/DocumentViewer.tsx | 76 +++++++++---- .../document-viewer/PdfPageList.test.tsx | 80 +++++++++++++- .../patterns/document-viewer/PdfPageList.tsx | 102 +++++++++++++----- 4 files changed, 261 insertions(+), 66 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index fa1bb19887..ef0a5a1572 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -2,7 +2,7 @@ import { act, fireEvent, render, screen } from "@testing-library/react"; import userEvent from "@testing-library/user-event"; import type { OpenedPdf, PdfSource } from "@v1/lib/pdf/types"; import type { PDFDocumentProxy } from "pdfjs-dist"; -import { createRef } from "react"; +import { createRef, useLayoutEffect, useRef } from "react"; import { afterEach, beforeEach, expect, it, vi } from "vitest"; import { DocumentViewer, type DocumentViewerHandle } from "./DocumentViewer"; @@ -27,18 +27,24 @@ vi.mock("@v1/lib/pdf/pdfjs", async (importOriginal) => { }; }); -let resize: ((width: number, height: number) => void) | null = null; +const resizeCallbacks = new Set<(entries: ResizeObserverEntry[]) => void>(); +// Every observer hears the size: the viewer's body and the page list's own. +const resize = (width: number, height: number) => { + for (const callback of resizeCallbacks) { + callback([{ contentRect: { width, height } } as ResizeObserverEntry]); + } +}; class FakeResizeObserver { - constructor(callback: (entries: ResizeObserverEntry[]) => void) { - resize = (width, height) => - callback([{ contentRect: { width, height } } as ResizeObserverEntry]); - } + constructor( + private readonly callback: (entries: ResizeObserverEntry[]) => void, + ) {} observe() { // Sizes arrive only when a test calls resize(). + resizeCallbacks.add(this.callback); } disconnect() { - // Nothing to release. + resizeCallbacks.delete(this.callback); } } @@ -69,7 +75,7 @@ const pageSizes = Array.from({ length: 3 }, () => ({ async function open() { await act(async () => opening.resolve({ document, pageSizes })); // The body is 864 px wide: 816 + the 48 px fit gutter. - act(() => resize?.(864, 600)); + act(() => resize(864, 600)); } function zoomLabel() { @@ -120,7 +126,7 @@ it("fits the width by default, and zooms in steps from there", async () => { it("refits when the viewer is resized", async () => { render(); await open(); - act(() => resize?.(456, 600)); + act(() => resize(456, 600)); expect(zoomLabel()).toBe("50%"); }); @@ -202,7 +208,7 @@ it("fits every page in fit page, so a wider page later on sets the zoom", async }), ); // Tall enough that only width limits: 816 px across, after the gutter. - act(() => resize?.(864, 2000)); + act(() => resize(864, 2000)); await userEvent.click(screen.getByRole("button", { name: "Fit page" })); // The landscape page is 1056 px wide at 100%: 816 / 1056 ≈ 77%. expect(zoomLabel()).toBe("77%"); @@ -342,3 +348,46 @@ it("scrolls to an active highlight that arrives after its id", async () => { restore(); } }); + +it("keeps a scroll a parent asks for in its layout effect on mount", async () => { + const restore = layOutEveryPage(); + function Parent() { + const ref = useRef(null); + useLayoutEffect(() => { + ref.current?.scrollToPage(2); + }, []); + return ; + } + try { + render(); + await open(); + expect(pagesRegion().scrollTop).toBe(2 * 1016); + } finally { + restore(); + } +}); + +it("scrolls to an active highlight chosen before its document is shown", async () => { + const restore = layOutEveryPage(); + try { + const { rerender } = render( + , + ); + await open(); + rerender( + , + ); + await open(); + expect(pagesRegion().scrollTop).toBe(onPage1Top); + } finally { + restore(); + } +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index fbafd93442..824bef2880 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -19,6 +19,7 @@ import { useCallback, useEffect, useImperativeHandle, + useLayoutEffect, useMemo, useRef, useState, @@ -53,6 +54,15 @@ type ScrollRequest = | { kind: "page"; index: number } | { kind: "highlight"; id: string }; +/** A request that could not run yet, and what it was made for. */ +interface PendingRequest { + request: ScrollRequest; + /** The source shown when it was made; another source drops it. */ + sourceKey: string; + /** Made for `activeHighlightId`, so clearing that id drops it. */ + fromActive: boolean; +} + interface ZoomState { mode: ZoomMode; /** The level in custom mode; ignored while fitting. */ @@ -118,9 +128,19 @@ export function DocumentViewer({ const [pageIndex, setPageIndex] = useState(0); const [bodyRef, bodySize] = useElementSize(); const listRef = useRef(null); - const pendingRef = useRef(null); + const pendingRef = useRef(null); const ready = state.status === "ready"; + const sourceKey = pdfSourceKey(source); + // Both kept in step at commit, before any parent's layout effect can make + // a request, so a request is always judged against the source it was made + // for. + const readyRef = useRef(ready); + const sourceKeyRef = useRef(sourceKey); + useLayoutEffect(() => { + readyRef.current = ready; + sourceKeyRef.current = sourceKey; + }, [ready, sourceKey]); const pageSizes = ready ? state.pageSizes : undefined; const pageCount = pageSizes?.length ?? 0; @@ -150,17 +170,24 @@ export function DocumentViewer({ // A request runs now if it can, or waits in pendingRef: for the pages while // loading, and for its highlight while that is not in the list yet. A newer // request replaces a waiting one. - const run = useCallback((request: ScrollRequest) => { + const run = useCallback((request: ScrollRequest, fromActive = false) => { const list = listRef.current; - if (!list) { - pendingRef.current = request; - return; - } - if (request.kind === "page") { + const wait = () => { + pendingRef.current = { + request, + sourceKey: sourceKeyRef.current, + fromActive, + }; + }; + if (!list || !readyRef.current) { + wait(); + } else if (request.kind === "page") { pendingRef.current = null; list.scrollToPage(request.index); + } else if (list.scrollToHighlight(request.id)) { + pendingRef.current = null; } else { - pendingRef.current = list.scrollToHighlight(request.id) ? null : request; + wait(); } }, []); @@ -173,30 +200,33 @@ export function DocumentViewer({ [run], ); - // A waiting request belongs to the document it was made for: another - // source drops it rather than scrolling a document nobody asked about. - const sourceKey = pdfSourceKey(source); - // biome-ignore lint/correctness/useExhaustiveDependencies: runs on each new source - useEffect(() => { - pendingRef.current = null; - }, [sourceKey]); - // A waiting request runs once the pages are laid out, and is tried again - // when the highlights change, in case its highlight has arrived. + // when the highlights change, in case its highlight has arrived. One made + // for another source is dropped rather than scrolling a document nobody + // asked about. // biome-ignore lint/correctness/useExhaustiveDependencies: retried on each new highlights list useEffect(() => { - if (ready && pendingRef.current) { - run(pendingRef.current); + const pending = pendingRef.current; + if (!ready || !pending) { + return; + } + if (pending.sourceKey === sourceKey) { + run(pending.request, pending.fromActive); + } else { + pendingRef.current = null; } - }, [ready, highlights, run]); + }, [ready, sourceKey, highlights, run]); + // The active highlight is scrolled to when it changes, and again for each + // new source, so one chosen before its document arrived is still found. + // biome-ignore lint/correctness/useExhaustiveDependencies: re-armed on each new source useEffect(() => { if (activeHighlightId) { - run({ kind: "highlight", id: activeHighlightId }); - } else if (pendingRef.current?.kind === "highlight") { + run({ kind: "highlight", id: activeHighlightId }, true); + } else if (pendingRef.current?.fromActive) { pendingRef.current = null; } - }, [activeHighlightId, run]); + }, [activeHighlightId, sourceKey, run]); const goToPage = useCallback( (index: number) => run({ kind: "page", index }), diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index eb3aa7123b..3ad5969494 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -104,8 +104,23 @@ function show(indexes: number[]) { act(() => FakeIntersectionObserver.current?.show(indexes)); } +let viewerResized: (() => void) | null = null; + +class FakeResizeObserver { + constructor(callback: () => void) { + viewerResized = callback; + } + observe() { + // Reports only when a test calls viewerResized(). + } + disconnect() { + viewerResized = null; + } +} + beforeEach(() => { vi.stubGlobal("IntersectionObserver", FakeIntersectionObserver); + vi.stubGlobal("ResizeObserver", FakeResizeObserver); vi.stubGlobal("requestAnimationFrame", (cb: FrameRequestCallback) => window.setTimeout(() => cb(0), 0), ); @@ -305,15 +320,24 @@ it("draws each highlight on its own page", () => { /** * Gives the list a layout, which happy-dom does not compute: pages 800 × 600 - * at zoom 1, 16 px apart after 16 px of padding, in a 500 × 400 viewport. + * at zoom 1, 16 px apart after 16 px of padding, in a viewport 500 wide and + * 400 tall unless `viewHeight` says otherwise. */ -function layOut(container: HTMLElement, zoom: () => number) { +function layOut( + container: HTMLElement, + zoom: () => number, + viewHeight: () => number = () => 400, +) { const root = container.querySelector("section"); if (!root) { throw new Error("no scroll region"); } - Object.defineProperty(root, "clientHeight", { value: 400 }); - Object.defineProperty(root, "clientWidth", { value: 500 }); + const count = container.querySelectorAll("[data-page-index]").length; + Object.defineProperties(root, { + clientHeight: { get: viewHeight }, + clientWidth: { value: 500 }, + scrollHeight: { get: () => 16 + count * (800 * zoom() + 16) }, + }); for (const page of container.querySelectorAll( "[data-page-index]", )) { @@ -450,6 +474,54 @@ it("keeps the current page right for a page much taller than the viewer", () => expect(onCurrentPageChange).toHaveBeenLastCalledWith(3); }); +it("reports the last page once the end is reached, even when others show as much", () => { + const { document } = fakeDocument(10); + const onCurrentPageChange = vi.fn(); + const { container } = render( + , + ); + // Two whole pages fit at the bottom; without the end rule page 8 would win + // the tie and Next would never reach page 9. + const root = layOut( + container, + () => 1, + () => 2000, + ); + scrollTo(root, root.scrollHeight - 2000); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(9); +}); + +it("updates the current page when the viewer changes height without scrolling", () => { + const { document } = fakeDocument(10); + const onCurrentPageChange = vi.fn(); + const { container } = render( + , + ); + let height = 400; + const root = layOut( + container, + () => 1, + () => height, + ); + // Page 2 spans 1648-2448 and page 3 2464-3264: 300 px of 2, 84 px of 3. + scrollTo(root, 2148); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(2); + // Taller: 300 px of 2, all 800 of 3. + height = 1200; + act(() => viewerResized?.()); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(3); +}); + it("reports the page jumped to without waiting for a scroll event", () => { const { document } = fakeDocument(10); const ref = createRef(); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index b8c0c27612..f490d20918 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -60,45 +60,69 @@ function pageTop(root: HTMLElement, page: HTMLElement): number { return page.offsetTop - (first?.offsetTop ?? 0); } -/** The last page whose top is at or above `y`, found by halving the range. */ -function pageAt(root: HTMLElement, pageCount: number, y: number): number { +/** The page elements in index order, read with one query per measurement. */ +function pageElements(root: HTMLElement): HTMLElement[] { + return Array.from(root.querySelectorAll("[data-page-index]")); +} + +/** + * The first index whose `edge` is past `y`, or the page count when none is, + * found by halving the range. + */ +function firstPast( + pages: readonly HTMLElement[], + y: number, + edge: (page: HTMLElement) => number, +): number { let low = 0; - let high = pageCount - 1; + let high = pages.length; while (low < high) { - const middle = Math.ceil((low + high) / 2); - const page = pageElement(root, middle); - if (page && pageTop(root, page) <= y) { - low = middle; + const middle = Math.floor((low + high) / 2); + const page = pages[middle]; + if (page && edge(page) > y) { + high = middle; } else { - high = middle - 1; + low = middle + 1; } } return low; } /** - * The page with the most of itself in view; the lower index wins a tie. Read - * from the layout, so it is exact for pages of any height and right straight - * after a jump, unlike intersection ratios, which only arrive at thresholds and - * a frame later. + * The page with the most of itself in view; the lower index wins a tie, and + * the last page wins once the end is reached, where a short last page could + * otherwise never show the most. Read from the layout, so it is exact for + * pages of any height and right straight after a jump, unlike intersection + * ratios, which only arrive at thresholds and a frame later. */ -function mostVisiblePage(root: HTMLElement, pageCount: number): number { +function mostVisiblePage( + root: HTMLElement, + pages: readonly HTMLElement[], +): number { + const last = pages.length - 1; + const scrollable = root.scrollHeight > root.clientHeight; + if ( + scrollable && + root.scrollTop >= root.scrollHeight - root.clientHeight - 1 + ) { + return last; + } + // In the content's own coordinates, where the padding sits above page 0. const top = root.scrollTop; const bottom = top + root.clientHeight; - let best = pageAt(root, pageCount, top); + let best = Math.min( + last, + firstPast(pages, top, (page) => page.offsetTop + page.offsetHeight), + ); let bestShown = Number.NEGATIVE_INFINITY; - for (let index = best; index < pageCount; index++) { - const page = pageElement(root, index); - if (!page) { - break; - } - const pageStart = pageTop(root, page); - if (pageStart >= bottom) { + for (let index = best; index <= last; index++) { + const page = pages[index]; + if (!page || page.offsetTop >= bottom) { break; } const shown = - Math.min(pageStart + page.offsetHeight, bottom) - - Math.max(pageStart, top); + Math.min(page.offsetTop + page.offsetHeight, bottom) - + Math.max(page.offsetTop, top); if (shown > bestShown) { best = index; bestShown = shown; @@ -150,19 +174,39 @@ export function PdfPageList({ // scroll and straight after a jump, so neither waits for a scroll event. const sync = useCallback(() => { const root = rootRef.current; - if (!root || pageCount === 0) { + const pages = root ? pageElements(root) : []; + if (!root || pages.length === 0) { return; } - const index = pageAt(root, pageCount, root.scrollTop); - const page = pageElement(root, index); + // The anchor uses the same coordinates as the jumps and the restore: + // page 0's top at 0. + const origin = pages[0]?.offsetTop ?? 0; + const y = root.scrollTop + origin; + const index = Math.max( + 0, + firstPast(pages, y, (page) => page.offsetTop) - 1, + ); + const page = pages[index]; if (page && page.offsetHeight > 0) { anchorRef.current = { index, - fraction: (root.scrollTop - pageTop(root, page)) / page.offsetHeight, + fraction: (y - page.offsetTop) / page.offsetHeight, }; } - setCurrent(mostVisiblePage(root, pageCount)); - }, [pageCount]); + setCurrent(mostVisiblePage(root, pages)); + }, []); + + // A taller or shorter viewer changes which page shows most without any + // scroll event. + useEffect(() => { + const root = rootRef.current; + if (!root || typeof ResizeObserver === "undefined") { + return; + } + const observer = new ResizeObserver(() => sync()); + observer.observe(root); + return () => observer.disconnect(); + }, [sync]); // A zoom change resizes every page; keep the same spot of the same page at // the top. The pages took their new size from their style at commit. From 8c9ce6aa5a4a60890cb1424e86beea7e196a9a1f Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Mon, 2 Nov 2026 06:59:05 +0000 Subject: [PATCH 07/11] UN-4505 Keep the page jumped to current while the view stays there When the last few pages fit together, a jump to one of them is clamped at the end of the scroll and the end rule reported the last page again, so Previous and PageUp could not leave it. The page jumped to now stays current until the reader scrolls. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/PdfPageList.test.tsx | 44 +++++++++++++++++++ .../patterns/document-viewer/PdfPageList.tsx | 14 +++++- 2 files changed, 57 insertions(+), 1 deletion(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index 3ad5969494..92196b218f 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -496,6 +496,50 @@ it("reports the last page once the end is reached, even when others show as much expect(onCurrentPageChange).toHaveBeenLastCalledWith(9); }); +it("pages back from the last page when the last pages fit together", () => { + const { document } = fakeDocument(10); + const ref = createRef(); + const onCurrentPageChange = vi.fn(); + const { container } = render( + , + ); + const root = layOut( + container, + () => 1, + () => 2000, + ); + // Clamp scrolling at the end, as a browser does. + let scrollTop = 0; + Object.defineProperty(root, "scrollTop", { + get: () => scrollTop, + set: (value: number) => { + scrollTop = Math.min( + Math.max(0, value), + root.scrollHeight - root.clientHeight, + ); + }, + }); + act(() => ref.current?.scrollToPage(9)); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(9); + // Page 8's top is past the end of the scroll, so the jump lands at the end. + act(() => ref.current?.scrollToPage(8)); + act(() => { + root.dispatchEvent(new Event("scroll")); + }); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(8); + act(() => ref.current?.scrollToPage(7)); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(7); + // A scroll by the reader goes back to measuring. + scrollTo(root, root.scrollHeight - root.clientHeight); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(9); +}); + it("updates the current page when the viewer changes height without scrolling", () => { const { document } = fakeDocument(10); const onCurrentPageChange = vi.fn(); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index f490d20918..87a03ac155 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -155,6 +155,11 @@ export function PdfPageList({ const byPage = useMemo(() => highlightsByPage(highlights), [highlights]); const rootRef = useRef(null); const anchorRef = useRef({ index: 0, fraction: 0 }); + // The last page jumped to and where the jump landed. While the view stays + // there, that page is current, even when the browser clamped the jump at + // the end and another page shows more: otherwise Previous could never + // leave the last page when the last few pages fit together. + const jumpRef = useRef<{ index: number; scrollTop: number } | null>(null); const [current, setCurrent] = useState(0); const setRoot = useCallback( @@ -193,7 +198,13 @@ export function PdfPageList({ fraction: (y - page.offsetTop) / page.offsetHeight, }; } - setCurrent(mostVisiblePage(root, pages)); + const jump = jumpRef.current; + if (jump && Math.abs(root.scrollTop - jump.scrollTop) < 1) { + setCurrent(jump.index); + } else { + jumpRef.current = null; + setCurrent(mostVisiblePage(root, pages)); + } }, []); // A taller or shorter viewer changes which page shows most without any @@ -234,6 +245,7 @@ export function PdfPageList({ const page = root && pageElement(root, clamped); if (root && page) { root.scrollTop = pageTop(root, page); + jumpRef.current = { index: clamped, scrollTop: root.scrollTop }; sync(); } }, From 790ebdc7eb0ce9ae43f9afb024c359557ea472ba Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Mon, 2 Nov 2026 08:25:27 +0000 Subject: [PATCH 08/11] UN-4505 Track the current page by share shown, centre highlights exactly, clamp a starting zoom - The current page is the one showing the largest share of itself, a page taller than the view counting the view as whole, so part of a tall page no longer beats a short page shown whole. - scrollToHighlight centres on the page's own top, padding included; page jumps keep the gap above the page. - A numeric initialZoom goes through clampZoom like a fitted one. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 18 ++++-- .../document-viewer/DocumentViewer.tsx | 3 +- .../document-viewer/PdfPageList.test.tsx | 55 +++++++++++++++++-- .../patterns/document-viewer/PdfPageList.tsx | 15 +++-- 4 files changed, 77 insertions(+), 14 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index ef0a5a1572..b9a3df10b1 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -130,6 +130,16 @@ it("refits when the viewer is resized", async () => { expect(zoomLabel()).toBe("50%"); }); +it.each([ + [0, "25%"], + [Number.NaN, "100%"], + [10, "400%"], +])("keeps a starting zoom of %s within range", async (initialZoom, label) => { + render(); + await open(); + expect(zoomLabel()).toBe(label); +}); + it("zooms with + and − while the pages have focus, and leaves other keys alone", async () => { render(); await open(); @@ -273,9 +283,9 @@ const onPage1 = { variant: "box" as const, tone: "default" as const, }; -// Page 1 starts at 1016 and the highlight's centre is 500 px into it; the -// viewer reports no height, so centring adds nothing. -const onPage1Top = 1016 + 500; +// Page 1's top is at 1032, padding included, and the highlight's centre is +// 500 px into it; the viewer reports no height, so centring adds nothing. +const onPage1Top = 1032 + 500; function pagesRegion() { return screen.getByRole("region", { name: "Document pages" }); @@ -318,7 +328,7 @@ it("scrolls to the active highlight when it changes, and to one set while loadin activeHighlightId="date" />, ); - expect(pagesRegion().scrollTop).toBe(2 * 1016 + 500); + expect(pagesRegion().scrollTop).toBe(16 + 2 * 1016 + 500); } finally { restore(); } diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index 824bef2880..dc43b25a98 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -6,6 +6,7 @@ import { viewerKeyAction } from "@v1/lib/pdf/keyboard"; import { pdfSourceKey } from "@v1/lib/pdf/pdfjs"; import type { PageSize, PdfSource } from "@v1/lib/pdf/types"; import { + clampZoom, fitZoom, MAX_ZOOM, MIN_ZOOM, @@ -102,7 +103,7 @@ function initialZoomState( initial: DocumentViewerProps["initialZoom"], ): ZoomState { if (typeof initial === "number") { - return { mode: "custom", level: initial }; + return { mode: "custom", level: clampZoom(initial) }; } return { mode: initial ?? "fit-width", level: 1 }; } diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx index fc494ef06c..58a1900ae9 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.test.tsx @@ -353,15 +353,21 @@ function layOut( Object.defineProperties(root, { clientHeight: { get: viewHeight }, clientWidth: { value: 500 }, - scrollHeight: { get: () => 16 + count * (800 * zoom() + 16) }, + scrollHeight: { + get: () => 16 + count * (800 * zoom() + 16), + configurable: true, + }, }); for (const page of container.querySelectorAll( "[data-page-index]", )) { const index = Number(page.getAttribute("data-page-index")); Object.defineProperties(page, { - offsetTop: { get: () => 16 + index * (800 * zoom() + 16) }, - offsetHeight: { get: () => 800 * zoom() }, + offsetTop: { + get: () => 16 + index * (800 * zoom() + 16), + configurable: true, + }, + offsetHeight: { get: () => 800 * zoom(), configurable: true }, offsetWidth: { get: () => 600 * zoom() }, offsetLeft: { get: () => 16 }, }); @@ -417,8 +423,9 @@ it("centres a highlight, across as well as down for a box", () => { found = ref.current?.scrollToHighlight("w") ?? false; }); expect(found).toBe(true); - // Page 2 starts at 1632; the box's centre is 0.275 × 800 = 220 into it. - expect(root.scrollTop).toBe(1632 + 220 - 200); + // Page 2's top is at 1648, padding included; the box's centre is + // 0.275 × 800 = 220 into it, and the view is 400 tall. + expect(root.scrollTop).toBe(1648 + 220 - 200); // 0.55 × 600 = 330 across, from the page's left edge at 16. expect(root.scrollLeft).toBe(16 + 330 - 250); expect(ref.current?.scrollToHighlight("missing")).toBe(false); @@ -471,6 +478,44 @@ it("reports the page showing most of itself", () => { expect(onCurrentPageChange).toHaveBeenLastCalledWith(4); }); +it("counts a short page shown whole over part of a taller one", () => { + const { document } = fakeDocument(3); + const onCurrentPageChange = vi.fn(); + const heights = [1000, 200, 1000]; + const { container } = render( + ({ width: 600, height }))} + zoom={1} + onCurrentPageChange={onCurrentPageChange} + />, + ); + const root = layOut( + container, + () => 1, + () => 600, + ); + // Pages 0-1000, 1016-1216 and 1232-2232, laid out by hand. + const tops = [16, 1032, 1248]; + for (const page of container.querySelectorAll( + "[data-page-index]", + )) { + const index = Number(page.getAttribute("data-page-index")); + Object.defineProperties(page, { + offsetTop: { get: () => tops[index] }, + offsetHeight: { get: () => heights[index] }, + }); + } + Object.defineProperty(root, "scrollHeight", { get: () => 2264 }); + // 700-1300: 300 px of page 0 (half the view), all 200 px of page 1, and + // 52 px of page 2. Page 1 shows the most of itself. + scrollTo(root, 700); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(1); + // 400-1000: page 0 fills the view, so it counts as shown whole. + scrollTo(root, 400); + expect(onCurrentPageChange).toHaveBeenLastCalledWith(0); +}); + it("keeps the current page right for a page much taller than the viewer", () => { const { document } = fakeDocument(10); const onCurrentPageChange = vi.fn(); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index 87a03ac155..8cb78127a3 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -89,7 +89,8 @@ function firstPast( } /** - * The page with the most of itself in view; the lower index wins a tie, and + * The page showing the largest share of itself in view (a page taller than + * the view counts the view as whole); the lower index wins a tie, and * the last page wins once the end is reached, where a short last page could * otherwise never show the most. Read from the layout, so it is exact for * pages of any height and right straight after a jump, unlike intersection @@ -121,8 +122,12 @@ function mostVisiblePage( break; } const shown = - Math.min(page.offsetTop + page.offsetHeight, bottom) - - Math.max(page.offsetTop, top); + (Math.min(page.offsetTop + page.offsetHeight, bottom) - + Math.max(page.offsetTop, top)) / + // A share of what the page could show: of itself, or of the view when + // it is taller than the view. Raw pixels would let part of a tall page + // beat a short page shown whole. + Math.max(1, Math.min(page.offsetHeight, root.clientHeight)); if (shown > bestShown) { best = index; bestShown = shown; @@ -257,7 +262,9 @@ export function PdfPageList({ return false; } const centreY = (highlight.y + highlight.h / 2) * page.offsetHeight; - root.scrollTop = pageTop(root, page) + centreY - root.clientHeight / 2; + // The page's own top, padding included: centred exactly, unlike a + // page jump, which keeps the gap above the page. + root.scrollTop = page.offsetTop + centreY - root.clientHeight / 2; if (highlight.variant === "box") { const centreX = (highlight.x + highlight.w / 2) * page.offsetWidth; root.scrollLeft = page.offsetLeft + centreX - root.clientWidth / 2; From 61e474954caf804f61e7d471c4498e97c85c32df Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:09:27 +0000 Subject: [PATCH 09/11] UN-4505 [FIX] Address review: keys in fields, measured scrollbar, page reset, deferred fit layout - viewerKeyAction leaves keys typed into an input, textarea, select or contenteditable to that field. - Fit modes measure the platform scrollbar once (lib/scrollbar) instead of assuming 16 px, so a 17 px classic scrollbar no longer forces a horizontal scrollbar. - The page index resets while rendering when the document changes, so the toolbar never shows the last document's page against the new count. - In a fit mode the page list waits for the viewer's size, so pages are not laid out at 100% first and again at the fitted zoom. - sync() reads the page elements once per list, not on every scroll. - Document identity uses objectKey from lib/pdf/pdfjs, shared with pdfSourceKey. - useElementSize measures once where ResizeObserver is missing, matching the guard in PdfPageList. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 63 ++++++++++++++- .../document-viewer/DocumentViewer.tsx | 80 +++++++++++-------- .../document-viewer/PdfPageList.test.tsx | 19 +++++ .../patterns/document-viewer/PdfPageList.tsx | 15 +++- frontend/src/v1/hooks/useElementSize.test.ts | 46 +++++++++++ frontend/src/v1/hooks/useElementSize.ts | 8 +- frontend/src/v1/lib/pdf/keyboard.test.ts | 17 ++++ frontend/src/v1/lib/pdf/keyboard.ts | 22 ++++- frontend/src/v1/lib/pdf/pdfjs.ts | 24 ++++-- frontend/src/v1/lib/scrollbar.test.ts | 34 ++++++++ frontend/src/v1/lib/scrollbar.ts | 29 +++++++ 11 files changed, 308 insertions(+), 49 deletions(-) create mode 100644 frontend/src/v1/hooks/useElementSize.test.ts create mode 100644 frontend/src/v1/lib/scrollbar.test.ts create mode 100644 frontend/src/v1/lib/scrollbar.ts diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index 95008835ca..436749f544 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -27,6 +27,25 @@ vi.mock("@v1/lib/pdf/pdfjs", async (importOriginal) => { }; }); +// Every page/count pair the toolbar is rendered with, to catch a stale one. +const toolbarRenders = vi.hoisted(() => [] as [number, number][]); +vi.mock("./ViewerToolbar", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + ViewerToolbar: (props: Parameters[0]) => { + toolbarRenders.push([props.pageIndex, props.pageCount]); + return actual.ViewerToolbar(props); + }, + }; +}); + +// The platform's scrollbar, as the viewer would measure it. +const scrollbar = vi.hoisted(() => ({ width: 16 })); +vi.mock("@v1/lib/scrollbar", () => ({ + scrollbarWidth: () => scrollbar.width, +})); + const resizeCallbacks = new Set<(entries: ResizeObserverEntry[]) => void>(); // Every observer hears the size: the viewer's body and the page list's own. const resize = (width: number, height: number) => { @@ -74,7 +93,7 @@ const pageSizes = Array.from({ length: 3 }, () => ({ async function open() { await act(async () => opening.resolve({ document, pageSizes })); - // The body is 864 px wide: 816 + the 48 px fit gutter. + // The body is 864 px wide: 816 + 32 px of padding + a 16 px scrollbar. act(() => resize(864, 600)); } @@ -83,6 +102,7 @@ function zoomLabel() { } beforeEach(() => { + scrollbar.width = 16; vi.stubGlobal("ResizeObserver", FakeResizeObserver); vi.stubGlobal("IntersectionObserver", FakeIntersectionObserver); }); @@ -405,3 +425,44 @@ it("scrolls to an active highlight chosen before its document is shown", async ( restore(); } }); + +it("fits the width to the measured scrollbar, so a 17 px one adds no horizontal scroll", async () => { + scrollbar.width = 17; + render(); + await open(); + const page = screen.getAllByRole("figure")[0]; + const width = Number.parseFloat( + page?.style.getPropertyValue("--page-width") ?? "", + ); + // 864 - 32 - 17 = 815 px for the page, not 816. + expect(width).toBeCloseTo(815); +}); + +it("waits for the viewer's size before laying out pages in a fit mode", async () => { + render(); + await act(async () => opening.resolve({ document, pageSizes })); + expect(screen.getByRole("toolbar")).toBeTruthy(); + expect(screen.queryAllByRole("figure")).toHaveLength(0); + act(() => resize(864, 600)); + expect(screen.getAllByRole("figure")).toHaveLength(3); +}); + +it("starts a new document on its first page", async () => { + const { rerender } = render(); + await open(); + fireEvent.keyDown(screen.getByRole("region", { name: "Document pages" }), { + key: "End", + }); + expect(screen.getByLabelText("Page")).toHaveProperty("value", "3"); + + toolbarRenders.length = 0; + rerender(); + const other = { ...document, numPages: 1 } as PDFDocumentProxy; + await act(async () => + opening.resolve({ document: other, pageSizes: pageSizes.slice(0, 1) }), + ); + expect(screen.getByLabelText("Page")).toHaveProperty("value", "1"); + expect(screen.getByText("/ 1")).toBeTruthy(); + // Not even one render showed page 3 of 1. + expect(toolbarRenders.filter(([index, count]) => index >= count)).toEqual([]); +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index dc43b25a98..7881722d4b 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -3,7 +3,7 @@ import { usePdfDocument } from "@v1/hooks/usePdfDocument"; import { cn } from "@v1/lib/cn"; import type { Highlight } from "@v1/lib/pdf/highlights"; import { viewerKeyAction } from "@v1/lib/pdf/keyboard"; -import { pdfSourceKey } from "@v1/lib/pdf/pdfjs"; +import { objectKey, pdfSourceKey } from "@v1/lib/pdf/pdfjs"; import type { PageSize, PdfSource } from "@v1/lib/pdf/types"; import { clampZoom, @@ -14,6 +14,7 @@ import { zoomIn, zoomOut, } from "@v1/lib/pdf/zoom"; +import { scrollbarWidth } from "@v1/lib/scrollbar"; import { type KeyboardEvent, type Ref, @@ -70,23 +71,16 @@ interface ZoomState { level: number; } +/** The page list's padding, both sides together, in CSS px. */ +const LIST_PADDING = 32; + /** - * Room around a fitted page, in CSS px: the list's padding on both sides plus - * a vertical scrollbar, so fitting never causes a horizontal scrollbar. + * Room around a fitted page, in CSS px: the list's padding plus this + * platform's scrollbar, measured rather than guessed, so fitting never causes + * a horizontal scrollbar. */ -const FIT_GUTTER = 48; - -// Gives each opened document its own key, so a new one gets a fresh page list: -// back at the top, with no window or anchor left over from the last. -const documentKeys = new WeakMap(); -let nextDocumentKey = 0; -function documentKey(document: object): number { - let key = documentKeys.get(document); - if (key === undefined) { - key = nextDocumentKey++; - documentKeys.set(document, key); - } - return key; +function fitGutter(): number { + return LIST_PADDING + scrollbarWidth(); } function widest(pageSizes: readonly PageSize[]): PageSize | undefined { @@ -127,21 +121,33 @@ export function DocumentViewer({ initialZoomState(initialZoom), ); const [pageIndex, setPageIndex] = useState(0); + // A new document starts on its first page: reset while rendering, so the + // toolbar never shows the last document's page against this one's count. + const shownDocument = state.status === "ready" ? state.document : null; + const [pageIndexFor, setPageIndexFor] = useState(shownDocument); + if (pageIndexFor !== shownDocument) { + setPageIndexFor(shownDocument); + setPageIndex(0); + } const [bodyRef, bodySize] = useElementSize(); const listRef = useRef(null); const pendingRef = useRef(null); const ready = state.status === "ready"; + // In a fit mode the zoom comes from the viewer's size, so the pages wait for + // that measurement instead of laying out at 100% first and again after. + const measured = zoomState.mode === "custom" || bodySize !== null; + const showPages = ready && measured; const sourceKey = pdfSourceKey(source); // Both kept in step at commit, before any parent's layout effect can make // a request, so a request is always judged against the source it was made // for. - const readyRef = useRef(ready); + const readyRef = useRef(showPages); const sourceKeyRef = useRef(sourceKey); useLayoutEffect(() => { - readyRef.current = ready; + readyRef.current = showPages; sourceKeyRef.current = sourceKey; - }, [ready, sourceKey]); + }, [showPages, sourceKey]); const pageSizes = ready ? state.pageSizes : undefined; const pageCount = pageSizes?.length ?? 0; @@ -152,7 +158,7 @@ export function DocumentViewer({ if (zoomState.mode === "fit-width") { const page = widest(pageSizes); return page - ? fitZoom(page, bodySize.width, null, FIT_GUTTER) + ? fitZoom(page, bodySize.width, null, fitGutter()) : zoomState.level; } // Fit page fits every page, not the current one: in a document that mixes @@ -162,7 +168,7 @@ export function DocumentViewer({ for (const page of pageSizes) { level = Math.min( level, - fitZoom(page, bodySize.width, bodySize.height, FIT_GUTTER), + fitZoom(page, bodySize.width, bodySize.height, fitGutter()), ); } return Number.isFinite(level) ? level : zoomState.level; @@ -208,7 +214,7 @@ export function DocumentViewer({ // biome-ignore lint/correctness/useExhaustiveDependencies: retried on each new highlights list useEffect(() => { const pending = pendingRef.current; - if (!ready || !pending) { + if (!showPages || !pending) { return; } if (pending.sourceKey === sourceKey) { @@ -216,7 +222,7 @@ export function DocumentViewer({ } else { pendingRef.current = null; } - }, [ready, sourceKey, highlights, run]); + }, [showPages, sourceKey, highlights, run]); // The active highlight is scrolled to when it changes, and again for each // new source, so one chosen before its document arrived is still found. @@ -291,18 +297,22 @@ export function DocumentViewer({ )}
{ready ? ( - + showPages && ( + + ) ) : ( { + const { document } = fakeDocument(20); + const { container } = render( + , + ); + const region = container.querySelector("section"); + if (!region) { + throw new Error("no region"); + } + const query = vi.spyOn(region, "querySelectorAll"); + for (let step = 0; step < 10; step++) { + act(() => { + region.scrollTop = step * 100; + region.dispatchEvent(new Event("scroll")); + }); + } + expect(query.mock.calls.length).toBeLessThanOrEqual(1); +}); diff --git a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx index 055838521f..2be77b2789 100644 --- a/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/PdfPageList.tsx @@ -171,6 +171,9 @@ export function PdfPageList({ // leave the last page when the last few pages fit together. const jumpRef = useRef<{ index: number; scrollTop: number } | null>(null); const [current, setCurrent] = useState(0); + // The page elements, read once: the list is keyed per document and its + // pages never remount, so scrolling never has to query the DOM again. + const pagesRef = useRef([]); const setRoot = useCallback( (element: HTMLElement | null) => { @@ -189,8 +192,14 @@ export function PdfPageList({ // scroll and straight after a jump, so neither waits for a scroll event. const sync = useCallback(() => { const root = rootRef.current; - const pages = root ? pageElements(root) : []; - if (!root || pages.length === 0) { + if (!root) { + return; + } + if (pagesRef.current.length !== pageCount) { + pagesRef.current = pageElements(root); + } + const pages = pagesRef.current; + if (pages.length === 0) { return; } // The anchor uses the same coordinates as the jumps and the restore: @@ -215,7 +224,7 @@ export function PdfPageList({ jumpRef.current = null; setCurrent(mostVisiblePage(root, pages)); } - }, []); + }, [pageCount]); // A taller or shorter viewer changes which page shows most without any // scroll event. diff --git a/frontend/src/v1/hooks/useElementSize.test.ts b/frontend/src/v1/hooks/useElementSize.test.ts new file mode 100644 index 0000000000..f0f118c915 --- /dev/null +++ b/frontend/src/v1/hooks/useElementSize.test.ts @@ -0,0 +1,46 @@ +import { act, renderHook } from "@testing-library/react"; +import { afterEach, expect, it, vi } from "vitest"; +import { useElementSize } from "./useElementSize"; + +afterEach(() => { + vi.unstubAllGlobals(); + vi.restoreAllMocks(); +}); + +it("measures once where ResizeObserver is missing, rather than throwing", () => { + vi.stubGlobal("ResizeObserver", undefined); + const element = document.createElement("div"); + vi.spyOn(element, "clientWidth", "get").mockReturnValue(640); + vi.spyOn(element, "clientHeight", "get").mockReturnValue(480); + const { result } = renderHook(() => useElementSize()); + act(() => result.current[0](element)); + expect(result.current[1]).toEqual({ width: 640, height: 480 }); +}); + +it("follows the element's size with a ResizeObserver", () => { + let report: ResizeObserverCallback = () => undefined; + vi.stubGlobal( + "ResizeObserver", + class { + constructor(callback: ResizeObserverCallback) { + report = callback; + } + observe() { + // Sizes arrive only through report(). + } + disconnect() { + // Nothing to release. + } + }, + ); + const { result } = renderHook(() => useElementSize()); + act(() => result.current[0](document.createElement("div"))); + expect(result.current[1]).toBeNull(); + act(() => + report( + [{ contentRect: { width: 300, height: 200 } } as ResizeObserverEntry], + {} as ResizeObserver, + ), + ); + expect(result.current[1]).toEqual({ width: 300, height: 200 }); +}); diff --git a/frontend/src/v1/hooks/useElementSize.ts b/frontend/src/v1/hooks/useElementSize.ts index e2410696ac..c88efaade6 100644 --- a/frontend/src/v1/hooks/useElementSize.ts +++ b/frontend/src/v1/hooks/useElementSize.ts @@ -7,7 +7,9 @@ export interface ElementSize { /** * An element's content-box size, kept current with a ResizeObserver. Returns a - * ref for the element and its size, null until it has been measured. + * ref for the element and its size, null until it has been measured. Where + * ResizeObserver is missing (old embedded webviews) it measures once, so a + * caller waiting for a size is never left waiting. */ export function useElementSize(): [ (element: T | null) => void, @@ -20,6 +22,10 @@ export function useElementSize(): [ if (element === null) { return; } + if (typeof ResizeObserver === "undefined") { + setSize({ width: element.clientWidth, height: element.clientHeight }); + return; + } const observer = new ResizeObserver(([entry]) => { if (!entry) { return; diff --git a/frontend/src/v1/lib/pdf/keyboard.test.ts b/frontend/src/v1/lib/pdf/keyboard.test.ts index f7ab917e0c..a8323ea18a 100644 --- a/frontend/src/v1/lib/pdf/keyboard.test.ts +++ b/frontend/src/v1/lib/pdf/keyboard.test.ts @@ -17,3 +17,20 @@ it("leaves other keys and modified keys to the browser", () => { expect(viewerKeyAction({ key: "-", metaKey: true })).toBeNull(); expect(viewerKeyAction({ key: "Home", altKey: true })).toBeNull(); }); + +it("leaves keys typed into a field to the field", () => { + const input = document.createElement("input"); + const textarea = document.createElement("textarea"); + const select = document.createElement("select"); + for (const target of [input, textarea, select]) { + expect(viewerKeyAction({ key: "-", target })).toBeNull(); + expect(viewerKeyAction({ key: "End", target })).toBeNull(); + } + // happy-dom does not compute isContentEditable, so give it the shape. + expect( + viewerKeyAction({ key: "PageDown", target: { isContentEditable: true } }), + ).toBeNull(); + expect( + viewerKeyAction({ key: "PageDown", target: document.createElement("div") }), + ).toBe("next-page"); +}); diff --git a/frontend/src/v1/lib/pdf/keyboard.ts b/frontend/src/v1/lib/pdf/keyboard.ts index f8033ca429..3c3ce1f9a9 100644 --- a/frontend/src/v1/lib/pdf/keyboard.ts +++ b/frontend/src/v1/lib/pdf/keyboard.ts @@ -7,19 +7,39 @@ export type ViewerKeyAction = | "zoom-in" | "zoom-out"; +const TEXT_FIELDS = new Set(["INPUT", "TEXTAREA", "SELECT"]); + +/** Whether a key pressed here belongs to a field the user is typing in. */ +function isEditable(target: unknown): boolean { + if (typeof target !== "object" || target === null) { + return false; + } + const element = target as { tagName?: unknown; isContentEditable?: unknown }; + return ( + element.isContentEditable === true || + (typeof element.tagName === "string" && TEXT_FIELDS.has(element.tagName)) + ); +} + /** * Maps a key to a viewer action. Keys pressed with Ctrl, Meta or Alt are left - * to the browser, so its own zoom and shortcuts keep working. + * to the browser, so its own zoom and shortcuts keep working, and keys typed + * into a field (an input, textarea, select or contenteditable drawn over the + * pages) stay with the field. */ export function viewerKeyAction(event: { key: string; ctrlKey?: boolean; metaKey?: boolean; altKey?: boolean; + target?: unknown; }): ViewerKeyAction | null { if (event.ctrlKey || event.metaKey || event.altKey) { return null; } + if (isEditable(event.target)) { + return null; + } switch (event.key) { case "PageUp": return "previous-page"; diff --git a/frontend/src/v1/lib/pdf/pdfjs.ts b/frontend/src/v1/lib/pdf/pdfjs.ts index 5e2dbbca75..e5d596cb8f 100644 --- a/frontend/src/v1/lib/pdf/pdfjs.ts +++ b/frontend/src/v1/lib/pdf/pdfjs.ts @@ -65,8 +65,21 @@ export function toDocumentParams(source: PdfSource): DocumentInitParameters { return { ...shared, data: bytes }; } -const dataSourceIds = new WeakMap(); -let nextDataSourceId = 0; +const objectKeys = new WeakMap(); +let nextObjectKey = 0; + +/** + * A number for an object's identity, the same for as long as the object + * lives. The one notion of identity for documents and byte sources. + */ +export function objectKey(value: object): number { + let key = objectKeys.get(value); + if (key === undefined) { + key = nextObjectKey++; + objectKeys.set(value, key); + } + return key; +} /** * A key that changes only when the document would: the URL and request options @@ -88,12 +101,7 @@ export function pdfSourceKey(source: PdfSource | null): string { headers, ]); } - let id = dataSourceIds.get(source.data); - if (id === undefined) { - id = nextDataSourceId++; - dataSourceIds.set(source.data, id); - } - return `data:${id}`; + return `data:${objectKey(source.data)}`; } function pageSize(page: PDFPageProxy): PageSize { diff --git a/frontend/src/v1/lib/scrollbar.test.ts b/frontend/src/v1/lib/scrollbar.test.ts new file mode 100644 index 0000000000..b9fed1c25b --- /dev/null +++ b/frontend/src/v1/lib/scrollbar.test.ts @@ -0,0 +1,34 @@ +import { afterEach, expect, it, vi } from "vitest"; +import { resetScrollbarWidth, scrollbarWidth } from "./scrollbar"; + +afterEach(() => { + resetScrollbarWidth(); + vi.restoreAllMocks(); +}); + +function withScrollbar(width: number) { + vi.spyOn(HTMLElement.prototype, "offsetWidth", "get").mockReturnValue(100); + vi.spyOn(HTMLElement.prototype, "clientWidth", "get").mockReturnValue( + 100 - width, + ); +} + +it("measures a classic scrollbar, and removes its probe", () => { + withScrollbar(17); + const before = document.body.childElementCount; + expect(scrollbarWidth()).toBe(17); + expect(document.body.childElementCount).toBe(before); +}); + +it("is 0 with overlay scrollbars", () => { + withScrollbar(0); + expect(scrollbarWidth()).toBe(0); +}); + +it("measures once", () => { + withScrollbar(15); + const append = vi.spyOn(document.body, "appendChild"); + scrollbarWidth(); + scrollbarWidth(); + expect(append).toHaveBeenCalledTimes(1); +}); diff --git a/frontend/src/v1/lib/scrollbar.ts b/frontend/src/v1/lib/scrollbar.ts new file mode 100644 index 0000000000..0e8ed359e3 --- /dev/null +++ b/frontend/src/v1/lib/scrollbar.ts @@ -0,0 +1,29 @@ +let measured: number | null = null; + +/** + * The width of a classic vertical scrollbar on this platform, in CSS px: 0 + * with overlay scrollbars (macOS, mobile), usually 15 to 17 elsewhere. + * Measured once with an off-screen probe, so nothing has to guess it. + */ +export function scrollbarWidth(doc: Document = document): number { + if (measured !== null) { + return measured; + } + const body = doc.body; + if (!body) { + return 0; + } + const probe = doc.createElement("div"); + // A probe, not page styling: it never paints and is removed at once. + probe.style.cssText = + "position:absolute;top:-9999px;width:100px;height:100px;overflow:scroll;visibility:hidden"; + body.appendChild(probe); + measured = Math.max(0, probe.offsetWidth - probe.clientWidth); + probe.remove(); + return measured; +} + +/** Forgets the measurement. For tests. */ +export function resetScrollbarWidth(): void { + measured = null; +} From 984c83ca40c593b1d0ae20d5110f85ee9e2cc976 Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Tue, 3 Nov 2026 10:21:42 +0000 Subject: [PATCH 10/11] UN-4505 [FIX] Drop a waiting active-highlight scroll once the highlight is no longer active The retry effect runs before the effect that cancels a cleared selection, so a render that both delivers the highlight and clears it scrolled to a selection the reader had just cleared. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 25 +++++++++++++++++++ .../document-viewer/DocumentViewer.tsx | 11 +++++--- 2 files changed, 33 insertions(+), 3 deletions(-) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index 436749f544..ed63a64160 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -383,6 +383,31 @@ it("scrolls to an active highlight that arrives after its id", async () => { } }); +it("does not scroll to an active highlight cleared as it arrives", async () => { + const restore = layOutEveryPage(); + try { + const { rerender } = render( + , + ); + await open(); + // One render brings the highlight in and clears the selection. + rerender( + , + ); + expect(pagesRegion().scrollTop).toBe(0); + } finally { + restore(); + } +}); + it("keeps a scroll a parent asks for in its layout effect on mount", async () => { const restore = layOutEveryPage(); function Parent() { diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index 7881722d4b..0eb263c595 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -210,19 +210,24 @@ export function DocumentViewer({ // A waiting request runs once the pages are laid out, and is tried again // when the highlights change, in case its highlight has arrived. One made // for another source is dropped rather than scrolling a document nobody - // asked about. + // asked about, and so is one made for an active highlight that is no longer + // active: this effect runs before the one below that would cancel it. // biome-ignore lint/correctness/useExhaustiveDependencies: retried on each new highlights list useEffect(() => { const pending = pendingRef.current; if (!showPages || !pending) { return; } - if (pending.sourceKey === sourceKey) { + const stillActive = + !pending.fromActive || + (pending.request.kind === "highlight" && + pending.request.id === activeHighlightId); + if (pending.sourceKey === sourceKey && stillActive) { run(pending.request, pending.fromActive); } else { pendingRef.current = null; } - }, [showPages, sourceKey, highlights, run]); + }, [showPages, sourceKey, highlights, activeHighlightId, run]); // The active highlight is scrolled to when it changes, and again for each // new source, so one chosen before its document arrived is still found. From 9ca6a28a47837595636c1d5b4de5306c19cc90b6 Mon Sep 17 00:00:00 2001 From: ali <117142933+muhammad-ali-e@users.noreply.github.com> Date: Tue, 3 Nov 2026 11:25:15 +0000 Subject: [PATCH 11/11] UN-4505 Drop a waiting scroll as soon as the source changes A scroll queued for document A survived a switch to B and back to A while neither had opened, because the retry effect only clears a stale request once pages are shown; by then the key matched A again. It is now dropped on the source change itself. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01R7trgcAzu6jxDnit3eeozm --- .../document-viewer/DocumentViewer.test.tsx | 17 +++++++++++++++++ .../patterns/document-viewer/DocumentViewer.tsx | 9 +++++++++ 2 files changed, 26 insertions(+) diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx index ed63a64160..77871d4267 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.test.tsx @@ -331,6 +331,23 @@ it("drops a scroll asked for one document when another is shown", async () => { } }); +it("drops a waiting scroll when the source changes, even back to its own document", async () => { + const restore = layOutEveryPage(); + try { + const ref = createRef(); + const other: PdfSource = { kind: "url", url: "/other.pdf" }; + const { rerender } = render(); + act(() => ref.current?.scrollToPage(2)); + // Away and back before either document has opened. + rerender(); + rerender(); + await open(); + expect(pagesRegion().scrollTop).toBe(0); + } finally { + restore(); + } +}); + it("scrolls to the active highlight when it changes, and to one set while loading", async () => { const restore = layOutEveryPage(); try { diff --git a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx index 0eb263c595..076b5ef480 100644 --- a/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx +++ b/frontend/src/v1/components/patterns/document-viewer/DocumentViewer.tsx @@ -207,6 +207,15 @@ export function DocumentViewer({ [run], ); + // A request waiting for one source is dropped as soon as the source changes, + // even while nothing is shown: switching away and back before either opens + // must not bring it back. + useEffect(() => { + if (pendingRef.current && pendingRef.current.sourceKey !== sourceKey) { + pendingRef.current = null; + } + }, [sourceKey]); + // A waiting request runs once the pages are laid out, and is tried again // when the highlights change, in case its highlight has arrived. One made // for another source is dropped rather than scrolling a document nobody