fix: balance multi-column sections at continuous section breaks (SD-3359)#3638
Open
tupizz wants to merge 10 commits into
Open
fix: balance multi-column sections at continuous section breaks (SD-3359)#3638tupizz wants to merge 10 commits into
tupizz wants to merge 10 commits into
Conversation
balanceSectionOnPage skipped every section with equalWidth=false plus explicit widths, so continuous newspaper sections declared as <w:cols w:num=N w:equalWidth=0> with equal <w:col w:w> children (the common case) never balanced and rendered single-column. Narrow the skip to GENUINELY-unequal widths: explicit widths that are all equal now balance like implicit equal columns. Genuinely-unequal widths still fill column-by-column (Word parity, unchanged). (SD-2324)
Per ECMA-376 §17.6.4, when columns are not equal width (w:equalWidth=0) the section-level w:cols/@w:space is ignored and the inter-column gap comes from each <w:col w:space>. extractColumns used the section space, over-spacing explicit columns so their widths scaled down to fit and diverged from Word (e.g. the 2002 ISDA sections). Use the per-column w:space for unequal columns; equal-width columns keep the section space. Advances SD-2629 for the uniform-spacing case. (SD-2324)
…ith-explicit-column
…2324) Equal-width sections (w:equalWidth="1" or omitted) now match Word: extraction drops child <w:col> widths and takes the gap from the section w:space (default 720), and normalizeColumnLayout honours per-column widths only when w:equalWidth="0". For explicit columns (w:equalWidth="0"), cap the count to min(w:num, valid child-width count) at the source, so a w:num larger than the provided <w:col> widths no longer creates surplus 1px phantom columns in the fill loop (which reads the raw count). A matching clamp in normalizeColumnLayout stays as a defensive net.
…ent w:cols (SD-2324) Adds three extraction unit tests for the landed column fix: count caps to the valid child-width count (four <w:col> but two usable w:w -> 2), equal mode takes the count from w:num (count 3, no children), and a section without <w:cols> yields no columnsPx.
A two-column section ending at a continuous section break rendered with lumpy columns (SD-3359, IT-1150): on the repro NDA the left column ran 169px deeper than the right, and the following single-column content started below the unbalanced overhang. Three defects, all in the balancing path: - balanceSectionOnPage fed the balancer atomic fragments (canBreak: false, no lineHeights), so a paragraph straddling the column boundary could not split. The balancer already computes line-level break points (blockBreakPoints) with widow/orphan control, but no caller consumed them. Paragraph fragments now opt in with their per-line heights, and a chosen break is applied as fragment surgery: the first half keeps the leading lines, a cloned second half carries the remaining lines to the top of the next column, the same fromLine/toLine and continuation model pagination uses. Paragraphs with w:keepLines (ECMA-376 17.3.1.14) and sectPr marker paragraphs stay atomic. - The binary search floored its target at the tallest block's full height. For a breakable paragraph the indivisible chunk is its tallest line, so the search could never reach the truly balanced height and packed the overflow lines into column 0. - The post-layout gate keyed "mid-doc continuous" off the section's own begin type (its sectPr w:type, 17.6.22) instead of the type of the break that ends it, which is the NEXT section's begin type. A 2-col section that merely started continuous balanced even when ended by a nextPage break; per the 17.18.77 note only a continuous break balances the previous section. Verified against the IT-1150 repro (169px -> 14px imbalance, trailing paragraph tucked directly below the balanced region) and eleven spec-derived document mutations (3 columns, explicit equal and unequal w:col, keepLines, explicit column break, nextPage/evenPage/nextColumn end breaks, w:sep, implicit and explicit body sectPr): all conform to ECMA-376 and the documented Word behaviors (sd-1655, sd-1480, mixed-columns-tabs-tnr). Adds 4 unit tests and 2 layoutDocument integration tests.
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
Base automatically changed from
tadeu/sd-2324-feature-render-multi-columns-with-explicit-column
to
main
June 5, 2026 12:31
…balanced-before-a-continuous
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30742a8d5b
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Comment on lines
+869
to
+872
| const secondHalf = { | ||
| ...f, | ||
| fromLine: splitLine, | ||
| toLine: originalToLine, |
There was a problem hiding this comment.
When this splits a paragraph fragment that was remeasured for a narrower column or floats, ...f copies the fragment’s full lines override onto the second half, and the first half keeps the same override after only toLine is changed. renderParagraphFragment prefers fragment.lines over measure.lines.slice(fromLine, toLine), so in real multi-column layouts that set fragment.lines both halves can render the entire pre-split paragraph instead of their respective line ranges, duplicating text across columns. The split needs to clear or partition lines alongside fromLine/toLine.
Useful? React with 👍 / 👎.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…balanced-before-a-continuous
…lumns Review follow-up. A paragraph fragment remeasured for a narrower column (or beside a float) carries its own `lines` array, and resolveParagraph renders that array INSTEAD of slicing measure.lines by fromLine/toLine. The column split cloned the fragment wholesale, so both halves kept the full remeasured array and each column rendered the entire paragraph. Three coordinated corrections: - The split slices `lines` across the halves (first keeps the leading slice, the clone carries the rest), so each column renders only its own lines. - Break points are computed against the fragment's own remeasured line heights when present; measure.lines describes the original width and can disagree in both count and height. - getFragmentHeight sums fragment.lines when present, matching how resolveLayout sizes such fragments, so balancing cursors agree with what the resolve stage actually renders. Adds a regression test with a remeasured fragment (22px lines vs a stale 20px measure) asserting the halves partition the lines and the column cursors advance by the remeasured heights.
…balanced-before-a-continuous
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
4 participants
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
A two-column section ending at a continuous section break rendered with lumpy columns. On the IT-1150 repro NDA the left column ran 169px deeper than the right, and the trailing single-column paragraph started below the unbalanced overhang. Word balances the section: per the ECMA-376 17.18.77 note, "a continuous section break balances the content of the previous section", defined as "starting the next section at the minimum section height such that all content constraints are met".
Linear: SD-3359
Root cause
Mid-page balancing infrastructure already existed and fired for this repro. Tracing showed three defects in the balancing path itself:
balanceSectionOnPagefed the balancer atomic fragments (canBreak: false, nolineHeights). The core algorithm already computes line-level break points with widow/orphan control (blockBreakPoints), but no caller ever consumed them. A 273px paragraph straddling the column boundary could not split, forcing a 169px imbalance.w:type, 17.6.22) instead of the break that ends it (the next section's begin type). A 2-col section that merely started continuous balanced even when ended by a nextPage break.Fix
fromLine..toLine. A chosen break is applied as fragment surgery: the first half keeps the leading lines, a cloned second half carries the rest to the top of the next column, the samefromLine/toLineand continuation-flag model pagination uses.w:keepLinesparagraphs (17.3.1.14) and sectPr marker paragraphs stay atomic.keepLinesBlockIdsis threaded from the layout walk into both balancing call sites.sectionEndBreakTypethat caused the off-by-one is corrected.Spec conformance matrix
Verified in the browser against the repro plus eleven mutations derived from ECMA-376:
Known pre-existing gap surfaced by the sweep, out of scope here: a
nextColumnbreak starts the next section on a new page instead of the following column.Test plan
balanceSectionOnPage(straddle split with line partition, keepLines, single tall paragraph, fromLine offset for pagination-split fragments), written failing firstlayoutDocumentintegration tests (straddle split end-to-end geometry; nextPage end break does not balance)