diff --git a/src/__tests__/components/ContinuousView.test.tsx b/src/__tests__/components/ContinuousView.test.tsx index 6334bd32..d6458220 100644 --- a/src/__tests__/components/ContinuousView.test.tsx +++ b/src/__tests__/components/ContinuousView.test.tsx @@ -793,6 +793,31 @@ describe('ContinuousView arrow navigation', () => { expect(props.onFocusedTokenRefChange).toHaveBeenNthCalledWith(1, 'tok-1'); expect(props.onFocusedTokenRefChange).toHaveBeenNthCalledWith(2, 'tok-2'); }); + + it('steps from the externally-imposed focus, not the stale pending index, after an external change interrupts an in-flight internal nav', async () => { + // Sequence: an external nav (tok-3) starts its fade while tok-1 is still displayed; the user + // clicks Next during the fade (internal nav in flight — this parent never echoes it); then a + // second external change lands back on the still-displayed tok-1. Because that value equals the + // displayed ref, the focus-change effect early-returns without clearing the in-flight marker, + // so only the render-phase external-override detection resyncs the pending index. Without it, + // the next step would advance from the stale pending index (group 2 → tok-1) instead of the + // externally-imposed position (group 1 → tok-0). + const book = makeBook(); + const props = requiredProps(book, { focusedTokenRef: 'tok-1' }); + const { rerender } = render(, withAnalysisStore); + + // External nav while idle: the fade starts; the displayed focus is still tok-1. + rerender(); + // Internal nav in flight: Next from the displayed group (tok-1) emits tok-2. + await userEvent.click(screen.getByRole('button', { name: 'Next token' })); + expect(props.onFocusedTokenRefChange).toHaveBeenNthCalledWith(1, 'tok-2'); + + // The parent imposes an external position (not the tok-2 echo) that matches the displayed ref. + rerender(); + + await userEvent.click(screen.getByRole('button', { name: 'Previous token' })); + expect(props.onFocusedTokenRefChange).toHaveBeenNthCalledWith(2, 'tok-0'); + }); }); // --------------------------------------------------------------------------- diff --git a/src/__tests__/components/InterlinearNavContext.test.tsx b/src/__tests__/components/InterlinearNavContext.test.tsx index 0be436d0..d52637ab 100644 --- a/src/__tests__/components/InterlinearNavContext.test.tsx +++ b/src/__tests__/components/InterlinearNavContext.test.tsx @@ -5,7 +5,11 @@ import type { SerializedVerseRef } from '@sillsdev/scripture'; import { act, renderHook } from '@testing-library/react'; import type { ReactNode } from 'react'; -import { InterlinearNavProvider, useInterlinearNav } from '../../components/InterlinearNavContext'; +import { + INTERNAL_NAV_TTL_MS, + InterlinearNavProvider, + useInterlinearNav, +} from '../../components/InterlinearNavContext'; import { RECENTER_FADE_MS } from '../../components/recenter-fade'; /** Tuple shape returned by the PAPI scroll-group hook. */ @@ -266,6 +270,33 @@ describe('InterlinearNavContext', () => { expect(result.current.consumeInternalNav(b)).toBe(true); }); + it('expires a stranded internal mark after the TTL so a later external navigation fades', () => { + // When React batches two rapid internal clicks (verse A then B in one frame), the host + // echoes only the final value: B's marker is consumed but A's is stranded. Once the TTL has + // passed, a later external navigation to A must classify as external (consume returns + // false), not be misread as internal by the stale marker. + jest.useFakeTimers(); + try { + const { result } = renderNav( + makeScrollGroupHook({ book: 'GEN', chapterNum: 1, verseNum: 1 }), + ); + const a: SerializedVerseRef = { book: 'GEN', chapterNum: 1, verseNum: 5 }; + const b: SerializedVerseRef = { book: 'GEN', chapterNum: 1, verseNum: 10 }; + + act(() => { + result.current.navigate(a, 'internal'); + result.current.navigate(b, 'internal'); + }); + // The host coalesces the batched navigations and echoes only the final value. + expect(result.current.consumeInternalNav(b)).toBe(true); + + jest.advanceTimersByTime(INTERNAL_NAV_TTL_MS + 1); + expect(result.current.consumeInternalNav(a)).toBe(false); + } finally { + jest.useRealTimers(); + } + }); + it('matches a verse-0 internal mark against the host-normalized verse-1 reference', () => { // An internal navigation stamped at chapter granularity (verse 0) must still be consumable // when the host echoes it back normalized to the chapter's first verse (verse 1) — the keys @@ -482,6 +513,36 @@ describe('InterlinearNavContext', () => { expect(result.current.fadePhase).toBe('in'); }); + it('re-engages the curtain when an expired stranded internal mark names the mid-reveal target', () => { + // A stranded internal marker (its echo never arrived) must stop exempting the verse once the + // TTL passes: a later external navigation to that verse landing mid-reveal still re-engages + // the curtain. Markers stamp at navigate time and RECENTER_FADE_MS (500ms) is far shorter + // than the TTL (3000ms), so the clock is advanced past the TTL *before* the reveal begins — + // advancing during the 'in' phase would fire the fade-in timer to 'idle' first. + const { result, rerender, setRef } = renderNavMutable({ + book: 'GEN', + chapterNum: 1, + verseNum: 1, + }); + + // Strand a marker for the eventual target: an internal navigation whose echo never arrives. + act(() => result.current.navigate({ book: 'ZEP', chapterNum: 3, verseNum: 1 }, 'internal')); + // Let the marker expire while the clock is still idle. + act(() => jest.advanceTimersByTime(INTERNAL_NAV_TTL_MS + 1)); + + // Cross-book navigation, then settle: the curtain is mid-reveal ('in'). + act(() => setRef({ book: 'ZEP', chapterNum: 1, verseNum: 1 })); + rerender(); + act(() => result.current.reportSettled()); + expect(result.current.fadePhase).toBe('in'); + + // An external navigation to the stranded verse lands mid-reveal. The expired marker must not + // exempt it: the curtain re-engages. + act(() => setRef({ book: 'ZEP', chapterNum: 3, verseNum: 1 })); + rerender(); + expect(result.current.fadePhase).toBe('out'); + }); + it('does not re-engage the curtain for a verse-0 echo during the fade-in', () => { // The host's chapter re-broadcast (verse 0 for the chapter already shown) is sticky — it // names the verse currently displayed, so it must not read as a fresh navigation mid-reveal. diff --git a/src/__tests__/components/Interlinearizer.test.tsx b/src/__tests__/components/Interlinearizer.test.tsx index 0e10e848..e9b5e600 100644 --- a/src/__tests__/components/Interlinearizer.test.tsx +++ b/src/__tests__/components/Interlinearizer.test.tsx @@ -419,13 +419,6 @@ describe('Interlinearizer', () => { expect(capturedSegmentViewPropsList[1].isActive).toBeFalsy(); }); - it('renders all segments when navigating to a title reference (verse 0)', () => { - const titleRef: SerializedVerseRef = { book: 'GEN', chapterNum: 1, verseNum: 0 }; - renderInterlinearizer({ book: GEN_1_MULTI_BOOK, scrRef: titleRef }); - - expect(screen.getAllByTestId('segment-view')).toHaveLength(2); - }); - it('calls setScrRef with the segment ref when a segment fires onSelect', () => { const mockNavigate = jest.fn(); renderInterlinearizer({ book: GEN_1_MULTI_BOOK, navigate: mockNavigate }); @@ -838,45 +831,6 @@ describe('Interlinearizer', () => { expect(screen.queryByText('Chapter 2')).not.toBeInTheDocument(); }); - it('tags each inline chapter header with its chapter number for snap targeting', () => { - renderInterlinearizer({ - book: GEN_TWO_CHAPTER_BOOK, - scrRef: { book: 'GEN', chapterNum: 1, verseNum: 1 }, - continuousScroll: false, - }); - - expect(screen.getByText('Chapter 1')).toHaveAttribute('data-chapter-start', '1'); - expect(screen.getByText('Chapter 2')).toHaveAttribute('data-chapter-start', '2'); - }); - - it('highlights the first verse of a chapter when the reference names verse 0 (the heading)', () => { - renderInterlinearizer({ - book: GEN_TWO_CHAPTER_BOOK, - scrRef: { book: 'GEN', chapterNum: 2, verseNum: 0 }, - continuousScroll: false, - }); - - // Verse 0 names the chapter heading, which is not a verse; the active-verse marker still lands - // on the chapter's first segment so a verse stays highlighted. - const activeIds = new Set( - capturedSegmentViewPropsList.filter((p) => p.isActive).map((p) => p.segment.id), - ); - expect([...activeIds]).toEqual(['GEN 2:1']); - }); - - it('scrolls the chapter heading to the top when the reference names verse 0', () => { - const scrollSpy = jest.spyOn(Element.prototype, 'scrollIntoView'); - renderInterlinearizer({ - book: GEN_TWO_CHAPTER_BOOK, - scrRef: { book: 'GEN', chapterNum: 2, verseNum: 0 }, - continuousScroll: false, - }); - - // The snap targets the chapter-2 heading element rather than the active verse-1 segment. - const heading = screen.getByText('Chapter 2'); - expect(scrollSpy.mock.contexts).toContain(heading); - }); - it('renders the snap-to-active-verse button when segments are present', () => { renderInterlinearizer({ book: GEN_1_MULTI_BOOK }); diff --git a/src/__tests__/hooks/useSegmentWindow.test.ts b/src/__tests__/hooks/useSegmentWindow.test.ts index ce2531fc..f64cc4d8 100644 --- a/src/__tests__/hooks/useSegmentWindow.test.ts +++ b/src/__tests__/hooks/useSegmentWindow.test.ts @@ -670,52 +670,6 @@ describe('useSegmentWindow', () => { expect(scrollIntoView).toHaveBeenCalledWith({ behavior: 'auto', block: 'start' }); }); - it('snaps the chapter heading (not the active verse) when the reference names verse 0', () => { - const book = makeBook(60, 60); - const scrollIntoView = jest.spyOn(Element.prototype, 'scrollIntoView'); - const { container, rerender } = renderSegmentWindow(book, { - book: 'GEN', - chapterNum: 1, - verseNum: 1, - }); - - // Mount both candidate targets: the active verse-1 segment and the chapter-2 heading. A verse-0 - // reference should snap the heading, leaving the active verse highlighted below it. - const active = document.createElement('div'); - active.setAttribute('aria-current', 'true'); - container.appendChild(active); - const heading = document.createElement('span'); - heading.setAttribute('data-chapter-start', '2'); - container.appendChild(heading); - - act(() => rerender({ b: book, ref: { book: 'GEN', chapterNum: 2, verseNum: 0 } })); - act(() => jest.advanceTimersByTime(RECENTER_FADE_MS)); - - expect(scrollIntoView.mock.contexts).toContain(heading); - expect(scrollIntoView.mock.contexts).not.toContain(active); - }); - - it('falls back to the active verse for a verse-0 reference when no heading is mounted', () => { - const book = makeBook(60, 60); - const scrollIntoView = jest.spyOn(Element.prototype, 'scrollIntoView'); - const { container, rerender } = renderSegmentWindow(book, { - book: 'GEN', - chapterNum: 1, - verseNum: 1, - }); - - // No `[data-chapter-start]` element (headings suppressed by chapterLabelInVerse), so the snap - // falls back to the active verse. - const active = document.createElement('div'); - active.setAttribute('aria-current', 'true'); - container.appendChild(active); - - act(() => rerender({ b: book, ref: { book: 'GEN', chapterNum: 2, verseNum: 0 } })); - act(() => jest.advanceTimersByTime(RECENTER_FADE_MS)); - - expect(scrollIntoView.mock.contexts).toContain(active); - }); - it('grows the snap spacer when scrollIntoView cannot reach the top', () => { const book = makeBook(60, 0); const scrollIntoView = jest.fn(); diff --git a/src/components/ContinuousView.tsx b/src/components/ContinuousView.tsx index d0fe9965..9b590414 100644 --- a/src/components/ContinuousView.tsx +++ b/src/components/ContinuousView.tsx @@ -259,6 +259,28 @@ export default function ContinuousView({ * group instead of two. */ const pendingPhraseIndexRef = useRef(0); + + /** + * `focusedTokenRef` prop value from the previous render. Lets the sync block below distinguish a + * prop that merely hasn't echoed an in-flight internal nav yet (unchanged since last render) from + * one the parent changed to an external position (changed to something other than the in-flight + * ref). + */ + const prevFocusedTokenRefPropRef = useRef(focusedTokenRef); + // If the prop changed to anything other than the in-flight internal ref, the parent imposed an + // external position instead of echoing the nav. Clear the in-flight marker so the pending index + // resyncs below; otherwise the next step() would advance from the stale pending index rather + // than the externally-imposed position. The focus-change effect can't cover this case: it + // early-returns without clearing the marker when the external value already matches the + // displayed ref. + if ( + internalFocusedTokenRefRef.current !== undefined && + focusedTokenRef !== prevFocusedTokenRefPropRef.current && + focusedTokenRef !== internalFocusedTokenRefRef.current + ) { + internalFocusedTokenRefRef.current = undefined; + } + prevFocusedTokenRefPropRef.current = focusedTokenRef; // Keep in sync with the rendered value so external jumps reset the pending index. When an // internal nav is still in flight (the parent hasn't echoed back yet), do not overwrite: a rapid // second click needs to read the already-advanced pending index rather than the stale rendered diff --git a/src/components/InterlinearNavContext.tsx b/src/components/InterlinearNavContext.tsx index cf907f6c..10d9228f 100644 --- a/src/components/InterlinearNavContext.tsx +++ b/src/components/InterlinearNavContext.tsx @@ -88,6 +88,28 @@ export function verseKey(ref: SerializedVerseRef): string { return `${normalized.book}:${normalized.chapterNum}:${normalized.verseNum}`; } +/** + * How long an unconsumed internal-navigation marker stays valid, in milliseconds. The host's echo + * normally consumes a marker within milliseconds, but when React batches rapid clicks (verse A then + * B in one frame) the host echoes only the final value, stranding A's marker. Without expiry, a + * much-later external navigation to A would consume the stale marker and skip its recenter fade. 3s + * is far beyond any echo round-trip yet well before the user could plausibly navigate back. + */ +export const INTERNAL_NAV_TTL_MS = 3000; + +/** + * The single freshness definition for an internal-navigation marker, shared by both marker readers + * — `consumeInternalNav` and the render-phase mid-reveal guard — so the two can never drift on what + * counts as expired. The boundary is consistent with eviction: a marker is fresh iff its age is at + * most {@link INTERNAL_NAV_TTL_MS}. + * + * @param stampedAt - The marker's `Date.now()` stamp, or `undefined` when no marker exists. + * @returns `true` when a marker exists and is within the TTL. + */ +function isInternalNavMarkerFresh(stampedAt: number | undefined): boolean { + return stampedAt !== undefined && Date.now() - stampedAt <= INTERNAL_NAV_TTL_MS; +} + /** * The single navigation surface for the Interlinearizer WebView. Owns the scripture reference, the * scroll-group linkage, the cross-book fade clock, and the internal/external classification of each @@ -128,7 +150,10 @@ export interface InterlinearNav { * Consumes a pending internal-navigation marker for `ref`: returns `true` (and clears the marker) * when the most recent {@link navigate} to this verse was `internal`, else `false`. The segment * window calls this when an anchor change arrives to decide whether to fade. Consuming clears the - * marker so a later _external_ navigation to the same verse still fades. + * marker so a later _external_ navigation to the same verse still fades. Markers older than + * {@link INTERNAL_NAV_TTL_MS} are ignored (and discarded): a marker stranded by React batching + * rapid clicks — where the host echoes only the last of several internal navigations — must not + * misclassify a later external navigation to the un-echoed verse. * * @param ref - The reference whose pending classification to consume. * @returns `true` if the navigation to `ref` was internal (skip the fade), else `false`. @@ -242,26 +267,36 @@ export function InterlinearNavProvider({ liveScrRefRef.current = liveScrRef; /** - * Verse keys of internal navigations still awaiting their host round-trip. A `navigate(ref, - * 'internal')` adds `verseKey(ref)`; `consumeInternalNav` removes it on match. A set (not a - * single value) handles rapid successive internal clicks: if two verses are clicked before the - * host delivers the first `liveScrRef`, both keys stay pending so neither delivery is misread as - * external. + * Verse keys of internal navigations still awaiting their host round-trip, each mapped to its + * `Date.now()` stamp. `navigate(ref, 'internal')` records `verseKey(ref)`; `consumeInternalNav` + * removes it on match. Keyed (not a single value) so that rapid successive clicks both stay + * pending and neither host delivery is misread as external. The stamp gives each marker a TTL + * ({@link INTERNAL_NAV_TTL_MS} — see its doc for why stranded markers must expire), honored by + * BOTH readers: `consumeInternalNav` (which also evicts expired markers) and the render-phase + * mid-reveal guard (a pure read — no eviction during render). */ - const pendingInternalNavRef = useRef>(new Set()); + const pendingInternalNavRef = useRef>(new Map()); const navigate = useCallback( (newScrRef: SerializedVerseRef, origin: NavOrigin = 'external') => { - if (origin === 'internal') pendingInternalNavRef.current.add(verseKey(newScrRef)); + if (origin === 'internal') pendingInternalNavRef.current.set(verseKey(newScrRef), Date.now()); setScrRef(newScrRef); }, [setScrRef], ); const consumeInternalNav = useCallback((ref: SerializedVerseRef) => { + const pending = pendingInternalNavRef.current; + // Evict expired markers before matching, so a marker stranded by a batched rapid-click (its + // echo never arrived) cannot be consumed by a later external navigation to the same verse. + // Eviction also bounds the map's size; freshness is the shared `isInternalNavMarkerFresh` + // definition so this reader and the mid-reveal guard cannot drift. + pending.forEach((stampedAt, pendingKey) => { + if (!isInternalNavMarkerFresh(stampedAt)) pending.delete(pendingKey); + }); const key = verseKey(ref); - if (!pendingInternalNavRef.current.has(key)) return false; - pendingInternalNavRef.current.delete(key); + if (!pending.has(key)) return false; + pending.delete(key); return true; }, []); @@ -298,17 +333,16 @@ export function InterlinearNavProvider({ } else if ( fadePhase === 'in' && verseKey(liveScrRef) !== verseKey(prevLiveScrRef) && - !pendingInternalNavRef.current.has(verseKey(liveScrRef)) + !isInternalNavMarkerFresh(pendingInternalNavRef.current.get(verseKey(liveScrRef))) ) { - // A follow-up external navigation landing while the cross-book reveal is still animating. The - // host resolves one picker selection as two navigations — the book change first, the precise - // target a beat later — so the second routinely arrives mid-fade-in. Left alone it would fade - // the freshly revealed content a second time (curtain up, content out, content in: the "double - // fade"). Instead, fold it into the same curtain cycle: re-engage the curtain (the CSS - // transition carries the opacity smoothly down from wherever the rise had reached), let the - // views re-anchor on the new verse behind it, and lift once on their settle. Internal echoes - // (a click made during the reveal) are exempt — the curtain must not drop over a navigation - // whose target is already on screen. + // A follow-up external navigation landing mid-fade-in: the host resolves one picker selection + // as two navigations (book change, then precise target), so the second routinely arrives while + // the reveal is still animating and would fade the fresh content a second time. Instead, + // re-engage the curtain (the CSS transition carries opacity smoothly down from wherever the + // rise reached) and lift once when the views settle on the new verse. Internal echoes (a click + // made during the reveal) are exempt — their target is already on screen — but the exemption + // honors the marker TTL, so a stranded marker cannot suppress the re-engage. Pure read, no + // eviction: this runs during render; `consumeInternalNav` handles eviction. /* v8 ignore next -- defensive: reportSettled always arms the fade-in timer alongside 'in' */ if (fadeInTimeoutRef.current !== undefined) clearTimeout(fadeInTimeoutRef.current); fadeInTimeoutRef.current = undefined; diff --git a/src/components/Interlinearizer.tsx b/src/components/Interlinearizer.tsx index 668a0c45..da8970a7 100644 --- a/src/components/Interlinearizer.tsx +++ b/src/components/Interlinearizer.tsx @@ -31,7 +31,12 @@ type InterlinearizerProps = Readonly<{ book: Book; /** When true, the horizontal token strip is shown above the segment list. */ continuousScroll: boolean; - /** Current scripture reference used to highlight the active verse. */ + /** + * Current scripture reference used to highlight the active verse. Must carry a normalized verse + * number (>= 1): production refs flow through `normalizeScrRef` in `InterlinearNavContext`, which + * maps a verse-0 (chapter heading) selection to verse 1 before it reaches this component. A raw + * verse-0 ref would match no segment, leaving focus unseeded and nothing highlighted. + */ scrRef: SerializedVerseRef; /** * BCP 47 tag for reading and writing gloss values. Defaults to `analysisLanguages[0]` of the @@ -209,7 +214,9 @@ function InterlinearizerInner({ updatePhrase(phraseMode.phraseId, phraseMode.originalTokens); setPhraseMode({ kind: 'view' }); // phraseMode is intentionally omitted: adding it would re-fire on every edit - // keystroke; isRevert changing to true guarantees phraseMode holds the revert values. + // keystroke; isRevert changing to true guarantees phraseMode holds the revert values — a + // guarantee that holds only while isRevert stays derived directly from phraseMode above, so + // don't move or memoize that derivation independently of this effect. // eslint-disable-next-line react-hooks/exhaustive-deps }, [isRevert, updatePhrase, setPhraseMode]); @@ -228,7 +235,10 @@ function InterlinearizerInner({ /* v8 ignore next -- activeSeg is always defined when the book includes the active verse */ setFocusedTokenRef(firstWordTokenRefOf(activeSeg)); // findActiveSegment is intentionally excluded: the verse-coordinate deps already capture the - // change we care about, and it changes identity on every scrRef update. + // change we care about, and it changes identity on every scrRef update. focusedTokenRef and + // tokenSegmentMap are excluded too — they are read only as guards; as deps they would re-run + // this effect on every focus move and clobber the deliberately-focused token with the verse's + // first word. // eslint-disable-next-line react-hooks/exhaustive-deps }, [scrRef.book, scrRef.chapterNum, scrRef.verseNum]); @@ -360,7 +370,8 @@ function InterlinearizerInner({ * @param props - Component props * @param props.book - Book data used by the continuous view and segment window * @param props.continuousScroll - Whether the continuous scroll view is shown - * @param props.scrRef - Current scripture reference + * @param props.scrRef - Current scripture reference; must be normalized (verse >= 1, see + * {@link InterlinearizerProps}). * @param props.initialAnalysis - Seed analysis data for the store; not reactive after mount * @param props.analysisLanguage - BCP 47 tag for gloss read/write * @param props.onSaveAnalysis - Called after each gloss write with the updated `TextAnalysis` diff --git a/src/components/SegmentListView.tsx b/src/components/SegmentListView.tsx index 28b04aa5..50f8cf2a 100644 --- a/src/components/SegmentListView.tsx +++ b/src/components/SegmentListView.tsx @@ -220,10 +220,7 @@ export default function SegmentListView({ {windowSegments.map((seg) => ( {!chapterLabelInVerse && chapterStartIds.has(seg.id) && ( - + {`Chapter ${seg.startRef.chapter}`} )} @@ -235,8 +232,7 @@ export default function SegmentListView({ isActive={ seg.startRef.book === displayScrRef.book && seg.startRef.chapter === displayScrRef.chapterNum && - (seg.startRef.verse === displayScrRef.verseNum || - (displayScrRef.verseNum === 0 && chapterStartIds.has(seg.id))) + seg.startRef.verse === displayScrRef.verseNum } onHoverPhrase={setHoveredPhraseId} onSelect={onSelect} diff --git a/src/components/controls/ViewOptionsDropdown.tsx b/src/components/controls/ViewOptionsDropdown.tsx index 276fc973..2d09a747 100644 --- a/src/components/controls/ViewOptionsDropdown.tsx +++ b/src/components/controls/ViewOptionsDropdown.tsx @@ -153,9 +153,14 @@ export default function ViewOptionsDropdown({ createPortal( /* Clicking outside the panel closes it. */ <> + {/* The invisible backdrop must stay BELOW the toolbar's stacking context (tw:z-10, + from the sticky TabToolbarContainer) so the gear button's onClick — not the + backdrop — closes the dropdown. Raising the button instead wouldn't work: the + toolbar caps its descendants at z-10 against this portaled sibling. + Keep panel (z-30) > backdrop. */}