diff --git a/apps/readest-app/src/__tests__/paragraph-mode.test.tsx b/apps/readest-app/src/__tests__/paragraph-mode.test.tsx index d2801910..e21b05d9 100644 --- a/apps/readest-app/src/__tests__/paragraph-mode.test.tsx +++ b/apps/readest-app/src/__tests__/paragraph-mode.test.tsx @@ -208,6 +208,89 @@ describe('paragraph mode', () => { }); }); + it('resumes without scrolling the underlying view so repeated enter/exit cannot rewind (#4717)', async () => { + const doc = createDoc('

Para A

Para B

Para C

'); + const { view, renderer } = createMockView([doc], 0); + const viewRef = { current: view } as React.RefObject; + + render(); + await waitFor(() => { + expect(hookApi?.paragraphState.currentRange).toBeTruthy(); + }); + + // Resuming/entering focuses the paragraph already at the reading position. + // Scrolling the underlying view to that paragraph's start rewinds whenever it + // began on an earlier page, so the view must NOT be moved on resume (#4717). + expect(renderer.goTo).not.toHaveBeenCalled(); + expect(renderer.scrollToAnchor).not.toHaveBeenCalled(); + }); + + it('resumes at the view live CFI even when the store progress is stale (#4717)', async () => { + const doc = createDoc('

Block zero

Block one

Block two

'); + const { view } = createMockView([doc], 0); + // The rAF-debounced store (mockGetProgress) returns null/stale; the view's + // live lastLocation CFI points at the third paragraph. Resume must follow the + // live CFI (resolved against the current doc), not fall back to chapter start. + const thirdParagraph = doc.querySelectorAll('p')[2]!; + (view as unknown as { lastLocation: { cfi: string } }).lastLocation = { cfi: 'cfi-live' }; + (view.resolveCFI as ReturnType).mockImplementation((cfi: string) => + cfi === 'cfi-live' + ? { + index: 0, + anchor: () => { + const r = doc.createRange(); + r.selectNodeContents(thirdParagraph); + return r; + }, + } + : null, + ); + const viewRef = { current: view } as React.RefObject; + + render(); + await waitFor(() => { + expect(hookApi?.paragraphState.currentRange?.toString()).toContain('Block two'); + }); + }); + + it('does not scroll the underlying view when exiting paragraph mode (#4717)', async () => { + const doc = createDoc('

Para A

Para B

'); + const { view, renderer } = createMockView([doc], 0); + const viewRef = { current: view } as React.RefObject; + + render(); + await waitFor(() => { + expect(hookApi?.paragraphState.currentRange).toBeTruthy(); + }); + + await act(async () => { + await hookApi?.toggleParagraphMode(); + }); + + expect(renderer.scrollToAnchor).not.toHaveBeenCalled(); + }); + + it('still scrolls the underlying view when navigating paragraphs', async () => { + const doc = createDoc('

Para A

Para B

Para C

'); + const { view, renderer } = createMockView([doc], 0); + const viewRef = { current: view } as React.RefObject; + + render(); + await waitFor(() => { + expect(hookApi?.paragraphState.currentRange).toBeTruthy(); + }); + + await act(async () => { + await hookApi?.goToNextParagraph(); + }); + + // Navigation to another paragraph must move the underlying view (the goTo + // runs after a rAF inside focusCurrentParagraph, so wait for it). + await waitFor(() => { + expect(renderer.goTo).toHaveBeenCalled(); + }); + }); + it('renders preserved presentation and layout-aware click zones in the overlay', async () => { const dispatchSpy = vi.spyOn(eventDispatcher, 'dispatch'); const overlayBookKey = 'overlay-book'; @@ -382,20 +465,52 @@ describe('paragraph mode', () => { expect(onClose).toHaveBeenCalledTimes(1); }); + const getDialog = (container: HTMLElement) => + container.querySelector('[role="dialog"]') as HTMLDivElement; + + it('focuses the dialog when it opens so it receives keys directly (#4717)', async () => { + const { container } = await renderVisibleOverlay(vi.fn()); + const dialog = getDialog(container); + expect(document.activeElement).toBe(dialog); + }); + it('exits when the toggle paragraph mode shortcut (Shift+P) is pressed (#4717)', async () => { const onClose = vi.fn(); - await renderVisibleOverlay(onClose); + const { container } = await renderVisibleOverlay(onClose); - fireEvent.keyDown(document.body, { key: 'P', shiftKey: true }); + fireEvent.keyDown(getDialog(container), { key: 'P', shiftKey: true }); expect(onClose).toHaveBeenCalledTimes(1); }); + it('exits when Escape is pressed on the dialog (#4717)', async () => { + const onClose = vi.fn(); + const { container } = await renderVisibleOverlay(onClose); + + fireEvent.keyDown(getDialog(container), { key: 'Escape' }); + + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('stops the toggle key from propagating so it cannot fire twice (#4717)', async () => { + const onClose = vi.fn(); + const { container } = await renderVisibleOverlay(onClose); + const windowSpy = vi.fn(); + window.addEventListener('keydown', windowSpy); + + fireEvent.keyDown(getDialog(container), { key: 'P', shiftKey: true }); + + // The dialog handler must stop propagation so the global useShortcuts + // handler never receives the same keypress (which would re-toggle). + expect(windowSpy).not.toHaveBeenCalled(); + window.removeEventListener('keydown', windowSpy); + }); + it('does not exit on an unrelated key while visible', async () => { const onClose = vi.fn(); - await renderVisibleOverlay(onClose); + const { container } = await renderVisibleOverlay(onClose); - fireEvent.keyDown(document.body, { key: 'x' }); + fireEvent.keyDown(getDialog(container), { key: 'x' }); expect(onClose).not.toHaveBeenCalled(); }); diff --git a/apps/readest-app/src/__tests__/utils/event.test.ts b/apps/readest-app/src/__tests__/utils/event.test.ts index a85163e6..873f437f 100644 --- a/apps/readest-app/src/__tests__/utils/event.test.ts +++ b/apps/readest-app/src/__tests__/utils/event.test.ts @@ -68,6 +68,31 @@ describe('EventDispatcher', () => { eventDispatcher.off('no-detail', fn); }); + it('does not call listeners added during an in-progress dispatch (snapshot semantics)', async () => { + // Regression: a listener that (re)subscribes another listener mid-dispatch + // must not have that new listener fire for the *current* event. The live + // Set used to be iterated directly, so a handler added during an awaited + // listener got invoked in the same dispatch — causing paragraph mode to + // toggle twice per keypress (#4717). + const added = vi.fn(); + const adder = vi.fn(async () => { + await Promise.resolve(); + eventDispatcher.on('reentrant', added); + }); + eventDispatcher.on('reentrant', adder); + + await eventDispatcher.dispatch('reentrant'); + expect(adder).toHaveBeenCalledOnce(); + expect(added).not.toHaveBeenCalled(); + + // The newly-added listener takes effect on the next dispatch. + await eventDispatcher.dispatch('reentrant'); + expect(added).toHaveBeenCalledOnce(); + + eventDispatcher.off('reentrant', adder); + eventDispatcher.off('reentrant', added); + }); + it('awaits async listeners sequentially', async () => { const order: string[] = []; const slowFn = vi.fn(async () => { diff --git a/apps/readest-app/src/app/reader/components/paragraph/ParagraphOverlay.tsx b/apps/readest-app/src/app/reader/components/paragraph/ParagraphOverlay.tsx index a0acda4f..7d0b0cbe 100644 --- a/apps/readest-app/src/app/reader/components/paragraph/ParagraphOverlay.tsx +++ b/apps/readest-app/src/app/reader/components/paragraph/ParagraphOverlay.tsx @@ -331,12 +331,21 @@ const ParagraphOverlay: React.FC = ({ }; }, [bookKey, addParagraph]); + // Focus the dialog when it opens (the dialog/alert pattern) so it receives + // keydowns directly via its own onKeyDown handler, regardless of where focus + // sat before — fixes Shift+P/Escape not toggling when focus was still inside + // the book iframe (#4717). useEffect(() => { if (!isVisible) return; + containerRef.current?.focus({ preventScroll: true }); + }, [isVisible]); - const handleKeyDown = (e: KeyboardEvent) => { + // Keydown handler bound to the dialog element. Keeps every key inside the + // overlay (stopPropagation) so the global shortcut handler never sees it — the + // toggle therefore fires exactly once. + const handleKeyDown = useCallback( + (e: React.KeyboardEvent) => { e.stopPropagation(); - e.stopImmediatePropagation(); if (e.key === 'Escape' || e.key === 'Backspace') { e.preventDefault(); @@ -344,10 +353,6 @@ const ParagraphOverlay: React.FC = ({ return; } - // The overlay swallows every keydown in the capture phase, so the global - // toggle shortcut (Shift+P by default) never reaches useShortcuts. Honor - // it here so the same shortcut that enters paragraph mode also exits it - // (#4717). if (matchesShortcut(e, loadShortcuts().onToggleParagraphMode.keys)) { e.preventDefault(); onCloseRef.current?.(); @@ -358,18 +363,13 @@ const ParagraphOverlay: React.FC = ({ if (action === 'next') { e.preventDefault(); eventDispatcher.dispatch('paragraph-next', { bookKey }); - return; - } - - if (action === 'prev') { + } else if (action === 'prev') { e.preventDefault(); eventDispatcher.dispatch('paragraph-prev', { bookKey }); } - }; - - window.addEventListener('keydown', handleKeyDown, true); - return () => window.removeEventListener('keydown', handleKeyDown, true); - }, [activePresentation, bookKey, isVisible, viewSettings]); + }, + [activePresentation, bookKey, viewSettings], + ); useEffect(() => { if (!isVisible) return; @@ -473,6 +473,9 @@ const ParagraphOverlay: React.FC = ({ const handleBackdropClick = useCallback( (e: React.MouseEvent) => { + // Keep keyboard focus on the dialog so Escape/Shift+P keep working after a + // tap moved focus elsewhere (e.g. into the book iframe). + containerRef.current?.focus({ preventScroll: true }); // Tapping the empty area around the paragraph used to exit, which made it // easy to leave paragraph mode by accident. Reveal the controls instead so // exiting stays an explicit action (the bar's exit button or Escape). @@ -487,6 +490,8 @@ const ParagraphOverlay: React.FC = ({ const handleContentClick = useCallback( (e: React.MouseEvent) => { e.stopPropagation(); + // Keep keyboard focus on the dialog so it keeps receiving keys after a tap. + containerRef.current?.focus({ preventScroll: true }); const now = Date.now(); if (now - lastTapTimeRef.current < 300) { @@ -543,6 +548,10 @@ const ParagraphOverlay: React.FC = ({ className={clsx( 'fixed inset-0 z-40', 'flex flex-col items-center justify-center', + // The dialog is focused programmatically (so it receives keys); it is not + // a tab stop, so suppress the focus ring that would otherwise outline the + // whole viewport. + 'outline-none', 'transition-opacity duration-300 ease-out', isOverlayMounted ? 'opacity-100' : 'opacity-0', )} @@ -555,7 +564,7 @@ const ParagraphOverlay: React.FC = ({ }} onClick={handleBackdropClick} onTouchStart={handleTouchStart} - onKeyDown={(e) => e.stopPropagation()} + onKeyDown={handleKeyDown} > {/* TTS "following audio" indicator, pinned top-center. Anchored below the top safe-area inset the overlay already accounts for; idle/unsupported diff --git a/apps/readest-app/src/app/reader/hooks/useParagraphMode.ts b/apps/readest-app/src/app/reader/hooks/useParagraphMode.ts index 40bc1cc5..dca45326 100644 --- a/apps/readest-app/src/app/reader/hooks/useParagraphMode.ts +++ b/apps/readest-app/src/app/reader/hooks/useParagraphMode.ts @@ -205,10 +205,14 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => const isSameDoc = progressRange?.startContainer?.ownerDocument === doc; const lastParagraph = lastParagraphRef.current; - const resolveRangeFromLocation = (): Range | null => { - if (!progressLocation) return null; + // Resolve a CFI against the CURRENT document. A CFI is document-instance + // independent, so this survives the section iframe being recreated when + // paragraph mode toggles — unlike a stored Range, which is bound to the + // document it was created in. + const resolveRangeFromCfi = (cfi?: string | null): Range | null => { + if (!cfi) return null; try { - const resolved = view.resolveCFI(progressLocation); + const resolved = view.resolveCFI(cfi); if (!resolved || resolved.index !== docIndex) return null; const anchor = resolved.anchor(doc); // Realm-safe: iframe-realm Range fails top-realm `instanceof Range`. @@ -228,26 +232,22 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => if (!lastParagraph || !progressLocation) return null; if (lastParagraph.progressLocation !== progressLocation) return null; if (lastParagraph.docIndex !== docIndex) return null; - try { - const resolved = view.resolveCFI(lastParagraph.paragraphCfi); - if (!resolved || resolved.index !== docIndex) return null; - const anchor = resolved.anchor(doc); - // Realm-safe: iframe-realm Range fails top-realm `instanceof Range`. - if (isRangeLike(anchor)) return anchor; - if (anchor) { - const range = doc.createRange(); - range.selectNodeContents(anchor); - return range; - } - } catch { - return null; - } - return null; + return resolveRangeFromCfi(lastParagraph.paragraphCfi); }; + // Resume from the freshest current position. The view's live lastLocation + // CFI is set synchronously by foliate on every relocate; the readerStore + // progress is rAF-debounced and can lag, which sent resume to a stale page + // (the chapter start) and let repeated enter/exit walk further back (#4717). + // The view's live lastLocation CFI is foliate-generated and well-formed, + // so it's the most reliable resume target. The stored last-paragraph CFI + // is `view.getCFI` of a paragraph block range, which can come out malformed + // and resolve to an empty range — so it must NOT take priority (#4717). const targetRange = + resolveRangeFromCfi(view.lastLocation?.cfi) ?? resolveRangeFromLastParagraph() ?? - (isSameDoc ? progressRange : resolveRangeFromLocation()); + (isSameDoc ? progressRange : null) ?? + resolveRangeFromCfi(progressLocation); if (targetRange && iteratorRef.current) { try { @@ -289,48 +289,59 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => return initPromise; }, [getPrimaryContent, viewRef, getProgress, updateStateFromIterator]); - const focusCurrentParagraph = useCallback(async () => { - const view = viewRef.current; - const iterator = iteratorRef.current; - if (!view || !iterator) return; + // `align` scrolls the underlying view so the focused paragraph sits at the page + // top. Pass `false` when entering/resuming paragraph mode: the focused paragraph + // is already on screen, and scrolling to its start rewinds the reading position + // whenever that paragraph began on an earlier page — repeated enter/exit would + // then walk back to the chapter start (#4717). Navigation passes `true` so the + // view follows to off-screen paragraphs. + const focusCurrentParagraph = useCallback( + async (align = true) => { + const view = viewRef.current; + const iterator = iteratorRef.current; + if (!view || !iterator) return; - const range = iterator.current(); - if (!range) return; + const range = iterator.current(); + if (!range) return; - await new Promise((r) => requestAnimationFrame(r)); + await new Promise((r) => requestAnimationFrame(r)); - if (focusResetTimerRef.current) { - clearTimeout(focusResetTimerRef.current); - } + if (focusResetTimerRef.current) { + clearTimeout(focusResetTimerRef.current); + } - const presentation = getParagraphPresentation( - range.startContainer.ownerDocument, - range, - getViewSettings(bookKeyRef.current), - ); + const presentation = getParagraphPresentation( + range.startContainer.ownerDocument, + range, + getViewSettings(bookKeyRef.current), + ); - isFocusingRef.current = true; - const docIndex = currentDocIndexRef.current; - const renderer = view.renderer as FoliateView['renderer'] & { - goTo?: (target: { index: number; anchor: Range }) => Promise; - }; - if (docIndex !== undefined && renderer.goTo) { - renderer.goTo({ index: docIndex, anchor: range }); - } else { - view.renderer.scrollToAnchor?.(range); - } - focusResetTimerRef.current = setTimeout(() => { - isFocusingRef.current = false; - }, 200); + if (align) { + isFocusingRef.current = true; + const docIndex = currentDocIndexRef.current; + const renderer = view.renderer as FoliateView['renderer'] & { + goTo?: (target: { index: number; anchor: Range }) => Promise; + }; + if (docIndex !== undefined && renderer.goTo) { + renderer.goTo({ index: docIndex, anchor: range }); + } else { + view.renderer.scrollToAnchor?.(range); + } + focusResetTimerRef.current = setTimeout(() => { + isFocusingRef.current = false; + }, 200); + } - eventDispatcher.dispatch('paragraph-focus', { - bookKey: bookKeyRef.current, - range, - index: iterator.currentIndex, - total: iterator.length, - presentation, - }); - }, [getViewSettings, viewRef]); + eventDispatcher.dispatch('paragraph-focus', { + bookKey: bookKeyRef.current, + range, + index: iterator.currentIndex, + total: iterator.length, + presentation, + }); + }, + [getViewSettings, viewRef], + ); // Sync-focus path for TTS following. Moves focus to `index` and scrolls/emits // exactly like focusCurrentParagraph but WITHOUT arming isFocusingRef. The @@ -640,7 +651,8 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => const success = await initIterator(); if (success) { - await focusCurrentParagraph(); + // Resume: don't scroll the underlying view (would rewind, #4717). + await focusCurrentParagraph(false); } } else { setViewSettings(bookKeyRef.current, { ...settings, paragraphMode: newConfig }); @@ -661,7 +673,9 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => docIndex, }; } - view.renderer.scrollToAnchor?.(range); + // Don't scroll to the paragraph on exit: navigation already moved the + // view to the focused paragraph, and re-anchoring to its start rewinds + // the reading position when it began on an earlier page (#4717). } } eventDispatcher.dispatch('paragraph-mode-disabled', { bookKey: bookKeyRef.current }); @@ -690,7 +704,8 @@ export const useParagraphMode = ({ bookKey, viewRef }: UseParagraphModeProps) => const init = async () => { const success = await initIterator(); if (success) { - await focusCurrentParagraph(); + // Resume on open: don't scroll the underlying view (would rewind, #4717). + await focusCurrentParagraph(false); } }; const timer = setTimeout(init, 100); diff --git a/apps/readest-app/src/types/view.ts b/apps/readest-app/src/types/view.ts index cf54822d..61883f87 100644 --- a/apps/readest-app/src/types/view.ts +++ b/apps/readest-app/src/types/view.ts @@ -102,6 +102,9 @@ export interface FoliateView extends HTMLElement { ) => Promise; book: BookDoc; tts: TTS | null; + // The most recent relocate location, set synchronously by foliate on every + // relocate — fresher than the rAF-debounced readerStore progress. + lastLocation?: { cfi?: string; range?: Range | null }; isFixedLayout: boolean; language: { locale?: LocaleWithTextInfo; diff --git a/apps/readest-app/src/utils/event.ts b/apps/readest-app/src/utils/event.ts index 882abb0b..48f10ed9 100644 --- a/apps/readest-app/src/utils/event.ts +++ b/apps/readest-app/src/utils/event.ts @@ -22,7 +22,13 @@ class EventDispatcher { const listeners = this.asyncListeners.get(event); if (listeners) { const customEvent = new CustomEvent(event, { detail }); - for (const listener of listeners) { + // Snapshot the listeners before iterating: an awaited listener may + // (re)subscribe another handler for the same event — e.g. a React effect + // re-running on a state change it triggered. Iterating the live Set would + // invoke that freshly-added handler in the same dispatch, double-firing + // the event (paragraph mode toggled twice per keypress, #4717). dispatchSync + // already snapshots for the same reason. + for (const listener of [...listeners]) { await listener(customEvent); } }