fix: render multi-column sections with explicit column definitions (SD-2324)#3618
fix: render multi-column sections with explicit column definitions (SD-2324)#3618tupizz wants to merge 5 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)
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
![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: 5b96267df2
ℹ️ 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".
| const firstChildSpace = columnChildren.find((child) => child?.attributes?.['w:space'] != null)?.attributes?.[ | ||
| 'w:space' | ||
| ]; | ||
| const gapTwips = equalWidth === false ? (firstChildSpace ?? 0) : (cols.attributes['w:space'] ?? firstChildSpace); |
There was a problem hiding this comment.
When a DOCX has explicit <w:col> children but omits w:equalWidth, ECMA-376 §17.6.4 treats the columns as not equal-width: the section-level w:cols/@w:space is ignored and spacing comes from the child <w:col> elements. This branch only applies that rule for equalWidth === false, so valid files that omit the attribute still use cols.attributes['w:space'], over-spacing the columns and scaling their explicit widths down—the same rendering issue this change is trying to fix.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
cubic analysis
No issues found across 4 files
Linked issue analysis
Linked issue: SD-2324: Feature: Render Multi-columns with explicit column definitions
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Preserve and render section-level explicit column definitions (including explicit equal-width continuous columns) | Code now treats explicit-but-equal column widths as balanceable; unit test added to assert balanced placement across columns. |
| ✅ | Honor per-column w:space for unequal columns so inter-column gaps match Word (ECMA-376 §17.6.4) | Extraction now uses per-column space for unequal columns and a unit test verifies the gap computation. |
| ✅ | Fix the reported bug where content rendered only in the left column for the ISDA document | Browser verification and before/after screenshots show the ISDA section flows across multiple columns rather than all content in the left column. |
| Validate behavior against both the full customer-reported document and targeted multi-column fixtures | Browser verification across the listed fixtures (including the full customer doc) is reported and unit tests added, but the broader pnpm test:layout (562-doc corpus) could not be run locally and remains to be exercised in CI. | |
| ✅ | Do not regress existing equal-width multi-column section rendering | Regression cases for equal-width columns remain unchanged and unit tests covering those cases pass. |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…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.
|
hey @tupizz, I checked this more deeply before adding to the branch. Your fix for
I verified this with small Word fixtures, then traced how SuperDoc reads and lays out the columns. |
…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.
|
FYI, PR #3618 is fixing the reported explicit column spacing issue. While validating it against Word, we found a few related column cases that need a broader follow-up. I opened SD-2629 for those: mixed column gaps, explicit widths staying as written, and making all column placement use the same source of truth. |
4 participants
Summary
DOCX sections that define explicit columns (
<w:cols w:num="N" w:equalWidth="0">with<w:col>children) rendered all content in the left column instead of flowing across the columns like Word. The reported 2002 ISDA Master Agreement and the targeted fixtures are affected. Two fixes, one per layer:Balance explicit equal-width continuous columns
fix(layout-engine)balanceSectionOnPageskipped everyequalWidth=falsesection that had explicit widths, so continuous newspaper sections whose<w:col w:w>children are all equal (the common case, e.g. 4×2340) never balanced and stayed single-column. The skip is now narrowed to GENUINELY-unequal widths; explicit-but-equal widths balance like implicit equal columns. Genuinely-unequal widths still fill column-by-column (Word parity, unchanged).Honor per-column
w:spacefor unequal columns (ECMA-376 §17.6.4)fix(layout-adapter)Per §17.6.4, when columns are not equal width the section-level
w:cols/@w:spaceis ignored and the inter-column gap comes from each<w:col w:space>.extractColumnsused the section space, over-spacing explicit columns so their widths scaled down to fit and diverged from Word. It now uses the per-column space for unequal columns; equal-width columns keep the section space. (Advances SD-2629 for the uniform-spacing case.)Builds on SD-2452 (continuous-break balancing, already shipped). Part of the SD-2549 multi-column epic. Both call sites that reach
balanceSectionOnPageare already continuous-scoped, so nextPage/body unequal sections are unaffected.Verification
OOXML-spec-grounded (§17.6.4 column definitions; §17.18.77 continuous balancing). Verified in the browser across the issue's fixtures (distinct fragment left-x = columns):
Test plan
pnpm test:layout(562-doc corpus): could not run locally (the R2/Wrangler corpus token is expired); needs CI ornpx wrangler login. This is the key regression gate for column/pagination changes.Notes
<w:col w:w>) still fill column-by-column rather than rebalancing: the height-balancer measures each fragment at a single width and cannot reflow per column. No fixture exercises that case, so it is out of scope here.Linear: SD-2324 (parent SD-2549; related SD-2629)