fix(layout): give the footnote band its own column layout and separator placement - #3961
Open
Nathaniel-260 wants to merge 13 commits into
Open
fix(layout): give the footnote band its own column layout and separator placement#3961Nathaniel-260 wants to merge 13 commits into
Nathaniel-260 wants to merge 13 commits into
Conversation
A section carrying `w:sectPr/w:bidi` fills its columns left to right, so the first paragraph of a Hebrew two-column section lands in the LEFT column. Word puts it on the right (ECMA-376 §17.6.1), and `SectionDirectionContext` already documents `pageDirection` as governing columns -- but no function in the column geometry ever received a direction. `ColumnLayout` now carries an optional `direction`, and `buildColumnGeometry` mirrors the strip about the CONTENT AREA when it is `'rtl'`. Indices stay in fill order, so every consumer that walks columns 0..n-1 keeps filling in document order and only the painted x changes; fill, hit testing, separators, balancing, floating anchors and footnotes all follow from that single source. The mirror axis is the content area and not the strip's own span because explicit widths are not scaled to fill it -- a strip that underfills must end up against the right margin with the slack on the left. Four consumers needed direction awareness of their own, and each failed silently without it: - `getColumnAtX` walked the geometry assuming x ascends with the index. - `toBalancingColumns` rebuilt the layout field by field and dropped the axis, so the balanced last page of an RTL section laid out left to right while every earlier page of the same section laid out right to left. - Footnote column attribution broke on the first match under the same ascending assumption, collapsing a page's notes into column 0: the left column's notes printed under the right column and its own note area stayed empty. - The DOM painter's separator gate read "content past the separator" as "content to the right", so a section whose content never left the first column drew a line Word does not draw. Absent `direction`, every path is byte-identical to before: verified across 46,080 comparisons of geometry and hit testing over 960 LTR configurations and 6 content widths.
…s end to end Follow-up to the RTL column-order fix in this branch, addressing review feedback on superdoc#3953. `balanceSectionOnPage` reconstructed document order by sorting the page's fragments on ASCENDING x, on the premise that the paginator fills column 0 first. That premise inverts under this branch: in an RTL section column 0 is the RIGHT column, so document order DESCENDS in x. The balancer consumed the trailing column first and wrote the balanced x/y back in that order, which scrambles the reading order of a balanced page rather than merely mirroring it. Measured on a 2-column RTL page of 6 paragraphs: x came back as [432, 96, 96, 96, 432, 432] instead of [432, 432, 432, 96, 96, 96]. The sort is now direction-relative. The existing RTL balancing test could not catch this because its fixture places every fragment at the same x, which makes the ascending sort a stable no-op. Two smaller geometry corrections: - A single column is mirrored too. The old guard skipped `count < 2`, so an explicit one-column section that underfills the content area stayed pinned to the LEFT margin, contradicting the axis rule the multi-column path applies. It is a provable no-op whenever the column fills the area, so equal-mode `count: 1` is byte-identical. - Per-column `gaps` are clamped to >= 0, matching the scalar `gap` above. OOXML cannot express a negative gutter (`w:space` is unsigned), but a hand-built layout could, and a gap negative enough to pull a column behind its predecessor would make an upright LTR strip answer hit tests as if it were mirrored. `ColumnLayoutForAnchor` and `ParagraphAnchorsContext.columns` now declare `direction` and `contentWidth`. Runtime was already correct because every caller passes a full normalized layout, but neither would have produced a type error if a future edit dropped the fields -- the exact failure mode that made the `toBalancingColumns` fix necessary. Coverage. Three paths in the previous commit survived mutation: `toBalancingColumns` dropping both spreads, the footnote column boundary reverted to its LTR-only form, and `determineColumn` in position-hit, which had no RTL coverage at all. Each now has a test that fails without its fix, and position-hit also covers three columns, which nothing exercised before. `tests/src/test-helpers/to-flow-blocks.ts` reads `w:sectPr/w:bidi` (ST_OnOff, so a bare element means on) and sets `columns.direction`, which makes `section-breaks-rtl-columns.test.ts` an end-to-end check from OOXML section properties down to fragment x. This is a TEST adapter and does not reach real documents: the production PM/OOXML adapter is not in this repository. It does double as a precise reference for what that adapter must do. Adds a consumer-typecheck fixture for the new public `ColumnLayout.direction`, reachable from outside the package through `Layout.columns`.
…trails Review follow-up on superdoc#3953. The RTL branch of the separator gate tested the fragment's LEFT edge, the same edge the LTR branch tests, which leaves the two asymmetric for anything wider than a column. `page.items` carries anchored drawings alongside column content, so a page-relative watermark or logo sits at `x = 0` spanning the page. Going right it is never past the separator; going left, a left-edge test always puts it past. An RTL section with `w:sep="1"` whose text all fits in the first column therefore drew a separator on the strength of the watermark alone -- a line Word does not draw, which is exactly what this gate exists to prevent. Each branch now tests the edge that trails in its own fill direction: the left edge going right, the right edge going left.
… not by an edge The column-separator gate asks whether a LATER column holds content, because Word draws no line next to an empty column. It answered by comparing a fragment edge against the separator x, choosing whichever edge trails in the fill direction. No edge can answer that question. Content wider than its column does not sit inside it, and `resolveTableFrame` places an over-wide table at a NEGATIVE offset from its column whenever the table is right-aligned or centred -- and `end` is the default justification for any bidiVisual table. So in an RTL section a wide table starts left of its own column and ends past the separator, while never having left the later column: both of its edges lie on the wrong side, and so does its origin. A negative `w:ind` puts a paragraph's origin in the gutter with the same effect. Use `fragment.columnIndex` instead -- the engine's own record of the owning column, written for paragraphs and tables as they are laid out, and documented as the field to trust "when overflow crosses margins". Geometry is the fallback for a fragment carrying no such record, and it is containment rather than `getColumnAtX` because containment can answer "no column": that is what keeps page-anchored objects out of the gate, a full-width watermark belonging to none. `findColumnContaining` is the new contracts helper for that fallback, the strict counterpart to `getColumnAtX`, which must clamp because a click has to select something. Its spans are half-open so that columns authored with no gutter do not both claim the boundary they share -- the boundary is exactly where the later column's content begins, and an inclusive bound would give it to the earlier column in LTR but not in RTL, making the two directions disagree. The painter's private separator helper now returns the geometry rather than bare x positions, so each separator stays paired with the column it follows instead of relying on array-index alignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ts origin Review follow-up on superdoc#3953. The separator gate reads `fragment.columnIndex` first and falls back to geometry, but the fallback tested containment of the fragment's ORIGIN, and the previous commit's rationale assumed the engine records `columnIndex` for paragraphs. It does not: the paginator writes it for tables (layout-table.ts) and for footnote bodies, and nowhere for an ordinary paragraph fragment. Paragraphs therefore always reach the fallback. That matters because a paragraph's origin can sit outside its own column. A negative `w:ind` hangs it into the gutter, and containment then answers "no column" -- so a later column holding only an outdented paragraph registered as empty and its separator was suppressed, a line Word draws. The same shape applies to an over-wide right-aligned or centred table, which `resolveTableFrame` places at a negative offset from its column. Attribution is now by overlap: the column whose span the fragment covers most, ties going to the earliest in fill order. Anything at least as wide as the whole content area still belongs to no column, which is what keeps page-anchored objects out of the gate -- a full-width watermark overlaps every column without being content of any, and counting it would draw a separator on a page whose text never left the first column. Both directions are covered: an outdented paragraph alone in a later column now draws its separator, and the watermark case still does not.
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 23 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/layout-engine/layout-bridge/src/incrementalLayout.ts">
<violation number="1" location="packages/layout-engine/layout-bridge/src/incrementalLayout.ts:4199">
P2: When only the resolved separator paragraph changes between incremental runs, retained footnote extras bypass this calculation and keep the old rule position. Include both separator paragraph properties in reuse invalidation or force separator reinjection when they change.</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| columnX, | ||
| columnWidth: contentWidth, | ||
| separatorWidth, | ||
| attrs: isContinuation |
There was a problem hiding this comment.
P2: When only the resolved separator paragraph changes between incremental runs, retained footnote extras bypass this calculation and keep the old rule position. Include both separator paragraph properties in reuse invalidation or force separator reinjection when they change.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/layout-engine/layout-bridge/src/incrementalLayout.ts, line 4199:
<comment>When only the resolved separator paragraph changes between incremental runs, retained footnote extras bypass this calculation and keep the old rule position. Include both separator paragraph properties in reuse invalidation or force separator reinjection when they change.</comment>
<file context>
@@ -4076,7 +4188,18 @@ export async function incrementalLayout(
+ columnX,
+ columnWidth: contentWidth,
+ separatorWidth,
+ attrs: isContinuation
+ ? footnotesInput.continuationSeparatorParagraph
+ : footnotesInput.separatorParagraph,
</file context>
Contributor
Author
There was a problem hiding this comment.
Valid, and it was already the known gap — but the comment stated only half of it, so I completed it (fe8d7b6).
The reuse path is gated, though not by the plan-input comparisons: retainedFootnotePlaneBaseAdoptable requires layoutReuse.dependencyProof.renderInputsUnchanged === true before it adopts a previous page's band fragments. A resolved separator paragraph changing means footnotes.xml, a style, or w:docDefaults changed, which is a render-input change — so an honest host proof already forces recomputation, and no invalidation of my own would add anything on top of a proof that is telling the truth.
What was missing was saying so. The field now documents that reuse safety rests on that proof, that an adopted band keeps the rule's old x, and that a host which starts populating the field must include it in the proof.
I deliberately did not add a separate signature comparison for it. The field is unpopulated today — the importer half is not in this PR — so the path is unreachable, and speculative invalidation machinery for an input nobody feeds yet is exactly the kind of thing this repo's review rules ask contributors not to add. The requirement is instead recorded where the person wiring it up will read it. If you would rather have the belt-and-braces check in the gate, say so and I will add it with a test that drives it.
…dges Follow-up to cubic's review of 2438bd9, and to three defects a QA pass over the same function found. All four are in code this PR added. `columnOwningSpan` answers "which column owns this box", and the separator gate asks it "does a LATER column hold content". A wrong answer that names a later column INVENTS a rule Word does not draw; one that names an earlier column or none SUPPRESSES a rule Word does draw. Both were reachable. **The width bound measured the strip, not the page.** Explicit widths are floored to >= 1px but never CAPPED -- nothing clamps their sum -- so an authored `w:num="2"` with two over-wide `w:col/@w` produces a strip WIDER than the content area. Against the strip's own span a page-wide graphic then measures as merely partial, and overlap attribution hands it to whichever column it covers most: `widths: [150, 600]` on a 624px area gives it 150px of column 0 against 426px of column 1. The threshold is now the smaller of the two bounds. Not RTL-specific, which the report had it as: the LTR strip runs 0..150 / 198..798 and the mirrored RTL one 474..624 / -174..426, and the graphic wins column 1 in both. **Neither edge test existed.** Attribution was overlap after a containment test, and both are wrong for a case the other answers, because an indent and an over-wide box produce the same shape from opposite causes: - A box on a column's LEADING edge is that column's, fit or no fit -- ordinary content, and content wider than its column, which overflows from that edge. Overlap alone gets it wrong once the columns are unequal enough for the spill to cover more of the neighbour: `widths: [100, 400]`, a 500px box at column 0's edge, 100px of its own column against 352px of the next. - A box on a column's TRAILING edge is that column's too, and that is a different question rather than a mirror. An indent moves only the leading edge, so a paragraph outdented FURTHER than the gutter has its origin inside the previous column while still ending exactly at its own column's trailing edge -- and containment then read its column as empty. Measured on equal 2-col geometry over 624px (col0 [0,288), col1 [336,624)): a column-1 paragraph outdented 72px is the box [264, 624]. Containment survives as the third rule, now fit-checked, and overlap as the fourth. `balanceSectionOnPage`'s `ordinalOf` reached the same four rules in the same order for the same reasons; the two differ only at the end, where a sort key must name a column and this may answer `null`. They should be one shared helper in `contracts`, and are not yet. **Folded the strip bounds out of `Math.min(...map)`.** `w:num` is bounded at 45 by the schema but nothing in the pipeline enforces it, and a host-built layout with a six-figure count overflowed the argument stack -- a paint-time crash out of `paint()`, taking the whole document with it, from a function whose only job is to answer conservatively. Two further fixes at the call site, from the same QA pass: **A float is not column content, and no width threshold can recognise one.** The threshold catches a full-width watermark, which is what it was written for, but `page.items` is `page.fragments.map(...)` with no anchor filtering, and an anchored object carries its own `measure.width` -- so a narrow one is the ordinary case. A 200px logo at page x 500 on a 2-column page whose text never leaves column 0 has its origin inside column 1 and lit the gate. Excluded by identity (`isAnchored`) instead. Every float, not only page-relative ones: `hRelativeFrom` is consumed at layout time and never reaches the fragment, and there is no evidence here about whether Word draws a rule beside a column holding a floating object and no text. Word's rule tracks text, and the gate is deliberately asymmetric, so the conservative reading is also the simpler one. **An out-of-range `columnIndex` is rejected, not clamped.** Clamping turned any stale or corrupt value into a real index -- `columnIndex: 5` on a two-column page became 1 -- which is exactly the "a later column holds content" the gate asks about, invented out of a number describing no column on the page. Falling through to geometry answers from the fragment's actual position. Floored first, so float drift on a valid index still resolves. Eleven tests, each pinning one rule. The page-bound and both edge tests were mutation-checked: restoring the old threshold or removing either edge rule fails exactly one test each, and three different ones.
The separator gate's comment said the paginator writes `columnIndex` "for tables and footnote bodies but not for ordinary paragraphs", and the last clause is wrong. `layout-paragraph.ts` sets it on a `kind: 'para'` fragment when `collapseSplitLineBreakCarrier` is on, and that comes from `splitCarrierMode === 'spaced'` — a purely document-driven predicate with no flag behind it: a line-break-only paragraph, followed by an anchored drawing, followed by a paragraph sharing its `sourceAnchor.sourceRef`, where the carrier has positive spacing. The claim was load-bearing. It says the record is absent for the kind that dominates a page, so `columnOwningSpan` carries the work and has to be right alone. That conclusion survives — a collapsed anchor carrier is a narrow shape, not the ordinary paragraph — but "paragraphs never carry one" would have justified deleting a rule the function needs, and a reader checking the premise would have found a counterexample and distrusted the rest. Listing the kinds instead of asserting a rule: tables at five sites in `layout-table.ts`, the three footnote body kinds in `incrementalLayout.ts`, and that one carrier paragraph. Comment only; no behavior change.
Nathaniel-260
force-pushed
the
fix/footnote-band-columns
branch
from
September 2, 2026 15:18
fe8d7b6 to
baae3aa
Compare
`Core` fails on `vp fmt --check`, and this is the only file it flags on the branch. Prettier's print width fits the four parameters on a single line at 118 characters; the multi-line form the earlier commit left there is the whole difference. `CI V2 Public / validate` is the aggregate job and fails only because `Core` did. Formatting only; no behavior change.
…t edge The separator gate's origin-containment step was gated on the box's right edge landing inside the column its origin is in. That gate has two problems, and they point the same way. It rejects a box that genuinely belongs to the column its origin is in. `layout-paragraph.ts` re-points a paragraph carrying `attrs.floatAlignment` of `right` or `center` at `columnX + (effectiveColumnWidth - maxLineWidth)` and never reduces `fragment.width`. So a 50px line in a 288px column is recorded as `x = columnX + 238` with `width` still 288: its origin is inside its own column and its right edge overhangs by 238px. The edge gate rejected it, the overlap vote then saw 50px of column 0 against 190px of column 1 and moved it, and a page whose text never left column 0 drew a separator — the same false positive as the narrow page-anchored object, reached with no anchored object at all. And an edge gate is dead code anyway. Pass it and the box lies wholly inside one column's span; `getColumnGeometry` never emits overlapping spans, so every other column's overlap is zero and the vote returns that same column regardless. Swept over outdents from 0 to 160px in 2px steps, an edge-gated containment step and plain overlap never disagreed once — so the step was doing no work while being the thing that broke the frame case. Width is what actually separates the two shapes, because the right edge overhangs in both. A box no wider than its column was placed in that column wherever its origin ended up. A box WIDER than its column may instead have been pulled LEFT out of it: a negative `w:ind` widens the fragment by the outdent, so an outdent larger than the gutter lands the origin in the PREVIOUS column while the content belongs to this one. Measured on equal 2-column geometry over a 624px content area (col 0 [0,288), col 1 [336,624)): a column-1 paragraph outdented 72px is the box [264, 624], origin in column 0, width 360 against a 288px column — it does not fit, the origin is distrusted, and overlap answers column 1 correctly. Both shapes are now pinned, and the pair is the test: the frame keeps its own column and draws no rule, the outdent falls through to overlap and draws one. Replaces an earlier test whose fixture was a 100px box at the outdented origin, which no layout path produces — a negative `w:ind` widens the fragment, so a narrow box at that origin is a fragment that really does start in column 0. `painters/dom` is 61 files / 1565 pass.
Nathaniel-260
force-pushed
the
fix/footnote-band-columns
branch
from
September 2, 2026 15:46
baae3aa to
13b79c1
Compare
cubic's review caught that the test standing for the width gate's rejection path never reaches it, and the same mistake was written into the gate's own comment as its justification. An outdented paragraph cannot reach that step. A negative `w:ind` widens the fragment by exactly the outdent it shifts by, so `x + width` lands on its own column's trailing edge for EVERY outdent -- the trailing-edge rule answers first and the gate never sees the box. On equal 2-col geometry over 624px (col0 [0,288), col1 [336,624)), a column-1 paragraph outdented 72px is [264, 624], and 624 IS column 1's trailing edge. Any other outdent lands there too. That fixture was the only guard on the gate, so the gate had none. Measured rather than assumed: replacing the width comparison with unconditional origin trust left all 39 tests in this file passing. The shape that does reach it is a centred over-wide box. `resolveTableFrame` centres an over-wide table inside its column at `col.x + (col.width - width) / 2`, a NEGATIVE offset once the table is wider than the column, so it begins inside an earlier column without ever having left its own -- and unlike the outdent, its right edge lands nowhere in particular. A 400px box centred in column 1 is [280, 680]: 680 misses column 1's 624 by 56, the origin 280 falls inside column 0, and 400 does not fit a 288px column, so the origin is rejected and overlap answers column 1, 288px against 8px. Under the same mutation this fixture fails, and it is the only test that does. Both comments now say what the mistake was rather than quietly swapping the fixture: a reader who checks the old justification finds a counterexample and has no way to tell how far the error spread. Test and comments only; no behavior change.
Nathaniel-260
force-pushed
the
fix/footnote-band-columns
branch
from
September 2, 2026 18:21
13b79c1 to
6f50c23
Compare
Contributor
Author
|
Force-pushed: rebased onto #3953's current tip. No change of my own, and nothing to re-review in my commits. This PR was branched from The rebase applied cleanly with no conflicts, and the resulting tree differs from the pre-rebase tip by exactly Verified locally before pushing: One caveat so it does not read as a regression if CI hiccups: |
…s content `getColumnAtX`'s mirrored branch tested an INCLUSIVE upper bound, so it disagreed with the half-open spans that `findColumnContaining` and the geometry itself use. With `w:space="0"` (ECMA-376 §17.6.3) adjacent columns share an edge, and in an RTL section that shared edge is the earlier fill column's own left edge -- exactly where its content is placed -- so the inclusive form handed it to the LATER column and every column boundary in a zero-gutter RTL section resolved one column too far. Two columns over 602px mirror to column 0 at [301,602) and column 1 at [0,301): `findColumnContaining(301)` answered 0 and `getColumnAtX(301)` answered 1, so the two resolvers disagreed at the one point they can be made to disagree about. The same bound also claimed the point on a column's trailing edge, which is gutter and belongs to the column preceding it in fill order. `cx <` is correct on both counts and makes the two resolvers agree everywhere they can both answer. Fixed here rather than one PR up the stack, where it was first written. This branch introduces the mirrored branch and its inclusive bound, so it is where the defect enters the tree; leaving it for superdoc#3962 meant superdoc#3953 and superdoc#3961 would both merge with a line already known to be wrong. Dormant in production either way -- nothing assigns `ColumnLayout.direction` yet -- but the review record should not carry a known defect across two merges when the fix is three lines. The RTL case in position-hit.test.ts worked its geometry out as column 1 spanning 336..528; the mirrored geometry puts it at 312..504, a full gutter off. Corrected, with the derivation spelled out, since that comment misleads a reader of this diff today. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…or placement A Hebrew (RTL) document with a two-column `continuous` section and `<w15:footnoteColumns w:val="1"/>` on every sectPr printed its footnotes in a strip one body column wide -- 253px of a 554px content area, tucked under one column -- with the separator rule at that strip's left edge. Word prints one strip across the whole content area with the rule at its right edge. Two independent defects, both from the note band having no geometry of its own. ## 1. The band reused the body's `w:cols` `w15:footnoteColumns` declares how many columns a section's FOOTNOTE BAND uses, independently of the body. Nothing read it, and every note-plane stage took its geometry from the body. `resolveFootnoteBandColumns` is now the single place that decision is made. Absent or `0` means "match the body" and returns the body layout unchanged; a count below the body's returns that many equal columns spanning the content area, with the body's gutter and fill direction. A count above the body's clamps back to the body, because the note planner carries a column's overflow to the SAME column on the next page rather than sideways into the next band column -- a wider band would paint a half-width strip beside an empty one and push notes onto later pages, strictly worse than the body-matching band the default already produces. Four note-plane stages moved onto the band's plane together, because a disagreement between any two of them is visible: - The measurement width, which broke every note's lines at a body column's width. It was also the MINIMUM body column width anywhere in the document, so one two-column section in the middle narrowed the notes of every single-column section too. It is now the narrowest BAND in the document. One global pass still serves the whole document -- `footnoteConstraints` is a single object, and a note's section is only known after the body is paginated -- so a document that mixes band widths under-fills its wider bands rather than spilling out of its narrower ones, and painting clamps to `min(bandColumnWidth, measurementWidth)` so the two can never disagree. - Reference-to-stack assignment, which keyed notes by the body column their anchor sits in. It now maps that body column through the band's count, so several body columns can feed one band stack -- in note order, because references are visited in document order and the map is monotone in the body index. - The planner and its reserve, which received the body's column count. A merged band is ONE stack whose height is the sum of the per-column stacks and not their max, so a reserve still taken from the body's columns let the band run past the page's bottom margin. - The painted x and width, including the retained-band and coupled-pagination paths that re-derive the geometry, and the warm seed that retains it. ## 2. The separator rule was pinned to the note column's left edge `<w:separator/>` is not engine decoration: it is a RUN, and it lives in its own paragraph in footnotes.xml (`w:footnote w:type="separator"`, id -1). Its horizontal placement is therefore the inline placement of a run in THAT paragraph -- its resolved inline direction, `w:jc` and `w:ind` -- which is what `resolveFootnoteSeparatorX` reads. Specifically not the section's `w:bidi`. The two axes are independent (§17.6.1 for the section, §17.3.1.6 for the paragraph, as `direction-context.ts` already states) and they come apart in practice: a Hebrew section whose separator paragraph resolves LTR gets a rule on the LEFT in Word, and an LTR section whose separator paragraph resolves RTL gets one on the right. Keying the placement on the section direction renders both of those backwards. Nothing about the section reaches the new function -- its inputs are the note column's own geometry and that one paragraph -- so neither case can regress by construction, and both are covered by tests. `w:jc` resolves the way `resolveTextAlign` already resolves it for text: an explicit left/right/center is physical, and `justify` or an absent value falls to the paragraph's start edge, which is the right in an RTL paragraph. A separator paragraph's single line is also its last, and justification never stretches a last line -- the document that surfaced this declares no direction of its own and inherits `<w:bidi/>` with `<w:jc w:val="both"/>` from `w:docDefaults`, and Word draws from the right. ## The importer half is deliberately not here Four inputs the importer has to populate, all optional and all inert until it does: `SectionBreakBlock.footnoteColumns` and `LayoutOptions.footnoteColumns` from `w:sectPr/w15:footnoteColumns/@w:val`, and `footnotes.separatorParagraph` / `continuationSeparatorParagraph` from the resolved `w:pPr` of the separator paragraphs. Only the host can resolve the latter: a separator paragraph usually declares nothing and inherits through the style chain, which the layout engine cannot see. Absent them, the band resolves to the same body layout it already used and the separator keeps its historical left edge, so every existing document renders byte-identically. The reader added to `to-flow-blocks.ts` is the reference implementation for the sectPr half, w15 prefix and the `0` default included. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two comments in the previous commit said less than the truth, and one of them carries a requirement for whoever wires the importer half. `resolveFootnoteBandColumns` claimed `withSeparator` "is never carried over". Only the merge branch drops it; the matching branch returns the body layout verbatim, `w:cols/@w:sep` included, and that verbatim return is exactly what makes a document without `w15:footnoteColumns` render byte-identically to before. Keeping it is inert -- `@w:sep` drives the VERTICAL rules the painter derives from `page.columns`, not from this layout, and no footnote consumer reads the band geometry's `separatorX` -- so the fix is to say what the code does, not to change it. `footnotes.separatorParagraph` was documented as paint-only and therefore absent from the plan-input comparisons that gate band reuse. True, but it left the reader without the other half: `retainedFootnotePlaneBaseAdoptable` requires the host's `renderInputsUnchanged` proof before adopting a previous page's band fragments, and an adopted band keeps the rule's old x. A host that starts populating this field must include it in that proof, or a change to the separator paragraph alone paints stale. Unreachable today, since nothing populates the field yet -- which is precisely why it needs to be written down before something does. Comments only; no behavior change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nathaniel-260
force-pushed
the
fix/footnote-band-columns
branch
from
September 2, 2026 19:46
6f50c23 to
a5bf476
Compare
Contributor
Author
One open decision left on this PR, and it is yours rather than mineEverything else on #3961 is now closed: three of the four review threads are resolved, and the one that recorded a defect shipping unfixed is gone because the fix moved down into #3953 ( What remains is a judgement call I do not think I should make unilaterally, so I would rather ask than guess. Should The situation, as I understand it:
So the question is really: do you want the gate to defend against a host whose proof is wrong, or is the proof the contract? If the proof is the contract, this is already correct and the documentation is the right artifact. If you would rather have belt-and-braces, say so and I will add the signature comparison with a test that actually drives it — I would rather write it now, while the reasoning is fresh, than leave it for whoever wires up the importer. No rush on my side; nothing else is blocked on this. |
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.
Fixes #3960
Important
Depends on #3953 and cannot merge before it.
mainhas no column direction at all today;#3953 is what gives
ColumnLayoutitsdirectionand mirrors the strip in RTL, and the footnoteband inherits that axis. Merging this first would not compile against
main.Two commits are mine —
fix(layout): give the footnote band its own column layout and separator placementanddocs(layout): state the footnote band's separator invariants accurately. Everything below them in the diff belongs to #3953 and disappears once it merges.CONTRIBUTING.mdasks formain, so the base ismainand the stack shows up in the diff.A Hebrew (RTL) document with a two-column
continuoussection and<w15:footnoteColumns w:val="1"/>on everysectPrprinted its footnotes in a strip one body column wide, with the separator rule on the wrong side of it. Word prints one strip across the content area, rule at the right.Geometry read out of the DOM in Chrome against the packaged dist, before the change (
data-layout-storyidentifies the band):A4 with 1800tw side margins and a 708tw gutter: content area
8306tw = 554pxat 96dpi, one body column(8306 - 708) / 2 = 3799tw = 253px. After the change the band is emitted at the content-area origin across its full width (x = marginLeft,width = 553.73px), with both columns' notes in one stack in note order. That is asserted directly on the layout infootnoteBandColumns.test.tsrather than eyeballed in a browser.One visible consequence worth naming: the
w:separatorrule is0.5xthe note column's width, which is pre-existing, so a full-width band gets a proportionally longer rule -- 277px rather than 126px here. That follows from the band being wider and is not something this PR changes; if Word's rule is a fixed length instead, that is a separate fix to the factor.Two independent defects. They are in one commit because the shared files carry hunks from both, and a hand-split would have produced a half-broken intermediate; the sections below and the commit message separate them, and I am happy to split the PR itself in two if you would rather review them apart.
1. The band reused the body's
w:colsw15:footnoteColumnswas never read anywhere in the tree, and every note-plane stage took its geometry from the body.resolveFootnoteBandColumnsis now the single place that decision is made, and four stages moved onto the band's plane together, because a disagreement between any two of them is visible:Absent the element, every path is byte-identical to before.
2. The separator rule was pinned to the column's left edge
<w:separator/>is a run inside its own paragraph infootnotes.xml, so its placement is that paragraph's resolved inline direction,w:jcandw:ind.It is deliberately not keyed on the section's
w:bidi, which is the tempting wrong fix. The axes are independent (§17.6.1 vs §17.3.1.6, asdirection-context.tsalready states) and they come apart: a Hebrew section whose separator paragraph resolves LTR gets a rule on the left in Word, and the reverse for an LTR section with an RTL separator paragraph. Nothing about the section reachesresolveFootnoteSeparatorX— its inputs are the note column's geometry and that one paragraph — and both divergent cases have tests.w:jcresolves exactly the wayresolveTextAlignalready resolves it for text.What this PR does not do
The importer half, which is where the two new inputs get populated:
SectionBreakBlock.footnoteColumns,LayoutOptions.footnoteColumnsw:sectPr/w15:footnoteColumns/@w:val(0/absent = match the body)footnotes.separatorParagraph,footnotes.continuationSeparatorParagraphw:pProfw:footnote w:type="separator"/"continuationSeparator", through the style chain — a separator paragraph usually declares nothing and inherits fromw:docDefaults/w:pPrDefaultUntil they are populated, documents render exactly as they do today: the band resolves to the body layout it already used, and the separator keeps its historical left edge. The
w15:footnoteColumnsreader added toto-flow-blocks.tsis the reference implementation, prefix and0default included.Design decisions worth a look
footnoteConstraintsis a single object and a note's section is only known after the body is paginated, so the width is the narrowest band in the document. Painting clamps tomin(bandColumnWidth, measurementWidth), so measurement and paint can never disagree; a document mixing band widths under-fills its wider bands rather than spilling out of its narrower ones. Per-section note measurement is the follow-up that removes the compromise.w:indnarrows the separator's extent;firstLine/hangingare not read. They move the first line of a text paragraph and I have no evidence Word applies them to a lone separator run — better handled once the behaviour is known than guessed at.Testing
44 new tests across 4 files, including the exact
sectPrset of the document that surfaced this, rebuilt synthetically (no.docxfixture added).npx vp test run --root packages/layout-engine/contracts— 32 files, 538 passnpx vp test run --root packages/layout-engine/layout-bridge— 96 files, 1895 passnpx vp test run --root packages/layout-engine/tests— 284 pass, plus one pre-existing failure noted belownpx tsc -b tsconfig.references.json— cleanvp linton the touched trees — no errors;vp fmtappliednode scripts/check-public-boundary.mjs— OKTwo things I could not run in my checkout, both unrelated to this change:
pnpm testat the root (the rootpreparescript fails in my shell) and anything scoped topackages/superdoc(installed engine0.10.0-next.12vs the declared0.10.1-next.1). Also pre-existing and untouched here:architecture-boundaries.test.tsGuard E flagsgetBoundingClientRectinpainters/dom/src/ruler/ruler-renderer.ts.No public API surface was grown — the new helpers are exports of
@superdoc/contracts, which is a private workspace package and is not re-exported fromsuperdoc's public index, so notests/consumer-typecheck/fixture applies.Verified through the paint path, not just the layout
Because a layout assertion does not prove anything reaches the screen, I also ran the full
incrementalLayout-> resolve ->DomPainterpipeline and readstyle.left/style.widthback off the painted elements. Measured, in CSS px:120, width553.731004.53< note 2 top1016.53276.87, left396.87footnoteColumnsomitted253.27, two separators624.53and826.53, gap2021026.53vs the page limit1026.53120396.87The last two are the pair that a section-direction fix would get backwards, so they are measured rather than argued.
The painter itself was checked for the same class of bug and is clean:
applyFragmentFrame/applyResolvedFragmentFramewritestyle.left/style.widthstraight from the fragment's ownx/widthand never re-derive them frompage.columns.One limit worth naming: text fragment wrappers get no explicit
style.height, so the "band bottom" figures above are a painted top read from the DOM plus the measured line height, not a second independent DOM measurement.No screenshot: the change is geometry, and it is pinned numerically instead.
Stacking
Repeated from the top because it decides merge order: this is built on #3953, which is what gives
ColumnLayoutitsdirectionand mirrors the strip in RTL.mainhas no column direction at all, so the band has no axis to inherit and this does not stand alone -- #3953 has to land first. The two commits named at the top are mine; everything below them belongs to that PR and disappears from this diff once it merges.🤖 Generated with Claude Code