diff --git a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt b/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt index b9325e1..560aeef 100644 --- a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt +++ b/app/src/main/java/io/theficos/ereader/ui/reader/ReaderScreen.kt @@ -31,8 +31,10 @@ import androidx.compose.runtime.saveable.rememberSaveable import androidx.compose.runtime.setValue import androidx.compose.ui.Alignment import androidx.compose.ui.Modifier +import androidx.compose.ui.layout.onSizeChanged import androidx.compose.ui.platform.LocalContext import androidx.compose.ui.platform.LocalView +import androidx.compose.ui.unit.IntSize import androidx.compose.ui.viewinterop.AndroidView import androidx.core.view.WindowCompat import androidx.core.view.WindowInsetsCompat @@ -137,6 +139,21 @@ fun ReaderScreen(viewModel: ReaderViewModel, onClose: () -> Unit) { onExit = viewModel::clearPendingResize, ) + // Last measured size of the reader content. Every change is reported to the + // view model, which re-anchors the reading position across it whatever caused + // it — see ReaderViewModel.onViewportChanged. This is keyed on the size so a + // resize arriving in several steps (an inset animation, say) restarts the wait + // instead of completing mid-flight. Readium's onPageChanged re-anchors as it + // re-paginates; this has the last word, once the size has stopped moving and + // the WebView's page grid is final. + var viewport by remember { mutableStateOf(IntSize.Zero) } + LaunchedEffect(viewport) { + if (viewport != IntSize.Zero) { + delay(RESIZE_SETTLE_MS) + viewModel.completeViewportResize() + } + } + // The app is edge-to-edge (MainActivity). Immersive reading uses that full // bleed: content draws behind the (hidden) bars and the chrome self-insets. // With immersive off, the reader behaves like any normal screen — inset the @@ -161,7 +178,11 @@ fun ReaderScreen(viewModel: ReaderViewModel, onClose: () -> Unit) { onPrev = viewModel::pageBackward, onNext = viewModel::pageForward, onToggleChrome = viewModel::toggleChrome, - onPageLoaded = viewModel::completeViewportResize, + onPageLoaded = viewModel::reanchorViewport, + onViewportChanged = { size -> + viewport = size + viewModel.onViewportChanged(size.width, size.height) + }, ) ReaderTopBar( @@ -228,6 +249,7 @@ private fun ReaderContent( onNext: () -> Unit, onToggleChrome: () -> Unit, onPageLoaded: () -> Unit, + onViewportChanged: (IntSize) -> Unit, ) { val activity = LocalContext.current as FragmentActivity val containerId = rememberSaveable { View.generateViewId() } @@ -235,7 +257,11 @@ private fun ReaderContent( var fragment by remember { mutableStateOf(null) } AndroidView( - modifier = Modifier.fillMaxSize(), + // This is the node whose height decides how much text fits in a Readium column, so + // it — not the window, and not the insets — is the authoritative viewport. + modifier = Modifier + .fillMaxSize() + .onSizeChanged(onViewportChanged), factory = { ctx -> ReaderTapDispatcher(ctx).apply { layoutParams = ViewGroup.LayoutParams( @@ -280,6 +306,24 @@ private fun ReaderContent( initialLocator = initialLocator, initialPreferences = preferences.toEpubPreferences(), paginationListener = paginationListener, + configuration = EpubNavigatorFragment.Configuration( + // Quire owns the reader's insets, so Readium must not also apply them. + // + // Left at its default (on), Readium pads its own page container by the + // system-bar insets whenever they are dispatched to its view. In full-screen + // reading the bars are hidden and the reader is deliberately full-bleed, so + // that padding is for bars that aren't there — and it arrived late: the page + // rendered edge to edge, then the first time the window regained focus the + // WebView finally re-measured against it and lost ~350px of height. That + // re-paginated the chapter under the reader, and because the resize happened + // inside Readium's own view tree, nothing here saw it coming: the post- + // re-pagination locator was published and written over the saved position, so + // the reader came back to the wrong page and stayed there (issue #95). + // + // With full-screen reading off it was simply double-inset: the whole reader + // subtree is already padded by WindowInsets.systemBars in ReaderScreen. + shouldApplyInsetsPadding = false, + ), ) val nav = (fm.fragmentFactory.instantiate( activity.classLoader, @@ -310,6 +354,13 @@ private fun ReaderContent( } } +/** + * How long to wait for Readium to re-paginate after the viewport changed before honouring the + * anchor anyway. onPageChanged normally gets there first; this only has to cover a resize that + * didn't re-paginate at all, so that locator publishing is never left suppressed. + */ +private const val RESIZE_SETTLE_MS = 600L + /** * Drives immersive full-screen: binds the OS status + navigation bars to the reader * chrome. Every window mutation is snapshotted once and reverted on exit so the rest of diff --git a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderViewModel.kt b/app/src/main/java/io/theficos/ereader/ui/reader/ReaderViewModel.kt index d1d1776..2050892 100644 --- a/app/src/main/java/io/theficos/ereader/ui/reader/ReaderViewModel.kt +++ b/app/src/main/java/io/theficos/ereader/ui/reader/ReaderViewModel.kt @@ -10,8 +10,11 @@ import io.theficos.ereader.reader.EpubAsset import io.theficos.ereader.reader.ProgressTracker import io.theficos.ereader.reader.ReaderPreferences import io.theficos.ereader.reader.ReaderPreferencesStore +import io.theficos.ereader.reader.PAGE_START_ANCHOR_JS import io.theficos.ereader.reader.ReadiumFactory import io.theficos.ereader.reader.locatorAtPercent +import io.theficos.ereader.reader.parsePageStartAnchor +import io.theficos.ereader.reader.resizeAnchor import kotlinx.coroutines.Dispatchers import kotlinx.coroutines.Job import kotlinx.coroutines.flow.MutableSharedFlow @@ -34,6 +37,11 @@ class ReaderViewModel( private val progress: ProgressRepository, private val readium: ReadiumFactory, private val preferencesStore: ReaderPreferencesStore, + // How the reader asks Readium whereabouts in the DOM the current page starts. A seam so + // the re-anchor can be tested without a live WebView; see ReaderViewportResizeTest. + private val readDomAnchor: suspend (EpubNavigatorFragment?) -> Locator? = + ::readPageStartAnchor, + private val nowMs: () -> Long = System::currentTimeMillis, ) : ViewModel() { private val _state = MutableStateFlow(ReaderUiState.Loading) @@ -87,6 +95,8 @@ class ReaderViewModel( } fun goTo(locator: Locator) { + clearPendingResize() + invalidateDomAnchor() viewModelScope.launch { navigator?.go(locator, false) } } @@ -128,28 +138,129 @@ class ReaderViewModel( if (suppressLocatorPublishing) return _currentLocator.value = locator _locatorUpdates.tryEmit(locator) + refreshDomAnchor() + } + + /** + * Keeps [domAnchor] tracking the page on screen. + * + * Asking Readium costs a round trip into the WebView's JavaScript, which is far too slow to + * do at the moment a rotation starts — the answer would come back describing the page after + * re-pagination, which is the one thing it must not be. So it is kept warm instead, refreshed + * whenever Readium reports a settled position, and read synchronously when the resize arms. + * + * Both guards here exist to keep a rotation from rewriting the anchor, which is the failure + * that made rotation drift compound. Which element anchors a page depends on the page's + * shape: a tall portrait column starts several paragraphs where a short landscape one starts + * a single paragraph, so reading the anchor in the orientation the reader is only passing + * through replaces it with an earlier one, and the return leg dutifully honours that. Turning + * a page moves the reader; rotating the device does not. + */ + private fun refreshDomAnchor() { + if (nowMs() < anchorPinnedUntil) return + viewModelScope.launch { + val nav = navigator + val dom = runCatching { readDomAnchor(nav) }.getOrNull() + // A resize may have armed while that round trip was in flight, in which case the + // answer describes the re-paginated page. Drop it and keep the pre-resize anchor. + if (!suppressLocatorPublishing && nowMs() >= anchorPinnedUntil) domAnchor = dom + } } private var pendingRotationAnchor: Locator? = null private var suppressLocatorPublishing: Boolean = false + private var viewportSize: Pair? = null + private var domAnchor: Locator? = null + private var anchorPinnedUntil: Long = 0L + + /** + * Reports the reader viewport's measured size, on every layout pass. + * + * Any change in that size re-paginates Readium's WebView, after which Readium reports + * whatever text happened to land on screen. Nothing marks that locator as junk, so left + * alone it is published to the HUD and written over the reader's saved position — the + * reader comes back to the wrong page and stays there. + * + * Everything else here arms the re-anchor by naming a cause: [beginViewportResize] is + * called from `MainActivity.onConfigurationChanged` for rotation, and from the + * full-screen-reading toggle. Issue #95 was a resize nobody had named, so this arms on + * the effect instead — the measured size — and covers causes we haven't thought of. + * (It only sees resizes of the Compose node; one that happens further down, inside + * Readium's own view tree, is invisible here. See `shouldApplyInsetsPadding` in + * ReaderScreen for the one that bit us.) + * + * The first report just establishes the baseline; the WebView is created at that size, so + * there is nothing to re-anchor. Zero sizes (pre-layout, or a detached view) are ignored + * so they never read as a resize in either direction. + */ + fun onViewportChanged(width: Int, height: Int) { + if (width <= 0 || height <= 0) return + val previous = viewportSize + viewportSize = width to height + if (previous == null || previous == viewportSize) return + beginViewportResize() + } + + /** + * Forgets where the page on screen begins, because the reader is being sent elsewhere. + * + * [domAnchor] describes the page being left. Until Readium reports the one being arrived at, + * a resize landing in between would otherwise fold that stale element into the new locator + * and send the reader straight back to the page they just jumped away from. + */ + private fun invalidateDomAnchor() { + domAnchor = null + } - // Called from MainActivity.onBeforeReaderConfigChange — runs BEFORE the - // Activity dispatches the configuration change down to fragments. Snapshots - // the current locator into pendingRotationAnchor and gates publishLocator so - // Readium's post-resize drifted emissions cannot overwrite the anchor. + // Called from MainActivity.onBeforeReaderConfigChange — runs BEFORE the Activity dispatches + // the configuration change down to fragments. Snapshots where the reader is into + // pendingRotationAnchor and gates publishLocator so Readium's post-resize drifted emissions + // cannot overwrite it. fun beginViewportResize() { - val anchor = _currentLocator.value ?: return - pendingRotationAnchor = anchor + // A single rotation arms this several times over — MainActivity as the configuration + // change is dispatched, then onViewportChanged as the measured size follows. Only the + // first of those still sees the page the reader is leaving, so it wins. + if (pendingRotationAnchor != null) return + val live = _currentLocator.value ?: return + pendingRotationAnchor = resizeAnchor(live, domAnchor) suppressLocatorPublishing = true } - // Called from the Readium PaginationListener when re-pagination completes - // after a viewport resize. No-op if no resize is pending (so it's safe to - // call on every onPageLoaded). Re-anchors via navigator.go(anchor, false), - // re-seeds the current-locator flow, and re-enables publishing. + /** + * Puts the reader back on the anchored page, and stays armed. Called from the Readium + * pagination listener on every re-pagination while a resize is in flight; a no-op when no + * resize is pending, so it is safe on every page turn. + * + * Re-pagination arrives in steps, and anchoring on the first one is not enough: wired that + * way, and with everything else here unchanged, rotation still landed on the wrong page and + * did so inconsistently, the same start position coming back two different ways. Readium + * scrolls to an anchor by snapping the element's offset to a page boundary, using a page + * width it caches in JavaScript and only recomputes when it is told the viewport moved, so + * an early re-anchor is measuring against a grid that is still the old one. Re-anchoring on + * every step and once more when the size stops changing ([completeViewportResize]) gives the + * settled layout the last word. + * + * Repeating this is only safe because the anchor is a DOM element: it lands in the same place + * however many times it is used. The progression fraction this used to carry was consumed a + * little by every application, which is why it could only ever be applied once. + */ + fun reanchorViewport() { + val anchor = pendingRotationAnchor ?: return + viewModelScope.launch { navigator?.go(anchor, false) } + } + + // Called when the viewport has stopped changing (and from the immersive transition's own + // settle timer). No-op if no resize is pending. Re-anchors one last time, now that the + // layout is final, re-seeds the current-locator flow and re-enables publishing. fun completeViewportResize() { val anchor = pendingRotationAnchor ?: return pendingRotationAnchor = null + // Readium reports the restored position of its own accord a moment after the jump — + // measured at around half a second. That report is the resize finishing, not the reader + // moving, so it must not be allowed to re-read the anchor from the page it just landed + // on. Hold the anchor over that window; the next page the reader actually turns to + // refreshes it normally. + anchorPinnedUntil = nowMs() + ANCHOR_PIN_MS viewModelScope.launch { navigator?.go(anchor, false) _currentLocator.value = anchor @@ -174,6 +285,10 @@ class ReaderViewModel( fun seek(percent: Double) { val target = previewLocator(percent) ?: return val nav = navigator ?: return + // An explicit jump supersedes any armed re-anchor: the anchor predates the seek, so + // honouring it afterwards would yank the reader back out of the page they just chose. + clearPendingResize() + invalidateDomAnchor() // Surface the target on the HUD synchronously, before the suspending nav.go() // call dispatches. This avoids a one-frame window where the slider thumb // would snap back to the pre-seek liveLocator after the UI clears its drag @@ -196,6 +311,13 @@ class ReaderViewModel( } } +/** + * How long the reading anchor is held after a viewport resize completes, covering Readium's own + * delayed report of where it landed. Long enough for that report (about half a second in + * practice), short enough that a page the reader turns to just after a rotation still registers. + */ +private const val ANCHOR_PIN_MS = 1_500L + sealed interface ReaderUiState { data object Loading : ReaderUiState data class Error(val message: String) : ReaderUiState @@ -206,3 +328,27 @@ sealed interface ReaderUiState { val savedProgress: Progress?, ) : ReaderUiState } + +/** + * Asks the navigator where the page on screen begins in the DOM. + * + * Runs [PAGE_START_ANCHOR_JS] in the current resource's web view and stamps the answer with that + * resource's href, so a stale anchor can be rejected later if the reader has moved on to another + * chapter. Falls back to Readium's own `firstVisibleElementLocator()` whenever the script + * declines to answer: a page with nothing starting on it, or a layout the script bows out of. + * That fallback anchors slightly earlier than the reader actually is — the very thing the script + * exists to improve on — but it is still an exact DOM anchor, so it costs at most a one-off + * shift rather than the compounding walk the progression fraction caused. + */ +private suspend fun readPageStartAnchor(navigator: EpubNavigatorFragment?): Locator? { + val nav = navigator ?: return null + val current = nav.currentLocator.value + val fromScript = runCatching { + parsePageStartAnchor( + json = nav.evaluateJavascript(PAGE_START_ANCHOR_JS), + href = current.href, + mediaType = current.mediaType, + ) + }.getOrNull() + return fromScript ?: runCatching { nav.firstVisibleElementLocator() }.getOrNull() +} diff --git a/app/src/test/java/io/theficos/ereader/ui/reader/ReaderViewportResizeTest.kt b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderViewportResizeTest.kt new file mode 100644 index 0000000..918f7a6 --- /dev/null +++ b/app/src/test/java/io/theficos/ereader/ui/reader/ReaderViewportResizeTest.kt @@ -0,0 +1,330 @@ +package io.theficos.ereader.ui.reader + +import androidx.room.Room +import androidx.test.core.app.ApplicationProvider +import com.google.common.truth.Truth.assertThat +import io.theficos.ereader.data.local.DocumentRepository +import io.theficos.ereader.data.local.ProgressRepository +import io.theficos.ereader.data.local.db.EReaderDatabase +import io.theficos.ereader.reader.ReaderPreferencesStore +import io.theficos.ereader.reader.ReadiumFactory +import kotlinx.coroutines.CompletableDeferred +import kotlinx.coroutines.Dispatchers +import kotlinx.coroutines.ExperimentalCoroutinesApi +import kotlinx.coroutines.test.UnconfinedTestDispatcher +import kotlinx.coroutines.test.resetMain +import kotlinx.coroutines.test.runTest +import kotlinx.coroutines.test.setMain +import org.junit.After +import org.junit.Before +import org.junit.Test +import org.junit.runner.RunWith +import org.readium.r2.navigator.epub.EpubNavigatorFragment +import org.readium.r2.shared.publication.Locator +import org.readium.r2.shared.util.Url +import org.readium.r2.shared.util.mediatype.MediaType +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * The reader keeps its place across a viewport resize by snapshotting the current locator, + * ignoring everything Readium emits while the WebView re-paginates, and then going back to the + * snapshot. These tests cover which resizes arm that. + * + * Issue #95 was a resize nobody had armed, and what it cost is the shape of every test here: a + * post-re-pagination locator reached the HUD and the progress row, so the reader came back to + * the wrong page and its saved place was gone. That particular resize is now prevented at + * source (ReaderScreen turns off Readium's own inset padding); arming on the measured viewport + * is the backstop for the next one. + */ +@OptIn(ExperimentalCoroutinesApi::class) +@RunWith(RobolectricTestRunner::class) +// The stock Application: EReaderApp builds the whole DI container on create, keystore and +// all, which this test has no use for. +@Config(sdk = [33], application = android.app.Application::class) +class ReaderViewportResizeTest { + + private lateinit var db: EReaderDatabase + private lateinit var vm: ReaderViewModel + + @Before fun setUp() { + Dispatchers.setMain(UnconfinedTestDispatcher()) + val context = ApplicationProvider.getApplicationContext() + db = Room.inMemoryDatabaseBuilder(context, EReaderDatabase::class.java) + .allowMainThreadQueries() + .build() + vm = ReaderViewModel( + documentId = 1L, + docs = DocumentRepository(db.documentDao()), + progress = ProgressRepository(db.progressDao()), + readium = ReadiumFactory(context), + preferencesStore = ReaderPreferencesStore(context), + ) + } + + @After fun tearDown() { + db.close() + Dispatchers.resetMain() + } + + private fun locatorAt(progression: Double): Locator = Locator( + href = Url("ch1.xhtml")!!, + mediaType = MediaType.XHTML, + locations = Locator.Locations(progression = progression, totalProgression = progression), + ) + + @Test fun `first viewport report only establishes the baseline`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + // Nothing to re-anchor: the WebView was created at this size, so Readium is still free + // to report where it actually is. + vm.publishLocator(locatorAt(0.6)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.6) + } + + @Test fun `an unchanged viewport does not arm a re-anchor`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + vm.onViewportChanged(1080, 2424) + + vm.publishLocator(locatorAt(0.6)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.6) + } + + @Test fun `a shorter viewport suppresses the drifted locator and restores the anchor`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + // Something outside the reader's control shortens the viewport. + vm.onViewportChanged(1080, 2219) + + // Readium re-paginates and reports wherever the text landed. That must not reach the + // HUD or, through the locator flow, the saved progress row. + vm.publishLocator(locatorAt(0.42)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.5) + + vm.completeViewportResize() + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.5) + + // Publishing is live again once the anchor has been honoured. + vm.publishLocator(locatorAt(0.55)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.55) + } + + @Test fun `a resize arriving in steps keeps the original anchor`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + // An inset animation resizes the viewport over several frames, with Readium emitting + // drifted locators in between. The anchor must stay the pre-resize one throughout. + vm.onViewportChanged(1080, 2380) + vm.publishLocator(locatorAt(0.41)) + vm.onViewportChanged(1080, 2300) + vm.publishLocator(locatorAt(0.40)) + vm.onViewportChanged(1080, 2219) + + vm.completeViewportResize() + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.5) + } + + @Test fun `a zero-sized viewport is not a resize`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + // A detached or not-yet-measured view reports zero; treating that as a resize would arm + // the re-anchor against a size the reader never actually had. + vm.onViewportChanged(0, 0) + vm.onViewportChanged(1080, 2424) + + vm.publishLocator(locatorAt(0.6)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.6) + } + + /** + * A [ReaderViewModel] whose DOM anchors come from [anchors] in order, the last one repeating. + * Standing in for a JavaScript round trip into Readium's WebView, which a unit test has no + * WebView for. + */ + /** Test clock, in milliseconds; advance it to step past the post-resize anchor hold. */ + private var clock = 0L + + private fun vmServing( + vararg anchors: Locator, + /** Held open from the second read onwards, to park one in flight across a rotation. */ + gate: CompletableDeferred? = null, + ): ReaderViewModel { + val context = ApplicationProvider.getApplicationContext() + var call = 0 + return ReaderViewModel( + documentId = 1L, + docs = DocumentRepository(db.documentDao()), + progress = ProgressRepository(db.progressDao()), + readium = ReadiumFactory(context), + preferencesStore = ReaderPreferencesStore(context), + readDomAnchor = { _: EpubNavigatorFragment? -> + if (call > 0) gate?.await() + anchors[minOf(call++, anchors.size - 1)] + }, + nowMs = { clock }, + ) + } + + private fun domAnchor(nth: Int) = Locator( + href = Url("ch1.xhtml")!!, + mediaType = MediaType.XHTML, + locations = Locator.Locations( + otherLocations = mapOf("cssSelector" to ":root > :nth-child(2) > :nth-child($nth)"), + ), + text = Locator.Text(highlight = "Paragraph $nth."), + ) + + private val Locator?.selector: String? + get() = this?.locations?.otherLocations?.get("cssSelector") as? String + + @Test fun `the anchor carries a DOM location, not just a progression`() = runTest { + val vm = vmServing(domAnchor(23)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + vm.onViewportChanged(2424, 1080) + vm.completeViewportResize() + + // Readium restores a locator precisely only when it carries text to match; without it, + // it maps the progression onto the new page grid and lands wherever that arithmetic + // happens to point. This is the whole fix: rotation must hand it a DOM location. + assertThat(vm.currentLocator.value?.text?.highlight).isEqualTo("Paragraph 23.") + assertThat(vm.currentLocator.value.selector) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + // ...while still carrying the position it left, for Readium's own fallback and for the + // percentage the HUD and the progress row read. + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.5) + } + + @Test fun `a rotation does not move the anchor, so returning is exact`() = runTest { + // Which element "starts the page" depends on the shape of the page: a tall portrait + // column starts several paragraphs where a short landscape one starts a single + // paragraph. Re-reading the anchor while the reader is rotated therefore replaces it + // with an earlier one, and honouring that on the way back walks the reader backwards a + // page at a time. A rotation is not a change of reading position: the anchor must be + // whatever the reader was last actually on. + val vm = vmServing(domAnchor(23), domAnchor(22), domAnchor(18)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + repeat(3) { + vm.onViewportChanged(2424, 1080) + vm.completeViewportResize() + vm.onViewportChanged(1080, 2424) + vm.completeViewportResize() + assertThat(vm.currentLocator.value.selector) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + } + } + + @Test fun `Readium's own report of the restored page does not move the anchor`() = runTest { + // Readium reports where it landed of its own accord, about half a second after the + // jump. That arrives as an ordinary publish, so without a hold it re-reads the anchor + // from the page the reader is only rotating through — which is how the drift crept back + // in intermittently even once the anchor was no longer refreshed explicitly. + val vm = vmServing(domAnchor(23), domAnchor(31)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + vm.onViewportChanged(2424, 1080) + vm.completeViewportResize() + clock += 500 + vm.publishLocator(locatorAt(0.39)) + + vm.onViewportChanged(1080, 2424) + vm.completeViewportResize() + assertThat(vm.currentLocator.value.selector) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + } + + @Test fun `moving the reader does move the anchor`() = runTest { + // The other half of the same rule: the anchor has to follow the reader when the reader + // is the one moving, page turns and swipes alike, both of which arrive as a publish. + val vm = vmServing(domAnchor(23), domAnchor(31)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + // A rotation, and then — once its dust has settled — a page turn. + vm.onViewportChanged(2424, 1080) + vm.completeViewportResize() + clock += 5_000 + vm.publishLocator(locatorAt(0.6)) + + vm.onViewportChanged(1080, 2424) + vm.completeViewportResize() + assertThat(vm.currentLocator.value.selector) + .isEqualTo(":root > :nth-child(2) > :nth-child(31)") + } + + @Test fun `an anchor read while the reader is resizing is discarded`() = runTest { + // The read is a round trip into the WebView's JavaScript. If a rotation starts while one + // is in flight, the answer that comes back describes the re-paginated page — exactly the + // page the anchor exists to avoid. Keeping the older, pre-rotation one is right even + // though it is a page turn behind. + val gate = CompletableDeferred() + val vm = vmServing(domAnchor(23), domAnchor(31), gate = gate) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + // A page turn, whose read parks on the gate, and then a rotation on top of it. + vm.publishLocator(locatorAt(0.6)) + vm.onViewportChanged(2424, 1080) + gate.complete(Unit) + vm.completeViewportResize() + + // The next rotation is where a swallowed answer would show up. + vm.onViewportChanged(1080, 2424) + vm.completeViewportResize() + assertThat(vm.currentLocator.value.selector) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + } + + @Test fun `re-anchoring mid-resize does not end the resize`() = runTest { + // Readium re-paginates in steps and the reader is put back on the anchor at each one, + // because an early step measures against a page grid that is still the old one. Those + // steps must not be mistaken for the end of the resize. + val vm = vmServing(domAnchor(23)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + vm.onViewportChanged(2424, 1080) + + vm.reanchorViewport() + vm.publishLocator(locatorAt(0.31)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.5) + + vm.completeViewportResize() + vm.publishLocator(locatorAt(0.55)) + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.55) + } + + @Test fun `a jump forgets the anchor rather than dragging the reader back`() = runTest { + // The cached anchor describes the page being left. A rotation landing between the jump + // and Readium's report of where it arrived would otherwise fold that element into the + // new locator, and Readium would honour the element over the progression. + val vm = vmServing(domAnchor(23)) + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + + vm.goTo(locatorAt(0.9)) + vm.onViewportChanged(2424, 1080) + vm.completeViewportResize() + + assertThat(vm.currentLocator.value.selector).isNull() + } + + @Test fun `seeking supersedes an armed re-anchor`() = runTest { + vm.publishLocator(locatorAt(0.5)) + vm.onViewportChanged(1080, 2424) + vm.onViewportChanged(1080, 2219) + + // A seek can land while a re-anchor is armed. The reader must stay where the seek put + // it rather than being yanked back to the anchor, which predates the seek. + vm.goTo(locatorAt(0.9)) + vm.publishLocator(locatorAt(0.9)) + vm.completeViewportResize() + + assertThat(vm.currentLocator.value?.locations?.progression).isEqualTo(0.9) + } +} diff --git a/reader/src/main/java/io/theficos/ereader/reader/PageStartAnchor.kt b/reader/src/main/java/io/theficos/ereader/reader/PageStartAnchor.kt new file mode 100644 index 0000000..a6406d8 --- /dev/null +++ b/reader/src/main/java/io/theficos/ereader/reader/PageStartAnchor.kt @@ -0,0 +1,114 @@ +package io.theficos.ereader.reader + +import org.json.JSONObject +import org.readium.r2.shared.publication.Locator +import org.readium.r2.shared.util.Url +import org.readium.r2.shared.util.mediatype.MediaType + +/** + * Finds the element that *begins* on the page currently on screen. + * + * Readium ships `readium.findFirstVisibleLocator()`, which returns the first element that is + * visible — anything whose box overlaps the page at all, including a paragraph that started + * two pages back and merely spills onto this one. That is the wrong end of the paragraph to + * anchor to, and it is wrong in a way that compounds: + * + * - going back to a locator scrolls to where its element *starts* (`scrollToRect` floors the + * element's offset to a page boundary), so restoring the first visible element lands on the + * page where that element started, one page or more before where the reader was; + * - the reader is then sitting on a page whose own first visible element is the one before + * that, so the next rotation anchors one element earlier again. + * + * Measured on the marker fixture, that walks backwards forever: anchor paragraph 11 landed on + * 10, then 7, 6, 4, 2, and finally the chapter heading, one rotation at a time. Capturing and + * restoring have to be inverses of each other, and they only are if the anchor is an element + * that starts on this page: restoring puts its start at the page's leading edge, which is this + * same page, and capturing there returns the same element. A fixed point. + * + * The script mirrors Readium's own walk — same `display: block` / `opacity: 0` skips, same + * descent to the deepest match — and changes one thing: an element qualifies when its box + * *begins* within the page (`left >= 0`) rather than merely reaching it (`right > 0`). + * Containers that only overlap are still descended into, because the element that opens the + * page is usually inside one. + * + * Returns `null`, and so leaves the caller on Readium's own answer, when nothing starts on this + * page (one paragraph filling it end to end), or when the layout is one this reasoning does not + * hold for: scrolled rather than paginated, or right-to-left, where the leading edge is the + * other side. + */ +const val PAGE_START_ANCHOR_JS = """ +(function () { + try { + if (typeof readium !== 'undefined' && readium.isFixedLayout) return null; + var de = document.scrollingElement || document.documentElement; + if (de.scrollHeight > de.clientHeight) return null; + if (getComputedStyle(document.documentElement).direction === 'rtl') return null; + var width = window.innerWidth; + function skip(el) { + var s = getComputedStyle(el); + if (!s) return false; + return s.getPropertyValue('display') !== 'block' || s.getPropertyValue('opacity') === '0'; + } + function opens(r) { return r.left >= -1 && r.left < width; } + function touches(r) { return r.right > 0 && r.left < width; } + function deepest(el) { + for (var i = 0; i < el.children.length; i++) { + var c = el.children[i]; + if (!skip(c) && opens(c.getBoundingClientRect())) return deepest(c); + } + return el; + } + function search(el) { + for (var i = 0; i < el.children.length; i++) { + var c = el.children[i]; + if (skip(c)) continue; + var r = c.getBoundingClientRect(); + if (opens(r)) return deepest(c); + if (touches(r)) { var f = search(c); if (f) return f; } + } + return null; + } + var el = search(document.body); + if (!el || !el.textContent) return null; + var parts = []; + for (var n = el; n && n.parentElement; n = n.parentElement) { + var i = Array.prototype.indexOf.call(n.parentElement.children, n) + 1; + parts.unshift(':nth-child(' + i + ')'); + } + return { cssSelector: [':root'].concat(parts).join(' > '), text: el.textContent }; + } catch (e) { + return null; + } +})(); +""" + +/** + * Turns what [PAGE_START_ANCHOR_JS] returned into the shape Readium restores from: a + * `cssSelector` under `locations`, and the element's text as `text.highlight`. + * + * Both fields matter and they do different jobs. Readium resolves this pair by looking the + * selector up to get a root and then matching the text inside it, so the selector is what makes + * it exact and the text is what makes `R2EpubPageFragment.loadLocator` choose that path at all — + * it only calls `scrollToLocator` when `text.highlight` is set. + * + * [href] and [mediaType] are the caller's, matching how Readium stamps its own + * `firstVisibleElementLocator()`: the script has no idea which resource it is running in. + * Returns `null` for the script's own `null`, for anything unparseable, and for a blank + * selector or text, none of which Readium could resolve. + */ +fun parsePageStartAnchor( + json: String?, + href: Url, + mediaType: MediaType, +): Locator? { + val raw = json?.trim()?.takeUnless { it.isEmpty() || it == "null" } ?: return null + val obj = runCatching { JSONObject(raw) }.getOrNull() ?: return null + val selector = obj.optString("cssSelector").takeUnless { it.isBlank() } ?: return null + val text = obj.optString("text").takeUnless { it.isBlank() } ?: return null + return Locator( + href = href, + mediaType = mediaType, + locations = Locator.Locations(otherLocations = mapOf("cssSelector" to selector)), + text = Locator.Text(highlight = text), + ) +} diff --git a/reader/src/main/java/io/theficos/ereader/reader/ResizeAnchor.kt b/reader/src/main/java/io/theficos/ereader/reader/ResizeAnchor.kt new file mode 100644 index 0000000..c421f12 --- /dev/null +++ b/reader/src/main/java/io/theficos/ereader/reader/ResizeAnchor.kt @@ -0,0 +1,46 @@ +package io.theficos.ereader.reader + +import org.readium.r2.shared.publication.Locator + +/** + * Builds the locator the reader goes back to after its viewport changes. + * + * Readium restores a locator in `R2EpubPageFragment.loadLocator()`, and what it does there + * depends entirely on what the locator carries: + * + * 1. `text.highlight != null` — `scrollToLocator()`: look the element up in the DOM by its + * CSS selector and scroll its box into view. Independent of how the text is paginated. + * 2. else `locations.htmlId` — `scrollToId()`: the same idea, by element id. + * 3. else — `item = round(progression * numPages)`, then `setCurrentItem(item)`. + * + * What Readium publishes on `currentLocator` never has the first two. It builds that locator + * from the EPUB positions service, which emits `Locations(progression, position)` and nothing + * else, so re-anchoring on it always took the third path: a fraction mapped onto a page grid. + * Rotation rebuilds that grid (in the fixture book, 12 portrait pages against 23 landscape + * ones) and the arithmetic runs while it is still being rebuilt, so the page it lands on is + * only loosely related to the page it left. Worse, Readium then publishes wherever it landed, + * that becomes the anchor for the next rotation, and the error walks: a real trace went + * paragraph 18 to 31 to 38 over three rotate-and-return trips. + * + * [dom] is where the page begins in the document, from [PAGE_START_ANCHOR_JS]: a `cssSelector` + * and the element's text, and no progression whatsoever. Handing that to Readium on its own + * would trade one bug for a worse one, because if the selector ever failed to resolve Readium + * would fall back to `progression ?: 0.0` and jump to the top of the chapter, and because the + * progress row and the HUD percentage both read their numbers off this same locator. + * + * So keep both. The DOM fields decide where to land; [live] keeps the fallback honest and the + * percentages unchanged. Falls back to [live] untouched whenever [dom] cannot be trusted: + * absent, textless (nothing for `scrollToLocator` to match), or pointing at another resource, + * which would be the dangerous case — selectors here are positional (`:nth-child(19)`), so one + * from a different chapter resolves happily against the wrong paragraph. + */ +fun resizeAnchor(live: Locator, dom: Locator?): Locator { + if (dom == null || dom.text.highlight.isNullOrBlank()) return live + if (dom.href != live.href) return live + return live.copy( + locations = live.locations.copy( + otherLocations = live.locations.otherLocations + dom.locations.otherLocations, + ), + text = dom.text, + ) +} diff --git a/reader/src/test/java/io/theficos/ereader/reader/PageStartAnchorTest.kt b/reader/src/test/java/io/theficos/ereader/reader/PageStartAnchorTest.kt new file mode 100644 index 0000000..8d56e84 --- /dev/null +++ b/reader/src/test/java/io/theficos/ereader/reader/PageStartAnchorTest.kt @@ -0,0 +1,64 @@ +package io.theficos.ereader.reader + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.readium.r2.shared.util.Url +import org.readium.r2.shared.util.mediatype.MediaType +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * Reading back what [PAGE_START_ANCHOR_JS] found. + * + * The script itself runs in Readium's WebView and is verified on a device; this covers the + * Kotlin side of the boundary, which has to survive the script declining to answer — it returns + * `null` for a page nothing starts on, and for layouts it bows out of — without turning that + * into an anchor Readium cannot resolve. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class PageStartAnchorTest { + + private val chapter = Url("ch1.xhtml")!! + + private fun parse(json: String?) = parsePageStartAnchor(json, chapter, MediaType.XHTML) + + @Test fun `builds the locator shape Readium restores from`() { + val anchor = parse( + """{"cssSelector":":root > :nth-child(2) > :nth-child(23)","text":"Paragraph 22."}""" + ) + + assertThat(anchor).isNotNull() + assertThat(anchor!!.locations.otherLocations["cssSelector"]) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + assertThat(anchor.text.highlight).isEqualTo("Paragraph 22.") + // The script has no idea which resource it is running in; the caller supplies that, and + // it is what later lets a stale anchor from another chapter be rejected. + assertThat(anchor.href).isEqualTo(chapter) + assertThat(anchor.mediaType).isEqualTo(MediaType.XHTML) + } + + @Test fun `declines the script's own null`() { + // What the script returns for a page with nothing starting on it, and for a scrolled or + // right-to-left layout. The caller falls back to Readium's own answer. + assertThat(parse("null")).isNull() + assertThat(parse(null)).isNull() + assertThat(parse("")).isNull() + assertThat(parse(" ")).isNull() + } + + @Test fun `declines anything unparseable rather than throwing`() { + assertThat(parse("{oops")).isNull() + assertThat(parse("\"a string\"")).isNull() + } + + @Test fun `declines a half-built anchor`() { + // Readium needs both halves: no selector means it would search the whole document for + // the text, no text means it never takes the precise path at all. + assertThat(parse("""{"text":"Paragraph 22."}""")).isNull() + assertThat(parse("""{"cssSelector":":root > :nth-child(2)"}""")).isNull() + assertThat(parse("""{"cssSelector":"","text":"Paragraph 22."}""")).isNull() + assertThat(parse("""{"cssSelector":":root","text":" "}""")).isNull() + } +} diff --git a/reader/src/test/java/io/theficos/ereader/reader/ResizeAnchorTest.kt b/reader/src/test/java/io/theficos/ereader/reader/ResizeAnchorTest.kt new file mode 100644 index 0000000..a983a7e --- /dev/null +++ b/reader/src/test/java/io/theficos/ereader/reader/ResizeAnchorTest.kt @@ -0,0 +1,91 @@ +package io.theficos.ereader.reader + +import com.google.common.truth.Truth.assertThat +import org.junit.Test +import org.junit.runner.RunWith +import org.readium.r2.shared.publication.Locator +import org.readium.r2.shared.util.Url +import org.readium.r2.shared.util.mediatype.MediaType +import org.robolectric.RobolectricTestRunner +import org.robolectric.annotation.Config + +/** + * What the reader goes back to after its viewport changes. + * + * The stakes are in [resizeAnchor]'s own documentation: Readium only restores a locator + * precisely when it carries `text.highlight`, and the locator Readium itself publishes never + * does, so a locator that reaches Readium without those fields is restored by mapping a + * fraction onto a page grid — the behaviour that walked the reader 20 paragraphs down the + * chapter over three rotations. + */ +@RunWith(RobolectricTestRunner::class) +@Config(sdk = [33]) +class ResizeAnchorTest { + + private val chapter = Url("ch1.xhtml")!! + + private fun live(progression: Double = 0.42) = Locator( + href = chapter, + mediaType = MediaType.XHTML, + title = "Chapter 1", + locations = Locator.Locations( + progression = progression, + position = 9, + totalProgression = 0.084, + ), + ) + + private fun dom( + href: Url = chapter, + selector: String? = ":root > :nth-child(2) > :nth-child(23)", + highlight: String? = "Chapter 1 paragraph 22 sentence 1", + ) = Locator( + href = href, + mediaType = MediaType.XHTML, + locations = Locator.Locations( + otherLocations = selector?.let { mapOf("cssSelector" to it) } ?: emptyMap(), + ), + text = Locator.Text(highlight = highlight), + ) + + @Test fun `carries the DOM fields Readium needs to restore precisely`() { + val anchor = resizeAnchor(live(), dom()) + + // Both are load-bearing: the highlight is what makes Readium choose scrollToLocator at + // all, and the selector is what makes that landing exact. + assertThat(anchor.text.highlight).isEqualTo("Chapter 1 paragraph 22 sentence 1") + assertThat(anchor.locations.otherLocations["cssSelector"]) + .isEqualTo(":root > :nth-child(2) > :nth-child(23)") + } + + @Test fun `keeps the live progression, so the fallback and the percentages still work`() { + val anchor = resizeAnchor(live(progression = 0.42), dom()) + + // If the selector ever fails to resolve, Readium falls back to `progression ?: 0.0` and + // would otherwise jump to the top of the chapter. These same fields are what the HUD + // percentage and the saved progress row are read from. + assertThat(anchor.locations.progression).isEqualTo(0.42) + assertThat(anchor.locations.totalProgression).isEqualTo(0.084) + assertThat(anchor.locations.position).isEqualTo(9) + assertThat(anchor.href).isEqualTo(chapter) + assertThat(anchor.title).isEqualTo("Chapter 1") + } + + @Test fun `falls back to the live locator when there is no DOM anchor`() { + assertThat(resizeAnchor(live(), null)).isEqualTo(live()) + } + + @Test fun `falls back when the DOM anchor has no text for Readium to match`() { + // Without a highlight Readium never reaches scrollToLocator, so a selector alone would + // be dead weight on the locator. + assertThat(resizeAnchor(live(), dom(highlight = null))).isEqualTo(live()) + assertThat(resizeAnchor(live(), dom(highlight = " "))).isEqualTo(live()) + } + + @Test fun `refuses a DOM anchor from another resource`() { + // The dangerous case. These selectors are positional, so one captured in a different + // chapter resolves perfectly happily against whatever paragraph sits at that index here. + val strayChapter = resizeAnchor(live(), dom(href = Url("ch2.xhtml")!!)) + assertThat(strayChapter).isEqualTo(live()) + } +}