BL-16627: improve BloomBridge process-book fix-up: auto-fit origami splits (both directions, nested stacks) and shrink oversized images - #8129
Closed
hatton wants to merge 5 commits into
Closed
Conversation
fitImageOverTextSplitOnPage() computed the right split and then discarded it whenever applying it would make the image pane smaller. On a page whose illustration is wider than 4:3, the image pane is bigger than the picture needs, the surplus is dead space, and the text below it gets clipped -- even though the page has room to spare. Measured case (A5Portrait, marginBox 469x703): the binary search found the text needs 54%, the image-fit cap found the image needs only 31% (so the text could safely have 69%), finalTextPercent landed on 69 -- and the only-grow guard threw it away because 69 >= the authored 50. The text overflowed its box by 26px (~1.1 lines) while 135px of the image pane stood empty. Replace that guard with a small deadband so the divider moves whenever it is worth moving in either direction. Growing the text pane up to textFloorForImageFit costs the illustration nothing -- past that point the image is width-limited and renders at the same size either way. Deliberately no cap at the authored split: when the text needs more than the image's fill point, the image really does get smaller, because clipped text is a failure and a smaller picture is a trade. The existing early-out still leaves a genuinely over-full page (text overflowing even at 90%) untouched. Checked before trusting a two-directional fit: computeImageFitFirstPanePercent is stable (spread 0.0 points across pane sizes from 10% to 90% on every qualifying page of a real book), so removing the one-way ratchet can't cause oscillation. Over that book, the old version moved 0 pages and left one overflowing; the new version moved 8, left alone the one page whose image genuinely wants 49.8%, and no page overflows afterwards. Also document the trap that made the image-fit cap useless on import: getImageAspectRatio() reads the .bloom-backgroundImage box, which on a fresh import is whatever the importer wrote (BloomBridge writes the full pane rect), so we were reading the pane's aspect rather than the picture's. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…o-pane pages
fitImageOverTextSplitOnPage() bailed on any nested split ("too complex, skip"),
which excluded one of the most common page shapes there is: text above, an
illustration, text below. Nothing ever chose those dividers on purpose. An
importer emits the shape as a right-nested chain of horizontal splits, and with
no explicit percentages Bloom's 50/50-per-split CSS default cascades into an
effective 50/25/25 -- so the top text block gets half the page for its one line,
the illustration is squeezed into a quarter, and the last (usually longest)
block is clipped. Auto-fit is precisely the code that should fix that, and it
declined to look.
Measured on FUV_Taale_Taale_Fulbe (Fulfulde, Nigeria, A5Portrait, marginBox
469x703), where 9 of the 11 content pages are this shape: every one sat at
351.5/175.75/175.75 px and 9 pages had text overflowing its box. After this
change 0 of them do. Page 8 goes from 352/176/176 (bottom text clipped
mid-line) to 154/325/224 -- the illustration nearly doubles and all the text
fits, which is about the proportion the printed source page uses.
Recognizing the shape: collectSplitStack() walks splits of ONE axis chained
down their second panes, each contributing its first pane as a slot and the
innermost contributing its second. Anything else nested is still declined -- a
split inside a FIRST pane, mixed axes, a second pane holding a split alongside
other content, a pane we can't classify as just-a-picture or just-text, an
illustration carrying overlays, or more than one illustration.
Fitting it: what makes a top/bottom stack tractable is that every pane in it is
the same WIDTH, so a text block's required height does not depend on where any
divider sits. Each text pane can therefore be measured on its own -- give it a
size, ask OverflowChecker whether it overflows, binary-search the boundary --
and the answers simply add up. Still measured, never estimated, same as the
two-pane path. The illustration takes what the text doesn't need, capped at the
size where it already fills the width; past that a taller pane is pure
whitespace, so the surplus goes back to the text panes in proportion to what
each needed. Left/right stacks are deliberately excluded: there the panes have
different widths, so the panes aren't independent and the one-at-a-time
measurement doesn't hold.
Governing rules are unchanged. A page whose text overflows even with nearly the
whole page to itself, or whose blocks together need more than the page has, is
over-full: there is no arrangement of these dividers that works, so we leave it
exactly as authored rather than producing something both clipped AND ugly.
Two details worth knowing:
- The illustration's fill-point cap is measured up front, while the stack is
still laid out as it was saved. The probing deliberately squeezes whichever
panes it isn't testing, including that one, so measuring later would read the
picture's chrome out of a pane collapsed to its minimum.
- A page we end up declining is restored from its saved style attributes rather
than by re-deriving percentages, so a pane that had no style keeps having
none. Re-deriving would stamp an explicit "50%" onto pages we chose not to
touch and put them in the saved HTML's diff for nothing.
Also de-duplicates the divider-writing and text-overflow helpers, which the
two-pane path now shares (no behavior change there).
The new spec covers shape recognition and the nested percentage arithmetic --
including that unset splits read as 50/25/25 rather than thirds, which is the
bug in one assertion. The measuring itself needs real geometry, which jsdom
does not provide (OverflowSpec is skipped there for the same reason), so it was
verified against this real book in Chromium instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Devin flagged (informational) an asymmetry the nested-stack work introduced: the new stack path restores a declined page from its saved style attributes, while the two-pane path still wrote `setTextPercent(originalTextPercent)`. Those look equivalent but aren't. Measuring lays the split out repeatedly, so by the time either path decides a page is out of reach the styles have been rewritten; on a split with no explicit percentage -- the common case, defaulting to 50% -- the write-back leaves an explicit "50%" behind on a page we deliberately chose not to touch, putting it in the saved HTML's diff for nothing. Generalize the capture/restore helpers to take SplitConfig[] and use them in both of the two-pane decline paths (over-full page, and move-not-worth-making). No layout change: the restored geometry is identical either way, and process-book just stops rewriting pages it left alone. Also record, at computeImageFitPaneSizePx, why only the chrome term is scale-divided (Devin's second informational note): it matches computeImageFitFirstPanePercent and the reference getImagePercent() in lib/split-pane/split-pane.ts, so changing it here alone would put this out of step with the re-fit that runs afterward. Scale is 1 in the off-screen processor, and at any other scale it can only skew the whitespace cap, never the no-overflow guarantee, which comes from measuring real text. Re-verified against FUV_Taale_Taale_Fulbe in Chromium: same fitted geometry as before, 9 pages overflowing before the feature and 0 after (the one remaining is a text/text page with no illustration, out of scope and genuinely over-full). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… does Three comment-only corrections, no behavior change. Two of them were saying things that are no longer true now that stacks are handled, and a reader who trusted them would be misled about the feature's scope: - captureContentForExternalProcessing (bloomEditing.ts) and ProcessBook's <summary> (BookProcessor.cs) both still described the feature as two-pane only, image-in-the-first-pane, and grow-only. Both now describe what it actually does: two-pane image-above-text and image-left-of-text, PLUS top-to-bottom stacks of three or more panes with one illustration and text in the rest, with each divider moving in either direction. - getImageAspectRatio's caveat asserted that BloomBridge "emits the background element at the full PANE rect", which measurement contradicts: on this book it writes the pane's height by the picture's proportional width, giving an aspect of 0.7869 against the picture's own 975x1239 = 0.7869 exactly. So the caveat named the wrong culprit and implied the image-fit cap is useless on import when it isn't. Kept the warning -- an importer that DOES size the box to the pane would cost us whitespace, and that is worth knowing -- but stated it generically and recorded what BloomBridge was actually measured to do, so nobody plans around a false specific. Also note in classifySlot why the overlay check is not redundant with the `!textGroup` test that precedes it: that test already excludes text bubbles, so the overlay check is what excludes image/video overlays. Devin flagged the pair as apparently-redundant, which is exactly the misreading that would lead someone to delete the check. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Member
Author
|
[Claude Opus 5 from Hatton's machine during preflight] Consulted Devin on 2026-07-29, up to commit Devin ran on each of the three pushed commits and I waited for the job matching that exact sha
The rest were confirmations rather than concerns — notably that the leftover-redistribution and Also green at this commit: CI ( Full preflight report, including the before/after page geometry and a few decisions for the |
hatton
changed the title
Member
Author
|
[Claude Fable 5 following a prompt from Hatton] Superseded by #8135: the branch was renamed from BL-16627-shrink-image-pane to BL-16627-process-book to match the card's broadened scope (the work now covers more of the process-book fix-up than the image-pane split), and a branch rename closes the PR that pointed at the old name. Same commits plus the new image-shrink step continue there. |
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.
Auto-fit of image/text origami splits gets two related improvements. Both are about the same
governing rules: never leave text overflowing, never waste space to no one's benefit, and always
measure the real browser layout rather than estimating font sizes.
1. Let auto-fit shrink the image pane, not just grow it (
7bea4c7)fitImageOverTextSplitOnPage()computed the right split and then discarded it whenever applying itwould make the image pane smaller. On a page whose illustration is wider than 4:3 the image pane is
bigger than the picture needs, the surplus is dead space, and the text below gets clipped — even
though the page has room to spare. Replaced the one-way ratchet with a small deadband so the divider
moves whenever it is worth moving in either direction.
2. Handle nested text/image/text stacks (
e455270)The function bailed on any nested split (
// nested split: too complex, skip), which excluded one ofthe most common page shapes there is: text above, an illustration, text below.
Nothing ever chose those dividers on purpose. An importer emits the shape as a right-nested chain of
horizontal splits, and with no explicit percentages Bloom's 50/50-per-split CSS default cascades into
an effective 50/25/25 — so the top text block gets half the page for its one line, the
illustration is squeezed into a quarter, and the last (usually longest) block is clipped.
What makes a top/bottom stack tractable is that every pane in it has the same width, so a text
block's required height doesn't depend on where any divider sits. Each text pane can therefore be
measured on its own — give it a size, ask
OverflowCheckerwhether it overflows, binary-search theboundary — and the answers add up. The illustration takes what the text doesn't need, capped at the
size where it already fills the width; the surplus past that goes back to the text panes in
proportion to what each needed.
Left/right stacks are deliberately excluded: there the panes have different widths, so they aren't
independent and the one-at-a-time measurement doesn't hold.
Still declined, as before: a split inside a first pane, mixed axes, a second pane holding a split
alongside other content, a pane that isn't cleanly just-a-picture or just-text, an illustration
carrying overlays, more than one illustration, and genuinely over-full pages (text that overflows
even with nearly the whole page, or blocks that together need more than the page has).
Measured result
FUV_Taale_Taale_Fulbe(Fulfulde, Nigeria; A5Portrait, marginBox 469x703), where 9 of the 11content pages are this shape. Every one sat at 351.5/175.75/175.75 px, and 9 pages had text
overflowing. Afterwards none do.
Page 15 is text/text with no illustration, so it is out of scope and correctly left untouched (it is
genuinely over-full).
Testing
autoFitImageOverTextSplitsSpec.tscovers shape recognition and the nested percentage arithmetic —including that unset splits read as 50/25/25 rather than thirds, which is the bug captured as an
assertion.
The measuring itself needs real geometry, which jsdom does not provide (
OverflowSpecis skippedthere for the same reason). That half was verified by loading the real saved book, with its real
Bloom CSS, in Chromium and running
fitImageOverTextSplits()against it — that is where the numbersabove come from.
Ref: https://issues.bloomlibrary.org/youtrack/issue/BL-16627
🤖 Generated with Claude Code
Devin review
This change is