fix(reader): land and hold figure annotations from the annotations panel in all three modes - #1127
Merged
Merged
Conversation
…tations panel TYPE_IMAGE annotations have no <mark data-riffle-ann="..."> in the DOM (only a CSS outline decoration). annotationOffsetTopDevicePx was returning null for them, so continuous mode fell back to progression-only landing and paginated mode hung for 10 s waiting for a Readium decoration that never exists. Fix: - Add imageSrc field to AnnotationNavigationEvent; populated from annotation.imageHref for TYPE_IMAGE annotations. - annotationOffsetTopDevicePx gains an optional imageSrc param; when no <mark> is found it falls back to img[src$="filename"] querySelector to locate the figure element. - ContinuousWindowController threads imageSrc through the navigation chain and stores it as pendingFocusImageSrc so scrollToFocusAnnotation can pass it to the JS query. - EpubReaderScreen skips the focusAnnotationId decoration-wait in paginated mode for image annotations (no decoration to wait for). iOS: annotationDecorationLocatorJson already uses the caption as a text.highlight anchor for Readium Swift to navigate near the figure. Pins this with two new commonTest assertions covering the captioned and uncaptioned cases.
…mageHref Annotations created before migration 46→47 have no imageHref. The previous fix only handled the imageHref case, so old figure annotations still fell back to progression-only landing. cfiStringToLocator already extracts the innermost element id from the CFI and stores it in locations.fragments. For TYPE_IMAGE with null imageHref, use that element id as imageSrc with a '#' prefix. annotationOffsetTopDevicePx now checks the prefix: '#id' → getElementById, plain path → img[src$='filename'].
…ix column-0 snap
O'Reilly EPUBs wrap figures in a <section id="ch01"> that spans the whole chapter.
cfiStringToLocator puts that section id into locations.fragments, so ColumnSnap's
snapToTargetColumnJs calls getElementById("ch01").getBoundingClientRect().left which
always returns 0 and snaps to column 0 (chapter start) instead of the figure's actual
column.
Clearing locations.fragments for TYPE_IMAGE annotations when in horizontal paginated
mode causes the snap to fall back to progression-based column selection, which correctly
lands at the column containing the figure.
… in continuous mode When navigating cross-chapter to a TYPE_IMAGE annotation with smoothTail=true, pendingInitialScroll fires at DOM-ready (before onPageFinished, before decorations are applied). annotationOffsetTopDevicePx returns null because no <mark> is in the DOM yet, so the fallback resolveAnchorThenLand() uses the CFI's anchorFragment (e.g. a containing <section id="ch04">) and pre-programmes revealSmooth() to call smoothScrollTo(section_top). Later, onAnnotationHighlightsApplied fires → scrollToFocusAnnotation → image src lookup succeeds → landOnAnnotationOffset → correct scroll to figure. The user briefly sees the figure. Then the still-pending revealSmooth() fires via onCurrentContentPainted and calls smoothScrollTo(section_top), yanking the reader to the wrong section-start position. Fix: introduce smoothTailRevealSuppressed flag. scrollToFocusAnnotation sets it when the annotation offset is found (also aborting any in-flight smooth animation via port.abortFling()). revealSmooth checks the flag and skips smoothScrollTo if set, but still calls notifyFirstLoadCompleteOnce() to lift the nav cover. The flag is reset in openWindowAt so fresh navigations are unaffected. iOS counterpart: the iOS annotation navigation path goes through Readium Swift's navigator.go(locator:) via annotationDecorationLocatorJson (which anchors on the figure's caption text). Readium Swift handles navigation atomically — no smooth-tail race is possible. The iOS code path is covered by AnnotationLocatorJsonTest in core:data commonTest (runs on iosSimulatorArm64 in CI).
…igation in continuous mode Annotation panel taps arrive via Compose bottom sheet and never call onTouchDown on ContinuousReaderView, so inWindowNavSupersededByTouch can remain false after a deliberate cross-chapter or same-window annotation jump. serverLocatorEvents (skipIfUserAlreadyInteracted=true) then passes the gate and fires a second scrollTo/openWindowAt, landing the reader back at the stale server position ~1 s after the figure was correctly shown. Fix (cross-window): set inWindowNavSupersededByTouch = true in the else branch of navigateTo before openWindowAt, so any server-resume that arrives while the new window is loading is suppressed. Fix (in-window): set inWindowNavSupersededByTouch = true inside the land() closure after the guard check, so any server-resume land() that was already posted concurrently sees the flag and bails — the annotation land() always runs first (it was posted earlier).
… navigations in continuous mode Pre-migration TYPE_IMAGE annotations have no imageHref stored and no CFI element-ID fragment in their locator, so annotationOffsetTopDevicePx returns null for them: the JS falls back to document.querySelector [data-riffle-ann='…'] which finds nothing because no <mark> is injected around image elements. Previously smoothTailRevealSuppressed was only set to true inside scrollToFocusAnnotation's callback — which requires a successful element lookup. When the callback returned null the flag stayed false, allowing revealSmooth to fire and smoothScrollTo(section_top) back to the CFI anchor position (the preface/chapter start), overriding the correct landing position. Fix: arm smoothTailRevealSuppressed = focusAnnotationId != null immediately in openWindowAt. When a focus annotation is in flight the anchor-fallback smooth scroll is suppressed from the moment the window opens, not only after the JS element lookup succeeds. The user lands at the CFI anchor for annotations whose element cannot be located (graceful degradation), and at the correct figure position for annotations that can be found — verified on AVD (emulator-5554) by navigating cross-chapter from ch02 to Figure 1-2 (a pre-migration TYPE_IMAGE with null imageHref). iOS is not affected: the fix is confined to ContinuousWindowController, an Android-specific engine. iOS uses Readium's built-in scroll navigation which does not have the anchor-fallback/decoration-override race. iOS annotation panel navigation is tracked separately as a pre-existing gap.
… post-landing height reflow When images above Figure 1-2 within the same target chapter load after the annotation has been focused, the chapter's onHeightMeasured fires and invokes reapplyLandingAfterFallback → annotationReland → scrollToFocusAnnotation. The previous code read pendingFocusImageSrc from the controller field, which was already null (cleared on the first successful land). annotationOffsetTopDevicePx returned null for TYPE_IMAGE (no <mark data-riffle-ann> element is injected for images, so the JS needs the img src to locate the element), so the re-land was a no-op. The scroll stayed at the old position (now showing "Who This Book Is For" instead of Figure 1-2) while the figure shifted down. Fix: annotationFocusRelandClosure now accepts pendingFocusImageSrc and captures it in the returned closure (capturedImageSrc). scrollToFocusAnnotation is updated to receive imageSrc as an explicit parameter instead of reading the field, so every re-invocation from the height-change loop passes the captured src and can re-query Figure 1-2's current pixel position correctly regardless of how many times pendingFocusImageSrc has been cleared by that point.
…ws above viewport
When navigating to an annotated figure in a chapter that is not near the top of the
window (e.g. ch09 with ch07+ch08 above it), images within ch07 or ch08 can load after
the initial measurement. This grows their slot, pushing ch09 and the focused figure
downward while the scroll position stays put. The result is the viewer scrolling to
content above the figure ("briefly navigates, then moves" in the two-step repro:
first figure → last figure in annotations list).
Fix: after applyChapterHeight, if the growing chapter is not the top chapter, was not
at placeholder height (so the placeholder paths don't cover it), is not the target, and
sits entirely above the current scroll position, apply scrollBy(delta) — the same
direct compensation used for i==0 and the wasPlaceholder above-viewport path. The delta
is small relative to the existing maxScrollY so no NestedScrollView clipping occurs.
…apter-height compensation Cross-chapter annotation navigation in continuous mode landed on the figure and then showed the start of the chapter above it. The window is rebuilt as [placeholder, target, placeholder]; when the deferred chapter above measures its real height, onHeightMeasured compensates with port.scrollBy(delta) from inside that WebView's doOnNextLayout, i.e. during ContinuousReaderView.onLayout. The view's scrollBy(x, y) override is short-circuited for the whole of onLayout (to block NestedScrollView's requestChildFocus -> scrollToChild), so the compensation was dropped and every slot below the grown chapter shifted under a stationary scroll position. The same path is used by prependChapter. Route the port's scrollBy through scrollTo(scrollY + dy): identical clamping, never subject to the suppression, which keeps blocking the View-level calls it was written for. Regression test (instrumentation, real layout pass): ContinuousReaderViewLayoutTimeScrollTest.portScrollBy_fromChildLayoutCallback_movesTheScroll fails with expected:<1500> but was:<0> against the previous port, and viewScrollBy_fromChildLayoutCallback_staysSuppressed pins that the suppression of View.scrollBy during onLayout is unchanged.
The connected-test result XML is named after the AVD ("... (AVD) - 7.1.1.xml"),
so the unquoted find | xargs sed pipeline word-split the path, counted 0 tests
and failed every otherwise-green harness run with exit 2.
…; repair mispaired caption highlights A long-press on an O'Reilly figure whose caption is an <h6> (not <figcaption>) was stored as a highlight of the NEXT figure's caption carrying this figure's image: the caption resolver's forward hunt climbs to the section, where the next <figure>'s wrapper div starts with "Figure N", and returned it. Tapping the annotation then navigated to the wrong figure. - JS resolver (commonMain) and its Kotlin mirror in CaptionHighlightUpgrader skip any candidate block that sits inside a different figure wrapper; the Kotlin mirror also gains the h6/h5/h4/h3 caption scan main added to the JS. - New sweep phase re-anchors an existing caption highlight whose single embedded figure has its own caption in the DOM while its snippet is another figure's caption (AnnotationStore.reanchorCaptionHighlight, id preserved, updatedAt bumped so the fix syncs). Prose highlights spanning a figure are left alone. The sweep now also runs for books with caption highlights but no legacy TYPE_IMAGE rows. Verified on the API-25 AVD against the affected book: sweep reports repaired=1, the panel entry and the landed figure now agree. Tests: FigureCaptionWalkerTest (app + commonTest, runs on iOS) pins the different-figure guard; AnnotationStoreImplTest (commonTest) covers reanchorCaptionHighlight; CaptionHighlightUpgraderTest covers own-h6 pairing, no borrowing from an uncaptioned figure, the repair, and two untouched cases.
…mediately Tapping a highlight without a note left the page "semi-turned" for ~10 s. The column-snap loop only looked the focused id up in the annotation-notes glyph group, which such a highlight never has, so ordinaryDone stayed disabled and the loop ran its full 600-frame cap. Every frame it re-ran readium.scrollToLocator, which aligns the range's left edge with the viewport edge without snapping — for a caption indented inside a figure wrapper that is 82 px into the column (measured: scrollLeft=898 at a 408 px pitch). - Look the focused id up in the "annotations" tint group as well; it carries the same id and is present from the first frame, so the landing snaps to the highlight's column at once and the loop finishes after the 60-frame stable window. - Floor scrollLeft to the containing column after scrollToLocator so no waiting frame is off-grid either. Verified on the API-25 AVD: the landing is on-grid at 0.7 s and stays. Tests: ColumnSnapJsBuilderTest pins both the group lookup and the floor (runs on iOS via commonTest); ReaderWebViewScriptsTest tokens updated mechanically; AnnotationFocusHarnessTest asserts the paginated landing rests on the column grid within 3 s.
…pter reflows Two mechanisms, both traced on the API-25 AVD with a scroll tracer, moved the reader off a figure it had just landed on after a cross-chapter jump: 1. Stale landing offset. The figure offset is queried at DOM-ready; the target chapter then reflows at load (193 555 -> 176 054 px here) and the landing used the old offset, two screens below the figure. Nothing re-landed because the re-land closure was only installed from onAnnotationHighlightsApplied, which never fires for a chapter whose only annotation is a figure border, and smooth-tail navigations left the slot null. The annotation re-land is now installed when the initial measurement is consumed (relandClosureAfterInitialMeasure), so every later target remeasure re-queries the figure's offset in the reflowed DOM. 2. Compensation undone by Chromium. When the chapter above the reader shrank, the controller compensated with scrollBy(-delta); Chromium then reported the WebView's internal scroll re-clamped against a renderer height still catching up, internalScrollCorrection classified it as an unmanaged gesture and folded it into the outer scroll — cancelling (part of) the compensation. Reports within HEIGHT_CHANGE_SETTLE_MS of that chapter's content-height change are now NONE; gestures never coincide with a height change, so plain-scroll folding is unchanged. Verified: Figure 1-2 -> Figure 7-6 jump lands on the figure and holds at 1/3/6/12 s with zero folds; the compensation arithmetic closes exactly. Tests: ContinuousAnnotationFocusReflowRaceTest covers the re-land selector (annotation wins even for smooth-tail, existing closure kept, smooth-tail without annotation stays empty, hard landing falls back to progression); ContinuousPositionTrackerTest covers the settle window (recent height change -> NONE, later -> FOLD, sub-CSS-px rounding still ADOPT).
… modes) Review findings on the branch, each fixed and re-verified on the API-25 AVD: - Continuous: the smooth tail was pre-suppressed for every annotation navigation, so when the first pass already resolved the figure the reader rested half a viewport short of it (pre-land position) unless a later remeasure re-landed. smoothTailRevealSuppressedAfterInitialLanding lets the tail complete when y IS the annotation. - Continuous: the above-viewport compensation for non-top, non-target chapters now covers shrink as well as growth, and growth scrolls on the next layout (like the placeholder path) so the NestedScrollView cannot clamp it. - Continuous: only user-initiated in-window landings mark server-resume refires as superseded; a server landing no longer cancels a still-parked user navigation. - Continuous: the height-change settle window is stamped only when the height actually changes, so genuine internal scrolls after an unchanged re-report still fold. - Paginated: floor after scrollToLocator tolerates a rect stored a pixel below a column boundary (+2 px) instead of landing one column early. - Vertical: TYPE_IMAGE fragment clearing applies to scroll mode too (its smooth tail otherwise targeted the section anchor = chapter top), and an annotation navigation in scroll mode stops after go(locator): the paginated column loop used to re-call scrollToLocator for 72 frames and drift a landed figure a screen away. - Caption repair runs before the duplicate merge, on the repaired rows, so a mispaired highlight is re-anchored instead of being folded into the genuine sibling with both images. Tests: ContinuousAnnotationFocusReflowRaceTest (suppression decision), ContinuousWindowControllerScrollGateTest (shrink compensation), ColumnSnapJsBuilderTest (scroll-mode guard, +2 px floor; runs on iOS), CaptionHighlightUpgraderTest (repair-before-merge), ReaderWebViewScriptsTest tokens updated mechanically.
pkmetski
force-pushed
the
pkmetski/annotation-figure-nav-focus
branch
from
September 29, 2026 17:20
35d7b9b to
3e91046
Compare
pkmetski
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Tapping a figure annotation in the annotations panel landed briefly and then jumped, or opened the wrong figure, or left the page mid-turn. Six distinct defects were traced on the API-25 AVD (scroll tracer + frame captures at 1/3/6/12 s, checking the landed content against the expected figure) and fixed:
doOnNextLayout, i.e. duringonLayout, whereContinuousReaderView.scrollByis deliberately swallowed — it had never executed since #1105. After a cross-chapter jump the chapter above grew ~140k px and the viewport ended at its start.scrollByroutes throughscrollTo, immune to the layout-time suppression (which still blocks NestedScrollView's ownscrollToChild).onAnnotationHighlightsApplied(never fires for a figure-only chapter).relandClosureAfterInitialMeasure).internalScrollCorrectionfolded it into the outer scroll and cancelled the compensation.HEIGHT_CHANGE_SETTLE_MSof that chapter's height change are ignored; plain-scroll folding unchanged.<h6>was stored with the next figure's caption + CFI (the resolver's forward hunt walked into the next<figure>), so navigation opened the wrong figure.commonMain, Kotlin mirror) refuse candidates inside a different figure wrapper; the Kotlin mirror gains the<h6>scan from #1125; an open-time sweep re-anchors already-mispaired rows (AnnotationStore.reanchorCaptionHighlight, runs before the duplicate merge).scrollToLocator, which aligns the range's left edge without snapping — page held "semi-turned" ~10 s.annotationstint group (present from frame 0);scrollLeftfloored to the containing column (+2 px tolerance).go(locator).Also: earlier commits on this branch (imageSrc query for figures, smooth-tail suppression, server-resume guard, reland imageSrc capture, non-top chapter compensation) remain; the review pass hardened them (tail completes when the first pass resolves the figure; shrink compensation; growth deferred to next layout; server landings don't supersede parked user navs). A Makefile fix quotes the harness result path (
(AVD) - 7.1.1.xml) so the "0 tests executed" guard stops failing green runs.Verification (API-25 AVD, library item
70e7d5a2…)repaired=1): the mispaired ch09 annotation now opens Figure 9-1 (thumbnail and landing agree).@IgnoredmanualPageFlipswritten as an empty<failure>by the result adapter (test(harness): continuous-mode AnnotationFocus + NavigationSnap manualPageFlips flake on CI phone harness #1109 artefact).Found while verifying, filed separately: #1129 (crash in
EpubNavigatorFragment.go()when switching reading mode right after an annotation navigation — not touched by this branch).Regression tests (each red on revert)
Android:
ContinuousReaderViewLayoutTimeScrollTest(instrumentation: portscrollByduring a child's layout callback moves the scroll;View.scrollBystays suppressed),ContinuousPositionTrackerTest(settle window),ContinuousAnnotationFocusReflowRaceTest(re-land selector, suppression decision, imageSrc capture),ContinuousWindowControllerScrollGateTest,CaptionHighlightUpgraderTest(own-<h6>pairing, no borrowing from an uncaptioned figure, repair, repair-before-merge, two must-not-touch cases),AnnotationFocusHarnessTest(paginated landing must rest on the column grid within 3 s),ReaderWebViewScriptsTest,FigureCaptionWalkerTest.Shared / iOS path:
feature:readercommonTestFigureCaptionWalkerTest(different-figure guard) andColumnSnapJsBuilderTest(annotations-group lookup, +2 px floor, scroll-mode guard);core:datacommonTestAnnotationStoreImplTest(reanchorCaptionHighlight) — all run oniosSimulatorArm64Test.iOS
Shared pieces (caption-resolver JS,
ColumnSnapJS,AnnotationStore.reanchorCaptionHighlight) live incommonMainwithcommonTestcoverage. The continuous engine (ContinuousWindowController/ContinuousReaderView), the Readium-Android snap loop hosting and the jsoup-basedCaptionHighlightUpgradersweep have no iOS counterpart — iOS continuous mode is Readium Swift's scroll mode and the upgrader was Android-only before this branch. iOS figure long-press/annotation-panel navigation remains the pre-existing gap noted on this PR previously; it needs its own issue.