From 97191a57c0ba2d3288bf959685fd57d8eb5719d9 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Mon, 1 Jun 2026 00:25:44 +0800 Subject: [PATCH] fix(reader): stop reading ruler creeping down on scroll (#4386) (#4388) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In scrolled mode the ruler's geometry-cache effect re-ran on every relocate — including those fired continuously while scrolling — and re-snapped the band to the next line forward from a screen-fixed anchor, so the band crept down the page as the reader scrolled. Place the band once on mount (and after a viewport-dimension change), but never re-snap on a plain scroll relocate. Click-driven snapping (the reading-ruler-move handler + pendingScrollAlign realign) is unchanged, and paginated mode is unaffected. Co-authored-by: Claude Opus 4.8 (1M context) --- .../components/ReadingRuler.test.tsx | 165 ++++++++++++++++-- .../app/reader/components/ReadingRuler.tsx | 36 +++- 2 files changed, 186 insertions(+), 15 deletions(-) diff --git a/apps/readest-app/src/__tests__/components/ReadingRuler.test.tsx b/apps/readest-app/src/__tests__/components/ReadingRuler.test.tsx index efb86ebc..1db462bc 100644 --- a/apps/readest-app/src/__tests__/components/ReadingRuler.test.tsx +++ b/apps/readest-app/src/__tests__/components/ReadingRuler.test.tsx @@ -1,21 +1,89 @@ import { cleanup, fireEvent, render, waitFor } from '@testing-library/react'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import ReadingRuler from '@/app/reader/components/ReadingRuler'; -import { ViewSettings } from '@/types/book'; +import { BookFormat, ViewSettings } from '@/types/book'; import { eventDispatcher } from '@/utils/event'; const saveViewSettings = vi.fn(); -vi.mock('@/context/EnvContext', () => ({ - useEnv: () => ({ envConfig: {} }), -})); +type RulerTestRect = { + top: number; + bottom: number; + left: number; + right: number; + width: number; + height: number; +}; -vi.mock('@/store/readerStore', () => ({ - useReaderStore: () => ({ - getProgress: () => null, - getView: () => ({ renderer: { columnCount: 1 } }), - }), -})); +// Mutable doubles read lazily by the store mock so individual tests can drive +// the progress range and the scrolled-mode visible contents. +let mockProgress: { range: unknown; pageinfo: { current: number } } | null = null; +let mockContents: Array<{ doc: unknown }> = []; + +vi.mock('@/context/EnvContext', () => { + // Stable envConfig ref; an unstable one would churn the throttled save → ruler + // setter → applyBlock callbacks and make the cache effect re-run every render. + const env = { envConfig: {} }; + return { useEnv: () => env }; +}); + +vi.mock('@/store/readerStore', () => { + // Stable store-method references (zustand returns the same selectors across + // renders); the ReadingRuler cache effect lists getView as a dependency, so an + // unstable ref would make it re-run on every render. + const getProgress = () => mockProgress; + const getView = () => ({ renderer: { columnCount: 1, getContents: () => mockContents } }); + const store = { getProgress, getView }; + return { useReaderStore: () => store }; +}); + +// Evenly spaced single-column text lines in iframe-content coordinates. +const makeLineRects = (count: number, pitch: number, height: number): RulerTestRect[] => + Array.from({ length: count }, (_, i) => ({ + top: i * pitch, + bottom: i * pitch + height, + left: 50, + right: 750, + width: 700, + height, + })); + +// A single visible section whose iframe is offset by `frameTop` along the scroll +// axis (negative = scrolled down). `buildScrolledLineBoxes` walks these contents. +const makeScrolledContents = ( + frameTop: number, + lineRects: RulerTestRect[], +): Array<{ doc: unknown }> => { + const doc: { + body: object; + createRange: () => unknown; + defaultView: { frameElement: { getBoundingClientRect: () => DOMRect } }; + } = { + body: {}, + createRange: () => ({ + startContainer: { ownerDocument: doc }, + selectNodeContents: () => {}, + getClientRects: () => lineRects, + }), + defaultView: { + frameElement: { + getBoundingClientRect: () => + ({ + x: 0, + y: frameTop, + top: frameTop, + left: 0, + right: 800, + bottom: frameTop + 5000, + width: 800, + height: 5000, + toJSON: () => ({}), + }) as DOMRect, + }, + }, + }; + return [{ doc }]; +}; vi.mock('@/helpers/settings', () => ({ saveViewSettings: (...args: unknown[]) => saveViewSettings(...args), @@ -38,6 +106,8 @@ describe('ReadingRuler', () => { beforeEach(() => { vi.clearAllMocks(); + mockProgress = null; + mockContents = []; Object.defineProperty(HTMLElement.prototype, 'clientHeight', { configurable: true, @@ -188,4 +258,79 @@ describe('ReadingRuler', () => { expect(consumed).toBe(false); expect(saveViewSettings).not.toHaveBeenCalled(); }); + + // Regression: issue #4386 — in scrolled mode the ruler used to re-snap on every + // relocate fired while scrolling, so its position crept down the page. It must + // stay fixed on screen while scrolling; snapping only happens on click. + it('keeps the ruler fixed on screen while scrolling in scrolled mode', () => { + const lineRects = makeLineRects(50, 100, 40); + mockProgress = { range: {}, pageinfo: { current: 0 } }; + mockContents = makeScrolledContents(0, lineRects); + const scrolledSettings = { ...viewSettings, scrolled: true } as ViewSettings; + + const props = { + bookKey: 'book-1', + isVertical: false, + rtl: false, + lines: 2, + position: 50, + opacity: 0.5, + color: 'transparent' as const, + bookFormat: 'EPUB' as BookFormat, + viewSettings: scrolledSettings, + gridInsets: { top: 0, right: 0, bottom: 0, left: 0 }, + }; + + const { container, rerender } = render(); + const rulerTop = () => + parseFloat((container.querySelector('.ruler') as HTMLDivElement).style.top); + + // Initial mount snaps the band to a real line, moving it off the 50% prop. + const initialTop = rulerTop(); + expect(initialTop).toBeGreaterThan(50); + + // Simulate scrolling: a new relocate range arrives with the content shifted + // up, but the section/page is unchanged. + mockProgress = { range: {}, pageinfo: { current: 0 } }; + mockContents = makeScrolledContents(-130, lineRects); + rerender(); + + // The band must not move on its own as the reader scrolls. + expect(rulerTop()).toBeCloseTo(initialTop, 5); + }); + + it('still snaps the ruler to lines when advancing by click in scrolled mode', async () => { + const lineRects = makeLineRects(50, 100, 40); + mockProgress = { range: {}, pageinfo: { current: 0 } }; + mockContents = makeScrolledContents(0, lineRects); + const scrolledSettings = { ...viewSettings, scrolled: true } as ViewSettings; + + const { container } = render( + , + ); + const rulerTop = () => + parseFloat((container.querySelector('.ruler') as HTMLDivElement).style.top); + const before = rulerTop(); + + const consumed = eventDispatcher.dispatchSync('reading-ruler-move', { + bookKey: 'book-1', + direction: 'forward', + }); + + expect(consumed).toBe(true); + await waitFor(() => { + expect(rulerTop()).toBeGreaterThan(before); + }); + }); }); diff --git a/apps/readest-app/src/app/reader/components/ReadingRuler.tsx b/apps/readest-app/src/app/reader/components/ReadingRuler.tsx index 6cb5ece4..94828244 100644 --- a/apps/readest-app/src/app/reader/components/ReadingRuler.tsx +++ b/apps/readest-app/src/app/reader/components/ReadingRuler.tsx @@ -176,6 +176,12 @@ const ReadingRuler: React.FC = ({ // In scrolled mode, set when a tap advances past the view edge and scrolls the // view; the next relocate realigns the band to the start/end of the new view. const pendingScrollAlignRef = useRef<'forward' | 'backward' | null>(null); + // In scrolled mode the band is fixed to the screen: it is snapped into place on + // the initial mount (and after a viewport-dimension change), but never re-snapped + // on a plain scroll relocate — that made it creep down the page (issue #4386). + // Snapping while scrolled is driven by clicks (the reading-ruler-move handler). + const scrolledPlacedRef = useRef(false); + const scrolledPlacedDimensionRef = useRef(0); const supportsLineSnap = !FIXED_LAYOUT_FORMATS.has(bookFormat); const columnCount = getView(bookKey)?.renderer?.columnCount ?? 1; @@ -333,12 +339,32 @@ const ReadingRuler: React.FC = ({ pending === 'forward' ? snapReadingRulerToLines(-Infinity, -Infinity, lines, 'forward', derivBoxes) : snapReadingRulerToLines(Infinity, Infinity, lines, 'backward', derivBoxes); - if (block) applyBlock(block.start, block.end, dimension, true); + if (block) { + applyBlock(block.start, block.end, dimension, true); + scrolledPlacedRef.current = true; + scrolledPlacedDimensionRef.current = dimension; + } } else if (!pageChanged) { - const block = - snapReadingRulerToLines(anchor, anchor, lines, 'forward', derivBoxes) ?? - snapReadingRulerToLines(Infinity, Infinity, lines, 'backward', derivBoxes); - if (block) applyBlock(block.start, block.end, dimension, false); + // In scrolled mode, only snap the band on the initial mount or after the + // viewport dimension changes (resize/relayout). A plain scroll fires a + // relocate without changing the dimension; re-snapping then would walk + // the band down the page as the reader scrolls (issue #4386). + const alreadyPlaced = + scrolled && + scrolledPlacedRef.current && + scrolledPlacedDimensionRef.current === dimension; + if (!alreadyPlaced) { + const block = + snapReadingRulerToLines(anchor, anchor, lines, 'forward', derivBoxes) ?? + snapReadingRulerToLines(Infinity, Infinity, lines, 'backward', derivBoxes); + if (block) { + applyBlock(block.start, block.end, dimension, false); + if (scrolled) { + scrolledPlacedRef.current = true; + scrolledPlacedDimensionRef.current = dimension; + } + } + } } } } catch {