From 38a6d3d9bab8d8457395f23531b31dee0d7d1df0 Mon Sep 17 00:00:00 2001 From: Huang Xin Date: Thu, 18 Jun 2026 20:36:31 +0800 Subject: [PATCH] fix(dict): stop iOS instant system dictionary popping multiple times (#4644) On iOS a single long-press emits several selectionchange events, so the instant quick action fired the system dictionary 2-3 times, stacking UIReferenceLibraryViewController sheets. Add a once-per-gesture latch to deferredAction (re-armed by beginGesture on touchstart/pointerdown) so the action runs at most once per gesture, mirroring the Android defer-to-touchend coalescing. Also fix two related cases: - Tapping outside to deselect after dismissing the dictionary occasionally re-opened it (~1/3): the deselect tap re-armed the latch and a racy lingering selectionchange re-fired. Gate the instant action on a long-press hold (isLongPressHold, 300ms, touch only) so a quick tap can't trigger it. - A Word Lens gloss tap ignored the system-dictionary setting (always opened the in-app popup); route it through handleDictionary so it honors the system dictionary like the toolbar and instant-quick-action paths. Verified on Android (Xiaomi) that the instant dictionary still fires once per long-press and re-arms for the next gesture; iOS double-popup and tap-to-deselect re-fire confirmed fixed. Co-authored-by: Claude Opus 4.8 (1M context) --- .../app/reader/utils/deferredAction.test.ts | 83 +++++++++++++++++-- .../reader/components/annotator/Annotator.tsx | 48 ++++++++++- .../src/app/reader/utils/deferredAction.ts | 52 ++++++++++-- 3 files changed, 165 insertions(+), 18 deletions(-) diff --git a/apps/readest-app/src/__tests__/app/reader/utils/deferredAction.test.ts b/apps/readest-app/src/__tests__/app/reader/utils/deferredAction.test.ts index 4fb5751a..e06a9dbc 100644 --- a/apps/readest-app/src/__tests__/app/reader/utils/deferredAction.test.ts +++ b/apps/readest-app/src/__tests__/app/reader/utils/deferredAction.test.ts @@ -1,8 +1,9 @@ import { describe, test, expect, vi } from 'vitest'; import { - cancelDeferredAction, + beginGesture, createDeferredActionState, flushDeferredAction, + isLongPressHold, runOrDeferAction, } from '@/app/reader/utils/deferredAction'; @@ -54,20 +55,64 @@ describe('deferredAction', () => { expect(second).toHaveBeenCalledTimes(1); }); - test('cancelDeferredAction discards a pending action without running it', () => { + test('beginGesture discards a pending action without running it', () => { const state = createDeferredActionState(); const action = vi.fn(); runOrDeferAction(state, true, action); - cancelDeferredAction(state); + beginGesture(state); flushDeferredAction(state); expect(action).not.toHaveBeenCalled(); }); + test('runs the action only once per gesture even when triggered repeatedly', () => { + // iOS long-press emits multiple selectionchange events for the same + // selection; each runs immediately (shouldDefer false). Only the first + // should fire so e.g. the system-dictionary sheet is presented once. + const state = createDeferredActionState(); + const action = vi.fn(); + + runOrDeferAction(state, false, action); + runOrDeferAction(state, false, action); + runOrDeferAction(state, false, action); + + expect(action).toHaveBeenCalledTimes(1); + }); + + test('beginGesture re-arms the action for the next gesture', () => { + const state = createDeferredActionState(); + const action = vi.fn(); + + runOrDeferAction(state, false, action); + runOrDeferAction(state, false, action); // same gesture: blocked + expect(action).toHaveBeenCalledTimes(1); + + beginGesture(state); // pointerdown/touchstart of the next gesture + runOrDeferAction(state, false, action); + expect(action).toHaveBeenCalledTimes(2); + }); + + test('iOS long-press scenario: multiple selectionchange events look up the word once', () => { + // Models the Annotator flow on iOS (no native-touch deferral): + // 1. pointerdown -> beginGesture (re-arm) + // 2. selectionchange -> runOrDeferAction(false) -> dictionary lookup + // 3. selectionchange -> runOrDeferAction(false) -> blocked (same gesture) + // 4. selectionchange -> runOrDeferAction(false) -> blocked (same gesture) + const state = createDeferredActionState(); + const lookup = vi.fn(); + + beginGesture(state); // pointerdown + runOrDeferAction(state, false, lookup); // selectionchange #1 + runOrDeferAction(state, false, lookup); // selectionchange #2 (echo) + runOrDeferAction(state, false, lookup); // selectionchange #3 (echo) + + expect(lookup).toHaveBeenCalledTimes(1); + }); + test('Android long-press scenario: selection-then-touchend runs the action once', () => { // Models the Annotator flow on Android: - // 1. touchstart -> androidTouchEnd = false (defer state cleared) + // 1. touchstart -> beginGesture (defer state cleared + re-armed) // 2. selectionchange -> handleQuickAction (deferred because !androidTouchEnd) // 3. touchend -> androidTouchEnd = true, flushDeferredAction const state = createDeferredActionState(); @@ -75,7 +120,7 @@ describe('deferredAction', () => { let androidTouchEnd = false; // touchstart - cancelDeferredAction(state); + beginGesture(state); androidTouchEnd = false; // selectionchange triggers the quick action @@ -90,5 +135,33 @@ describe('deferredAction', () => { // A subsequent stray touchend must not re-run the action flushDeferredAction(state); expect(quickAction).toHaveBeenCalledTimes(1); + + // An iOS-style echo (immediate trigger) after touchend within the same + // gesture must not re-run it either. + runOrDeferAction(state, false, quickAction); + expect(quickAction).toHaveBeenCalledTimes(1); + }); +}); + +describe('isLongPressHold', () => { + const MIN = 300; + + test('no recorded pointerdown (e.g. mouse) always qualifies', () => { + expect(isLongPressHold(0, 999_999, MIN)).toBe(true); + }); + + test('a held long-press (elapsed >= threshold) qualifies', () => { + // iOS surfaces a touch selection only after the ~500ms OS long-press. + expect(isLongPressHold(1000, 1000 + 500, MIN)).toBe(true); + }); + + test('exactly at the threshold qualifies', () => { + expect(isLongPressHold(1000, 1000 + MIN, MIN)).toBe(true); + }); + + test('a quick tap (elapsed < threshold) does NOT qualify', () => { + // The tap-to-deselect bug: a tap re-reports the lingering selection a few + // tens of ms after its pointerdown — must not fire the instant action. + expect(isLongPressHold(1000, 1000 + 40, MIN)).toBe(false); }); }); diff --git a/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx b/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx index b6a5bb1a..5ab8cd99 100644 --- a/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx +++ b/apps/readest-app/src/app/reader/components/annotator/Annotator.tsx @@ -38,9 +38,10 @@ import { eventDispatcher } from '@/utils/event'; import { findTocItemBS } from '@/services/nav'; import { throttle } from '@/utils/throttle'; import { - cancelDeferredAction, + beginGesture, createDeferredActionState, flushDeferredAction, + isLongPressHold, runOrDeferAction, } from '../../utils/deferredAction'; import { Insets } from '@/types/misc'; @@ -174,6 +175,10 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ // (Android long-press selects text via selectionchange before touchend). The // pending action runs on touchend so popups don't open under an active touch. const deferredQuickActionRef = useRef(createDeferredActionState()); + // Timestamp of the latest touch pointerdown (0 for mouse). Used to require a + // long-press hold before the instant quick action fires, so a tap-to-deselect + // can't re-open the dictionary off a racy lingering selectionchange (iOS). + const pointerDownTimeRef = useRef(0); // Set when a Word Lens gloss tap synthesizes a selection so the // selection-change effect opens the dictionary popup instead of the // annotation toolbar. Cleared as soon as it's consumed. @@ -353,7 +358,7 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ const ev = event.detail as NativeTouchEventType; if (ev.type === 'touchstart') { androidTouchEndRef.current = false; - cancelDeferredAction(deferredQuickActionRef.current); + beginGesture(deferredQuickActionRef.current); handleTouchStart(); } else if (ev.type === 'touchmove') { // The Android pointer engagement signal (throttled in MainActivity.kt). @@ -382,6 +387,25 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ detail.doc?.addEventListener('touchstart', handleTouchStart, opts); detail.doc?.addEventListener('touchmove', handleTouchmove, opts); detail.doc?.addEventListener('touchend', handleTouchEnd); + // Re-arm the instant quick action at the start of each gesture. Android does + // this via the native-touch touchstart above; iOS/desktop have no such path, + // and a single iOS long-press emits multiple selectionchange events for the + // same word — without re-arming, the system-dictionary sheet stacked twice + // (the action fired once per event instead of once per gesture). + if (!appService?.isAndroidApp) { + detail.doc?.addEventListener( + 'pointerdown', + (ev: Event) => { + beginGesture(deferredQuickActionRef.current); + // Remember when the gesture started so the instant quick action can + // require a long-press hold (touch only — mouse selections fire on + // pointerup and shouldn't be time-gated). + pointerDownTimeRef.current = + (ev as PointerEvent).pointerType === 'mouse' ? 0 : Date.now(); + }, + opts, + ); + } detail.doc?.addEventListener('pointerdown', handlePointerDown.bind(null, doc, index), opts); detail.doc?.addEventListener('pointermove', handlePointerMove.bind(null, doc, index), opts); detail.doc?.addEventListener('pointercancel', handlePointerCancel.bind(null, doc, index)); @@ -775,7 +799,21 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ // eslint-disable-next-line react-hooks/exhaustive-deps }, []); + // A real touch selection only appears after the OS long-press (~500ms); a + // quick tap that re-reports a lingering selection fires far sooner. + const quickActionMinHoldMs = 300; + const handleQuickAction = () => { + // iOS/desktop immediate path: only fire from a long-press hold. Without this + // a tap-to-deselect after dismissing the system dictionary occasionally + // re-opened it off a racy lingering selectionchange. Android defers to + // touchend (a deliberate lift) and is left as-is. + if ( + !appService?.isAndroidApp && + !isLongPressHold(pointerDownTimeRef.current, Date.now(), quickActionMinHoldMs) + ) { + return; + } const action = viewSettings.annotationQuickAction; const runAction = () => { switch (action) { @@ -867,8 +905,10 @@ const Annotator: React.FC<{ bookKey: string; contentInsets: Insets }> = ({ const { enableAnnotationQuickActions, annotationQuickAction } = viewSettings; if (wantWordLensDict) { - setShowAnnotPopup(false); - setShowDictionaryPopup(true); + // Route through handleDictionary so a Word Lens gloss tap honours the + // dictionary settings (system dictionary vs the in-app popup) — same + // as the selection-toolbar and instant-quick-action dictionary paths. + handleDictionary(); } else if (enableAnnotationQuickActions && annotationQuickAction && isTextSelected.current) { handleQuickAction(); } else { diff --git a/apps/readest-app/src/app/reader/utils/deferredAction.ts b/apps/readest-app/src/app/reader/utils/deferredAction.ts index 4a02f4c1..58debb9f 100644 --- a/apps/readest-app/src/app/reader/utils/deferredAction.ts +++ b/apps/readest-app/src/app/reader/utils/deferredAction.ts @@ -1,14 +1,32 @@ -// Tiny helper for actions that need to wait for an asynchronous "ready" signal -// before running. Used by the Annotator on Android: a long-press selects text -// while the finger is still down, but the quick action (translate/dictionary/ -// tts/etc.) must not fire until the user lifts (touchend), or the popup it -// opens would be dismissed by the in-progress touch. +// Tiny helper for the Annotator's instant quick action (translate/dictionary/ +// tts/etc.). It solves two related touch-gesture problems: +// +// * On Android a long-press selects text via selectionchange while the finger +// is still down, so the action is deferred until touchend — otherwise the +// popup it opens is dismissed by the in-progress touch (closes #3935). +// * A single long-press can emit MULTIPLE selectionchange events for the same +// selection. iOS is the worst offender: it re-confirms the native selection +// after our deselect(), firing the action again. Running per event stacked +// duplicate popups — e.g. two/three system-dictionary sheets on iOS. The +// `fired` latch makes the action run at most once per gesture; `beginGesture` +// re-arms it at the next touchstart/pointerdown. export interface DeferredActionState { pending: (() => void) | null; + // Whether the quick action has already run for the current gesture. + fired: boolean; } -export const createDeferredActionState = (): DeferredActionState => ({ pending: null }); +export const createDeferredActionState = (): DeferredActionState => ({ + pending: null, + fired: false, +}); + +const runOnce = (state: DeferredActionState, action: () => void): void => { + if (state.fired) return; + state.fired = true; + action(); +}; export const runOrDeferAction = ( state: DeferredActionState, @@ -20,16 +38,32 @@ export const runOrDeferAction = ( return; } state.pending = null; - action(); + runOnce(state, action); }; export const flushDeferredAction = (state: DeferredActionState): void => { const fn = state.pending; if (!fn) return; state.pending = null; - fn(); + runOnce(state, fn); }; -export const cancelDeferredAction = (state: DeferredActionState): void => { +// Start of a new touch gesture: drop any pending action and re-arm the latch so +// the next action can run. Call on touchstart/pointerdown. +export const beginGesture = (state: DeferredActionState): void => { state.pending = null; + state.fired = false; }; + +// A touch instant quick action must only fire from a long-press hold, never a +// quick tap. After the system dictionary is dismissed iOS re-selects the word; +// tapping outside to deselect it is a WebView pointerdown that re-arms the latch, +// and a racy selectionchange can re-report the lingering selection a few tens of +// ms later — which used to re-open the dictionary (~1/3 of taps). A genuine iOS +// touch selection only appears after the OS long-press (~500ms), so we require +// at least `minHoldMs` to have elapsed since the gesture's pointerdown. +// +// `pointerDownTime === 0` means no touch pointerdown was recorded (e.g. mouse on +// desktop), where this gate does not apply and the action always qualifies. +export const isLongPressHold = (pointerDownTime: number, now: number, minHoldMs: number): boolean => + pointerDownTime === 0 || now - pointerDownTime >= minHoldMs;