-
Notifications
You must be signed in to change notification settings - Fork 11
feat: EPUB image tap detection #159
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
14 commits
Select commit
Hold shift + click to select a range
66d38cd
feat: WIP: implement image tap opening (outside of viewer)
ddfreiling d2b3b64
chore: add missing flutter_readium.dart changes
ddfreiling b218a2a
fix(image-tap): robust web hit-testing, Android getResourceBytes, ill…
ddfreiling 2194060
refactor(image-tap): simplify post-rebase cleanup
ddfreiling 971bf1a
fix(image-tap): correct alt text, Rect type, viewType name, syncPolic…
ddfreiling 51cfb0b
feat(image-tap): add Android image-tap detection + web detector tests
ddfreiling dbc289e
docs(changelog): move image-tap entries to Unreleased, note Android s…
ddfreiling 7cfe817
docs(platform-interface): changelog for ImageTapEvent + getResourceBytes
ddfreiling df91f29
Merge branch 'main' into feat/epub-image-tap
ddfreiling d5c8d46
Merge remote-tracking branch 'origin' into feat/epub-image-tap
ddfreiling cca0667
fix: imageTap should not triggering for comics
ddfreiling 5ae96c3
Merge remote-tracking branch 'origin/main' into feat/epub-image-tap
ddfreiling 7cab8b2
Merge remote-tracking branch 'origin/main' into feat/epub-image-tap
ddfreiling dbb8b33
refactor(ios): improve organization of swift code
ddfreiling File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
153 changes: 153 additions & 0 deletions
153
docs/parity/native-reader-interaction-navigation-plan.md
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,153 @@ | ||
| # Native Reader Interaction vs Navigation Plan | ||
|
|
||
| ## Goal | ||
|
|
||
| Separate generic reader interaction from page navigation so manual panning inside | ||
| Nota comics does not accidentally trigger native page turns or narration manual | ||
| mode. | ||
|
|
||
| This should be a separate PR from EPUB image-tap detection. The current PR found | ||
| the issue while testing Android MO Nota comics, but the behavior touches shared | ||
| reader input semantics across Dart, Android, and iOS. | ||
|
|
||
| ## Current symptoms | ||
|
|
||
| - On Android, manual panning inside a Nota MO comic page can still trigger | ||
| `goForward` / `goBackward` between pages. | ||
| - The trigger was isolated to `_onInteraction()` being called from | ||
| `ReadiumReaderWidget`'s `Listener.onPointerMove` path. | ||
| - Removing the native notification from pointer-move interaction reduces the | ||
| immediate problem, but it also changes how swipe gestures enter narration | ||
| manual mode. | ||
|
|
||
| ## Relevant code | ||
|
|
||
| - `flutter_readium/lib/reader_widget.dart` | ||
| - `Listener.onPointerMove` classifies small movement as `userSwipe`. | ||
| - `_onInteraction()` hides controls and calls `_channel?.notifyUserNavigation()`. | ||
| - Semantic edge regions call `_channel?.goForward()` / `goBackward()` directly. | ||
| - `flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/fragments/EpubReaderFragment.kt` | ||
| - Installs `DirectionalNavigationAdapter` on the `OverflowableNavigator`. | ||
| - Readium owns edge-tap / keyboard navigation at the native navigator layer. | ||
| - `flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/ReadiumReader.kt` | ||
| - Explicit `epubGoForward` / `epubGoBackward` already call | ||
| `enterManualModeIfNarrating(...)` before navigating. | ||
| - `flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/ReadiumReaderWidget.kt` | ||
| - Method-channel `notifyUserNavigation` only enters narration manual mode; it | ||
| should not itself page-turn. | ||
| - `flutter_readium/assets/_helper_scripts/src/NotaComicBookPage.ts` | ||
| - Nota comic manual pan/zoom uses the cloned image overlay. | ||
| - Touch listeners are passive today, so native navigator input may still see | ||
| the same gesture. | ||
|
|
||
| ## Working hypothesis | ||
|
|
||
| `ReadiumReaderWidget` currently conflates two concepts: | ||
|
|
||
| 1. Generic interaction: wake lock, hide controls, center/edge tap UI handling. | ||
| 2. Navigation intent: user intentionally moved to another page and narration | ||
| should enter manual mode. | ||
|
|
||
| Now that Android and iOS use native `DirectionalNavigationAdapter`, the Flutter | ||
| wrapper should not infer navigation from `onPointerMove`. Native page-turn paths | ||
| should own navigation/manual-mode transitions. | ||
|
|
||
| The remaining accidental page turn may come from native navigator input seeing | ||
| the same touch gesture used by the Nota comic overlay. If so, fixing only Dart is | ||
| insufficient; the comic overlay must consume or isolate manual pan gestures. | ||
|
|
||
| ## Debug checklist | ||
|
|
||
| 1. Add temporary logs in `reader_widget.dart`: | ||
| - pointer down/move/up | ||
| - whether movement crossed the `userSwipe` threshold | ||
| - whether `_channel.notifyUserNavigation()` was called | ||
| 2. Add temporary logs in Android `ReadiumReaderWidget`: | ||
| - method-channel `goForward`, `goBackward`, `notifyUserNavigation` | ||
| 3. Add temporary logs in `ReadiumReader.epubGoForward` / `epubGoBackward` and | ||
| `EpubReaderFragment.goForward` / `goBackward`. | ||
| 4. Add temporary JS logs in `NotaComicBookPage.ts`: | ||
| - `touchstart`, `touchmove`, `touchend` | ||
| - whether `manualOverrideActive` is true | ||
| - whether the event target is inside `.nota-comicbook-page-container` | ||
| 5. Reproduce on Android MO Nota comic: | ||
| - pan while zoomed/manual | ||
| - edge tap intentionally | ||
| - swipe intentionally | ||
| - compare event ordering in logs | ||
|
|
||
| Expected evidence: | ||
|
|
||
| - If `goForward` / `goBackward` method-channel logs fire, Dart/UI code is still | ||
| requesting navigation. | ||
| - If native `onPageChanged` happens without method-channel `goForward` / | ||
| `goBackward`, the page turn comes from native navigator input. | ||
| - If the native turn disappears when `DirectionalNavigationAdapter` is | ||
| temporarily disabled, Readium input handling is the source. | ||
| - If the native turn disappears when the comic overlay consumes manual pan | ||
| events, the source is gesture propagation from the overlay to Readium. | ||
|
|
||
| ## Proposed implementation direction | ||
|
|
||
| ### Phase 1: Split Dart interaction semantics | ||
|
|
||
| - Rename/split `_onInteraction()` into two explicit concepts: | ||
| - hide controls / refresh wake lock | ||
| - notify navigation/manual-mode intent | ||
| - Do not call `notifyUserNavigation` from `onPointerMove`. | ||
| - Keep center tap and edge tap behavior explicit. | ||
| - Consider deleting `notifyUserNavigation` calls from Flutter entirely once | ||
| native swipe/edge/key navigation is verified to enter manual mode on both iOS | ||
| and Android. | ||
|
|
||
| ### Phase 2: Make native navigation the authority | ||
|
|
||
| - Treat native `goForward` / `goBackward` paths as the authoritative signal for | ||
| manual navigation during narration. | ||
| - Verify Android `DirectionalNavigationAdapter` page turns pass through code that | ||
| enters manual mode. If they do not, add a native input listener or navigator | ||
| callback at the Android layer instead of using Flutter pointer movement. | ||
| - Do the same verification on iOS. | ||
|
|
||
| ### Phase 3: Isolate Nota comic manual pan | ||
|
|
||
| - When `.nota-comicbook-page-container` is in manual zoom/pan mode, test a | ||
| non-passive touch/pointer listener that calls `preventDefault()` and/or | ||
| `stopPropagation()` for single-finger pan inside the active overlay. | ||
| - Keep normal page swipe available when the comic overlay is not in manual mode. | ||
| - Avoid blocking pinch gestures needed by the comic helper. | ||
|
|
||
| ## Acceptance criteria | ||
|
|
||
| - Panning inside a zoomed Nota MO comic page does not change EPUB spine/page. | ||
| - Intentional native page navigation still works. | ||
| - During active narration, intentional page navigation enters manual mode and | ||
| emits `onNarrationSyncChanged(false)`. | ||
| - Center tap still toggles controls. | ||
| - Edge tap behavior remains intentional and documented. | ||
| - Behavior is checked on Android first; iOS parity is verified or explicitly | ||
| documented. | ||
|
|
||
| ## Suggested validation | ||
|
|
||
| - Android manual smoke test with `50272-nota-comics.webpub`: | ||
| - start narration | ||
| - enter manual zoom/pan | ||
| - pan horizontally and vertically inside the page | ||
| - confirm no page turn | ||
| - use intentional edge/swipe page turn | ||
| - confirm page turn and manual-mode event semantics | ||
| - Run `bin/format && bin/analyze` before PR. | ||
| - If touching Kotlin: run | ||
| `cd flutter_readium/example/android && ./gradlew :flutter_readium:compileDebugKotlin`. | ||
| - If touching helper scripts: run `bin/build_helper_scripts` or the equivalent | ||
| npm helper build used by the repo. | ||
|
|
||
| ## Open questions | ||
|
|
||
| - Does Android `DirectionalNavigationAdapter` page navigation pass through | ||
| `ReadiumReader.epubGoForward` / `epubGoBackward`, or does it navigate entirely | ||
| inside the navigator fragment? | ||
| - Should Flutter keep semantic edge regions for accessibility only, or should | ||
| native expose accessible page-turn actions directly? | ||
| - Is `notifyUserNavigation` still needed after native input paths are verified? |
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
89 changes: 89 additions & 0 deletions
89
flutter_readium/android/src/main/kotlin/dk/nota/flutterreadium/EpubImageTapBridge.kt
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,89 @@ | ||
| package dk.nota.flutterreadium | ||
|
|
||
| import android.webkit.JavascriptInterface | ||
| import kotlinx.coroutines.ExperimentalCoroutinesApi | ||
| import org.json.JSONObject | ||
| import org.readium.r2.shared.publication.Publication | ||
| import java.net.URI | ||
|
|
||
| private const val TAG = "EpubImageTapBridge" | ||
|
|
||
| /** | ||
| * JavaScript interface injected into each EPUB resource WebView via | ||
| * [org.readium.r2.navigator.epub.EpubNavigatorFragment.Configuration.registerJavascriptInterface]. | ||
| * | ||
| * Exposes a single entry point callable from content JS: | ||
| * `FlutterImageBridge.onImageTapped(JSON.stringify(payload))` | ||
| * | ||
| * The bridge resolves the publication-relative href from the absolute `img.src` | ||
| * URL by suffix-matching the path component against all known publication links. | ||
| */ | ||
| @OptIn(ExperimentalCoroutinesApi::class) | ||
| class EpubImageTapBridge { | ||
| @JavascriptInterface | ||
| fun onImageTapped(json: String) { | ||
| val publication = | ||
| ReadiumReader.currentPublication ?: run { | ||
| PluginLog.w(TAG, "::onImageTapped. No publication available.") | ||
| return | ||
| } | ||
| val channel = | ||
| ReadiumReader.currentReaderWidget?.channel ?: run { | ||
| PluginLog.w(TAG, "::onImageTapped. No reader channel available.") | ||
| return | ||
| } | ||
| try { | ||
| val data = JSONObject(json) | ||
| if (publication.suppressesImageTapEvents(data)) { | ||
| PluginLog.d(TAG, "::onImageTapped. Suppressed for publication/content type.") | ||
| return | ||
| } | ||
| val srcUrl = data.optString("srcUrl") | ||
| val href = | ||
| resolvePublicationHref(srcUrl) ?: run { | ||
| PluginLog.w(TAG, "::onImageTapped. Could not resolve href for srcUrl=$srcUrl") | ||
| return | ||
| } | ||
|
|
||
| val result = | ||
| JSONObject().apply { | ||
| put("href", href) | ||
| val alt = data.optString("alt").ifEmpty { null } | ||
| if (alt != null) put("alt", alt) | ||
| val rectData = data.optJSONObject("rect") | ||
| if (rectData != null) put("rect", rectData) | ||
| val nw = data.optInt("nw").takeIf { it > 0 } | ||
| val nh = data.optInt("nh").takeIf { it > 0 } | ||
| if (nw != null) put("pixelWidth", nw) | ||
| if (nh != null) put("pixelHeight", nh) | ||
| } | ||
|
|
||
| PluginLog.d(TAG, "::onImageTapped href=$href") | ||
| channel.onImageTapped(result.toString()) | ||
| } catch (e: Exception) { | ||
| PluginLog.w(TAG, "::onImageTapped. Failed to process event: $e") | ||
| } | ||
| } | ||
|
|
||
| private fun resolvePublicationHref(srcUrl: String): String? { | ||
| if (srcUrl.isEmpty()) return null | ||
| val publication = ReadiumReader.currentPublication ?: return null | ||
| return try { | ||
| val imgPath = URI(srcUrl).path | ||
| val allLinks = publication.readingOrder + publication.resources | ||
| allLinks | ||
| .find { link -> | ||
| val linkHref = link.href.toString() | ||
| imgPath.endsWith("/$linkHref") || imgPath == linkHref | ||
| }?.href | ||
| ?.toString() | ||
| ?: imgPath.trimStart('/') | ||
| } catch (e: Exception) { | ||
| PluginLog.w(TAG, "::resolvePublicationHref. URI parse failed for srcUrl=$srcUrl: $e") | ||
| null | ||
| } | ||
| } | ||
|
|
||
| private fun Publication.suppressesImageTapEvents(data: JSONObject) = | ||
| conformsTo(Publication.Profile.DIVINA) || data.optBoolean("notaComicPageImage", false) | ||
| } |
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
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
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
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
Oops, something went wrong.
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We used to have this feature but removed it because it worked really bad with large files over the bridge.
Could we save to a tmp file and return the file path instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getLinkContent and getString removed back in May
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
True, maybe we should scope it to smaller ressources or just images 🤔
Will investigate effects of storing in local app storage and loading that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's do it afterwards