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) <noreply@anthropic.com>
This commit is contained in:
@@ -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(<ReadingRuler {...props} />);
|
||||
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(<ReadingRuler {...props} />);
|
||||
|
||||
// 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(
|
||||
<ReadingRuler
|
||||
bookKey='book-1'
|
||||
isVertical={false}
|
||||
rtl={false}
|
||||
lines={2}
|
||||
position={50}
|
||||
opacity={0.5}
|
||||
color='transparent'
|
||||
bookFormat='EPUB'
|
||||
viewSettings={scrolledSettings}
|
||||
gridInsets={{ top: 0, right: 0, bottom: 0, left: 0 }}
|
||||
/>,
|
||||
);
|
||||
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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
@@ -176,6 +176,12 @@ const ReadingRuler: React.FC<ReadingRulerProps> = ({
|
||||
// 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<ReadingRulerProps> = ({
|
||||
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 {
|
||||
|
||||
Reference in New Issue
Block a user