fix(reader): give the page back its top and bottom margins - #98
Merged
Conversation
The text has been running the full height of the screen since the last release, and on a phone with a camera in the display it runs straight under it — the reporter has been tilting their Pixel 8 Pro on every page to read the first couple of sentences. Readium applies the page's vertical margins and the display-cutout clearance in the same pass, and that pass only ever ran from the window-insets listener it registers when `shouldApplyInsetsPadding` is on. Turning that flag off to stop it re-paginating the chapter under the reader (#95) therefore took the margins with it, not just the late resize. Nothing else was applying either, so the page went edge to edge: on an emulator with a punch-hole the WebView measured [0,0][1080,2424], where the same build before #95 settles at [0,241][1080,2319] — 136px of camera plus 105px of margin at the top, 105px at the bottom. Quire already owns the reader's insets, so it now owns the page's margins too. ReaderPageArea holds the text clear of `WindowInsets.displayCutout` — Android's own per-device, per-orientation answer to what is covering the screen, so a punch-hole, a notch or nothing at all all come through the same value with no device list to keep current — and adds Readium's own 40dp (20dp landscape) on top of it. Asking for the cutout unconditionally is safe because `windowInsetsPadding` subtracts what an ancestor already consumed: with full-screen reading off the subtree is padded by the system bars, which in portrait cover the cutout, and the page comes away with nothing left to clear rather than inset twice. Two things fall out of putting the margins here rather than in Readium's view tree. The size that is left is measured by the same `onSizeChanged` the re-anchor machinery keys on, so insets arriving a frame late are just another viewport change and the reader keeps its place across them — which is what #95 could not do. And the strip the padding exposes is Quire's to paint, so ReaderTheme now carries the colour ReadiumCSS paints the page with and the margin reads as part of the page instead of a band around it; ReaderThemeTest pins those against Readium's own values so an upgrade that repaints a theme fails there rather than on screen. Verified on an emulator with a punch-hole cutout, against the book from #95. Portrait [0,241][1080,2319] and landscape [136,53][2424,1027] — the camera moves to the left edge on rotation and the page moves with it. Three background-and-resume cycles and three rotate-and-return trips leave the portrait screenshot byte-identical, so #95 stays fixed. With full-screen reading off the page sits at [0,242][1080,2256]: status bar plus margin, the cutout adding nothing. Dark theme samples black in the margin as well as the page. Closes #97
11 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The reader's text has been running the full height of the screen since v2026.08.18.227, and on a phone with a camera in the display it runs straight under it. Readium applies the page's vertical margins and the display-cutout clearance in the same pass, and that pass only ever ran from the window-insets listener it registers when
shouldApplyInsetsPaddingis on, so turning that flag off in #95 took the margins with it and not just the late resize. Quire applies both itself now, from the same place it already measures the viewport, so the page keeps its margins without losing the reader's place when the insets arrive a frame late.Component(s)
What changed
ReaderPageAreaholds the text clear ofWindowInsets.displayCutoutand adds Readium's own 40dp (20dp landscape) on top. Using the cutout inset means there is no device list to keep current: a punch-hole, a notch, or nothing at all all come back through the same per-device, per-orientation value. Asking for it unconditionally is safe becausewindowInsetsPaddingsubtracts what an ancestor already consumed, so with full-screen reading off — where the subtree is already padded by the system bars, which in portrait cover the cutout — the page comes away with nothing left to clear rather than inset twice.Two things follow from owning the margins here rather than in Readium's view tree:
onSizeChangedthe re-anchor machinery keys on, so insets arriving late are just another viewport change. That is the failure mode Fullscreen readin mode #95 could not cover, because it happened inside Readium's own views where nothing here could see it.ReaderThemenow carries the colour ReadiumCSS paints the page with and the margin reads as part of the page instead of a band around it.Verification
Emulator with a punch-hole cutout enabled, on the marker book from #95. WebView bounds, screen 1080x2424 at 420dpi:
[0,0][1080,2424][0,241][1080,2319][136,53][2424,1027][0,242][1080,2256]241px = 136 of camera + 105 of margin. In landscape the camera moves to the left edge and the page moves with it. With full-screen reading off it is status bar plus margin, the cutout adding nothing.
Three background-and-resume cycles and three rotate-and-return trips leave the portrait screenshot byte-identical, so #95 stays fixed. Dark theme samples
#000000in the margin as well as in the page.For reference, the same build at the commit before #95 settles at
[0,241][1080,2319]too, but only after the first refocus — which is the resize that made it lose the reader's place.Checklist
core/identitychanged: matching change on the other side + fixtures updated (N/A)Tests: three new
ReaderPageAreaTestcases covering cutout clearance, margin with nothing to clear, and an ancestor's inset not being applied twice; plus aReaderThemeTestguard pinning each theme's page colour against Readium's own, so a Readium upgrade that repaints a theme fails there rather than on screen. Full suite green (166 app tests, reader module tests).Related issues
Closes #97, refs #95