diff --git a/flutter_readium/CHANGELOG.md b/flutter_readium/CHANGELOG.md index 6ee55d3d..94d37642 100644 --- a/flutter_readium/CHANGELOG.md +++ b/flutter_readium/CHANGELOG.md @@ -232,6 +232,14 @@ Bundle built javascript helpers, previously accidentally skipped by release pipe ### Fixed +- **Media Overlay ToC navigation to cue-less anchors** — tapping a ToC entry whose + fragment points at a heading or element with no narration cue (e.g. `chap1.xhtml#title`) + now seeks audio to the start of that chapter when navigating to a *different* chapter, + and leaves playback untouched when the anchor is within the *current* chapter (no + spurious rewind). Previously iOS and Android kept audio playing at the wrong position + (visual/audio desync on chapter cross), and Web always rewound to the chapter start + even for same-chapter anchors. + - **iOS media-overlay playback crashes on malformed sync-narration data** — starting playback in a publication with a reversed or non-finite audio time fragment (`t=start,end` where `end < start`) no longer traps with `Range requires lowerBound <= upperBound`, and a `narration` block with no diff --git a/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/models/FlutterMediaOverlay.kt b/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/models/FlutterMediaOverlay.kt index 1b929701..45e50d21 100644 --- a/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/models/FlutterMediaOverlay.kt +++ b/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/models/FlutterMediaOverlay.kt @@ -115,9 +115,19 @@ data class FlutterMediaOverlay( * Find the media overlay item from the given locator. * A locator can either be an audio+time based locator or a text+id based locator. * This allows us to map back and forth between audio and text. + * + * [allowResourceFallback] gates the *imprecise* resource-first fallbacks (an + * unmatched text id, or no id at all on an HTML resource): when true, an uncued + * anchor maps to the first cue of the resource; when false, returns null so the + * caller can leave playback untouched. Pass false when audio is already playing + * in this same resource (cross-resource check is done at the navigator level). + * See issue #139. */ @OptIn(InternalReadiumApi::class) - fun findItemFromLocator(locator: Locator): FlutterMediaOverlayItem? { + fun findItemFromLocator( + locator: Locator, + allowResourceFallback: Boolean = true, + ): FlutterMediaOverlayItem? { val href = locator.href if (!href.isEquivalent(Url.invoke(textFile)) && !href.isEquivalent(Url.invoke(audioFile))) { return null @@ -134,8 +144,10 @@ data class FlutterMediaOverlay( return items.firstOrNull { item -> item.textFile == href.path } } + // Reflowable text: try exact DOM element id match first; fall through on no match. locator.getTextId()?.let { textId -> - return findItemFromTextId(href, textId) + findItemFromTextId(href, textId)?.let { return it } + PluginLog.d(TAG, "::findItemFromLocator - textId '$textId' matched no cue for href=${href.path}") } locator.progression?.let { progression -> @@ -145,11 +157,12 @@ data class FlutterMediaOverlay( return item } - if (locator.locations.fragments.isEmpty() && locator.mediaType.isHtml) { - // No fragment on a text document → first item of the resource. + if (allowResourceFallback && locator.mediaType.isHtml) { + // No cue matched — fall back to first item of the resource (covers both + // no-fragment HTML and an id that has no narration entry e.g. a heading). PluginLog.d( TAG, - "::findItemFromLocator - no fragment in html locator, returning first item for href=${href.path}", + "::findItemFromLocator - resource-fallback: first item for href=${href.path}", ) return items.firstOrNull { item -> item.textFile == href.path @@ -158,7 +171,7 @@ data class FlutterMediaOverlay( PluginLog.d( TAG, - "::findItemFromLocator - no time or textId in locator, cannot find item for locator=$locator", + "::findItemFromLocator - no match for locator=$locator", ) return null diff --git a/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/navigators/SyncAudiobookNavigator.kt b/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/navigators/SyncAudiobookNavigator.kt index e520abab..94debd5b 100644 --- a/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/navigators/SyncAudiobookNavigator.kt +++ b/flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/navigators/SyncAudiobookNavigator.kt @@ -212,9 +212,23 @@ class SyncAudiobookNavigator( @OptIn(InternalReadiumApi::class) private fun mapTextLocatorToMediaOverlayLocator(locator: Locator): Locator? { + // Only allow the imprecise resource-first fallback when navigating into a + // *different* resource than the one currently playing — an uncued anchor + // within the current chapter must not rewind audio. See issue #139. + val curAudioLoc = audioNavigator?.currentLocator?.value + val curTextFile: String? = + if (curAudioLoc != null) { + val duration = publication.getReadingOrderItemDuration(curAudioLoc.href) + val timeOffset = curAudioLoc.locations.timeWithDuration(duration) ?: 0.seconds + mediaOverlays.firstNotNullOfOrNull { mo -> mo?.findItemInRange(curAudioLoc.href, timeOffset) }?.textFile + } else { + null + } + val crossResource = curTextFile == null || curTextFile != locator.href.path + val mediaOverlay = mediaOverlays.firstNotNullOfOrNull { mo -> - mo?.findItemFromLocator(locator) + mo?.findItemFromLocator(locator, allowResourceFallback = crossResource) } val syncAudioLocator = diff --git a/flutter_readium/ios/flutter_readium/Sources/flutter_readium/model/FlutterMediaOverlay.swift b/flutter_readium/ios/flutter_readium/Sources/flutter_readium/model/FlutterMediaOverlay.swift index 467578bd..32df72b7 100644 --- a/flutter_readium/ios/flutter_readium/Sources/flutter_readium/model/FlutterMediaOverlay.swift +++ b/flutter_readium/ios/flutter_readium/Sources/flutter_readium/model/FlutterMediaOverlay.swift @@ -34,12 +34,18 @@ struct FlutterMediaOverlay { return items.first(where: { $0.textId == textId }) } - func itemFromLocator(_ locator: Locator) -> FlutterMediaOverlayItem? { + /// `allowResourceFallback` gates the *imprecise* resource-first fallbacks (an + /// unmatched text id, or no id at all on an HTML resource): when true, an uncued + /// anchor maps to the first cue of the resource; when false, returns nil so the + /// caller can leave playback untouched. Pass false when audio is already playing + /// in this same resource (cross-resource check is done at the navigator level). + /// See issue #139. + func itemFromLocator(_ locator: Locator, allowResourceFallback: Bool = true) -> FlutterMediaOverlayItem? { let href = locator.href.string if (textFile != href && audioFile != href) { return nil } - + // Audio time fragment → exact item by time range. let timeOffset = locator.timeOffset if (timeOffset != nil) { @@ -53,14 +59,18 @@ struct FlutterMediaOverlay { return items.first(where: { $0.textFile == href }) } - // Reflowable text: match by DOM element id when present. - let textId = locator.textId - if (textId != nil) { - return itemFromTextId(textId!, inHref: href) + // Reflowable text: try exact DOM element id match first. + if let textId = locator.textId, let item = itemFromTextId(textId, inHref: href) { + return item } - // No fragment on a text document → first item of the resource. - if (locator.locations.fragments.isEmpty && [MediaType.html, MediaType.xhtml].contains(locator.mediaType)) { + // No id, or id matched no cue → resource-first fallback, gated by policy. + // Covers: no fragment on an HTML resource, AND an id that has no narration cue + // (e.g. a ToC entry pointing at a heading that lacks its own sync data). + if allowResourceFallback && [MediaType.html, MediaType.xhtml].contains(locator.mediaType) { + if locator.textId != nil { + Log.navigator.warn("itemFromLocator: textId matched no cue in \(href); falling back to first item in resource") + } return items.first(where: { $0.textFile == href }) } diff --git a/flutter_readium/ios/flutter_readium/Sources/flutter_readium/navigator/FlutterMediaOverlayNavigator.swift b/flutter_readium/ios/flutter_readium/Sources/flutter_readium/navigator/FlutterMediaOverlayNavigator.swift index f797df29..bb1cf92f 100644 --- a/flutter_readium/ios/flutter_readium/Sources/flutter_readium/navigator/FlutterMediaOverlayNavigator.swift +++ b/flutter_readium/ios/flutter_readium/Sources/flutter_readium/navigator/FlutterMediaOverlayNavigator.swift @@ -12,7 +12,6 @@ import ReadiumNavigator public class FlutterMediaOverlayNavigator : FlutterAudioNavigator { internal var mediaOverlays: [FlutterMediaOverlay] = [] - internal var lastMediaOverlayItem: FlutterMediaOverlayItem? = nil public override var currentLocator: Locator? { get { @@ -180,7 +179,13 @@ public class FlutterMediaOverlayNavigator : FlutterAudioNavigator Log.navigator.debug("mapTextLocatorToMediaOverlayAudioLocator - nil text locator") return nil } - guard let matchingMediaOverlayItem = self.mediaOverlays.firstMap({ $0.itemFromLocator(textLocator) }), + // Only allow the imprecise resource-first fallback when tapping into a + // *different* resource than the one currently playing — an uncued anchor + // within the current chapter must not rewind audio. See issue #139. + let currentTextFile = audioLocator.flatMap { mediaOverlayItemFromAudioLocator($0)?.textFile } + let crossResource = currentTextFile == nil + || currentTextFile != textLocator.href.string + guard let matchingMediaOverlayItem = self.mediaOverlays.firstMap({ $0.itemFromLocator(textLocator, allowResourceFallback: crossResource) }), var audioLocator = matchingMediaOverlayItem.asAudioLocator else { Log.navigator.warn("mapTextLocatorToMediaOverlayAudioLocator - no media overlay matched text locator " + "href=\(textLocator.href.string) mediaType=\(textLocator.mediaType.string) fragments=\(textLocator.locations.fragments)") diff --git a/flutter_readium/ios/flutter_readium/Tests/flutter_readiumTests/FlutterMediaOverlayTests.swift b/flutter_readium/ios/flutter_readium/Tests/flutter_readiumTests/FlutterMediaOverlayTests.swift new file mode 100644 index 00000000..62293bae --- /dev/null +++ b/flutter_readium/ios/flutter_readium/Tests/flutter_readiumTests/FlutterMediaOverlayTests.swift @@ -0,0 +1,82 @@ +import XCTest +import ReadiumShared +@testable import flutter_readium + +private func makeItem(audio: String, text: String, position: Int = 0) -> FlutterMediaOverlayItem { + FlutterMediaOverlayItem(audio: audio, text: text, position: position) +} + +private func makeOverlay(_ items: [FlutterMediaOverlayItem]) -> FlutterMediaOverlay { + FlutterMediaOverlay(items: items, readingOrderDuration: nil) +} + +private func htmlLocator(href: String, fragment: String? = nil) -> Locator { + // Use a #-prefixed fragment so Locator.textId picks it up via + // `locations.fragments.first(where: { $0.hasPrefix("#") })`. + let frags: [String] = fragment.map { ["#\($0)"] } ?? [] + return Locator( + href: URL(string: href)!, + mediaType: MediaType.xhtml, + locations: .init(fragments: frags) + ) +} + +final class FlutterMediaOverlayTests: XCTestCase { + + private let overlay: FlutterMediaOverlay = { + makeOverlay([ + makeItem(audio: "chap1.mp3#t=0,5", text: "chap1.xhtml#p1"), + makeItem(audio: "chap1.mp3#t=5,10", text: "chap1.xhtml#p2"), + makeItem(audio: "chap2.mp3#t=0,8", text: "chap2.xhtml#q1"), + ]) + }() + + // MARK: Exact id match (always returns, regardless of flag) + + func testExactIdMatchReturnsItem() { + let loc = htmlLocator(href: "chap1.xhtml", fragment: "p2") + let item = overlay.itemFromLocator(loc, allowResourceFallback: false) + XCTAssertEqual(item?.textId, "p2") + } + + func testExactIdMatchUnaffectedByFallbackFlag() { + let loc = htmlLocator(href: "chap1.xhtml", fragment: "p2") + XCTAssertNotNil(overlay.itemFromLocator(loc, allowResourceFallback: true)) + XCTAssertNotNil(overlay.itemFromLocator(loc, allowResourceFallback: false)) + } + + // MARK: Unmatched id — gated fallback + + func testUnmatchedIdReturnsFirstItemWhenFallbackAllowed() { + // ToC entry chap1.xhtml#title — "title" has no cue, cross-resource tap. + let loc = htmlLocator(href: "chap1.xhtml", fragment: "title") + let item = overlay.itemFromLocator(loc, allowResourceFallback: true) + XCTAssertEqual(item?.textId, "p1") + } + + func testUnmatchedIdReturnsNilWhenFallbackDisallowed() { + // Same resource as currently playing — must not rewind. + let loc = htmlLocator(href: "chap1.xhtml", fragment: "title") + XCTAssertNil(overlay.itemFromLocator(loc, allowResourceFallback: false)) + } + + // MARK: No fragment on HTML resource — gated fallback + + func testNoFragmentReturnsFirstItemWhenFallbackAllowed() { + let loc = htmlLocator(href: "chap1.xhtml") + let item = overlay.itemFromLocator(loc, allowResourceFallback: true) + XCTAssertEqual(item?.textId, "p1") + } + + func testNoFragmentReturnsNilWhenFallbackDisallowed() { + let loc = htmlLocator(href: "chap1.xhtml") + XCTAssertNil(overlay.itemFromLocator(loc, allowResourceFallback: false)) + } + + // MARK: Wrong href — always nil + + func testWrongHrefReturnsNil() { + let loc = htmlLocator(href: "chap3.xhtml", fragment: "p1") + XCTAssertNil(overlay.itemFromLocator(loc, allowResourceFallback: true)) + } +} diff --git a/flutter_readium/web/src/ReadiumReader.ts b/flutter_readium/web/src/ReadiumReader.ts index 01a74203..3a574199 100644 --- a/flutter_readium/web/src/ReadiumReader.ts +++ b/flutter_readium/web/src/ReadiumReader.ts @@ -32,6 +32,7 @@ import { SyncNarrationItem, detectSyncNarration, findItemByAudioTime, + normalizeHref, textLocatorForItem, textLocatorToAudioLocator, } from "./mediaoverlay/syncNarration"; @@ -233,7 +234,16 @@ class _ReadiumReader { // seek audio nav, and also scroll the visual navigator to the text position. // Mirrors FlutterMediaOverlayNavigator.seek(toLocator:) on iOS/Android. if (this._audioNav && this._syncItems.length > 0) { - const audioLocator = textLocatorToAudioLocator(this._syncItems, locator); + // Only fall back to the resource's first cue when this ToC/bookmark tap + // crosses into a *different* resource than the one currently playing — + // an uncued anchor within the current chapter must not rewind audio to + // the chapter top. See issue #139. + const curAudioLoc = this._audioNav.currentLocator; + const curTime = getTime(curAudioLoc.locations) ?? this._audioNav.currentTime; + const curItem = findItemByAudioTime(this._syncItems, curAudioLoc.href, curTime); + const crossResource = + !curItem || normalizeHref(curItem.textHref) !== normalizeHref(locator.href); + const audioLocator = textLocatorToAudioLocator(this._syncItems, locator, crossResource); if (audioLocator) { const wasPlaying = this._audioNav.isPlaying; log.info( diff --git a/flutter_readium/web/src/__tests__/syncNarration.test.ts b/flutter_readium/web/src/__tests__/syncNarration.test.ts index cb3028a7..5af7d29e 100644 --- a/flutter_readium/web/src/__tests__/syncNarration.test.ts +++ b/flutter_readium/web/src/__tests__/syncNarration.test.ts @@ -360,6 +360,11 @@ describe("textLocatorToAudioLocator", () => { expect(audioLoc!.locations.fragments).toContain("t=0"); }); + it("returns undefined for a no-textId locator when resource fallback is disallowed", () => { + const loc = new Locator({ href: "chap1.html", type: "text/html", locations: new LocatorLocations({}) }); + expect(textLocatorToAudioLocator(items, loc, false)).toBeUndefined(); + }); + it("falls back to first item in href when textId has no match", () => { // "chap1.html#unknown" — no SyncNarrationItem with textId="unknown" const loc = textLocator("chap1.html", "unknown"); @@ -370,6 +375,21 @@ describe("textLocatorToAudioLocator", () => { expect(audioLoc!.locations.fragments).toContain("t=0"); }); + it("returns undefined for an unmatched textId when resource fallback is disallowed", () => { + // Same-resource ToC tap at a cue-less anchor must not rewind audio (issue #139). + const loc = textLocator("chap1.html", "unknown"); + expect(textLocatorToAudioLocator(items, loc, false)).toBeUndefined(); + }); + + it("still maps an exact href+id match when resource fallback is disallowed", () => { + // Gating only suppresses the imprecise fallback; a real cue match always maps. + const loc = textLocator("chap1.html", "p2"); + const audioLoc = textLocatorToAudioLocator(items, loc, false); + expect(audioLoc).not.toBeUndefined(); + expect(audioLoc!.href).toBe("chap1.mp3"); + expect(audioLoc!.locations.fragments).toContain("t=5"); + }); + it("uses progression within the item range when audioStart/End are set", () => { // item p2: audioStart=5, audioEnd=10; progression=0.5 → timeOffset = 5 + 0.5*(10-5) = 7.5 const loc = new Locator({ diff --git a/flutter_readium/web/src/mediaoverlay/syncNarration.ts b/flutter_readium/web/src/mediaoverlay/syncNarration.ts index 1ecf9404..ae13c587 100644 --- a/flutter_readium/web/src/mediaoverlay/syncNarration.ts +++ b/flutter_readium/web/src/mediaoverlay/syncNarration.ts @@ -215,12 +215,20 @@ export function combinedLocatorForItem( * * Returns undefined when no matching item is found. * + * `allowResourceFallback` gates the *imprecise* resource-first fallbacks (an + * unmatched text id, or no id at all): when true, an uncued anchor maps to the + * first cue of the resource; when false, it returns undefined so the caller + * leaves playback untouched. Callers pass false when audio is already playing + * *within the same resource* — a ToC tap at a cue-less heading in the current + * chapter shouldn't rewind audio to the chapter top. See issue #139. + * * Mirrors FlutterMediaOverlayNavigator.mapTextLocatorToMediaOverlayAudioLocator * (iOS) and SyncAudiobookNavigator.mapTextLocatorToMediaOverlayLocator (Android). */ export function textLocatorToAudioLocator( items: SyncNarrationItem[], - textLocator: Locator + textLocator: Locator, + allowResourceFallback = true ): Locator | undefined { const targetHref = textLocator.href; log.debug(`Mapping text locator to audio: href="${targetHref}", ${items.length} items`); @@ -241,18 +249,20 @@ export function textLocatorToAudioLocator( // Primary: exact href + textId match (ID-anchored ToC entry, decoration callback, etc.). // Fallback: first item in matching href (covers ToC entries whose fragment points at a // heading or section that has no Sync Narration item — e.g. `chap1.xhtml#title`). - // Mirrors iOS/Android's "no fragments + HTML → first item by href" fallback in + // The fallback is *imprecise* (resource start, not the anchor), so it's gated by + // `allowResourceFallback` — suppressed when audio is already in this same resource. + // Mirrors iOS/Android's gated resource-first fallback in // FlutterMediaOverlay.itemFromLocator / findItemFromLocator. let match: SyncNarrationItem | undefined; if (targetId) { match = hrefMatches.find((item) => item.textId === targetId); - if (!match && hrefMatches.length > 0) { + if (!match && allowResourceFallback && hrefMatches.length > 0) { log.warn( `textLocatorToAudioLocator: no SyncNarrationItem matched textId "${targetId}" in ${targetHrefNormalized}; falling back to first item in resource.` ); match = hrefMatches[0]; } - } else { + } else if (allowResourceFallback) { match = hrefMatches[0]; }