Conversation
…gator2-improvements
…gator2-improvements
…gator2-improvements
mickael-menu
left a comment
There was a problem hiding this comment.
I didn't review everything, but we will review again the public APIs together when releasing the beta.
A lot of doc comments are also missing, I guess we can add them when the API is more stable. But if you could add some to help understand the major components that would be helpful 🙏
readium/navigators/common/src/main/java/org/readium/navigator/common/LocationElements.kt
Outdated
Show resolved
Hide resolved
.../reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebPublication.kt
Show resolved
Hide resolved
...flowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRenditionState.kt
Outdated
Show resolved
Hide resolved
...rnals/src/main/kotlin/org/readium/navigator/web/internals/webview/WebViewScrollController.kt
Outdated
Show resolved
Hide resolved
...flowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRenditionState.kt
Outdated
Show resolved
Hide resolved
...flowable/src/main/kotlin/org/readium/navigator/web/reflowable/resource/ReflowableResource.kt
Outdated
Show resolved
Hide resolved
.../reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/css/ReadiumCssProperties.kt
Show resolved
Hide resolved
...eb/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/layout/LayoutConstants.kt
Show resolved
Hide resolved
|
It is still draft because I didn't solve all the issues due to the upgrade to Compose 1.10, but thanks for the review. |
There was a problem hiding this comment.
Pull request overview
This PR updates the new Web-based navigators (reflowable + fixed-layout) by improving location/progression handling, adding new JS/native bridges for selection + “move to” locations, and upgrading bundled Readium CSS assets.
Changes:
- Add richer location/viewport tracking (positions + per-resource progression ranges) and improve restoration/navigation flows.
- Introduce SelectionListener + Move APIs/bridges to support selection-aware taps and navigation to HTML id / CSS selector / text anchors.
- Upgrade bundled Readium CSS assets and adjust layout/margins/padding handling across scroll/paged modes.
Reviewed changes
Copilot reviewed 51 out of 77 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/resource/ReflowableWebViewport.kt | New viewport model describing visible reading order, progressions and positions. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/resource/ReflowableResourceState.kt | Track pending navigation requests + computed progression ranges per resource. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/resource/ReflowableResource.kt | Wire Move/Selection listener APIs, update scroll/progression update flow, selection-aware tap behavior. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/preferences/ReflowableWebPreferencesEditor.kt | Minor API cleanup for ReadiumCssLayout factory call. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/layout/LayoutResolver.kt | New layout resolver supporting scroll + paginated with safe-drawing insets. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/layout/LayoutConstants.kt | Shared constants for layout/margins/line lengths. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/css/ReadiumCssProperties.kt | Add scroll padding RS properties; adjust CSS serialization. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/css/ReadiumCssInjector.kt | Apply new Layout model; add scroll padding injection and vertical-text view handling. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRenditionState.kt | Major navigation/controller changes: mutexed navigation, viewport computation, new progression/position mapping. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRenditionFactory.kt | Require PositionsService + reject restricted publications; pass position counts into publication model. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRendition.kt | Update composition to use new location updates + safe drawing insets + background handling. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebPublication.kt | Add position-number based position/totalProgression computation utilities. |
| readium/navigators/web/reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebLocations.kt | Extend go-locations (htmlId/cssSelector/textAnchor) and export locations with position/totalProgression. |
| readium/navigators/web/reflowable/build.gradle.kts | Dependency adjustments (Compose foundation/core exposure). |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webview/WebViewScrollController.kt | Split start/end progressions; add snap + moveToOffset; adjust progression math. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webview/RelaxedWebView.kt | ActionMode handling improvements + over-scroll workaround; add invokeOnWebViewUpToDate helper. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webview/ComposableWebView.kt | Simplify WebView wrapper layout (remove LazyRow wrapper). |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webapi/SelectionListenerApi.kt | New JS interface for selection start/end callbacks. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webapi/ReflowableMoveApi.kt | New native API to query scroll offsets for locations via JS. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/webapi/ReflowableApiState.kt | Add Move API availability callback plumbing. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/util/Padding.kt | Add WindowInsets helpers (asAbsolutePaddingValues + symmetric). |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/pager/RenditionPager.kt | Add nestedScroll consumption connection. |
| readium/navigators/web/internals/src/main/kotlin/org/readium/navigator/web/internals/pager/ConsumingNestedScrollConnection.kt | New nested scroll connection that consumes child scroll input. |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/webPub/ReadiumCSS-webPub.css | Upgrade generated Readium CSS (v2.0.0-beta.24). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/rtl/ReadiumCSS-default.css | Upgrade generated Readium CSS (rtl default). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/rtl/ReadiumCSS-before.css | Upgrade generated Readium CSS (rtl before). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/rtl/ReadiumCSS-after.css | Upgrade generated Readium CSS (rtl after). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-vertical/ReadiumCSS-default.css | Upgrade generated Readium CSS (cjk-vertical default). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-vertical/ReadiumCSS-before.css | Upgrade generated Readium CSS (cjk-vertical before). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-vertical/ReadiumCSS-after.css | Upgrade generated Readium CSS (cjk-vertical after). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-horizontal/ReadiumCSS-default.css | Upgrade generated Readium CSS (cjk-horizontal default). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-horizontal/ReadiumCSS-before.css | Upgrade generated Readium CSS (cjk-horizontal before). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/cjk-horizontal/ReadiumCSS-after.css | Upgrade generated Readium CSS (cjk-horizontal after). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/android-fonts-patch/android-fonts-patch.css | Update Readium CSS patch references/headers. |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/android-fonts-patch/ReadMe.md | Update Readium CSS patch doc links. |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/ReadiumCSS-ebpaj_fonts_patch.css | Update Readium CSS patch references/headers. |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/ReadiumCSS-default.css | Upgrade generated Readium CSS (default). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/ReadiumCSS-before.css | Upgrade generated Readium CSS (before). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/ReadiumCSS-after.css | Upgrade generated Readium CSS (after). |
| readium/navigators/web/internals/src/main/assets/readium/navigator/web/internals/generated/readium-css/ReadMe.md | Update generated Readium CSS readme (disabled settings list). |
| readium/navigators/web/internals/scripts/src/index-reflowable-injectable.ts | Expose Move + SelectionListener bridges to the global window. |
| readium/navigators/web/internals/scripts/src/index-fixed-single.ts | Adjust FixedSingleSelectionBridge construction to pass iframe. |
| readium/navigators/web/internals/scripts/src/common/selection.ts | Add SelectionReporter to track selectionchange events. |
| readium/navigators/web/internals/scripts/src/common/gestures.ts | Remove selection detection from gesture detector (now handled via selection listener). |
| readium/navigators/web/internals/scripts/src/common/decoration.ts | Add logging + harden TextQuote anchor range creation with try/catch. |
| readium/navigators/web/internals/scripts/src/bridge/reflowable-move-bridge.ts | New move bridge computing offsets for css selector / htmlId / text anchor. |
| readium/navigators/web/internals/scripts/src/bridge/reflowable-initialization-bridge.ts | Initialize Move bridge + SelectionReporter and add Move API ready callback. |
| readium/navigators/web/internals/scripts/src/bridge/all-selection-bridge.ts | Adjust fixed selection coordinates to parent coordinates using iframe info. |
| readium/navigators/web/internals/scripts/src/bridge/all-listener-bridge.ts | Add SelectionListener bridge plumbing into adapter. |
| readium/navigators/web/internals/scripts/pnpm-lock.yaml | Update dependencies (Readium CSS beta.24, TypeScript 5.9.3). |
| readium/navigators/web/internals/scripts/package.json | Bump @readium/css to 2.0.0-beta.24. |
| readium/navigators/web/internals/build.gradle.kts | Dependency adjustments (Compose foundation exposure). |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/spread/SpreadWebView.kt | Ensure WebView background matches requested backgroundColor. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/spread/SingleViewportSpread.kt | Decoration diffing fixes and type inference simplification. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/spread/DoubleViewportSpread.kt | Decoration group iteration fixes. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/FixedWebRenditionState.kt | Mutexed navigation; pagerState re-creation on layout changes; selection delegate changes. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/FixedWebRenditionFactory.kt | Reject restricted publications. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/FixedWebRendition.kt | Composition refactor; add backgroundColor param; compute/export position + totalProgression. |
| readium/navigators/web/fixedlayout/src/main/kotlin/org/readium/navigator/web/fixedlayout/FixedWebLocations.kt | Export locations with position + totalProgression; align decoration location interfaces. |
| readium/navigators/web/fixedlayout/build.gradle.kts | Dependency adjustments (Compose foundation exposure). |
| readium/navigators/web/common/src/main/kotlin/org/readium/navigator/web/common/FontFamilyDeclarations.kt | Remove unused serialization + adjust immutable collection imports/usages. |
| readium/navigators/web/common/build.gradle.kts | Dependency adjustments (Compose foundation exposure). |
| readium/navigators/common/src/main/java/org/readium/navigator/common/Locations.kt | Add TextAnchorLocation interface. |
| readium/navigators/common/src/main/java/org/readium/navigator/common/LocationElements.kt | Add HtmlId/TextAnchor types + Comparable support; change Position to 1-based Int. |
| readium/navigators/common/build.gradle.kts | Dependency adjustments (Compose foundation exposure). |
| readium/navigator/src/main/java/org/readium/r2/navigator/epub/css/ReadiumCss.kt | Use core-ktx toUri() instead of Uri.parse. |
| demos/navigator/src/main/java/org/readium/demo/navigator/preferences/UserPreferences.kt | Minor formatting cleanup. |
| demos/navigator/src/main/java/org/readium/demo/navigator/decorations/HighlightsManager.kt | Fix highlight ID generation (incrementing var). |
| demos/navigator/build.gradle.kts | Add kotlinx.serialization.json dependency. |
Files not reviewed (1)
- readium/navigators/web/internals/scripts/pnpm-lock.yaml: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
.../reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/css/ReadiumCssProperties.kt
Show resolved
Hide resolved
.../reflowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebPublication.kt
Show resolved
Hide resolved
...flowable/src/main/kotlin/org/readium/navigator/web/reflowable/ReflowableWebRenditionState.kt
Show resolved
Hide resolved
readium/navigators/web/internals/scripts/src/bridge/reflowable-move-bridge.ts
Outdated
Show resolved
Hide resolved
...rnals/src/main/kotlin/org/readium/navigator/web/internals/webview/WebViewScrollController.kt
Show resolved
Hide resolved
mickael-menu
left a comment
There was a problem hiding this comment.
@qnga Could you just review the comments left by Copilot before merging? Thank you
Uh oh!
There was an error while loading. Please reload this page.