fix(layout): compare rendered gutters, and order a balanced page by column - #3962
Open
Nathaniel-260 wants to merge 20 commits into
Open
fix(layout): compare rendered gutters, and order a balanced page by column#3962Nathaniel-260 wants to merge 20 commits into
Nathaniel-260 wants to merge 20 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.
All reported issues were addressed across 25 files
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
…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/column-gaps-and-balance-order
branch
from
September 2, 2026 15:24
8684df9 to
942194e
Compare
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
`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/column-gaps-and-balance-order
branch
from
September 2, 2026 15:52
62a70df to
566fcb0
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/column-gaps-and-balance-order
branch
from
September 2, 2026 18:36
566fcb0 to
906fab7
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>
`columnRenderLayoutsEqual` skipped per-column `gaps`, with a note saying they were ignored "until geometry/separators consume them (step 4)". Step 4 landed: `buildColumnGeometry` reads `gaps[i] ?? gap` for both the column x and the separator x, so a gaps-only delta moves every column after the first. While they were skipped, two sections differing only in their per-column gaps compared render-equal. Nothing split the region and nothing invalidated the normalized-columns cache, so the later section was laid out with the earlier one's gutters, and the painter drew the whole page's separators from them. Comparing the authored arrays would trade that for the opposite defect, because they do not describe what renders. `resolveColumnLayout` emits `gaps` only when the author supplied them and pads a short array with 0, while geometry falls back to the scalar gap and floors at 0. So the authored arrays differ where the output is identical -- an omitted array against one spelling out the scalar gap, or a negative gutter against 0 -- and a spurious split is not a cache miss but a layout change: a continuous section break with changed columns resets to column 0 mid-page, so following content restarts in the first column instead of continuing where it was. They also match where the output differs, since a short `[20]` falls back to the scalar gap for the gutter it omits rather than to 0. Derive the effective gutters the way `normalizeColumnLayout` does instead, which keeps this predicate exactly as discriminating as the geometry it stands in for. Two tests named after the temporary state -- "before geometry uses gaps" and "when only later per-column gaps differ" -- asserted the old behaviour and now assert the new one.
When a multi-column section ends mid-page, `balanceSectionOnPage` redistributes
its fragments and writes the balanced x and y back onto them in the order it
derived. That order was a sort on raw x, on the premise that every fragment in a
column shares one origin -- so getting it wrong reorders the page rather than
merely laying it out oddly.
The premise does not hold. A negative `w:ind` and a float offset shift a
paragraph's origin; `resolveTableFrame` right-aligns or centres an over-wide
table, which moves its origin outside the column entirely. A difference of 1e-7
was enough to swap two paragraphs.
Sort by the column each fragment occupies instead. The ordinal is fill order, so
it needs no RTL special case, where an x comparison did because column 0 sits on
the right and document order descends in x.
Resolving that ordinal takes three steps, and it always yields a number: a sort
key that is sometimes absent leaves the comparator mixing two metrics, which is
not a total order, and `Array.prototype.sort` may then return different orders
for the same input -- it does differ between engines, so the product and the
suite would not agree.
- the column the engine recorded, where it kept one. Tables carry it; ordinary
paragraphs do not, which is why the remaining steps carry most fragments.
- otherwise the column containing the origin, which stays correct for content
that merely overflows its column -- an overlap comparison gets that wrong
once the columns are unequal enough for the spill to cover more than the
column it came from.
- otherwise the column the box covers most, for an origin hung into a gutter,
falling back to the clamping hit-test walk when the box touches no column.
Ties resolve on y and then on arrival order, so the sort is total and stable.
…icit region `columnRenderLayoutsEqual` compared the scalar `gap` ahead of the explicit/equal mode branch, so it applied to both. In explicit mode the scalar is only the fallback for a gutter that `gaps` does not supply, and `effectiveColumnGaps` already folds it in at that position -- so two layouts whose `gaps` spell out every gutter draw the same columns in the same places no matter what the scalar says. They split a region anyway and invalidated the normalized-columns cache, and a continuous section break with changed columns resets to column 0 mid-page, so the following content restarted in the first column instead of continuing where it was. Equal mode still compares it. There the scalar IS every gutter, and `normalizeColumnLayout` subtracts the total from the content area before dividing it, so it sets the column width too. Nothing else can stand in for it. One way the scalar still reaches explicit WIDTHS, and the reason for the sub-pixel guard: `normalizeColumnLayout` floors a fabricated width at 1px and collapses to a single column when the usable width falls to epsilon, both gated on the sign of `contentWidth - gap * (count - 1)`, which the scalar moves. It only bites when an authored width is itself sub-pixel, because at 1px or more the floor and the collapse are both no-ops. Rather than model a content-width-dependent branch in a predicate documented as content-width-INDEPENDENT, such a pair is refused as equal outright. Split out of the RTL boundary fix this commit used to carry. That fix moved down to `fix/rtl-column-order`, where the bound it corrects is introduced; the rebase left this commit holding only the gap-comparison change, which was always a separate subject. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…rigin
Balancing orders a page by column ordinal, then writes the balanced x/y back onto
the fragments in that order — so a wrong ordinal reorders the page rather than
merely laying it out oddly. For a fragment with no recorded `columnIndex` the
ordinal came from whichever column CONTAINED its origin, which identifies the owner
only while the origin has not been shifted out of it. `resolveTableFrame` places an
over-wide table that justifies to `end` at `col.x + (col.width - width)`, a negative
offset, and `end` is the default justification for any `w:bidiVisual` table — so
such a table begins inside an EARLIER column while never having left its own.
Containment named that earlier column and balancing moved the table ahead of the
content it follows. `createAnchoredTableFragment` records no `columnIndex` at all,
so a floating over-wide table arrives here with nothing but its box.
Ownership now resolves in order: the recorded column; a leading-edge match, which
covers ordinary content and content that overflows rightward from its own column's
start; a trailing-edge match, which covers the right-aligned placement above;
origin containment gated on the box actually FITTING that column, which covers an
indent; greatest overlap, which covers a centred over-wide table and an origin hung
into a gutter by a negative `w:ind` or a float offset; then the clamping walk, which
always names one. Every step earns its place — removing any of the five fails
exactly one of the new tests, and the order matters too: a table spanning the whole
content area ends on the LAST column's trailing edge, so the leading edge has to be
asked first.
Keep the recorded column consistent with where balancing actually put the fragment,
in both places balancing moves one. The placement loop rewrites it for a fragment
moved between columns, and the SD-3359 split branch — which inherits it through the
`{...f}` spread while placing the half in `col + 1`, so it went on naming the first
half's column — rewrites it for the second half. A record that contradicts the
placement is worse than none now that ordering prefers it over any geometry and the
painter's separator gate reads the same field to decide which columns hold content:
a table balanced out of column 0 kept reporting column 0, leaving its new column
reading as empty and suppressing a rule Word draws. Neither site invents a record
where the paginator wrote none, because the presence of a value is itself the
engine's evidence of ownership.
An anchored table's `x` does not identify the column it flows in. `resolveAnchoredGraphicX` places one justified to `end` at `col.x + (col.width - width)` — a negative offset from its own column once the table is wider than it — and `end` is the default justification for any `w:bidiVisual` table. Such a table BEGINS inside an earlier column without ever having left its own, so its origin names the wrong column. `TableFragment.columnIndex` exists for exactly this, documented as the flow column "distinct from visual x when overflow crosses margins". The in-flow table paths record it at five sites in `layout-table.ts`; `createAnchoredTableFragment` never did, which is why every consumer had to infer the column from coordinates and why the inference existed at all. `state.columnIndex` was already in scope at both call sites, and `layout-paragraph.ts` already recorded it into `registeredAnchoredTablePlacements` right beside the fragment — the owner was known and simply was not written onto the fragment. Measured on two equal columns over a 624px content area (288px each, 48px gutter): a 500px table owned by column 1 lands at content x 124, inside column 0. Three consumers read the field ahead of any geometry: - `ordinalOf` in `column-balancing.ts` sorts a balanced page by it. With the record absent, an anchored table in a later column sorted ahead of the content that precedes it. - `determineTableColumn` falls back to `determineColumn(fragment.x)`, so a click on such a table answered the wrong column. Nothing in the balancer's own geometry hardening reaches this consumer. - The painter's column-separator gate. That one is unaffected in practice: the gate now skips floats outright, because `hRelativeFrom` never reaches the fragment and it cannot tell a page-anchored object from column content. The parameter is optional and absent means absent, not column 0: the gate would read a fabricated 0 as content the page does not have. NaN and Infinity are rejected on the same reasoning; a fractional or negative ordinal is floored into range, because it still names a column the caller meant.
The equal-mode branch argued that the scalar gap's remaining route into explicit WIDTHS — normalize's `Math.max(1, …)` floor, keyed on the sign of `contentWidth - gap * (count - 1)` — was out of scope because this predicate is documented content-width-independent. It is not out of scope any more: the explicit branch now guards it with `hasSubPixelWidth`, and the parenthetical read as if that guard were not there. Point at it instead, and record why `< 1` is the right threshold — at 1px or more both the floor and the epsilon collapse are no-ops, so the guard costs nothing on any real document. Comment only; no behaviour change.
`ordinalOf`'s first comment said "Tables carry it; ordinary paragraphs do not", and the second half is wrong. `layout-paragraph.ts` sets `columnIndex` 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, which is why it is worth a commit of its own. It says the record is absent for the kind that dominates a page, so the rules below it carry the work and have 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 step this 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.
Adversarial review of the previous commit found two regressions in the geometric fallback it added, both reproduced end to end, and both worse than the plain containment it replaced. This replaces that whole heuristic with one test that holds for every shape either of them covered. The trailing-edge snap is unusable, not merely imprecise. `resolveTableFrame` puts an over-wide `end`-justified table between an earlier column's left edge and its OWN column's right edge, and the owner is the later column. A box of identical shape centred in column 0 — an anchored graphic sized to the content area — lies between exactly those same two edges, with exactly the same per-column overlaps, and the owner is the earlier column. For a content-area-wide box centred in column 0 the right edge is `(columnWidth + contentWidth) / 2`, which lands on the middle column's trailing edge for EVERY odd column count, so this is structural rather than a coincidence of one geometry. No rule reading the box can separate those two; only the record can, which is why `columnIndex` is asked first and why recording it on anchored tables was the right fix for the case that started this. Gating containment on the box's right EDGE staying inside its column was the second regression. `layout-paragraph.ts` re-points a `floatAlignment` right/centre fragment at `columnX + (columnWidth - maxLineWidth)` and does NOT reduce its width, so a 50px line in a 200px column keeps width 200 and overhangs by 150. The edge gate rejected it, the overlap vote then saw 50px of its own column against 130px of the next, and the paragraph was re-emitted at the END of the page. No table involved — one paragraph attribute. What the origin can be trusted on is the box's WIDTH. A box no wider than the column its origin sits in was placed in that column, wherever in it the origin ended up: ordinary content, a `w:ind` indent, and the right/centre float above. A box wider than that column may instead have been pulled left out of its own column, because a negative `w:ind` widens the fragment by the outdent — so an outdent larger than the gutter lands the origin inside the PREVIOUS column, which plain containment then named. That case is why the overlap vote below is still needed and is now covered; it was silently wrong before either version of this code. Every step is pinned, and this time the mutations were run: dropping the recorded column fails two tests, dropping containment or re-adding the edge gate fails the right-aligned float, dropping the overlap vote fails the outdent, and re-adding the trailing-edge snap fails both the content-area-wide table and the centred box. The previous commit claimed the same and was wrong — its steps covered for each other, so no single removal showed up.
`normalizeColumnLayout` mapped every authored gutter through
`Math.max(0, value)`. For an `undefined` ENTRY inside the array that is NaN, and
NaN is not nullish, so `buildColumnGeometry`'s `gaps?.[i] ?? gap` could not
rescue it — the hole propagated into the x of every column after it.
`{gap: 40, gaps: [30, undefined]}` over a 720px area normalized to
`gaps: [30, NaN]` and painted `col2.x = NaN`: a column with no position at all.
The fallback a hole should take is the one a SHORT array already takes. Both
`buildColumnGeometry` and `effectiveColumnGaps` read `gaps[i] ?? gap`, so an
array that stops early falls back to the scalar per missing gutter; an array with
a gap in the middle now does the same. A non-finite entry takes the same route,
since the reason to distrust it is identical.
Unreachable today: `gaps?: number[]` forbids a hole under TypeScript and nothing
in this repo constructs a `gaps` array. It stops being unreachable the day the
importer projects `w:cols/w:col/@w:space` per column, which is what the whole
per-column gap path exists for — and the failure mode is a column painted at NaN,
which is worth a guard rather than a note.
Second effect, and the reason this belongs beside `columnRenderLayoutsEqual`:
the predicate compared `effectiveColumnGaps`, which already folded the hole to
the scalar, against geometry that did not. The two agreed on the answer and
disagreed on the layout — a section pair that compared EQUAL while one of them
rendered a NaN column. They now agree on both.
Also records that `hasSubPixelWidth` is complete rather than merely in scope: the
epsilon collapse needs the maximum authored width at or under the epsilon, and
both epsilons in the tree (1e-4 in `layout-engine/src/index.ts`, 1e-2 in
`layout-bridge/src/incrementalLayout.ts`) are below 1px, so that route also
implies a sub-pixel width. No path the guard misses.
`contracts` is 30 files / 516 pass on this branch.
Nathaniel-260
force-pushed
the
fix/column-gaps-and-balance-order
branch
from
September 2, 2026 19:46
906fab7 to
d7c4964
Compare
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 #3966
Fixes #3967
Those two are the pre-existing pair, now filed separately so they are reviewable on their own terms.
Related but not fixed here: #3964, which is
normalizeColumnLayoutandresolveColumnLayoutdisagreeing about which gutter survives a dropped column — a different function and a different
trigger from #3966.
Six defects in the column-layout and column-balancing path. The first two are older than the RTL work and reachable in plain LTR documents; the rest came out of review on this PR and on #3953. A commit each.
A seventh used to be here: the
getColumnAtXRTL boundary fix has moved down into #3953 (ad9bde6), where the bound it corrects is introduced, so that #3953 and #3961 no longer merge carrying a line already known to be wrong. The commit that carried it here also held an unrelatedcolumnRenderLayoutsEqualchange; that stayed, renamed to describe what it actually does.Important
Depends on #3953 and cannot merge before it. Not merely for convenience: the balancing sort
this replaces was last touched there, and the geometry it reads carries
directionandcontentWidth, which #3953 introduces.The nine commits above
fix(contracts): resolve an RTL column boundary the way geometry places contentare mine.In order:
compare the gutters that render,order a balanced page by column,stop an unreachable scalar gap from splitting an explicit region,decide which column owns a fragment from more than its origin,record the flow column on an anchored table fragment,stop the gap comment contradicting the sub-pixel guard,name the fragment kinds that actually record a column,trust a fragment's origin by its width, not by its edges, andfall a hole in the per-column gaps back to the scalar. Everything below them belongs to #3953 and disappears once itmerges.
This is stacked directly on #3953, not on #3961. The two share no files, so #3961's footnote
band work is deliberately not in this diff. Either can merge first once #3953 is in.
CONTRIBUTING.mdasks formain, and its branch does not exist here to target, so the base ismainand the stack shows up in the diff. The first commit is independent of the RTL work andcould be split out against
mainon its own if you would rather take it that way.1. Per-column gaps were invisible to render equality
columnRenderLayoutsEqualskipped per-columngaps, with a note saying they were ignored "until geometry/separators consume them (step 4)". Step 4 has landed:buildColumnGeometryreadsgaps[i] ?? gapfor both the column x and the separator x, so a gaps-only delta moves every column after the first.While they were skipped, two sections differing only in their per-column gaps compared render-equal. Nothing split the region and nothing invalidated the normalized-columns cache, so the later section was laid out with the earlier one's gutters, and the painter drew the whole page's separators from them.
Why not just compare the authored arrays. They do not describe what renders, so that trades the bug for its mirror image.
resolveColumnLayoutemitsgapsonly when the author supplied them and pads a short array with0, while geometry falls back to the scalar gap and floors at0:gap: 48, nogapsgaps: [48, 48]gaps: [48, 0]gaps: [48, -10]gaps: [20]gaps: [20, 0]A spurious split is not a harmless cache miss.
isColumnConfigChanging→forceMidPageRegionresetsstate.columnIndexto 0, so content after the break restarts in the first column instead of continuing where it was, and an extracolumnRegionsentry is emitted — which is what bounds the painter's separator spans.So the comparison derives the effective gutters the way
normalizeColumnLayoutdoes —gaps[i]positionally when present, the scalar gap otherwise, each floored at 0 — which keeps the predicate exactly as discriminating as the geometry it stands in for.Two tests named after the temporary state ("...before geometry uses gaps", "...when only later per-column gaps differ") asserted the old behaviour and now assert the new one. Both fail if the comparison is removed.
2. Column balancing reconstructed document order from raw x
When a multi-column section ends mid-page,
balanceSectionOnPageredistributes its fragments and writes the balanced x and y back onto them in the order it derived. That order was a sort on raw x, on the premise that every fragment in a column shares one origin — so getting it wrong reorders the page rather than merely laying it out oddly.The premise does not hold:
w:indshifts a paragraph's origin (layout-paragraph.tsaddsnegativeLeftIndent),offsetX), as doesfloatAlignment: right|center,resolveTableFrameright-aligns or centres an over-wide table, moving its origin outside the column entirely — andendis the default justification for anybidiVisualtable.A difference of 1e-7 was enough to swap two paragraphs.
Sorting by the column each fragment occupies fixes it, and the ordinal is fill order, so it needs no RTL special case — an x comparison needed one because column 0 sits on the right there and document order descends in x.
The ordinal always resolves to a number. A sort key that is sometimes absent leaves the comparator mixing two metrics, which is not a total order;
Array.prototype.sortmay then return different orders for the same input, and it does differ between engines — so the product and this repo's own suite would not agree on the same page. Three steps, in order:columnIndex; ordinary paragraph fragments do not, so the remaining steps carry most fragments.Ties resolve on
y, then on arrival order, so the sort is total and stable.Tests
contracts540 ·layout-engine(bun) 970 ·layout-bridge1895 ·painters/dom1552.tsc -b tsconfig.references.jsonexits 0,vp fmt --checkclean.One pre-existing failure, unrelated and present on a clean base:
painters/dompersistent-page-surface"1,003 pages", a scale test that exceeds its budget under worker contention and passes in isolation.Every new and changed assertion was mutation-checked. Removing the gutter comparison fails three tests across two packages; ignoring the recorded
columnIndex, forcing the ordinal path off, reversing the ordinal order, dropping the margin conversion, and dropping theytie-break each fail a test named for that behaviour.New coverage: authored-vs-effective gutter pairs in both directions, an over-wide table whose origin lands in the neighbouring column, a shuffled input array (the paginator's array order is not a contract), and a left margin wide enough to be mistaken for a column offset.
Not included
Two findings from the same review are left out to keep this reviewable, both in files this branch does not touch:
resolveColumnLayoutre-pairsgapsafter dropping unusable widths whilenormalizeColumnLayoutslices positionally, so{count: 3, equalWidth: false, widths: [0, 200, 150], gaps: [10, 40]}yields[10]vs[40]. Direction-independent, and it also feedspage.columns.columnOwningSpanin the DOM painter attributes by greatest overlap. That drops an over-wide table at its own column's origin when the box reaches the strip width, and names the wrong column when the widths are genuinely unequal.