From 95cf92dee80c9281da68728674905462211cd51e Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Tue, 1 Sep 2026 08:59:03 +0300 Subject: [PATCH 01/20] fix(layout): place the first column on the right in RTL sections MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../contracts/src/column-layout.test.ts | 144 ++++++++++++++++++ .../contracts/src/column-layout.ts | 83 +++++++++- packages/layout-engine/contracts/src/index.ts | 15 ++ .../layout-bridge/src/incrementalLayout.ts | 17 ++- .../src/column-balancing.test.ts | 27 ++++ .../layout-engine/src/column-balancing.ts | 13 ++ .../layout-engine/layout-engine/src/index.ts | 5 + .../src/renderer-column-separators.test.ts | 28 ++++ .../painters/dom/src/renderer.ts | 9 +- 9 files changed, 330 insertions(+), 11 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index 5aca69b3e8..7ee520d4a2 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -88,6 +88,7 @@ describe('normalizeColumnLayout', () => { gap: 0, widths: [480], width: 480, + contentWidth: 480, }); }); @@ -97,6 +98,7 @@ describe('normalizeColumnLayout', () => { gap: 24, widths: [300, 300], width: 300, + contentWidth: 624, }); }); @@ -109,6 +111,7 @@ describe('normalizeColumnLayout', () => { widths: [100, 200], equalWidth: false, width: 200, + contentWidth: 624, }); }); @@ -123,6 +126,7 @@ describe('normalizeColumnLayout', () => { widths: [200, 400], equalWidth: false, width: 400, + contentWidth: 300, }); }); @@ -133,6 +137,7 @@ describe('normalizeColumnLayout', () => { gap: 24, widths: [300, 300], width: 300, + contentWidth: 624, }); }); @@ -143,6 +148,7 @@ describe('normalizeColumnLayout', () => { widths: [300, 300], equalWidth: true, width: 300, + contentWidth: 624, }); }); @@ -155,6 +161,7 @@ describe('normalizeColumnLayout', () => { widths: [192, 384], equalWidth: false, width: 384, + contentWidth: 624, }); }); @@ -163,6 +170,7 @@ describe('normalizeColumnLayout', () => { count: 1, gap: 0, width: 0, + contentWidth: 0, }); }); }); @@ -399,3 +407,139 @@ describe('columnRenderLayoutsEqual (SD-2629)', () => { expect(columnRenderLayoutsEqual({ count: 2, gap: 24 }, undefined)).toBe(false); }); }); + +describe('RTL section column order', () => { + /** A4 body: 602px of content, two equal columns, 48px gutter (720tw). */ + const twoEqual = (direction?: 'ltr' | 'rtl'): ColumnLayout => ({ + count: 2, + gap: 48, + ...(direction ? { direction } : {}), + }); + + it('puts the first column on the right without reordering indices', () => { + const ltr = getColumnGeometry(normalizeColumnLayout(twoEqual(), 602)); + const rtl = getColumnGeometry(normalizeColumnLayout(twoEqual('rtl'), 602)); + + // Fill order is the index; only the painted x moves. + expect(ltr.map((col) => col.index)).toEqual([0, 1]); + expect(rtl.map((col) => col.index)).toEqual([0, 1]); + expect(ltr.map((col) => col.x)).toEqual([0, 325]); + expect(rtl.map((col) => col.x)).toEqual([325, 0]); + // Widths and the strip's total span are untouched by the mirror. + expect(rtl.map((col) => col.width)).toEqual(ltr.map((col) => col.width)); + expect(Math.max(...rtl.map((col) => col.x + col.width))).toBe(602); + }); + + it('leaves an LTR layout exactly where it was', () => { + // The regression that matters most: every existing producer omits `direction`. + expect(getColumnGeometry(normalizeColumnLayout(twoEqual(), 602))).toEqual( + getColumnGeometry(normalizeColumnLayout({ count: 2, gap: 48 }, 602)), + ); + }); + + it('does not mirror a single column', () => { + const rtl = getColumnGeometry(normalizeColumnLayout({ count: 1, gap: 48, direction: 'rtl' }, 602)); + expect(rtl).toEqual([{ index: 0, x: 0, width: 602, gapAfter: 0 }]); + }); + + it('pins an underfilling explicit strip to the RIGHT margin, not the left', () => { + // Word does not scale authored widths to fill the content area, so two 192px columns in a 602px + // body leave 170px of slack. In LTR the slack falls on the right; mirrored, it must fall on the + // left. Mirroring about the strip's own span instead of the content area would leave the whole + // strip pinned left and merely swap the columns inside it — the document would still read as + // left-aligned, which is the bug this whole change exists to fix. + const columns: ColumnLayout = { + count: 2, + gap: 48, + equalWidth: false, + widths: [192, 192], + direction: 'rtl', + }; + const rtl = getColumnGeometry(normalizeColumnLayout(columns, 602)); + + expect(rtl.map((col) => col.x)).toEqual([410, 170]); + // Column 0's right edge is the right margin; the slack is on the left. + expect(rtl[0].x + rtl[0].width).toBe(602); + expect(Math.min(...rtl.map((col) => col.x))).toBe(170); + }); + + it('lets an overfull explicit strip run past the LEFT margin', () => { + // The mirror image of the documented LTR overflow: authored widths are not scaled down either, + // so the strip runs off the far margin — which in RTL is the left one. + const columns: ColumnLayout = { + count: 2, + gap: 24, + equalWidth: false, + widths: [200, 400], + direction: 'rtl', + }; + const rtl = getColumnGeometry(normalizeColumnLayout(columns, 300)); + + expect(rtl[0].x + rtl[0].width).toBe(300); + expect(rtl[1].x).toBe(-324); + }); + + it('mirrors the separator onto the same physical gutter', () => { + const columns: ColumnLayout = { + count: 2, + gap: 50, + equalWidth: false, + widths: [200, 352], + withSeparator: true, + direction: 'rtl', + }; + const rtl = getColumnGeometry(normalizeColumnLayout(columns, 602)); + + // Column 1 (left) spans [0,352], column 0 (right) spans [402,602]; the gutter is 352..402 + // and the separator sits at its midpoint. + expect(rtl[0]).toEqual({ index: 0, x: 402, width: 200, gapAfter: 50, separatorX: 377 }); + expect(rtl[1]).toEqual({ index: 1, x: 0, width: 352, gapAfter: 0 }); + expect(getColumnSeparatorPositions(rtl, 96)).toEqual([473]); + }); + + it('resolves a point to the column that visually contains it', () => { + const rtl = getColumnGeometry(normalizeColumnLayout(twoEqual('rtl'), 602)); + + // Right half is the FIRST column now; left half is the second. + expect(getColumnAtX(rtl, 400)).toBe(0); + expect(getColumnAtX(rtl, 100)).toBe(1); + // Edges stay inside their own column. + expect(getColumnAtX(rtl, 602)).toBe(0); + expect(getColumnAtX(rtl, 0)).toBe(1); + // A point in the gutter belongs to the column preceding it in fill order — the same rule the + // LTR branch applies, mirrored. This is what keeps a drag crossing the gutter from jumping. + expect(getColumnAtX(rtl, 300)).toBe(0); + }); + + it('keeps LTR hit testing byte-identical', () => { + const ltr = getColumnGeometry(normalizeColumnLayout(twoEqual(), 602)); + expect(getColumnAtX(ltr, 100)).toBe(0); + expect(getColumnAtX(ltr, 300)).toBe(0); + expect(getColumnAtX(ltr, 400)).toBe(1); + }); + + it('honors originX in both directions', () => { + const rtl = getColumnGeometry(normalizeColumnLayout(twoEqual('rtl'), 602)); + expect(getColumnX(rtl, 0, 96)).toBe(421); + expect(getColumnX(rtl, 1, 96)).toBe(96); + expect(getColumnAtX(rtl, 500, 96)).toBe(0); + expect(getColumnAtX(rtl, 200, 96)).toBe(1); + }); + + it('carries direction through clone and normalize', () => { + expect(cloneColumnLayout(twoEqual('rtl')).direction).toBe('rtl'); + expect(cloneColumnLayout(twoEqual()).direction).toBeUndefined(); + expect(normalizeColumnLayout(twoEqual('rtl'), 602).direction).toBe('rtl'); + expect(resolveColumnLayout(twoEqual('rtl')).direction).toBe('rtl'); + }); + + it('treats direction as paint-significant in both equality checks', () => { + // A section that only flips direction must split regions and invalidate the cache; treating it + // as equal would leave the previous geometry painted. + expect(columnLayoutsEqual(twoEqual('rtl'), twoEqual('ltr'))).toBe(false); + expect(columnRenderLayoutsEqual(twoEqual('rtl'), twoEqual('ltr'))).toBe(false); + // Absent means ltr, so omitting it must not read as a change. + expect(columnLayoutsEqual(twoEqual(), twoEqual('ltr'))).toBe(true); + expect(columnRenderLayoutsEqual(twoEqual(), twoEqual('ltr'))).toBe(true); + }); +}); diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index 83474a655f..e1ec4ba8e5 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -1,4 +1,4 @@ -import type { ColumnLayout } from './index.js'; +import type { BaseDirection, ColumnLayout } from './index.js'; /** * Resolved geometry for a single column. `x` and `separatorX` are CONTENT-RELATIVE (measured from @@ -15,7 +15,24 @@ export type ColumnGeometry = { separatorX?: number; }; -export type NormalizedColumnLayout = ColumnLayout & { width: number }; +export type NormalizedColumnLayout = ColumnLayout & { + width: number; + /** + * The content-area width the layout was normalized against, in px. + * + * Only RTL geometry reads it, and it exists because `width` above is the WIDEST column, not the + * strip: explicit widths are deliberately not scaled to fill the content area (Word renders an + * authored 2880tw column as 2880tw and leaves the slack), so a strip of explicit columns can be + * narrower — or wider — than the area it sits in. Mirroring such a strip about its own span would + * keep it pinned to the LEFT margin and only swap the columns inside it, which is not what Word + * does: the first column belongs against the RIGHT margin and the slack falls on the left. + * + * Optional because `getColumnGeometry` also accepts hand-built layouts (column balancing assembles + * one directly). When absent, RTL mirrors about the strip's own span, which is exact whenever the + * columns fill the area — always true in equal mode. + */ + contentWidth?: number; +}; export function widthsEqual(a?: number[], b?: number[]): boolean { if (!a && !b) return true; @@ -69,6 +86,7 @@ export function cloneColumnLayout(columns?: ColumnLayout): ColumnLayout { ...(Array.isArray(columns.gaps) ? { gaps: [...columns.gaps] } : {}), ...(columns.equalWidth !== undefined ? { equalWidth: columns.equalWidth } : {}), ...(columns.withSeparator !== undefined ? { withSeparator: columns.withSeparator } : {}), + ...(columns.direction !== undefined ? { direction: columns.direction } : {}), } : { count: 1, gap: 0 }; } @@ -116,7 +134,14 @@ export function resolveColumnLayout(input: ColumnLayout): ColumnLayout { * own `gaps[i]` when provided (SD-2629 step 4), falling back to the uniform scalar gap; the last * column has no following gap. The separator sits at the midpoint of that column's own gap. */ -function buildColumnGeometry(widths: number[], gap: number, withSeparator: boolean, gaps?: number[]): ColumnGeometry[] { +function buildColumnGeometry( + widths: number[], + gap: number, + withSeparator: boolean, + gaps?: number[], + direction?: BaseDirection, + contentWidth?: number, +): ColumnGeometry[] { const geometry: ColumnGeometry[] = []; let x = 0; for (let i = 0; i < widths.length; i += 1) { @@ -128,7 +153,26 @@ function buildColumnGeometry(widths: number[], gap: number, withSeparator: boole geometry.push(col); x += width + gapAfter; } - return geometry; + if (direction !== 'rtl' || geometry.length < 2) return geometry; + + // RTL: the FIRST column belongs on the right (ECMA-376 §17.6.1). Mirror rather than reverse the + // array: `index` stays the FILL order, so every consumer that walks columns 0..n-1 keeps filling + // in document order and only the painted x changes. `x` stays the LEFT edge of the column, which + // is what the whole geometry API and its callers mean by `x`. `gapAfter` is likewise untouched — + // it is the gap after this column in fill order, and in RTL that gap lies to its left, exactly + // where the mirrored x places it. + // + // The mirror axis is the CONTENT AREA, not the strip: explicit widths are not scaled to fill it + // (see normalizeColumnLayout), so a strip that underfills must end up against the RIGHT margin + // with the slack on the left — mirroring about the strip's own span would leave it pinned left + // and merely swap the columns inside it. Falls back to the span when the area is unknown, which + // is exact whenever the columns fill it (always so in equal mode). + const span = contentWidth ?? x; + return geometry.map((col) => ({ + ...col, + x: span - (col.x + col.width), + ...(col.separatorX === undefined ? {} : { separatorX: span - col.separatorX }), + })); } export function normalizeColumnLayout( @@ -176,6 +220,8 @@ export function normalizeColumnLayout( gap: 0, width: Math.max(0, contentWidth), ...(input?.withSeparator !== undefined ? { withSeparator: input.withSeparator } : {}), + ...(input?.direction !== undefined ? { direction: input.direction } : {}), + contentWidth: Math.max(0, contentWidth), }; } @@ -186,7 +232,9 @@ export function normalizeColumnLayout( ...(gaps && gaps.length > 0 ? { gaps } : {}), ...(input?.equalWidth !== undefined ? { equalWidth: input.equalWidth } : {}), ...(input?.withSeparator !== undefined ? { withSeparator: input.withSeparator } : {}), + ...(input?.direction !== undefined ? { direction: input.direction } : {}), width, + contentWidth: Math.max(0, contentWidth), }; } @@ -207,7 +255,14 @@ export function getColumnGeometry(normalized: NormalizedColumnLayout): ColumnGeo Array.isArray(normalized.widths) && normalized.widths.length > 0 ? normalized.widths : new Array(count).fill(normalized.width); - return buildColumnGeometry(widths, normalized.gap, Boolean(normalized.withSeparator), normalized.gaps); + return buildColumnGeometry( + widths, + normalized.gap, + Boolean(normalized.withSeparator), + normalized.gaps, + normalized.direction, + normalized.contentWidth, + ); } // --------------------------------------------------------------------------- @@ -242,13 +297,23 @@ export function getColumnSeparatorPositions(geometry: ColumnGeometry[], originX .map((col) => originX + (col.separatorX as number)); } -/** Index of the column containing absolute `x` (clicks in a gap map to the preceding column). */ +/** + * Index of the column containing absolute `x` (clicks in a gap map to the preceding column). + * + * The walk is direction-aware and cannot assume ascending `x`: in an RTL section column 0 sits on + * the right, so `x` DESCENDS with the index. The mirrored branch keeps the same rule the LTR branch + * states — a point in a gap belongs to the column that precedes it in FILL order — which is what + * makes a drag that crosses the gutter keep extending from the column it is leaving instead of + * jumping. Direction is read off the geometry rather than taken as an argument, so every existing + * caller keeps working unchanged. + */ export function getColumnAtX(geometry: ColumnGeometry[], x: number, originX = 0): number { if (geometry.length === 0) return 0; const cx = x - originX; + const mirrored = geometry.length > 1 && geometry[1].x < geometry[0].x; let result = 0; for (const col of geometry) { - if (cx >= col.x) result = col.index; + if (mirrored ? cx <= col.x + col.width : cx >= col.x) result = col.index; else break; } return result; @@ -263,6 +328,7 @@ export function columnLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): boolean a.gap === b.gap && a.equalWidth === b.equalWidth && Boolean(a.withSeparator) === Boolean(b.withSeparator) && + (a.direction ?? 'ltr') === (b.direction ?? 'ltr') && widthsEqual(a.widths, b.widths) && widthsEqual(a.gaps, b.gaps) ); @@ -287,6 +353,9 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo if (resolveColumnCount(a) !== resolveColumnCount(b)) return false; if ((a.gap ?? 0) !== (b.gap ?? 0)) return false; if (Boolean(a.withSeparator) !== Boolean(b.withSeparator)) return false; + // Direction IS paint-significant: it decides which side column 0 lands on, so two layouts that + // differ only here must split regions and invalidate the normalized-columns cache. + if ((a.direction ?? 'ltr') !== (b.direction ?? 'ltr')) return false; if (mode === 'explicit') { const ra = resolveColumnLayout(a); const rb = resolveColumnLayout(b); diff --git a/packages/layout-engine/contracts/src/index.ts b/packages/layout-engine/contracts/src/index.ts index e8b30134aa..447aa3d6cb 100644 --- a/packages/layout-engine/contracts/src/index.ts +++ b/packages/layout-engine/contracts/src/index.ts @@ -21,6 +21,7 @@ export type { } from './direction-context.js'; export { getParagraphInlineDirection, getTableVisualDirection } from './direction-context.js'; import type { + BaseDirection, ParagraphDirectionContext, RunBidiContext, RunScriptContext, @@ -2886,6 +2887,20 @@ export type ColumnLayout = { * mode uses the scalar `gap`. When absent, consumers fall back to the uniform `gap`. (SD-2629) */ gaps?: number[]; + /** + * Section page direction, from `w:sectPr/w:bidi`. Decides which side the FIRST column sits on: + * `'ltr'` (default) fills left to right, `'rtl'` fills right to left, matching Word. + * + * Per ECMA-376 §17.6.1 a section's `w:bidi` governs section-level chrome — page numbers, gutters + * and columns — and is independent of the paragraph inline direction (§17.3.1.6). It is carried + * here, on the column layout itself, because `getColumnGeometry` is the single source every + * column consumer reads for positioning (fill, hit testing, separators, balancing, floating + * anchors, footnotes); threading the axis alongside the widths keeps those consumers from having + * to re-derive it, and keeps them from disagreeing. + * + * Absent means `'ltr'`. Every existing producer therefore keeps its current geometry unchanged. + */ + direction?: BaseDirection; }; /** diff --git a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts index 7f4be68d14..8745d7ac63 100644 --- a/packages/layout-engine/layout-bridge/src/incrementalLayout.ts +++ b/packages/layout-engine/layout-bridge/src/incrementalLayout.ts @@ -1643,14 +1643,25 @@ const assignFootnotesToColumns = ( if (fragment?.kind === 'table' && typeof fragment.columnIndex === 'number') { columnIndex = Math.max(0, Math.min(columns.count - 1, fragment.columnIndex)); } else if (fragment && typeof fragment.x === 'number') { - // Geometry-derived midpoint assignment: assign the ref to the column whose right edge plus - // half its own gap the fragment falls before. Per-column widths/gaps come from the resolved + // Geometry-derived midpoint assignment: assign the ref to the column whose far edge plus + // half its own gap the fragment falls short of. Per-column widths/gaps come from the resolved // geometry, preserving the prior midpoint rule. The old uniform-stride branch was unreachable // for count>1 (normalized columns always carry widths). (SD-2629 4c) + // + // "Far edge" is direction-relative: in an RTL section column 0 sits on the right, so x + // DESCENDS with the index and the fragment must be compared against the column's LEFT edge + // minus half its gap instead. Walking the geometry with the LTR test in an RTL section + // matched column 0 for every fragment, which collapsed all of a page's footnotes into the + // first column's group — the left column's notes printed under the right column and its own + // note area stayed empty. const geometry = getColumnGeometry(columns); + const mirrored = geometry.length > 1 && geometry[1].x < geometry[0].x; columnIndex = Math.max(0, geometry.length - 1); for (const col of geometry) { - if (fragment.x < columns.left + col.x + col.width + col.gapAfter / 2) { + const boundary = mirrored + ? columns.left + col.x - col.gapAfter / 2 + : columns.left + col.x + col.width + col.gapAfter / 2; + if (mirrored ? fragment.x >= boundary : fragment.x < boundary) { columnIndex = col.index; break; } diff --git a/packages/layout-engine/layout-engine/src/column-balancing.test.ts b/packages/layout-engine/layout-engine/src/column-balancing.test.ts index 26b02b30e4..8e2158ab5a 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.test.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.test.ts @@ -347,6 +347,33 @@ describe('balanceSectionOnPage', () => { return { fragments, measureMap, blockSectionMap }; } + it('keeps an RTL section right-to-left on the balanced page', () => { + // Balancing REBUILDS the geometry and then overwrites every fragment's x from it, so a dropped + // direction does not fail loudly: the last page of a two-column Hebrew section would simply be + // laid out left-to-right while every earlier page of the same section was right-to-left. + const top = 96; + const { fragments, measureMap, blockSectionMap } = buildSectionFixture(2, 6, 20, top); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, direction: 'rtl', contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // The FIRST three paragraphs land in the RIGHT column (x = left margin + 288 + 48), the last + // three in the left one — the mirror image of the LTR case above. + expect(fragments.slice(0, 3).map((f) => f.x)).toEqual([432, 432, 432]); + expect(fragments.slice(3).map((f) => f.x)).toEqual([96, 96, 96]); + }); + it('balances the target section and returns the tallest balanced column bottom', () => { // 6 equal paragraphs in a 2-col section → 3+3 balanced, tallest col ends at top + 3×20 = top + 60. const top = 96; diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index 7a5dcee3b9..bbc6bbda18 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -7,6 +7,7 @@ */ import { getColumnGeometry, getColumnX, hasGenuinelyUnequalExplicitColumnWidths } from '@superdoc/contracts'; +import type { BaseDirection } from '@superdoc/contracts'; // ============================================================================ // Types and Interfaces @@ -657,6 +658,16 @@ export interface SectionColumnLayout { */ gaps?: number[]; equalWidth?: boolean; + /** + * Section page direction (`w:sectPr/w:bidi`) and the content width it was normalized against. + * + * Declared here — and not left to the structural subset above — because balancing REBUILDS the + * geometry and then overwrites every fragment's `x` from it. A balanced page that dropped these + * would be laid out left-to-right while every earlier page of the same RTL section was laid out + * right-to-left, so the last page of a two-column Hebrew section would visibly flip. + */ + direction?: BaseDirection; + contentWidth?: number; } export interface BalanceSectionOnPageArgs { @@ -864,6 +875,8 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu width: columnWidth, ...(Array.isArray(sectionColumns.widths) ? { widths: sectionColumns.widths } : {}), ...(Array.isArray(sectionColumns.gaps) ? { gaps: sectionColumns.gaps } : {}), + ...(sectionColumns.direction !== undefined ? { direction: sectionColumns.direction } : {}), + ...(sectionColumns.contentWidth !== undefined ? { contentWidth: sectionColumns.contentWidth } : {}), }); const columnX = (columnIndex: number): number => getColumnX(balancedGeometry, columnIndex, args.margins.left); diff --git a/packages/layout-engine/layout-engine/src/index.ts b/packages/layout-engine/layout-engine/src/index.ts index bc4ec340cf..692f670f17 100644 --- a/packages/layout-engine/layout-engine/src/index.ts +++ b/packages/layout-engine/layout-engine/src/index.ts @@ -5193,6 +5193,11 @@ function toBalancingColumns(normalized: NormalizedColumns): SectionColumnLayout ...(Array.isArray(normalized.widths) ? { widths: normalized.widths } : {}), ...(Array.isArray(normalized.gaps) ? { gaps: normalized.gaps } : {}), ...(normalized.equalWidth !== undefined ? { equalWidth: normalized.equalWidth } : {}), + // Direction and the content width it was measured against travel with the widths: balancing + // rebuilds the geometry and overwrites fragment x from it, so dropping them here would lay the + // balanced page out left-to-right inside an otherwise right-to-left section. + ...(normalized.direction !== undefined ? { direction: normalized.direction } : {}), + ...(normalized.contentWidth !== undefined ? { contentWidth: normalized.contentWidth } : {}), }; } diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index 564b0f863e..b2f7a3c483 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -83,6 +83,34 @@ describe('DomPainter renderColumnSeparators', () => { expect(seps[0].style.height).toBe('864px'); }); + it('gates an RTL separator on the LEFT column, which is the later one there', () => { + // In an RTL section column 0 sits on the right, so "content past the separator" — the + // condition Word uses to decide whether to draw the line at all — is content to its LEFT. + // With the LTR test, a fragment that never left the FIRST column satisfies `x >= separatorX` + // and the painter draws a line Word does not draw. + const firstColumnOnly = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [fragAt(432)], + }); + paintOnce(buildLayout(firstColumnOnly), mount); + expect(querySeparators(mount)).toHaveLength(0); + + mount.remove(); + mount = document.createElement('div'); + document.body.append(mount); + + const bothColumns = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [fragAt(432), fragAt(96)], + }); + paintOnce(buildLayout(bothColumns), mount); + + const seps = querySeparators(mount); + expect(seps).toHaveLength(1); + // Equal columns fill the content area, so the gutter — and the line in it — is where it was. + expect(seps[0].style.left).toBe('408px'); + }); + it('draws count-1 separators for 3 equal columns', () => { const page = buildPage({ columns: { count: 3, gap: 48, withSeparator: true }, diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index ce9fa00941..410b6e6f28 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1793,8 +1793,15 @@ export class DomPainter { // separator on whether any fragment sits past it within the region. const fragmentsInRegion = page.items.filter((item) => item.y >= yStart - 0.5 && item.y < yEnd + 0.5); + // "Past the separator" means "in a later column", which is the LEFT side in an RTL section: + // there column 0 sits on the right, so an `f.x >= separatorX` test is satisfied by content + // that never left the FIRST column and the gate would draw a line Word does not draw. + const laterColumnsAreLeft = columns.direction === 'rtl'; + for (const separatorX of separatorPositions) { - const hasContentPastSeparator = fragmentsInRegion.some((f) => f.x >= separatorX); + const hasContentPastSeparator = fragmentsInRegion.some((f) => + laterColumnsAreLeft ? f.x < separatorX : f.x >= separatorX, + ); if (!hasContentPastSeparator) continue; const separatorEl = this.doc.createElement('div'); From 99cc23420a0cba2a8f7dfd4887e0d6fc0c0bad2a Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Tue, 1 Sep 2026 19:19:38 +0300 Subject: [PATCH 02/20] fix(layout): order RTL balancing by document order, and cover the axis end to end Follow-up to the RTL column-order fix in this branch, addressing review feedback on #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`. --- .../contracts/src/column-layout.test.ts | 54 ++++- .../contracts/src/column-layout.ts | 28 ++- .../contracts/src/graphic-placement.ts | 7 + .../test/footnoteColumnPlacement.test.ts | 63 ++++++ .../layout-bridge/test/position-hit.test.ts | 29 +++ .../src/column-balancing.test.ts | 46 +++++ .../layout-engine/src/column-balancing.ts | 16 +- .../layout-engine/src/layout-paragraph.ts | 5 +- .../src/section-breaks-rtl-columns.test.ts | 193 ++++++++++++++++++ .../src/test-helpers/section-test-utils.ts | 21 +- .../tests/src/test-helpers/to-flow-blocks.ts | 26 +++ .../src/layout-rtl-column-direction.ts | 34 +++ 12 files changed, 506 insertions(+), 16 deletions(-) create mode 100644 packages/layout-engine/tests/src/section-breaks-rtl-columns.test.ts create mode 100644 tests/consumer-typecheck/src/layout-rtl-column-direction.ts diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index 7ee520d4a2..f1b77fec99 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -437,11 +437,63 @@ describe('RTL section column order', () => { ); }); - it('does not mirror a single column', () => { + it('is a no-op for a single column that fills the content area', () => { const rtl = getColumnGeometry(normalizeColumnLayout({ count: 1, gap: 48, direction: 'rtl' }, 602)); expect(rtl).toEqual([{ index: 0, x: 0, width: 602, gapAfter: 0 }]); }); + it('pins a single underfilling explicit column to the RIGHT margin', () => { + // One column has no order to flip, but it still has a side. `` + // with an authored width narrower than the body leaves slack, and in an RTL section that slack + // belongs on the left — the same axis rule the multi-column strip follows. + const rtl = getColumnGeometry( + normalizeColumnLayout({ count: 1, gap: 0, equalWidth: false, widths: [200], direction: 'rtl' }, 602), + ); + expect(rtl).toEqual([{ index: 0, x: 402, width: 200, gapAfter: 0 }]); + + // LTR keeps the slack on the right, as before. + const ltr = getColumnGeometry(normalizeColumnLayout({ count: 1, gap: 0, equalWidth: false, widths: [200] }, 602)); + expect(ltr).toEqual([{ index: 0, x: 0, width: 200, gapAfter: 0 }]); + }); + + it('mirrors three columns with per-column gaps onto the right physical gutters', () => { + const columns: ColumnLayout = { + count: 3, + gap: 0, + equalWidth: false, + widths: [100, 150, 200], + gaps: [20, 40], + withSeparator: true, + direction: 'rtl', + }; + const rtl = getColumnGeometry(normalizeColumnLayout(columns, 602)); + + // Fill order still runs 0,1,2; the strip is laid out right to left from the right margin. + expect(rtl.map((col) => col.index)).toEqual([0, 1, 2]); + expect(rtl.map((col) => col.x)).toEqual([502, 332, 92]); + // Column 0's right edge is the right margin, and each separator is the midpoint of the gutter + // between the columns it actually separates. + expect(rtl[0].x + rtl[0].width).toBe(602); + expect(getColumnSeparatorPositions(rtl, 0)).toEqual([492, 312]); + // Hit testing descends with the index and every column claims its own span. + expect(getColumnAtX(rtl, 550)).toBe(0); + expect(getColumnAtX(rtl, 400)).toBe(1); + expect(getColumnAtX(rtl, 150)).toBe(2); + }); + + it('clamps a negative per-column gap so an LTR layout cannot read as mirrored', () => { + // OOXML cannot express a negative gutter (`w:space` is unsigned), but a host-built layout can. + // Left unclamped, `gaps: [-100]` pulls column 1 back behind column 0 and the direction-aware + // consumers — which infer the axis from x monotonicity — would answer hit tests as if the + // upright layout were mirrored. + const ltr = getColumnGeometry( + normalizeColumnLayout({ count: 2, gap: 0, equalWidth: false, widths: [50, 50], gaps: [-100] }, 602), + ); + expect(ltr.map((col) => col.x)).toEqual([0, 50]); + expect(getColumnAtX(ltr, 20)).toBe(0); + expect(getColumnAtX(ltr, 80)).toBe(1); + }); + it('pins an underfilling explicit strip to the RIGHT margin, not the left', () => { // Word does not scale authored widths to fill the content area, so two 192px columns in a 602px // body leave 170px of slack. In LTR the slack falls on the right; mirrored, it must fall on the diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index e1ec4ba8e5..f167d958aa 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -153,14 +153,17 @@ function buildColumnGeometry( geometry.push(col); x += width + gapAfter; } - if (direction !== 'rtl' || geometry.length < 2) return geometry; + if (direction !== 'rtl') return geometry; - // RTL: the FIRST column belongs on the right (ECMA-376 §17.6.1). Mirror rather than reverse the - // array: `index` stays the FILL order, so every consumer that walks columns 0..n-1 keeps filling - // in document order and only the painted x changes. `x` stays the LEFT edge of the column, which - // is what the whole geometry API and its callers mean by `x`. `gapAfter` is likewise untouched — - // it is the gap after this column in fill order, and in RTL that gap lies to its left, exactly - // where the mirrored x places it. + // RTL: the FIRST column belongs on the right (ECMA-376 §17.6.1). A single column is mirrored too: + // it is a no-op when the column fills the content area, but an explicit column that underfills it + // still belongs against the RIGHT margin, by the same axis rule as a multi-column strip. + // + // Mirror rather than reverse the array: `index` stays the FILL order, so every consumer that + // walks columns 0..n-1 keeps filling in document order and only the painted x changes. `x` stays + // the LEFT edge of the column, which is what the whole geometry API and its callers mean by `x`. + // `gapAfter` is likewise untouched — it is the gap after this column in fill order, and in RTL + // that gap lies to its left, exactly where the mirrored x places it. // // The mirror axis is the CONTENT AREA, not the strip: explicit widths are not scaled to fill it // (see normalizeColumnLayout), so a strip that underfills must end up against the RIGHT margin @@ -209,8 +212,17 @@ export function normalizeColumnLayout( } // Per-column gaps drive geometry in explicit mode (step 4); equal mode uses the uniform gap. + // + // Clamped to >= 0 like the scalar `gap` above. OOXML cannot express a negative gutter — `w:space` + // is ST_TwipsMeasure, unsigned — and letting one through breaks the invariant the geometry API + // relies on: that in an LTR layout `x` rises with the column index. Direction-aware consumers read + // that monotonicity to tell a mirrored strip from an upright one, so a negative gap wide enough to + // pull a column back behind its predecessor would make an LTR layout answer hit tests as if it + // were RTL. const gaps = - explicitWidths.length > 0 && Array.isArray(input?.gaps) ? input.gaps.slice(0, Math.max(0, count - 1)) : undefined; + explicitWidths.length > 0 && Array.isArray(input?.gaps) + ? input.gaps.slice(0, Math.max(0, count - 1)).map((value) => Math.max(0, value)) + : undefined; const width = widths.reduce((max, value) => Math.max(max, value), 0); diff --git a/packages/layout-engine/contracts/src/graphic-placement.ts b/packages/layout-engine/contracts/src/graphic-placement.ts index 744063713b..b80ead7eb6 100644 --- a/packages/layout-engine/contracts/src/graphic-placement.ts +++ b/packages/layout-engine/contracts/src/graphic-placement.ts @@ -1,4 +1,5 @@ import { getColumnGeometry, getColumnX } from './column-layout.js'; +import type { BaseDirection } from './direction-context.js'; /** ECMA-376 Part 1 §20.4.3.4 (`ST_RelFromH`). */ export const ANCHOR_H_RELATIVE_VALUES = [ @@ -105,6 +106,12 @@ export type ColumnLayoutForAnchor = { // stride; equal columns reduce to the old stride. (SD-2629) widths?: number[]; gaps?: number[]; + // Section page direction and the content width it was normalized against, both read by + // getColumnGeometry. Declared rather than left to structural pass-through: a column-relative + // anchor in an RTL section must resolve against the mirrored geometry, and silently dropping + // these would place it against the wrong margin with no type error to catch it. + direction?: BaseDirection; + contentWidth?: number; }; /** diff --git a/packages/layout-engine/layout-bridge/test/footnoteColumnPlacement.test.ts b/packages/layout-engine/layout-bridge/test/footnoteColumnPlacement.test.ts index 4fcb62854b..73dc0fc1b7 100644 --- a/packages/layout-engine/layout-bridge/test/footnoteColumnPlacement.test.ts +++ b/packages/layout-engine/layout-bridge/test/footnoteColumnPlacement.test.ts @@ -81,6 +81,69 @@ describe('Footnotes in columns', () => { expect(footnoteTwoFragment?.x).toBeCloseTo(columnTwoX, 2); }); + it('places footnotes in the mirrored column of their reference in an RTL section', async () => { + // Footnote refs are assigned to a column by comparing the reference fragment's x against each + // column's far edge plus half its gap. "Far edge" is direction-relative: in an RTL section + // column 0 sits on the right and x DESCENDS with the index, so the left-to-right test matches + // column 0 for every fragment and collapses the whole page's notes into the first column's + // group — the left column's notes print under the right column and its own note area is empty. + const paragraphOne = makeParagraph('para-1', 'Column 1 text', 0); + const columnBreak: FlowBlock = { kind: 'columnBreak', id: 'col-break-1' }; + const paragraphTwo = makeParagraph('para-2', 'Column 2 text', 40); + + const footnoteOne = makeParagraph('footnote-1-0-paragraph', 'Footnote one', 0); + const footnoteTwo = makeParagraph('footnote-2-0-paragraph', 'Footnote two', 0); + + const measureBlock = vi.fn(async (block: FlowBlock) => { + if (block.kind === 'columnBreak') { + return { kind: 'columnBreak' } as Measure; + } + const textLength = block.kind === 'paragraph' ? (block.runs?.[0]?.text?.length ?? 1) : 1; + const lineHeight = block.id.startsWith('footnote-') ? 10 : 18; + return makeMeasure(lineHeight, textLength); + }); + + const columns = { count: 2, gap: 20, direction: 'rtl' as const }; + const margins = { top: 60, right: 60, bottom: 60, left: 60 }; + const pageSize = { w: 600, h: 800 }; + + const result = await incrementalLayout( + [], + null, + [paragraphOne, columnBreak, paragraphTwo], + { + pageSize, + margins, + columns, + footnotes: { + refs: [ + { id: '1', pos: 2 }, + { id: '2', pos: 42 }, + ], + blocksById: new Map([ + ['1', [footnoteOne]], + ['2', [footnoteTwo]], + ]), + }, + }, + measureBlock, + ); + + const page = result.layout.pages[0]; + const columnWidth = (pageSize.w - margins.left - margins.right - columns.gap) / columns.count; + // Mirrored: fill column 0 is the RIGHT one, fill column 1 the left. + const firstColumnX = margins.left + columnWidth + columns.gap; + const secondColumnX = margins.left; + + const footnoteOneFragment = page.fragments.find((fragment) => fragment.blockId === footnoteOne.id); + const footnoteTwoFragment = page.fragments.find((fragment) => fragment.blockId === footnoteTwo.id); + + expect(footnoteOneFragment?.x).toBeCloseTo(firstColumnX, 2); + expect(footnoteTwoFragment?.x).toBeCloseTo(secondColumnX, 2); + // The two notes must land in DIFFERENT columns; collapsing them into one is the failure mode. + expect(footnoteOneFragment?.x).not.toBeCloseTo(footnoteTwoFragment?.x ?? 0, 2); + }); + it('keeps footnotes in the owning column for wide overflow tables', async () => { const paragraphOne = makeParagraph('para-1', 'Column 1 text', 0); const columnBreak: FlowBlock = { kind: 'columnBreak', id: 'col-break-1' }; diff --git a/packages/layout-engine/layout-bridge/test/position-hit.test.ts b/packages/layout-engine/layout-bridge/test/position-hit.test.ts index 421d93ccdb..4fa56d90c7 100644 --- a/packages/layout-engine/layout-bridge/test/position-hit.test.ts +++ b/packages/layout-engine/layout-bridge/test/position-hit.test.ts @@ -111,6 +111,35 @@ describe('determineColumn (SD-2629: resolved per-column boundaries)', () => { expect(determineColumn(layout, 540, page)).toBe(2); }); + it('resolves a click to the visually containing column in an RTL section', () => { + // In an RTL section column 0 sits against the RIGHT margin, so a click on the right half of the + // page selects the FIRST column. Resolving this with the left-to-right rule sends every click to + // the wrong column — the issue's "clicks will select the wrong column". + const columns = { count: 3, gap: 24, direction: 'rtl' as const }; + const page = { + columns, + margins: { left: 96, right: 96 }, + size: { w: 816, h: 1056 }, + } as unknown as Page; + const layout = { pageSize: { w: 816, h: 1056 }, columns, pages: [page] } as unknown as Layout; + + // Content width 624 -> 192px columns. Mirrored, column 0 spans 528..720, column 1 336..528, + // column 2 96..288 (absolute). + expect(determineColumn(layout, 700, page)).toBe(0); + expect(determineColumn(layout, 400, page)).toBe(1); + expect(determineColumn(layout, 150, page)).toBe(2); + // The outer margins stay with their own end columns. + expect(determineColumn(layout, 816, page)).toBe(0); + expect(determineColumn(layout, 0, page)).toBe(2); + + // Same geometry without the direction keeps answering left to right. + const ltrColumns = { count: 3, gap: 24 }; + const ltrPage = { ...page, columns: ltrColumns } as unknown as Page; + const ltrLayout = { pageSize: { w: 816, h: 1056 }, columns: ltrColumns, pages: [ltrPage] } as unknown as Layout; + expect(determineColumn(ltrLayout, 700, ltrPage)).toBe(2); + expect(determineColumn(ltrLayout, 150, ltrPage)).toBe(0); + }); + it('maps a hit to its mid-page column region, not the page-start columns (SD-2629)', () => { // A continuous section break splits the page: region 0 (y 96-300) is single-column; region 1 // (y 300-700) is two-column. page.columns is only the page-START config (single column), so a diff --git a/packages/layout-engine/layout-engine/src/column-balancing.test.ts b/packages/layout-engine/layout-engine/src/column-balancing.test.ts index 8e2158ab5a..e49901ca8c 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.test.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.test.ts @@ -374,6 +374,52 @@ describe('balanceSectionOnPage', () => { expect(fragments.slice(3).map((f) => f.x)).toEqual([96, 96, 96]); }); + it('reads an already-columnised RTL page in document order, not left to right', () => { + // Balancing re-derives document order from the fragments' current positions, because the + // paginator fills column 0 top-to-bottom before moving on. In an RTL section column 0 is the + // RIGHT one, so document order DESCENDS in x; ordering the page left-to-right would feed the + // balancer the trailing column first and scramble the reading order of the balanced page. + const top = 96; + const RIGHT = 432; // left margin 96 + column width 288 + gap 48 + const LEFT = 96; + // Paragraphs 0-3 were laid out in the right column, 4-5 spilled into the left one. + const placements: Array<{ x: number; y: number }> = [ + { x: RIGHT, y: top }, + { x: RIGHT, y: top + 20 }, + { x: RIGHT, y: top + 40 }, + { x: RIGHT, y: top + 60 }, + { x: LEFT, y: top }, + { x: LEFT, y: top + 20 }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ blockId: id, x: placement.x, y: placement.y, width: 288, kind: 'para' }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, direction: 'rtl', contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // 3+3 balance, still in document order: 0-2 in the right column, 3-5 in the left one. + expect(fragments.map((f) => f.x)).toEqual([RIGHT, RIGHT, RIGHT, LEFT, LEFT, LEFT]); + expect(fragments.map((f) => f.y)).toEqual([top, top + 20, top + 40, top, top + 20, top + 40]); + }); + it('balances the target section and returns the tallest balanced column bottom', () => { // 6 equal paragraphs in a 2-col section → 3+3 balanced, tallest col ends at top + 3×20 = top + 60. const top = 96; diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index bbc6bbda18..6fe1dd848c 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -794,12 +794,18 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu precedingHeight: precedingHeightBeforeTable, }); - // Order fragments in document order: by current column (x → left-to-right), - // then by y within each column. During unbalanced layout the paginator fills - // column 0 top-to-bottom, then column 1, etc. — so (x, y) preserves the - // original sequence. + // Order fragments in document order: by current column, then by y within each column. During + // unbalanced layout the paginator fills column 0 top-to-bottom, then column 1, etc. — so column + // order followed by y preserves the original sequence. + // + // Which way "column order" runs across the page is direction-relative. In an RTL section column 0 + // is the RIGHT one, so document order DESCENDS in x; sorting ascending there would feed the + // balancer the trailing column first and silently scramble the balanced page's reading order, + // since the balanced x/y are then written back onto the fragments in this order. + const columnOrder = + sectionColumns.direction === 'rtl' ? (a: number, b: number) => b - a : (a: number, b: number) => a - b; const ordered = [...sectionFragments].sort((a, b) => { - if (a.x !== b.x) return a.x - b.x; + if (a.x !== b.x) return columnOrder(a.x, b.x); return a.y - b.y; }); diff --git a/packages/layout-engine/layout-engine/src/layout-paragraph.ts b/packages/layout-engine/layout-engine/src/layout-paragraph.ts index 1115b1f972..f20843c160 100644 --- a/packages/layout-engine/layout-engine/src/layout-paragraph.ts +++ b/packages/layout-engine/layout-engine/src/layout-paragraph.ts @@ -19,6 +19,7 @@ import type { TableAnchor, TableWrap, ParagraphLineRegion, + ColumnLayoutForAnchor, } from '@superdoc/contracts'; import { computeFragmentPmRange, @@ -469,7 +470,9 @@ export type ParagraphAnchorsContext = { columnWidth: number; pageWidth: number; pageMargins: PageMargins; - columns: { width: number; gap: number; count: number }; + // Carries the resolved column layout through to resolveAnchoredGraphicX, direction included: a + // column-relative anchor in an RTL section resolves against the mirrored geometry. + columns: ColumnLayoutForAnchor; placedAnchoredIds: Set; }; diff --git a/packages/layout-engine/tests/src/section-breaks-rtl-columns.test.ts b/packages/layout-engine/tests/src/section-breaks-rtl-columns.test.ts new file mode 100644 index 0000000000..26179f7c94 --- /dev/null +++ b/packages/layout-engine/tests/src/section-breaks-rtl-columns.test.ts @@ -0,0 +1,193 @@ +/** + * RTL Section Column Order Tests + * + * A section carrying `w:sectPr/w:bidi` fills its columns right to left: the first paragraph belongs + * in the RIGHT column and overflow spills into the left one (ECMA-376 §17.6.1). Column widths, the + * gutter and the text direction inside each column are governed elsewhere and must not move. + * + * Regression coverage for the issue where fill order was a fixed left-to-right and the section + * direction was never consulted on the column axis. + * + * @module section-breaks-rtl-columns.test + */ + +import { describe, it, expect, beforeEach } from 'vite-plus/test'; +import type { Layout } from '@superdoc/contracts'; +import { + createPMDocWithSections, + convertAndLayout, + pmToFlowBlocks, + getSectionBreaks, + PAGE_SIZES, + resetBlockIdCounter, + type TestSectionProps, +} from './test-helpers/section-test-utils.js'; + +/** Enough numbered paragraphs to overflow the first column, so the fill order is observable. */ +const NUMBERED_PARAGRAPHS = Array.from( + { length: 24 }, + (_, index) => `Paragraph number ${index + 1}. ${'filler '.repeat(20)}`, +); + +const TWO_COLUMN_SECTION: TestSectionProps = { + type: 'nextPage', + pageSize: PAGE_SIZES.LETTER_PORTRAIT, + columns: { count: 2, gap: 48 }, +}; + +const layoutTwoColumnSection = async (props: TestSectionProps): Promise => { + const pmDoc = createPMDocWithSections([{ paragraphs: NUMBERED_PARAGRAPHS }], props); + return convertAndLayout(pmDoc, { pageSize: PAGE_SIZES.LETTER_PORTRAIT }); +}; + +/** `blockId` is `-paragraph`, which is the document order of the source array. */ +const paragraphIndex = (blockId: string): number => Number.parseInt(blockId, 10); + +type ColumnReadout = { + /** Distinct fragment x values on the page, ascending. */ + columnXs: number[]; + /** Paragraph indices in each column, keyed by that column's x, each in visual top-to-bottom order. */ + indicesByX: Map; +}; + +const readFirstPage = (layout: Layout): ColumnReadout => { + const fragments = [...layout.pages[0].fragments] + .filter((fragment) => fragment.blockId.endsWith('-paragraph')) + .sort((a, b) => a.y - b.y); + + const indicesByX = new Map(); + for (const fragment of fragments) { + const x = Math.round(fragment.x); + if (!indicesByX.has(x)) indicesByX.set(x, []); + indicesByX.get(x)!.push(paragraphIndex(fragment.blockId)); + } + + return { columnXs: [...indicesByX.keys()].sort((a, b) => a - b), indicesByX }; +}; + +/** True when `indices` is 0,1,2,… — i.e. this column holds a contiguous prefix of the document. */ +const isAscendingPrefix = (indices: number[]): boolean => indices.every((value, i) => value === i); + +describe('Section Breaks - RTL Column Order', () => { + beforeEach(() => { + resetBlockIdCounter(); + }); + + it('starts an RTL section in the right column and overflows into the left one', async () => { + const layout = await layoutTwoColumnSection({ ...TWO_COLUMN_SECTION, bidi: true }); + const { columnXs, indicesByX } = readFirstPage(layout); + + expect(columnXs).toHaveLength(2); + const [leftX, rightX] = columnXs; + const right = indicesByX.get(rightX)!; + const left = indicesByX.get(leftX)!; + + // Paragraph 1 opens the section, in the RIGHT column. + expect(right[0]).toBe(0); + // The right column holds a contiguous prefix and the left column continues it, so reading + // right-then-left reproduces document order exactly. + expect(isAscendingPrefix(right)).toBe(true); + expect(left).toEqual(left.map((_, i) => right.length + i)); + // Both columns are actually used — otherwise "first column is on the right" proves nothing. + expect(left.length).toBeGreaterThan(0); + }); + + it('leaves an LTR section filling left to right', async () => { + const layout = await layoutTwoColumnSection(TWO_COLUMN_SECTION); + const { columnXs, indicesByX } = readFirstPage(layout); + + const [leftX, rightX] = columnXs; + expect(indicesByX.get(leftX)![0]).toBe(0); + expect(isAscendingPrefix(indicesByX.get(leftX)!)).toBe(true); + expect(indicesByX.get(rightX)![0]).toBeGreaterThan(0); + }); + + it('moves only the order — column widths and the gutter are untouched', async () => { + const ltr = readFirstPage(await layoutTwoColumnSection(TWO_COLUMN_SECTION)); + resetBlockIdCounter(); + const rtl = readFirstPage(await layoutTwoColumnSection({ ...TWO_COLUMN_SECTION, bidi: true })); + + // Identical geometry: the same two column origins, so no width or gutter moved. + expect(rtl.columnXs).toEqual(ltr.columnXs); + + // And an exact mirror of the assignment: whatever LTR put in the left column, RTL puts in the + // right one, paragraph for paragraph. Comparing only the x values or the fragment total would + // pass even with the mirror ripped out, since both are invariant under it. + const [leftX, rightX] = ltr.columnXs; + expect(rtl.indicesByX.get(rightX)).toEqual(ltr.indicesByX.get(leftX)); + expect(rtl.indicesByX.get(leftX)).toEqual(ltr.indicesByX.get(rightX)); + }); + + it('keeps a balanced last page right-to-left', async () => { + // A multi-column section that ends mid-page gets its last page re-balanced, which REBUILDS the + // column geometry and overwrites every fragment's x from it. That rebuild is a separate code + // path from ordinary fill, so it can lose the axis on its own: the tail page of a two-column + // Hebrew section would flip to left-to-right while every earlier page stayed right-to-left. + const balanced = async (bidi: boolean) => { + resetBlockIdCounter(); + const pmDoc = createPMDocWithSections( + [ + { + paragraphs: Array.from({ length: 8 }, (_, i) => `Paragraph number ${i + 1}. ${'word '.repeat(30)}`), + props: { + type: 'continuous', + pageSize: PAGE_SIZES.LETTER_PORTRAIT, + columns: { count: 2, gap: 48 }, + ...(bidi ? { bidi: true } : {}), + }, + }, + { paragraphs: ['Tail section, back to a single column'] }, + ], + { type: 'continuous', pageSize: PAGE_SIZES.LETTER_PORTRAIT }, + ); + const layout = await convertAndLayout(pmDoc, { pageSize: PAGE_SIZES.LETTER_PORTRAIT }); + // Only the multi-column section's own paragraphs; the tail section is single-column. + const columnised = layout.pages[0].fragments.filter( + (fragment) => fragment.blockId.endsWith('-paragraph') && paragraphIndex(fragment.blockId) < 8, + ); + return columnised.sort((a, b) => paragraphIndex(a.blockId) - paragraphIndex(b.blockId)); + }; + + const ltr = await balanced(false); + const rtl = await balanced(true); + + // Balancing actually engaged: the 8 paragraphs are split across both columns, not stacked in one. + const ltrXs = [...new Set(ltr.map((f) => Math.round(f.x)))]; + expect(ltrXs).toHaveLength(2); + const [leftX, rightX] = ltrXs.sort((a, b) => a - b); + + // LTR balances into the left column first; RTL into the right one. Same split, mirrored sides. + expect(ltr.map((f) => Math.round(f.x))).toEqual([leftX, leftX, leftX, leftX, rightX, rightX, rightX, rightX]); + expect(rtl.map((f) => Math.round(f.x))).toEqual([rightX, rightX, rightX, rightX, leftX, leftX, leftX, leftX]); + // Balancing must not disturb the vertical rhythm either. + expect(rtl.map((f) => Math.round(f.y))).toEqual(ltr.map((f) => Math.round(f.y))); + }); + + it('treats an explicitly disabled w:bidi as left to right', async () => { + // `` is the section opting out, not opting in. + const layout = await layoutTwoColumnSection({ ...TWO_COLUMN_SECTION, bidi: false }); + const { columnXs, indicesByX } = readFirstPage(layout); + + expect(indicesByX.get(columnXs[0])![0]).toBe(0); + }); + + it('carries the section direction onto the column layout, and only when columns exist', async () => { + const withColumns = pmToFlowBlocks( + createPMDocWithSections([{ paragraphs: ['a'] }], { ...TWO_COLUMN_SECTION, bidi: true }), + ); + expect(getSectionBreaks(withColumns.blocks).map((block) => block.columns)).toEqual([ + { count: 2, gap: 48, direction: 'rtl' }, + ]); + + // A single-column RTL section has no order to flip. The adapter must not invent a column layout + // for it, or an unstyled section would start to look like it carries explicit column properties. + const singleColumn = pmToFlowBlocks( + createPMDocWithSections([{ paragraphs: ['a'] }], { + type: 'nextPage', + pageSize: PAGE_SIZES.LETTER_PORTRAIT, + bidi: true, + }), + ); + expect(getSectionBreaks(singleColumn.blocks).map((block) => block.columns)).toEqual([undefined]); + }); +}); diff --git a/packages/layout-engine/tests/src/test-helpers/section-test-utils.ts b/packages/layout-engine/tests/src/test-helpers/section-test-utils.ts index d52cf554ca..33993e027c 100644 --- a/packages/layout-engine/tests/src/test-helpers/section-test-utils.ts +++ b/packages/layout-engine/tests/src/test-helpers/section-test-utils.ts @@ -21,6 +21,11 @@ export type TestSectionProps = { orientation?: 'portrait' | 'landscape'; pageSize?: { w: number; h: number }; columns?: { count: number; gap: number }; + /** + * Section page direction (`w:sectPr/w:bidi`). RTL puts the FIRST column against the right margin + * and fills right to left, the way Word lays out a Hebrew or Arabic multi-column section. + */ + bidi?: boolean; margins?: { header?: number; footer?: number }; /** Vertical alignment of content within the section's pages */ vAlign?: 'top' | 'center' | 'bottom' | 'both'; @@ -189,6 +194,16 @@ function createSectPrElements(sectionProps: TestSectionProps): Array): Record => asRecord(element.attributes); +/** + * Word ST_OnOff: a bare `` means ON, and only the explicit falsy spellings turn it off + * (ECMA-376 §22.9.2.7). Mirrors `parseOnOff` in the style engine. + */ +const ST_OFF = new Set(['0', 'false', 'off']); +const readOnOff = (attrs: Record): boolean => { + const raw = asString(attrs['w:val']); + return raw == null ? true : !ST_OFF.has(raw.trim().toLowerCase()); +}; + const readSectPr = (sectPr: unknown): Partial => { const elements = Array.isArray(asRecord(sectPr).elements) ? (asRecord(sectPr).elements as Record[]) : []; const out: Partial = {}; + // `w:bidi` and `w:cols` are siblings in any order, so the direction is collected here and applied + // to the column layout after the loop. + let pageIsRtl = false; for (const element of elements) { const name = asString(element.name); @@ -188,6 +201,11 @@ const readSectPr = (sectPr: unknown): Partial => { continue; } + if (name === 'w:bidi') { + pageIsRtl = readOnOff(attrs); + continue; + } + if (name === 'w:vAlign') { out.vAlign = asString(attrs['w:val']) as SectionBreakBlock['vAlign']; continue; @@ -203,6 +221,14 @@ const readSectPr = (sectPr: unknown): Partial => { } } + // Section `w:bidi` governs section-level chrome, and on the column axis it decides which side the + // FIRST column sits on (ECMA-376 §17.6.1). Only applied when the section actually declares + // columns: absent `w:cols` means a single column, which has no order to flip, and synthesising a + // layout here would make an unstyled section look like it carries explicit column properties. + if (pageIsRtl && out.columns) { + out.columns = { ...out.columns, direction: 'rtl' }; + } + return out; }; diff --git a/tests/consumer-typecheck/src/layout-rtl-column-direction.ts b/tests/consumer-typecheck/src/layout-rtl-column-direction.ts new file mode 100644 index 0000000000..0c5274f5d6 --- /dev/null +++ b/tests/consumer-typecheck/src/layout-rtl-column-direction.ts @@ -0,0 +1,34 @@ +import type { Layout } from 'superdoc'; + +// `Layout.columns` is a `ColumnLayout`, and the section page direction (`w:sectPr/w:bidi`) travels +// on it because column geometry is what the axis decides: which side the FIRST column sits on. +// The field is reachable from outside the package through this nested shape, so it is pinned here. + +type PublicColumnLayout = NonNullable; +type PublicColumnDirection = NonNullable; + +// Both literals a section can carry have to be assignable from outside the package. +const rtlSection: PublicColumnLayout = { count: 2, gap: 48, direction: 'rtl' }; +const ltrSection: PublicColumnLayout = { count: 2, gap: 48, direction: 'ltr' }; + +// Absent means LTR, so a consumer that never heard of the axis must still type-check. +const directionless: PublicColumnLayout = { count: 2, gap: 48 }; + +// The field is optional, and reading it back yields the same union — no widening to `string`. +declare const layout: Layout; +const readDirection: PublicColumnDirection | undefined = layout.columns?.direction; + +const rtl: PublicColumnDirection = 'rtl'; +const ltr: PublicColumnDirection = 'ltr'; + +// A consumer must be able to hand a value it read straight back into a layout it builds. +declare const observed: PublicColumnDirection; +const roundTripped: PublicColumnLayout = { count: 3, gap: 24, direction: observed }; + +void rtlSection; +void ltrSection; +void directionless; +void readDirection; +void rtl; +void ltr; +void roundTripped; From 5552e2c29a07ac79e61af2331a1efa603f109608 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Tue, 1 Sep 2026 19:54:40 +0300 Subject: [PATCH 03/20] fix(layout): gate the RTL column separator on the fragment edge that trails Review follow-up on #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. --- .../dom/src/renderer-column-separators.test.ts | 15 +++++++++++++++ .../layout-engine/painters/dom/src/renderer.ts | 9 ++++++++- 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index b2f7a3c483..2b9169f5c3 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -111,6 +111,21 @@ describe('DomPainter renderColumnSeparators', () => { expect(seps[0].style.left).toBe('408px'); }); + it('does not let a page-wide anchored graphic satisfy the RTL gate', () => { + // `page.items` carries anchored drawings as well as column content, and a page-relative + // watermark sits at x = 0 spanning the whole page. Testing its LEFT edge against the + // separator makes it 'past' the separator in RTL while the same item is never past it in + // LTR, so a section whose text never left the first column would draw a line Word does not. + const watermark: Fragment = { ...fragAt(0), width: 816 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [fragAt(432), watermark], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(0); + }); + it('draws count-1 separators for 3 equal columns', () => { const page = buildPage({ columns: { count: 3, gap: 48, withSeparator: true }, diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 410b6e6f28..2f1631e2c5 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1796,11 +1796,18 @@ export class DomPainter { // "Past the separator" means "in a later column", which is the LEFT side in an RTL section: // there column 0 sits on the right, so an `f.x >= separatorX` test is satisfied by content // that never left the FIRST column and the gate would draw a line Word does not draw. + // + // Each branch tests the edge that TRAILS in its own fill direction: the left edge going + // right, the right edge going left. Testing `f.x` in both would leave the branches + // asymmetric for anything wider than a column. `page.items` also carries page-anchored + // graphics, and a full-width watermark sits at `x = 0` — never past the separator going + // right, always past a left-edge test going left — so a section whose text never left the + // first column would draw a separator on the strength of the watermark alone. const laterColumnsAreLeft = columns.direction === 'rtl'; for (const separatorX of separatorPositions) { const hasContentPastSeparator = fragmentsInRegion.some((f) => - laterColumnsAreLeft ? f.x < separatorX : f.x >= separatorX, + laterColumnsAreLeft ? f.x + (f.width ?? 0) <= separatorX : f.x >= separatorX, ); if (!hasContentPastSeparator) continue; From b9e7e5163a2e6a7aff71748ef6294027287244b3 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Tue, 1 Sep 2026 23:01:30 +0300 Subject: [PATCH 04/20] fix(painter): resolve a separator's neighbouring column by ownership, 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 --- .../contracts/src/column-layout.test.ts | 70 ++++++++++++++++ .../contracts/src/column-layout.ts | 30 +++++++ packages/layout-engine/contracts/src/index.ts | 1 + .../src/renderer-column-separators.test.ts | 80 +++++++++++++++++++ .../painters/dom/src/renderer.ts | 74 +++++++++++------ 5 files changed, 231 insertions(+), 24 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index f1b77fec99..3ded61a82e 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -15,6 +15,7 @@ import { resolveColumnLayout, resolveColumnMode, widthsEqual, + findColumnContaining, } from './column-layout.js'; describe('widthsEqual', () => { @@ -595,3 +596,72 @@ describe('RTL section column order', () => { expect(columnRenderLayoutsEqual(twoEqual(), twoEqual('ltr'))).toBe(true); }); }); + +describe('findColumnContaining', () => { + // 3 equal columns over 624px with a 24px gap: 192px columns at 0, 216, 432. + const ltr = getColumnGeometry(normalizeColumnLayout({ count: 3, gap: 24 }, 624)); + const rtl = getColumnGeometry(normalizeColumnLayout({ count: 3, gap: 24, direction: 'rtl' }, 624)); + + it('resolves an x inside a column to that column, in both directions', () => { + expect(findColumnContaining(ltr, 10)).toBe(0); + expect(findColumnContaining(ltr, 300)).toBe(1); + expect(findColumnContaining(ltr, 500)).toBe(2); + // Mirrored: column 0 is the rightmost, so the same points answer in reverse. + expect(findColumnContaining(rtl, 10)).toBe(2); + expect(findColumnContaining(rtl, 300)).toBe(1); + expect(findColumnContaining(rtl, 500)).toBe(0); + }); + + it('answers null in a gutter instead of clamping to a neighbour', () => { + // The gap between column 0 and 1 runs 192..216 in LTR. + expect(findColumnContaining(ltr, 200)).toBeNull(); + // getColumnAtX, which exists for hit testing, must still clamp there. + expect(getColumnAtX(ltr, 200)).toBe(0); + }); + + it('answers null outside the strip entirely, in both directions', () => { + expect(findColumnContaining(ltr, -50)).toBeNull(); + expect(findColumnContaining(ltr, 700)).toBeNull(); + expect(findColumnContaining(rtl, -50)).toBeNull(); + expect(findColumnContaining(rtl, 700)).toBeNull(); + }); + + it('identifies a fragment WIDER than its column by its origin', () => { + // An over-wide table is placed at its column's left edge and overflows rightward in BOTH + // directions. Its origin still names its column; its trailing edge does not, which is exactly + // why an edge comparison cannot answer this question. + const originOfLastColumn = rtl[2].x; + expect(findColumnContaining(rtl, originOfLastColumn)).toBe(2); + // The same fragment's right edge, 500px later, has left the column and reads as another one. + expect(findColumnContaining(rtl, originOfLastColumn + 500)).not.toBe(2); + }); + + it('gives a shared zero-gap boundary to the column that STARTS there', () => { + // `w:space="0"` makes adjacent columns share an endpoint, and that endpoint is exactly where + // the later column's content is placed. Inclusive spans would hand it to the column that ends + // there instead, and — because the scan runs in fill order — would do so in LTR but not in RTL, + // making the two directions disagree. + const zeroGap = getColumnGeometry(normalizeColumnLayout({ count: 2, gap: 0 }, 624)); + expect(zeroGap.map((col) => col.x)).toEqual([0, 312]); + expect(findColumnContaining(zeroGap, 311.9)).toBe(0); + expect(findColumnContaining(zeroGap, 312)).toBe(1); + + // The mirrored strip has to answer the same way about its own shared boundary. + const zeroGapRtl = getColumnGeometry(normalizeColumnLayout({ count: 2, gap: 0, direction: 'rtl' }, 624)); + expect(zeroGapRtl.map((col) => col.x)).toEqual([312, 0]); + expect(findColumnContaining(zeroGapRtl, 312)).toBe(0); + expect(findColumnContaining(zeroGapRtl, 311.9)).toBe(1); + }); + + it('honors originX', () => { + expect(findColumnContaining(ltr, 106, 96)).toBe(0); + expect(findColumnContaining(ltr, 96 + 300, 96)).toBe(1); + expect(findColumnContaining(ltr, 0, 96)).toBeNull(); + }); + + it('treats a single column as a column', () => { + const one = getColumnGeometry(normalizeColumnLayout({ count: 1, gap: 0 }, 624)); + expect(findColumnContaining(one, 300)).toBe(0); + expect(findColumnContaining(one, 900)).toBeNull(); + }); +}); diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index f167d958aa..19b0bc425d 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -309,6 +309,36 @@ export function getColumnSeparatorPositions(geometry: ColumnGeometry[], originX .map((col) => originX + (col.separatorX as number)); } +/** + * Index of the column whose OWN span contains absolute `x`, or `null` when `x` lies in no column at + * all — a gutter, the page margins, or something that is not column flow in the first place. + * + * This is the strict counterpart to `getColumnAtX` below, and the two exist because paint-time and + * hit-testing want opposite answers. A click has to select something, so `getColumnAtX` clamps and + * hands a gap to its neighbouring column. Asking "is there content in a later column" must not + * clamp: `page.items` carries page-anchored objects, and a full-width watermark belongs to no + * column, so answering with one makes it evidence for chrome Word does not draw. + * + * Direction-agnostic by construction. It tests containment in each column's own span instead of + * comparing against a boundary, so it does not care whether `x` ascends or descends with the index, + * and — unlike an edge test — it is not fooled by a fragment WIDER than its column. An over-wide + * table is placed at its column's left edge and overflows rightward in both directions, so its + * origin still identifies its column while its trailing edge does not. + * + * Spans are half-open — `[x, x + width)` — so that adjacent columns authored with no gutter at all + * (`w:space="0"`) do not both claim the boundary they share. That boundary is exactly where the + * later column's own content is placed, and an inclusive upper bound would hand it to the earlier + * column instead. Columns are scanned in fill order and the first containing span wins, which + * after that only matters for an overfull explicit strip whose columns genuinely overlap. + */ +export function findColumnContaining(geometry: ColumnGeometry[], x: number, originX = 0): number | null { + const cx = x - originX; + for (const col of geometry) { + if (cx >= col.x && cx < col.x + col.width) return col.index; + } + return null; +} + /** * Index of the column containing absolute `x` (clicks in a gap map to the preceding column). * diff --git a/packages/layout-engine/contracts/src/index.ts b/packages/layout-engine/contracts/src/index.ts index 447aa3d6cb..aa4675ab18 100644 --- a/packages/layout-engine/contracts/src/index.ts +++ b/packages/layout-engine/contracts/src/index.ts @@ -163,6 +163,7 @@ export { cloneColumnLayout, columnLayoutsEqual, columnRenderLayoutsEqual, + findColumnContaining, getColumnAtX, getColumnGapAfter, getColumnGeometry, diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index 2b9169f5c3..8c175751b3 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -126,6 +126,86 @@ describe('DomPainter renderColumnSeparators', () => { expect(querySeparators(mount)).toHaveLength(0); }); + it('draws the RTL separator for a wide right-aligned table whose x is OUTSIDE its column', () => { + // The real shape of over-wide content, not the idealised one. `resolveTableFrame` right-aligns + // a table inside its column, and `end` is the default justification for any bidiVisual table, + // so an RTL table wider than its column gets a NEGATIVE offset: it starts left of its own + // column and ends past the separator. Neither of its edges identifies the column it belongs + // to, and neither does its origin. `columnIndex` — which the engine records as it lays the + // fragment out — does. + const wideRtlTable: Fragment = { ...fragAt(-116), width: 500, columnIndex: 1 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [{ ...fragAt(432), columnIndex: 0 }, wideRtlTable], + }); + paintOnce(buildLayout(page), mount); + + const seps = querySeparators(mount); + expect(seps).toHaveLength(1); + expect(seps[0].style.left).toBe('408px'); + }); + + it('still ignores an over-wide table that belongs to the FIRST column', () => { + // The other half of the contract: overflowing out of column 0 is not evidence that a later + // column holds anything, whichever direction the columns run and wherever the box lands. + const rtl = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [{ ...fragAt(220), width: 500, columnIndex: 0 }], + }); + paintOnce(buildLayout(rtl), mount); + expect(querySeparators(mount)).toHaveLength(0); + + mount.remove(); + mount = document.createElement('div'); + document.body.append(mount); + + const ltr = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [{ ...fragAt(96), width: 500, columnIndex: 0 }], + }); + paintOnce(buildLayout(ltr), mount); + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('counts a fragment nudged out of its column by a negative indent', () => { + // A negative `w:ind` puts a paragraph's origin in the gutter, outside every column span. The + // engine still knows which column it belongs to, so the line must be drawn. + const outdented: Fragment = { ...fragAt(422), columnIndex: 1 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [{ ...fragAt(96), columnIndex: 0 }, outdented], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(1); + }); + + it('attributes a fragment on a zero-gap column boundary to the LATER column', () => { + // With `w:space="0"` adjacent columns share an endpoint, and that endpoint is exactly where + // the later column's content starts. Column spans are half-open so the boundary belongs to + // the column that begins there, not the one that ends there. + const page = buildPage({ + columns: { count: 2, gap: 0, withSeparator: true }, + fragments: [fragAt(96), fragAt(408)], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(1); + }); + + it('does not let a page-wide anchored graphic satisfy the LTR gate either', () => { + // The watermark guard is not an RTL special case: an item that belongs to no column is not + // evidence for any separator, whichever way the columns run. + const watermark: Fragment = { ...fragAt(0), width: 816 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), watermark], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(0); + }); + it('draws count-1 separators for 3 equal columns', () => { const page = buildPage({ columns: { count: 3, gap: 48, withSeparator: true }, diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 2f1631e2c5..ae88e470f3 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1,6 +1,7 @@ import type { ChartDrawing, CellBorders, + ColumnGeometry, ColumnLayout, CustomGeometryData, DrawingBlock, @@ -54,8 +55,8 @@ import { expandRunsForInlineNewlines, formatPageNumber, formatSectionPageNumberText, + findColumnContaining, getColumnGeometry, - getColumnSeparatorPositions as getColumnSeparatorPositionsFromGeometry, isPositionedParagraphFrame, normalizeColumnLayout, resolveColumnMode, @@ -1782,34 +1783,54 @@ export class DomPainter { const regionHeight = yEnd - yStart; if (regionHeight <= 0) continue; - const separatorPositions = this.getColumnSeparatorPositions(columns, leftMargin, contentWidth); - if (separatorPositions.length === 0) continue; + const geometry = this.resolveSeparatorColumnGeometry(columns, contentWidth); + if (!geometry) continue; // Word only renders the column separator between columns that both have // content. For a 2-col page where col 1 is empty (e.g. the last page of // a multi-column section that fits in col 0, or a `nextPage` section // where Word fills col 0 first without balancing), Word draws no line // even when the section's `w:cols` declared `w:sep="1"`. Gate each - // separator on whether any fragment sits past it within the region. + // separator on whether any LATER column in fill order holds content. const fragmentsInRegion = page.items.filter((item) => item.y >= yStart - 0.5 && item.y < yEnd + 0.5); - // "Past the separator" means "in a later column", which is the LEFT side in an RTL section: - // there column 0 sits on the right, so an `f.x >= separatorX` test is satisfied by content - // that never left the FIRST column and the gate would draw a line Word does not draw. + // Ask which column OWNS each fragment rather than comparing an edge against the separator. + // An edge test has to pick which edge trails in the fill direction, and no choice is right: + // content wider than its column does not sit inside it. `resolveTableFrame` places a + // right-aligned or centred over-wide table at a NEGATIVE offset from its column — and `end` + // is the default justification for any bidiVisual table — so in an RTL section such a table + // starts left of its own column and ends past the separator, while never having left the + // later column at all. // - // Each branch tests the edge that TRAILS in its own fill direction: the left edge going - // right, the right edge going left. Testing `f.x` in both would leave the branches - // asymmetric for anything wider than a column. `page.items` also carries page-anchored - // graphics, and a full-width watermark sits at `x = 0` — never past the separator going - // right, always past a left-edge test going left — so a section whose text never left the - // first column would draw a separator on the strength of the watermark alone. - const laterColumnsAreLeft = columns.direction === 'rtl'; - - for (const separatorX of separatorPositions) { - const hasContentPastSeparator = fragmentsInRegion.some((f) => - laterColumnsAreLeft ? f.x + (f.width ?? 0) <= separatorX : f.x >= separatorX, - ); + // `fragment.columnIndex` is the engine's own record of the owning column, written for + // paragraphs and tables alike as they are laid out, and documented as the field to trust + // "when overflow crosses margins". Geometry is only the fallback, for a fragment that + // carries no such record. + // + // Falling back to containment rather than to `getColumnAtX` is deliberate: containment can + // answer "no column", and that is what keeps page-anchored objects out of the gate. + // `page.items` carries them, and a full-width watermark belongs to no column — counting it + // would draw a separator on a page whose text never left the first column. + const lastColumnIndex = geometry.length - 1; + const occupiedColumns = new Set(); + for (const item of fragmentsInRegion) { + // `page.items` are paint items; the engine's record of the owning column lives on the + // source fragment they point back to. + const owned = (item as { fragment?: { columnIndex?: number } }).fragment?.columnIndex; + const columnIndex = + typeof owned === 'number' && Number.isFinite(owned) + ? Math.max(0, Math.min(lastColumnIndex, Math.floor(owned))) + : findColumnContaining(geometry, item.x, leftMargin); + if (columnIndex !== null) occupiedColumns.add(columnIndex); + } + + // Iterating the geometry (rather than a positions array) keeps each separator paired with the + // column it follows, which is what "a later column" is measured against. + for (const column of geometry) { + if (column.separatorX === undefined) continue; + const hasContentPastSeparator = [...occupiedColumns].some((index) => index > column.index); if (!hasContentPastSeparator) continue; + const separatorX = leftMargin + column.separatorX; const separatorEl = this.doc.createElement('div'); separatorEl.dataset.superdocColumnSeparator = 'true'; @@ -1826,7 +1847,12 @@ export class DomPainter { } } - private getColumnSeparatorPositions(columns: ColumnLayout, leftMargin: number, contentWidth: number): number[] { + /** + * The resolved column geometry this region's separators are drawn from, or null when the region + * draws none. Returns the geometry rather than bare x positions because the gate needs to know + * WHICH column each separator follows, not only where it sits. + */ + private resolveSeparatorColumnGeometry(columns: ColumnLayout, contentWidth: number): ColumnGeometry[] | null { // SD-2629: separator positions come from the one resolved column geometry (the same source as // fill count and column widths), not a re-derivation here. The caller has already gated on // withSeparator and count > 1. @@ -1838,12 +1864,12 @@ export class DomPainter { // raw equalWidth:true config carrying stray widths still takes the equal-mode guard. Legacy guard. if (resolveColumnMode(columns) === 'equal') { const equalWidth = (contentWidth - columns.gap * (normalized.count - 1)) / normalized.count; - if (equalWidth <= 1) return []; + if (equalWidth <= 1) return null; } const geometry = getColumnGeometry(normalized); - if (geometry.length <= 1) return []; - if (geometry.some((column) => column.width <= 1)) return []; - return getColumnSeparatorPositionsFromGeometry(geometry, leftMargin); + if (geometry.length <= 1) return null; + if (geometry.some((column) => column.width <= 1)) return null; + return geometry; } private renderDecorationsForPage(pageEl: HTMLElement, page: ResolvedPage, pageIndex: number): void { if (this.isSemanticFlow) return; From 2438bd9d4b065339c9f8e9a228bcc55d46f30405 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 02:36:53 +0300 Subject: [PATCH 05/20] fix(painter): attribute a fragment to its column by overlap, not by its origin Review follow-up on #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. --- .../src/renderer-column-separators.test.ts | 16 ++++++ .../painters/dom/src/renderer.ts | 57 +++++++++++++++---- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index 8c175751b3..42b1f8c54f 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -206,6 +206,22 @@ describe('DomPainter renderColumnSeparators', () => { expect(querySeparators(mount)).toHaveLength(0); }); + it('still draws the separator when the later column holds only an outdented paragraph', () => { + // The paginator records `columnIndex` for tables and footnote bodies but not for ordinary + // paragraphs, so a paragraph reaches the geometry fallback. A negative `w:ind` puts its + // origin in the gutter, outside its own column: attributing by containment of the origin + // would find no column and suppress a line Word draws. + // 2 equal columns of 288 in a 624 content area: column 1 starts at 96 + 288 + 48 = 432. + const outdented: Fragment = { ...fragAt(432 - 40), width: 288 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), outdented], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(1); + }); + it('draws count-1 separators for 3 equal columns', () => { const page = buildPage({ columns: { count: 3, gap: 48, withSeparator: true }, diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index ae88e470f3..39d4cf19c3 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1054,6 +1054,46 @@ function svgEffectColor(value: TextEffectColor): string | undefined { * - Incremental re-rendering when only specific blocks change * - Hyperlink rendering with security sanitization and accessibility */ +/** + * The column that owns a fragment spanning `[x, x + width)` in content-relative coordinates, or + * `null` when it belongs to no column. + * + * Attribution is by OVERLAP rather than by containment of the origin, because an origin can sit + * outside its own column in two ordinary cases: a paragraph with a negative `w:ind` hangs into the + * gutter, and `resolveTableFrame` places a right-aligned or centred over-wide table at a negative + * offset from its column. The paginator records `columnIndex` for tables and footnote bodies but + * NOT for ordinary paragraphs, so an outdented paragraph alone in a later column reaches this + * fallback — and answering `null` for it would suppress a separator Word draws. + * + * Anything at least as wide as the whole content area belongs to no column. That is what keeps + * page-anchored objects out of the gate: `page.items` carries them, and a full-width watermark + * overlaps every column without being content of any. The same rule catches an over-wide table that + * reaches here without a recorded column, and `null` is the safe answer there too — it can only + * ever suppress a separator, never invent one. + * + * Ties go to the earliest column in fill order, which arises only for an overfull explicit strip + * whose columns genuinely overlap. + */ +function columnOwningSpan(geometry: ColumnGeometry[], x: number, width: number): number | null { + if (geometry.length === 0) return null; + + const span = Number.isFinite(width) && width > 0 ? width : 0; + const contentStart = Math.min(...geometry.map((col) => col.x)); + const contentEnd = Math.max(...geometry.map((col) => col.x + col.width)); + if (span >= contentEnd - contentStart) return null; + + let best: number | null = null; + let bestOverlap = 0; + for (const col of geometry) { + const overlap = Math.min(x + span, col.x + col.width) - Math.max(x, col.x); + if (overlap > bestOverlap) { + bestOverlap = overlap; + best = col.index; + } + } + return best; +} + export class DomPainter { private readonly options: PainterOptions; private mount: HTMLElement | null = null; @@ -1802,25 +1842,20 @@ export class DomPainter { // starts left of its own column and ends past the separator, while never having left the // later column at all. // - // `fragment.columnIndex` is the engine's own record of the owning column, written for - // paragraphs and tables alike as they are laid out, and documented as the field to trust - // "when overflow crosses margins". Geometry is only the fallback, for a fragment that - // carries no such record. - // - // Falling back to containment rather than to `getColumnAtX` is deliberate: containment can - // answer "no column", and that is what keeps page-anchored objects out of the gate. - // `page.items` carries them, and a full-width watermark belongs to no column — counting it - // would draw a separator on a page whose text never left the first column. + // `fragment.columnIndex` is the engine's own record of the owning column, and it is the + // first thing consulted. Today the paginator writes it for tables and footnote bodies but + // not for ordinary paragraphs, so the geometry fallback below carries most fragments and + // has to be right on its own. const lastColumnIndex = geometry.length - 1; const occupiedColumns = new Set(); for (const item of fragmentsInRegion) { - // `page.items` are paint items; the engine's record of the owning column lives on the + // `page.items` are paint items; the engine`s record of the owning column lives on the // source fragment they point back to. const owned = (item as { fragment?: { columnIndex?: number } }).fragment?.columnIndex; const columnIndex = typeof owned === 'number' && Number.isFinite(owned) ? Math.max(0, Math.min(lastColumnIndex, Math.floor(owned))) - : findColumnContaining(geometry, item.x, leftMargin); + : columnOwningSpan(geometry, item.x - leftMargin, item.width); if (columnIndex !== null) occupiedColumns.add(columnIndex); } From db5947e1ba17f42943b1fa4dee7259877cde15eb Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:02:02 +0300 Subject: [PATCH 06/20] fix(painter): bound column attribution by the page, and by both box edges 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. --- .../src/renderer-column-separators.test.ts | 327 +++++++++++++++++- .../painters/dom/src/renderer.ts | 154 +++++++-- 2 files changed, 459 insertions(+), 22 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index 42b1f8c54f..a2b1f99903 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, it } from 'vite-plus/test'; import { createTestPainter as createDomPainter } from './_test-utils.js'; -import type { ColumnRegion, Fragment, Layout, Page } from '@superdoc/contracts'; +import type { ColumnRegion, Fragment, FlowBlock, Layout, Measure, Page } from '@superdoc/contracts'; // These tests pin down DomPainter's column-separator rendering: // - the fallback path (page.columns only, no mid-page regions) @@ -51,6 +51,44 @@ const paintOnce = (layout: Layout, mount: HTMLElement): void => { painter.paint(layout, mount); }; +// Like paintOnce, but registers extra blocks/measures first. `_test-utils`'s +// paint() auto-synthesizes a block+measure for any 'para' fragment (see +// createTestPainter in _test-utils.ts), but NOT for image/drawing/table +// fragments — resolveImageItem (layout-resolved/resolveImage.ts) throws +// "Missing block/measure entry" without a matching entry, so anchored-float +// fixtures below must supply one explicitly. +const paintWithBlocks = (layout: Layout, mount: HTMLElement, blocks: FlowBlock[], measures: Measure[]): void => { + const painter = createDomPainter({ blocks, measures }); + painter.paint(layout, mount); +}; + +// Minimal image block/measure pair for anchored-float fixtures. `resolveImageItem` +// only checks the block/measure KIND matches ('image'/'image'); it never reads +// width/height off either — those come straight from the fragment — so one +// fixed pair covers every float test below regardless of the fragment's own size. +const FLOAT_BLOCK_ID = 'float-fixture'; +const floatBlock: FlowBlock = { + kind: 'image', + id: FLOAT_BLOCK_ID, + src: 'data:image/gif;base64,R0lGODlhAQABAAAAACw=', + attrs: {}, +}; +const floatMeasure: Measure = { kind: 'image', width: 10, height: 10, scale: 1, naturalWidth: 10, naturalHeight: 10 }; + +// An anchored (floating) image fragment at a given page x/width. `isAnchored: true` +// is what `renderColumnSeparators`'s gate loop reads directly off `item.fragment` +// (FIX 1); `columnIndex` is never set here, so — pre-fix — attribution falls +// through to `columnOwningSpan` exactly like an ordinary fragment would. +const floatAt = (x: number, width: number, y: number = 100): Fragment => ({ + kind: 'image', + blockId: FLOAT_BLOCK_ID, + x, + y, + width, + height: 10, + isAnchored: true, +}); + describe('DomPainter renderColumnSeparators', () => { let mount: HTMLElement; @@ -222,6 +260,60 @@ describe('DomPainter renderColumnSeparators', () => { expect(querySeparators(mount)).toHaveLength(1); }); + it('ignores content that overflows the FIRST column when nothing recorded its column', () => { + // The sibling test above pins the same contract for a fragment the engine tagged with + // `columnIndex: 0`. Ordinary paragraphs never carry that tag, so this one goes through the + // geometry fallback — and attributing by overlap alone answers column 1 here: with + // `widths: [100, 400]` a 500px box starting at column 0's own edge covers 100px of column 0 + // and 352px of column 1. Overflowing out of column 0 is not evidence that column 1 holds + // anything, so the origin has to be consulted before the overlap. + const overflowing: Fragment = { ...fragAt(96), width: 500 }; + const page = buildPage({ + columns: { count: 2, gap: 48, widths: [100, 400], equalWidth: false, withSeparator: true }, + fragments: [overflowing], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('does not let a page-wide graphic satisfy the gate when explicit widths overfill the page', () => { + // Explicit widths are floored to >= 1px but never CAPPED, so [150, 600] with a 48px gap + // occupies 798px inside a 624px content area. Measured against the strip's OWN span a + // page-wide item is merely partial, and overlap attribution then hands it to the column it + // covers most: column 1, the wider one (150px of column 0 against 426px of column 1). That is + // exactly the "a later column holds content" the gate asks about, so the line would be drawn + // on a page where Word draws none. Bounding by the page content area as well is what stops it. + // + // Direction-independent: 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. + const overfull = { count: 2, gap: 48, widths: [150, 600], equalWidth: false, withSeparator: true }; + // Spans the content area exactly: x = leftMargin, width = 816 - 96 - 96. + const pageWide: Fragment = { ...fragAt(96), width: 624 }; + // Column 0 runs 96..246 in LTR page coordinates and 570..720 in RTL; column 1 runs 294..894 + // and -78..522. The second row is a positive control: ordinary content in the later column + // still draws the line, so the suppression above is not vacuous. + const cases = { + ltr: { first: fragAt(96), later: fragAt(300) }, + rtl: { first: fragAt(570), later: fragAt(200) }, + } as const; + + for (const direction of ['ltr', 'rtl'] as const) { + const { first, later } = cases[direction]; + for (const [fragments, expected] of [ + [[first, pageWide], 0], + [[first, later], 1], + ] as const) { + mount.remove(); + mount = document.createElement('div'); + document.body.append(mount); + + paintOnce(buildLayout(buildPage({ columns: { ...overfull, direction }, fragments })), mount); + expect(querySeparators(mount)).toHaveLength(expected); + } + } + }); + it('draws count-1 separators for 3 equal columns', () => { const page = buildPage({ columns: { count: 3, gap: 48, withSeparator: true }, @@ -505,4 +597,237 @@ describe('DomPainter renderColumnSeparators', () => { expect(seps[0].style.height).toBe('300px'); }); }); + + // Reference geometry for every test below, derived (not assumed) from the real + // normalizeColumnLayout/getColumnGeometry: page 816x1056, margins 96 all round → + // contentWidth = 816 - 96 - 96 = 624. `{count:2, gap:48, withSeparator:true}` in + // equal mode gives availableWidth = 624 - 48 = 576, so each column is 576/2 = 288. + // Column 0 is content-relative [0,288), column 1 is [336,624) (288 + the 48 gap), + // and the separator sits at the gutter midpoint, content x 312. Page x = content x + // + leftMargin(96): col0 → page [96,384), col1 → page [432,720), separator → page + // 408. In an RTL section the same equal-width strip mirrors about the content area + // and lands on the identical page x's — verified in the existing "gates an RTL + // separator on the LEFT column" test above — because column 0's mirrored span + // [336,624) is column 1's un-mirrored span and vice versa. + + describe('FIX 1 - a float never lights the content-presence gate', () => { + // `page.items` is `page.fragments.map(...)` with no anchor filtering (renderer.ts, + // around the `occupiedColumns` loop), and an anchored fragment carries its own + // width — so a narrow float is the ordinary case the gate has to reject, not an + // exception. Verified against the pre-fix gate loop (git HEAD, commit 2438bd9, + // before `if (source?.isAnchored === true) continue;` existed) via a scratch + // replica built on the real normalizeColumnLayout/getColumnGeometry: for every + // case below, the pre-fix loop (no anchor check, `columnOwningSpan` = pure overlap) + // puts the float in column 1, drawing a separator at page x 408; the current gate + // excludes it and draws none. + it('excludes an anchored float whose origin sits inside the other column', () => { + // Body text never leaves column 0 (page x 96, content x 0). A 200px-wide + // anchored float at page x 500 has content x 500-96=404, inside column 1's + // [336,624) span (404 < 624). Pre-fix: columnOwningSpan(404, 200) — overlap + // with col0 is min(604,288)-max(404,0) = -116 → 0; overlap with col1 is + // min(604,624)-max(404,336) = 200. Column 1 wins on overlap alone, occupied + // becomes {0,1}, and column 0's separator (content x 312) draws because a + // LATER column (1) is occupied. Post-fix the float is skipped before + // `columnOwningSpan` ever runs, occupied stays {0}, and the gate stays shut. + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), floatAt(500, 200)], + }); + paintWithBlocks(buildLayout(page), mount, [floatBlock], [floatMeasure]); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('excludes an anchored float sitting entirely in the gutter', () => { + // A 40px float at page x 416 has content x 416-96=320, inside the gutter + // (288 <= 320 < 336) — outside BOTH columns' own spans. Pre-fix overlap still + // awards it column 1: overlap with col0 is min(360,288)-max(320,0) = -32 → 0; + // overlap with col1 is min(360,624)-max(320,336) = 24 > 0, so column 1 wins by + // the only nonzero margin. Same outcome as the wider float above — occupied + // {0,1} pre-fix draws the separator, {0} post-fix does not — confirming the + // exclusion isn't just catching floats that already sit in a column's span. + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), floatAt(416, 40)], + }); + paintWithBlocks(buildLayout(page), mount, [floatBlock], [floatMeasure]); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('excludes an anchored float in an RTL section too', () => { + // Mirrored geometry: column 0 (fill-order first) is on the RIGHT, content + // [336,624) / page [432,720); column 1 is on the LEFT, content [0,288) / page + // [96,384) (see the reference-geometry note above this describe block, and the + // existing "gates an RTL separator on the LEFT column" test, which pins the + // same mirrored spans). Body text stays in column 0 (page x 432). A 200px + // float at page x 150 has content x 150-96=54, entirely inside column 1's + // [0,288) span (54+200=254 < 288) — overlap picks it with no ambiguity: overlap + // with col1 is the full 200, overlap with col0 is 0. Pre-fix that occupies + // column 1 and draws the separator (content x 312 either direction, since + // equal columns fill the content area exactly); post-fix the float is skipped + // and only column 0 is occupied, so the gate stays shut — the exclusion is + // direction-independent, matching FIX 1's own reasoning (`hRelativeFrom` never + // reaches the fragment, so every float is excluded, not only page-relative ones). + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true, direction: 'rtl' }, + fragments: [fragAt(432), floatAt(150, 200)], + }); + paintWithBlocks(buildLayout(page), mount, [floatBlock], [floatMeasure]); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('still draws the separator for a NON-anchored fragment at the same position (guard)', () => { + // Positive control for the exclusion above, same page x 500 / width 200 as the + // first case, but the fragment is ordinary column content (isAnchored omitted). + // This is not expected to fail against pre-fix HEAD — it doesn't: both the + // pre-fix pure-overlap columnOwningSpan and the current one attribute this box + // to column 1 (overlap/fit both favor it, since the box sits entirely past + // column 0), so both draw the separator. It's here as a guard against + // over-breadth: proving the FIX 1 exclusion is keyed on `isAnchored` + // specifically, not on "any narrow box past the first column." + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), { ...fragAt(500), width: 200 }], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(1); + expect(querySeparators(mount)[0].style.left).toBe('408px'); + }); + }); + + describe('FIX 2 - an out-of-range recorded columnIndex is rejected, not clamped', () => { + it('rejects columnIndex:5 on a 2-column page rather than clamping it to column 1', () => { + // The only fragment on the page sits at page x 96 (content x 0), which geometry + // alone attributes to column 0. It also carries a stale/corrupt `columnIndex: 5` + // — there is no column 5 on a 2-column page (lastColumnIndex is 1). Pre-fix (git + // HEAD): `Math.max(0, Math.min(1, Math.floor(5)))` clamps that to column 1, + // occupying it and drawing the separator at page x 408 even though nothing is + // really there. Post-fix: 5 is outside [0, lastColumnIndex] after flooring, so + // the record is rejected outright and attribution falls through to geometry, + // which (correctly) says column 0 — leaving column 1 unoccupied and the gate shut. + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [{ ...fragAt(96), columnIndex: 5 }], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('still lets a valid recorded columnIndex beat geometry (guard)', () => { + // Same page x 96 (content x 0) that geometry alone would call column 0, but + // this time columnIndex:1 is IN range (0 <= 1 <= lastColumnIndex 1). This is not + // expected to fail against pre-fix HEAD — it doesn't: a valid record was never + // clamped either version, only an out-of-range one, so both accept it and draw + // the separator. It's here to guard the boundary the fix drew: rejection is for + // out-of-range records specifically, not for every mismatch between the record + // and where geometry would have placed the fragment. + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [{ ...fragAt(96), columnIndex: 1 }], + }); + paintOnce(buildLayout(page), mount); + + const seps = querySeparators(mount); + expect(seps).toHaveLength(1); + expect(seps[0].style.left).toBe('408px'); + }); + + it('floors a near-integer columnIndex before range-checking it (guard)', () => { + // columnIndex: 1.0000001 (ordinary float drift, not corruption) floors to 1, + // which IS in range, and resolves to column 1 — it must not be discarded as + // "out of range" by comparing the unfloored 1.0000001 against lastColumnIndex + // first. Not expected to fail against pre-fix HEAD — it doesn't: the pre-fix + // clamp expression also floors before comparing (`Math.floor(owned)` is the + // innermost call in both versions), so this pins the flooring order rather than + // distinguishing the two. Kept as a guard because the reject-vs-clamp rewrite + // touched this exact expression and a reordering slip here would be silent. + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [{ ...fragAt(96), columnIndex: 1.0000001 }], + }); + paintOnce(buildLayout(page), mount); + + const seps = querySeparators(mount); + expect(seps).toHaveLength(1); + expect(seps[0].style.left).toBe('408px'); + }); + }); + + describe('FIX 3 - columnOwningSpan gates origin containment on the box fitting its column', () => { + // IMPORTANT (see the written report): this test does NOT fail against pre-fix + // HEAD. A scratch replica swept outdent 0..160px in 2px steps for a 100px + // fragment nominally in column 1 (col0 [0,288), col1 [336,624)) and compared the + // pre-fix pure-overlap columnOwningSpan against the current origin+fit one on + // every step: they never disagreed. The reason is structural, not incidental — + // whenever the fit check passes, the box lies entirely inside one column's span, + // and since normalizeColumnLayout/getColumnGeometry never produce overlapping + // column spans, overlap-alone would trivially pick that same column too (its + // overlap with any other, disjoint column is exactly 0). So for any geometry + // reachable through the shared geometry helpers, "origin, gated on fit, else + // overlap" and "overlap alone" are provably the same function. This test is kept + // as a pin on CURRENT behavior (and documentation of the fit gate's intent) — + // protection against a future regression in the fit gate itself, such as one + // that accidentally rejects the fallback to overlap — not a pre-fix regression + // test. + it('still attributes an outdented paragraph to its own (later) column', () => { + // A 100px fragment belongs to column 1 (its unindented origin would be content + // x 336, page x 432) but a negative `w:ind` outdents it 60px, to content x 276 + // / page x 372 — inside column 0's [0,288) span, so containment of the origin + // alone (with no fit check and no overlap fallback) would misattribute it to + // column 0. With the fit check: byOrigin = column 0, but the box doesn't fit + // (276+100=376 > 288+0.01), so attribution falls through to overlap, which + // favors column 1 (overlap 40 vs 12 — see the outdent-sweep comment above). + // Ordinary paragraphs never carry a recorded `columnIndex` (the paginator only + // records it for tables and footnote bodies), so this reaches the geometry + // fallback, not the recorded-columnIndex branch FIX 2 covers. + const outdented: Fragment = { ...fragAt(372), width: 100 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), outdented], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(1); + expect(querySeparators(mount)[0].style.left).toBe('408px'); + }); + }); + + describe('FIX 4 - columnOwningSpan folds the geometry bound instead of spreading it', () => { + it('does not throw for a six-figure column count', () => { + // `Math.min(...arr)`/`Math.max(...arr)` pass every element as a call argument, + // and V8 has a hard argument-count ceiling on that: a scratch check on this + // machine (plain `node -e`, this repo's pinned Node) found the largest array + // Math.min(...arr) still accepts is 124729 elements — 124730 throws + // `RangeError: Maximum call stack size exceeded`. 150000 is comfortably past + // that measured threshold (and the task-suggested count), with margin for a + // deeper call stack inside the real test runner. + // + // Reaching columnOwningSpan at all takes a layout that survives + // resolveSeparatorColumnGeometry's OWN pre-geometry guard: equal-mode columns + // are rejected pre-geometry when (contentWidth - gap*(count-1))/count <= 1. + // With gap 0 that requires contentWidth > count, so this page is 200020px wide + // with 10px margins (contentWidth 200000) against a 150000-column layout — + // equalWidth = 200000/150000 ≈ 1.33, just over the guard, and each of the + // 150000 columns floors to that same ~1.33px width, so geometry.some(w<=1) + // (the other pre-existing guard) doesn't reject it either. A single fragment at + // the content origin (page x 10) is enough to reach columnOwningSpan — it isn't + // testing WHICH column wins, only that resolving one doesn't crash the paint. + // + // Confirmed via the scratch replica against this exact 150000-column geometry: + // the pre-fix (git HEAD) columnOwningSpan throws `RangeError: Maximum call + // stack size exceeded` on `Math.min(...geometry.map(...))`; the current, + // fold-based one returns a plain column index with no throw. + const page = buildPage({ + margins: { top: 10, right: 10, bottom: 10, left: 10 }, + columns: { count: 150000, gap: 0, withSeparator: true }, + fragments: [{ ...fragAt(10), width: 100 }], + }); + + expect(() => paintOnce(buildLayout(page, { w: 200020, h: 400 }), mount)).not.toThrow(); + }); + }); }); diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 39d4cf19c3..3fd78987a3 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1058,29 +1058,114 @@ function svgEffectColor(value: TextEffectColor): string | undefined { * The column that owns a fragment spanning `[x, x + width)` in content-relative coordinates, or * `null` when it belongs to no column. * - * Attribution is by OVERLAP rather than by containment of the origin, because an origin can sit - * outside its own column in two ordinary cases: a paragraph with a negative `w:ind` hangs into the - * gutter, and `resolveTableFrame` places a right-aligned or centred over-wide table at a negative - * offset from its column. The paginator records `columnIndex` for tables and footnote bodies but - * NOT for ordinary paragraphs, so an outdented paragraph alone in a later column reaches this - * fallback — and answering `null` for it would suppress a separator Word draws. + * Four rules after the width bound, in this order, because each is wrong for the case the next one + * answers. `balanceSectionOnPage`'s `ordinalOf` asks the same four for the same reasons; the two + * differ only at the end, where a sort key must name a column and this may answer `null`. * - * Anything at least as wide as the whole content area belongs to no column. That is what keeps - * page-anchored objects out of the gate: `page.items` carries them, and a full-width watermark - * overlaps every column without being content of any. The same rule catches an over-wide table that - * reaches here without a recorded column, and `null` is the safe answer there too — it can only - * ever suppress a separator, never invent one. + * 0. WIDTH BOUND. Anything at least as wide as the area it would have to be content OF belongs to no + * column. That is what keeps page-anchored objects out of the gate: `page.items` carries them, + * and a full-width watermark overlaps every column without being content of any. It comes first, + * before every rule below: in a mirrored RTL strip the LAST column reaches furthest left, so a + * watermark at x = 0 sits on that column's leading edge and would be read as content of it. * - * Ties go to the earliest column in fill order, which arises only for an overfull explicit strip - * whose columns genuinely overlap. + * The area has two bounds and the threshold is the smaller, because either alone leaks. The + * strip's own span, since explicit widths are not scaled to fill the page (see + * `normalizeColumnLayout`) and an underfilling strip is narrower than the content area. And the + * page content area, since those widths are not CAPPED either — nothing clamps their sum — so an + * authored `w:num="2"` with two over-wide `w:col/@w` produces a strip WIDER than the page, and + * against the strip bound alone a page-wide graphic measures as merely partial. + * + * 1. LEADING EDGE. A box that starts on a column's own edge is that column's, fit or no fit. That is + * ordinary content, and it is also content WIDER than its column, which overflows from that same + * edge. Asked before any overlap test, because an over-wide box can cover more of a wide + * neighbour than of the narrow column it came from: `widths: [100, 400]` puts a 500px box that + * starts at column 0's edge 100px into column 0 and 352px into column 1. + * + * 2. TRAILING EDGE — a different question, not a mirror of the first. 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 + * `resolveTableFrame` places an over-wide table justified to `end` at a negative offset from its + * own column, which likewise begins in an earlier column without ever having left its own. + * + * 3. THE ORIGIN, while the box still FITS the column it starts in. The fit is what makes the origin + * evidence: a smaller indent leaves the origin inside its own column and the box inside it too. A + * centred over-wide table also begins inside an earlier column, and there the origin is no + * evidence at all — which is what the fit test rejects. + * + * 4. OVERLAP, for a box whose origin sits in no column: hung into a gutter by a negative `w:ind` or + * a float offset. Ties go to the earliest column in fill order, which arises only for an overfull + * strip whose columns genuinely overlap. + * + * `null` at the end is the safe answer here, unlike in `ordinalOf`, which must clamp because a sort + * key that is sometimes absent is not a total order. The question here is "does a LATER column hold + * content", so `null` can only ever suppress a separator, never invent one. + */ +/** + * Sub-pixel slack for "this edge IS that column's edge". Column x values reach fragments through + * `getColumnX`, so unindented content matches exactly and this only absorbs float drift — it stays + * two orders of magnitude below the smallest indent a document can author (1 twip = 1/1440in). */ -function columnOwningSpan(geometry: ColumnGeometry[], x: number, width: number): number | null { +const COLUMN_EDGE_EPSILON = 0.01; + +function columnOwningSpan( + geometry: ColumnGeometry[], + x: number, + width: number, + contentWidth: number, +): number | null { if (geometry.length === 0) return null; const span = Number.isFinite(width) && width > 0 ? width : 0; - const contentStart = Math.min(...geometry.map((col) => col.x)); - const contentEnd = Math.max(...geometry.map((col) => col.x + col.width)); - if (span >= contentEnd - contentStart) return null; + // Folded rather than spread into `Math.min`/`Math.max`: `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 overflows + // the argument stack — a paint-time crash, from a function whose whole job is to answer + // conservatively. + let stripStart = Infinity; + let stripEnd = -Infinity; + for (const col of geometry) { + if (col.x < stripStart) stripStart = col.x; + if (col.x + col.width > stripEnd) stripEnd = col.x + col.width; + } + // The page bound applies only when the page reports a usable content width. A malformed page can + // report zero or a negative one, and letting that become the threshold would reject every fragment + // and blank out separators that belong on the page. + const pageSpan = Number.isFinite(contentWidth) && contentWidth > 0 ? contentWidth : Infinity; + if (span >= Math.min(stripEnd - stripStart, pageSpan)) return null; + + // A box on a column's LEADING edge is that column's, fit or no fit. That covers ordinary content, + // and it covers content WIDER than its column, which overflows rightward from that same edge in + // either direction. Asked before any overlap test, because an over-wide box can cover more of a + // wide neighbour than of the narrow column it came from: `widths: [100, 400]` puts a 500px box + // that starts at column 0's edge 100px into column 0 and 352px into column 1. + for (const col of geometry) { + if (Math.abs(x - col.x) <= COLUMN_EDGE_EPSILON) return col.index; + } + + // A box on a column's TRAILING edge is that column's too, and it is a different question rather + // than a mirror of the one above. 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 — 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]. And + // `resolveTableFrame` places an over-wide table justified to `end` at a NEGATIVE offset from its + // own column, which likewise begins inside an earlier column without ever having left its own. + for (const col of geometry) { + if (Math.abs(x + span - (col.x + col.width)) <= COLUMN_EDGE_EPSILON) return col.index; + } + + // Containment of the origin, but only while the box still FITS the column it starts in. The fit is + // what makes the origin evidence of ownership: an indent moves a fragment's origin without + // changing which column it flows in, and it still ends inside that column. Content that starts in + // one column and ends past it has a shifted origin instead — an over-wide table justified to `end` + // begins inside an EARLIER column, having never left its own — and there the origin names the + // wrong column. Dropping the fit test suppressed a rule Word draws: a paragraph outdented further + // than the gutter has its origin in the previous column, so its own column read as empty. Measured + // on equal 2-col geometry over 624px (col0 [0,288), col1 [336,624)): a 100px fragment in column 1 + // outdented 72px sits at origin 264, inside column 0, while overlap correctly answers 1. + const byOrigin = findColumnContaining(geometry, x); + if (byOrigin !== null) { + const originColumn = geometry.find((col) => col.index === byOrigin); + if (originColumn && x + span <= originColumn.x + originColumn.width + COLUMN_EDGE_EPSILON) return byOrigin; + } let best: number | null = null; let bestOverlap = 0; @@ -1851,11 +1936,38 @@ export class DomPainter { for (const item of fragmentsInRegion) { // `page.items` are paint items; the engine`s record of the owning column lives on the // source fragment they point back to. - const owned = (item as { fragment?: { columnIndex?: number } }).fragment?.columnIndex; + const source = (item as { fragment?: { columnIndex?: number; isAnchored?: boolean } }).fragment; + + // A FLOAT is not column content, and the width threshold below cannot recognise one. That + // threshold catches a full-width watermark, which is the case 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, not the exception. + // A 200px logo placed at page x 500 on a 2-column page whose text never leaves column 0 has + // its origin inside column 1 and lit this gate, drawing a rule Word does not draw. The same + // logo 84px further left lands in the gutter and wins column 1 on overlap instead. + // + // Every float is excluded, not only the page-relative ones. `hRelativeFrom` is consumed at + // layout time and never reaches the fragment, so telling a page-anchored float from a + // column-anchored one here means reaching back through `item.block`, and I have no evidence + // about the case that would distinguish them: 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 — excluding an item can only ever suppress a rule, never invent + // one — so the conservative reading is also the simpler one. If a document turns up where + // Word draws that rule, the fix is to admit column-anchored floats specifically. + if (source?.isAnchored === true) continue; + + // An out-of-range record is REJECTED, not clamped. Clamping turned any stale or corrupt value + // into a real column index — `columnIndex: 5` on a two-column page became 1 — which is + // exactly the "a later column holds content" this gate asks about, invented out of a number + // that describes no column on this page. Falling through to geometry answers from the + // fragment's actual position instead. Flooring first, so ordinary float drift on a valid + // index still resolves rather than being thrown away as out of range. + const owned = source?.columnIndex; + const recorded = typeof owned === 'number' && Number.isFinite(owned) ? Math.floor(owned) : null; const columnIndex = - typeof owned === 'number' && Number.isFinite(owned) - ? Math.max(0, Math.min(lastColumnIndex, Math.floor(owned))) - : columnOwningSpan(geometry, item.x - leftMargin, item.width); + recorded !== null && recorded >= 0 && recorded <= lastColumnIndex + ? recorded + : columnOwningSpan(geometry, item.x - leftMargin, item.width, contentWidth); if (columnIndex !== null) occupiedColumns.add(columnIndex); } From f2a36d83068ce731d715e9ff9cb949ce42bf43b8 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:07:00 +0300 Subject: [PATCH 07/20] docs(painter): name the fragment kinds that actually record a column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/layout-engine/painters/dom/src/renderer.ts | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 3fd78987a3..3564239d73 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1928,9 +1928,12 @@ export class DomPainter { // later column at all. // // `fragment.columnIndex` is the engine's own record of the owning column, and it is the - // first thing consulted. Today the paginator writes it for tables and footnote bodies but - // not for ordinary paragraphs, so the geometry fallback below carries most fragments and - // has to be right on its own. + // first thing consulted. It reaches only a few fragment kinds: tables (`layout-table.ts`, + // five sites), the three footnote body kinds in `incrementalLayout.ts`, and a paragraph ONLY + // when it is a collapsed split-line-break anchor carrier (`layout-paragraph.ts`, under + // `collapseSplitLineBreakCarrier`) — a narrow document shape, not the ordinary paragraph. So + // the geometry fallback below carries almost every fragment on the page and has to be right + // on its own; the record is a shortcut for the cases that keep one, not the main path. const lastColumnIndex = geometry.length - 1; const occupiedColumns = new Set(); for (const item of fragmentsInRegion) { From 02fe51366c156d445db1ddff58b11c323fb5ef3b Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:40:43 +0300 Subject: [PATCH 08/20] style(painter): put the columnOwningSpan signature on one line `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. --- packages/layout-engine/painters/dom/src/renderer.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 3564239d73..803378b87b 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1107,12 +1107,7 @@ function svgEffectColor(value: TextEffectColor): string | undefined { */ const COLUMN_EDGE_EPSILON = 0.01; -function columnOwningSpan( - geometry: ColumnGeometry[], - x: number, - width: number, - contentWidth: number, -): number | null { +function columnOwningSpan(geometry: ColumnGeometry[], x: number, width: number, contentWidth: number): number | null { if (geometry.length === 0) return null; const span = Number.isFinite(width) && width > 0 ? width : 0; From 3da48e36541ffcdf24d659c26b9f9f1b51a40c13 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:44:17 +0300 Subject: [PATCH 09/20] fix(painter): trust a fragment's origin by its width, not by its right edge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/renderer-column-separators.test.ts | 67 +++++++++++-------- .../painters/dom/src/renderer.ts | 31 ++++++--- 2 files changed, 59 insertions(+), 39 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index a2b1f99903..4e5af09377 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -757,34 +757,44 @@ describe('DomPainter renderColumnSeparators', () => { }); }); - describe('FIX 3 - columnOwningSpan gates origin containment on the box fitting its column', () => { - // IMPORTANT (see the written report): this test does NOT fail against pre-fix - // HEAD. A scratch replica swept outdent 0..160px in 2px steps for a 100px - // fragment nominally in column 1 (col0 [0,288), col1 [336,624)) and compared the - // pre-fix pure-overlap columnOwningSpan against the current origin+fit one on - // every step: they never disagreed. The reason is structural, not incidental — - // whenever the fit check passes, the box lies entirely inside one column's span, - // and since normalizeColumnLayout/getColumnGeometry never produce overlapping - // column spans, overlap-alone would trivially pick that same column too (its - // overlap with any other, disjoint column is exactly 0). So for any geometry - // reachable through the shared geometry helpers, "origin, gated on fit, else - // overlap" and "overlap alone" are provably the same function. This test is kept - // as a pin on CURRENT behavior (and documentation of the fit gate's intent) — - // protection against a future regression in the fit gate itself, such as one - // that accidentally rejects the fallback to overlap — not a pre-fix regression - // test. - it('still attributes an outdented paragraph to its own (later) column', () => { - // A 100px fragment belongs to column 1 (its unindented origin would be content - // x 336, page x 432) but a negative `w:ind` outdents it 60px, to content x 276 - // / page x 372 — inside column 0's [0,288) span, so containment of the origin - // alone (with no fit check and no overlap fallback) would misattribute it to - // column 0. With the fit check: byOrigin = column 0, but the box doesn't fit - // (276+100=376 > 288+0.01), so attribution falls through to overlap, which - // favors column 1 (overlap 40 vs 12 — see the outdent-sweep comment above). - // Ordinary paragraphs never carry a recorded `columnIndex` (the paginator only - // records it for tables and footnote bodies), so this reaches the geometry - // fallback, not the recorded-columnIndex branch FIX 2 covers. - const outdented: Fragment = { ...fragAt(372), width: 100 }; + describe('a box wider than its column is the only origin the gate distrusts', () => { + // Why WIDTH and not the right edge, stated once for both cases below. An edge gate would be + // dead code: pass it and the box lies wholly inside one column's span, and since + // `getColumnGeometry` never emits overlapping spans, every other column's overlap is zero and + // the vote below returns the same column anyway. Swept over outdents from 0 to 160px in 2px + // steps, an edge-gated containment step and plain overlap never disagreed once. The width gate + // is what makes the step do work, because it admits the one shape whose right edge overhangs + // while its origin is still authoritative -- the right-aligned frame in the first test. + it('keeps a right-aligned framed paragraph in the column its origin is in', () => { + // `w:framePr` with `xAlign="right"` re-points the fragment at + // `columnX + (effectiveColumnWidth - maxLineWidth)` (layout-paragraph.ts, `floatAlignment`) + // and leaves `width` at the FULL column width, so the recorded box overhangs the gutter and + // the next column while the text never left column 0. Two equal 288px columns over 624 put a + // frame whose longest line is 50px at content-relative 238 with width 288, i.e. the box + // [238, 526]: neither edge lands on a column edge, and overlap alone favours the neighbour -- + // 50px of column 0 against 190px of column 1 -- so the gate drew a separator on a page with + // nothing in its second column. + // + // This is why origin containment is gated on the box's WIDTH and not on its right edge. The + // box is 288 wide against a 288px column, so it fits, and its origin is believed. Gating on + // the right edge rejects it (526 > 288) and hands it to the overlap vote, which is wrong. + const framed: Fragment = { ...fragAt(96 + 288 - 50), width: 288 }; + const page = buildPage({ + columns: { count: 2, gap: 48, withSeparator: true }, + fragments: [fragAt(96), framed], + }); + paintOnce(buildLayout(page), mount); + + expect(querySeparators(mount)).toHaveLength(0); + }); + + it('still distrusts the origin of a box that outgrew its column', () => { + // The mirror shape, and the reason the width gate is a gate rather than an unconditional + // trust: a negative `w:ind` widens the fragment by the outdent, so a column-1 paragraph + // outdented 72px past the 48px gutter is the box [264, 624] -- origin inside column 0, width + // 360 against a 288px column. It does NOT fit, so the origin is not believed, and overlap + // answers column 1 (24px against 288px). A separator belongs here. + const outdented: Fragment = { ...fragAt(96 + 264), width: 360 }; const page = buildPage({ columns: { count: 2, gap: 48, withSeparator: true }, fragments: [fragAt(96), outdented], @@ -792,7 +802,6 @@ describe('DomPainter renderColumnSeparators', () => { paintOnce(buildLayout(page), mount); expect(querySeparators(mount)).toHaveLength(1); - expect(querySeparators(mount)[0].style.left).toBe('408px'); }); }); diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index 803378b87b..c3d43af935 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1147,19 +1147,30 @@ function columnOwningSpan(geometry: ColumnGeometry[], x: number, width: number, if (Math.abs(x + span - (col.x + col.width)) <= COLUMN_EDGE_EPSILON) return col.index; } - // Containment of the origin, but only while the box still FITS the column it starts in. The fit is - // what makes the origin evidence of ownership: an indent moves a fragment's origin without - // changing which column it flows in, and it still ends inside that column. Content that starts in - // one column and ends past it has a shifted origin instead — an over-wide table justified to `end` - // begins inside an EARLIER column, having never left its own — and there the origin names the - // wrong column. Dropping the fit test suppressed a rule Word draws: a paragraph outdented further - // than the gutter has its origin in the previous column, so its own column read as empty. Measured - // on equal 2-col geometry over 624px (col0 [0,288), col1 [336,624)): a 100px fragment in column 1 - // outdented 72px sits at origin 264, inside column 0, while overlap correctly answers 1. + // Containment of the origin, gated on the box being no WIDER than the column it starts in — not on + // its right edge landing inside that column. The distinction is the whole content of this step. + // + // A box that fits its column was placed in that column wherever the origin ended up: ordinary + // content, a `w:ind` indent, and — the case that makes this step load-bearing — a paragraph + // carrying `attrs.floatAlignment` of `right` or `center`. `layout-paragraph.ts` re-points such a + // fragment 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: origin inside its own column, right edge 238px past it. An edge gate rejects that, + // and the overlap vote below then sees 50px of column 0 against 190px of column 1 and moves it — + // so a page whose text never left column 0 drew a separator, with no anchored object involved at + // all. (Nothing in this repo's production code sets `floatAlignment`; it arrives from the + // adapter outside the layout engine, so the OOXML feature behind it is deliberately not named.) + // + // A box WIDER than its column may instead have been pulled LEFT out of it, and there the origin is + // no evidence: a negative `w:ind` widens the fragment by the outdent, so an outdent bigger than + // the gutter lands the origin in the PREVIOUS column while the content belongs to this one. + // 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], whose origin is in column 0 and whose overlap correctly + // answers 1. Width is what separates the two shapes; the right edge overhangs in both. const byOrigin = findColumnContaining(geometry, x); if (byOrigin !== null) { const originColumn = geometry.find((col) => col.index === byOrigin); - if (originColumn && x + span <= originColumn.x + originColumn.width + COLUMN_EDGE_EPSILON) return byOrigin; + if (originColumn && span <= originColumn.width + COLUMN_EDGE_EPSILON) return byOrigin; } let best: number | null = null; From 18c7b65f7487a45121ca5817069d40de3d88623c Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 19:05:01 +0300 Subject: [PATCH 10/20] test(painter): guard the width gate with a shape that can reach it 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. --- .../src/renderer-column-separators.test.ts | 23 +++++++++++++------ .../painters/dom/src/renderer.ts | 21 ++++++++++++----- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts index 4e5af09377..1572c5e215 100644 --- a/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts +++ b/packages/layout-engine/painters/dom/src/renderer-column-separators.test.ts @@ -789,15 +789,24 @@ describe('DomPainter renderColumnSeparators', () => { }); it('still distrusts the origin of a box that outgrew its column', () => { - // The mirror shape, and the reason the width gate is a gate rather than an unconditional - // trust: a negative `w:ind` widens the fragment by the outdent, so a column-1 paragraph - // outdented 72px past the 48px gutter is the box [264, 624] -- origin inside column 0, width - // 360 against a 288px column. It does NOT fit, so the origin is not believed, and overlap - // answers column 1 (24px against 288px). A separator belongs here. - const outdented: Fragment = { ...fragAt(96 + 264), width: 360 }; + // The shape that actually reaches the gate's rejection path. `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. A 400px box centred in column 1 of two equal 288px columns over + // 624 is [280, 680]: neither edge lands on a column edge (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 the overlap vote answers column 1, 288px against 8px. + // + // An outdented paragraph does NOT reach here, though this test used one until cubic pointed + // out that it could not. 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 and the + // trailing-edge rule answers first. Worth recording rather than quietly swapping the fixture: + // that outdent was the only guard on this gate, and replacing the width comparison with + // unconditional origin trust left all 39 tests in this file passing. + const centredOverWide: Fragment = { ...fragAt(96 + 280), width: 400 }; const page = buildPage({ columns: { count: 2, gap: 48, withSeparator: true }, - fragments: [fragAt(96), outdented], + fragments: [fragAt(96), centredOverWide], }); paintOnce(buildLayout(page), mount); diff --git a/packages/layout-engine/painters/dom/src/renderer.ts b/packages/layout-engine/painters/dom/src/renderer.ts index c3d43af935..06dcc4e977 100644 --- a/packages/layout-engine/painters/dom/src/renderer.ts +++ b/packages/layout-engine/painters/dom/src/renderer.ts @@ -1161,12 +1161,21 @@ function columnOwningSpan(geometry: ColumnGeometry[], x: number, width: number, // all. (Nothing in this repo's production code sets `floatAlignment`; it arrives from the // adapter outside the layout engine, so the OOXML feature behind it is deliberately not named.) // - // A box WIDER than its column may instead have been pulled LEFT out of it, and there the origin is - // no evidence: a negative `w:ind` widens the fragment by the outdent, so an outdent bigger than - // the gutter lands the origin in the PREVIOUS column while the content belongs to this one. - // 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], whose origin is in column 0 and whose overlap correctly - // answers 1. Width is what separates the two shapes; the right edge overhangs in both. + // A box WIDER than its column may instead have been pulled OUT of it, and there the origin is no + // evidence at all. `resolveTableFrame` centres an over-wide table inside its column, placing it 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. Measured on equal 2-col + // geometry over 624px (col0 [0,288), col1 [336,624)): a 400px box centred in column 1 is + // [280, 680], whose origin is in column 0 and whose overlap correctly answers 1, 288px against + // 8px. Width is what separates the two shapes; the right edge overhangs in both. + // + // An outdented paragraph is NOT this case, though it looks like it and was written here as the + // justification once. 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 rule above has + // already answered and this step never sees it. That mistake is worth recording rather than just + // deleting: it was also the fixture guarding this gate, so the gate had no test at all. Replacing + // the width comparison with unconditional origin trust left all 39 tests in + // `renderer-column-separators.test.ts` passing. const byOrigin = findColumnContaining(geometry, x); if (byOrigin !== null) { const originColumn = geometry.find((col) => col.index === byOrigin); From ad9bde61e6e1d25c860b369ef0a9b704f104da5e Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 22:38:47 +0300 Subject: [PATCH 11/20] fix(contracts): resolve an RTL column boundary the way geometry places content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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 #3962 meant #3953 and #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 --- .../contracts/src/column-layout.test.ts | 27 +++++++++++++++++++ .../contracts/src/column-layout.ts | 12 ++++++++- .../layout-bridge/test/position-hit.test.ts | 5 ++-- 3 files changed, 41 insertions(+), 3 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index 3ded61a82e..c48a25c25e 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -564,6 +564,33 @@ describe('RTL section column order', () => { expect(getColumnAtX(rtl, 300)).toBe(0); }); + it('resolves an RTL column boundary the same way containment does', () => { + // `w:space="0"` (ECMA-376 §17.6.3) makes adjacent columns share an edge, which is the one point + // hit testing and containment can be made to disagree about. Two columns over 602px mirror to + // column 0 at [301,602) and column 1 at [0,301), so 301 is column 0's leading edge and column + // 1's trailing one at the same time. + const flush = getColumnGeometry(normalizeColumnLayout({ count: 2, gap: 0, direction: 'rtl' }, 602)); + expect(flush.map((col) => col.x)).toEqual([301, 0]); + + // Half-open spans put a shared edge in the column that STARTS there, which in RTL is the earlier + // column in fill order. An inclusive mirrored bound handed it to column 1 instead, so every + // column boundary in a zero-gutter RTL section resolved one column too far — and disagreed with + // the containment the geometry places content by. + expect(findColumnContaining(flush, 301)).toBe(0); + expect(getColumnAtX(flush, 301)).toBe(0); + // A hair to the left is genuinely column 1's, in both resolvers. + expect(findColumnContaining(flush, 300.99)).toBe(1); + expect(getColumnAtX(flush, 300.99)).toBe(1); + + // With a gutter the same bound over-claimed the point on a column's TRAILING edge, which is + // gutter and belongs to the column preceding it in fill order. + const gutter = getColumnGeometry(normalizeColumnLayout(twoEqual('rtl'), 602)); + expect(gutter[1]).toEqual({ index: 1, x: 0, width: 277, gapAfter: 0 }); + expect(findColumnContaining(gutter, 277)).toBeNull(); + expect(getColumnAtX(gutter, 277)).toBe(0); + expect(getColumnAtX(gutter, 276.99)).toBe(1); + }); + it('keeps LTR hit testing byte-identical', () => { const ltr = getColumnGeometry(normalizeColumnLayout(twoEqual(), 602)); expect(getColumnAtX(ltr, 100)).toBe(0); diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index 19b0bc425d..44294cfcbd 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -348,6 +348,16 @@ export function findColumnContaining(geometry: ColumnGeometry[], x: number, orig * makes a drag that crosses the gutter keep extending from the column it is leaving instead of * jumping. Direction is read off the geometry rather than taken as an argument, so every existing * caller keeps working unchanged. + * + * Both branches test a HALF-OPEN span, so this agrees with `findColumnContaining` on every boundary + * the two can both answer. The LTR branch gets that from `cx >= col.x`: a point on a shared edge is + * the later column's, because that is where the later column's content begins. The mirrored branch + * has to say the same thing from the other side — the shared edge is the EARLIER fill column's left + * edge there — which is `cx < col.x + col.width`, exclusive. An inclusive bound handed that point to + * the later column, contradicting the half-open span the geometry places content in, and it also + * pulled in the point one pixel-width past a column's trailing edge, which is gutter and belongs to + * the preceding column. With `w:space="0"` the two coincide and every column boundary in an RTL + * section resolved one column too far. */ export function getColumnAtX(geometry: ColumnGeometry[], x: number, originX = 0): number { if (geometry.length === 0) return 0; @@ -355,7 +365,7 @@ export function getColumnAtX(geometry: ColumnGeometry[], x: number, originX = 0) const mirrored = geometry.length > 1 && geometry[1].x < geometry[0].x; let result = 0; for (const col of geometry) { - if (mirrored ? cx <= col.x + col.width : cx >= col.x) result = col.index; + if (mirrored ? cx < col.x + col.width : cx >= col.x) result = col.index; else break; } return result; diff --git a/packages/layout-engine/layout-bridge/test/position-hit.test.ts b/packages/layout-engine/layout-bridge/test/position-hit.test.ts index 4fa56d90c7..80ecb0b749 100644 --- a/packages/layout-engine/layout-bridge/test/position-hit.test.ts +++ b/packages/layout-engine/layout-bridge/test/position-hit.test.ts @@ -123,8 +123,9 @@ describe('determineColumn (SD-2629: resolved per-column boundaries)', () => { } as unknown as Page; const layout = { pageSize: { w: 816, h: 1056 }, columns, pages: [page] } as unknown as Layout; - // Content width 624 -> 192px columns. Mirrored, column 0 spans 528..720, column 1 336..528, - // column 2 96..288 (absolute). + // Content width 624 -> 192px columns with a 24px gutter between them. Mirrored, column 0 spans + // 528..720, column 1 312..504, column 2 96..288 (absolute) -- each start is the previous + // column's start less width+gap, so the gutters are 504..528 and 288..312. expect(determineColumn(layout, 700, page)).toBe(0); expect(determineColumn(layout, 400, page)).toBe(1); expect(determineColumn(layout, 150, page)).toBe(2); From 794833958ecd4f55501c44df728aa1c87e55b6c1 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 03:34:46 +0300 Subject: [PATCH 12/20] fix(contracts): compare the gutters that render, not the authored gaps `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. --- .../contracts/src/column-layout.test.ts | 47 ++++++++++++++++++- .../contracts/src/column-layout.ts | 36 ++++++++++++-- .../layout-engine/src/index.test.ts | 12 +++-- .../layout-engine/src/section-breaks.test.ts | 7 ++- 4 files changed, 89 insertions(+), 13 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index c48a25c25e..d2b88ed957 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -382,13 +382,58 @@ describe('columnRenderLayoutsEqual (SD-2629)', () => { expect(columnRenderLayoutsEqual({ count: 2, gap: 24 }, { count: 2, gap: 48 })).toBe(false); }); - it('treats explicit layouts differing only by per-column gaps as render-equal until geometry flips', () => { + it('splits explicit layouts that differ only by per-column gaps', () => { + // Geometry has flipped: `buildColumnGeometry` reads `gaps[i] ?? gap` for both the column x and + // the separator x, so this delta moves column 2 by 72px. While it compared equal, two sections + // differing only here shared a region and the later one was laid out with the earlier's gutters. expect( columnRenderLayoutsEqual( { count: 3, gap: 24, widths: [100, 100, 300], gaps: [24, 24], equalWidth: false }, { count: 3, gap: 24, widths: [100, 100, 300], gaps: [24, 96], equalWidth: false }, ), + ).toBe(false); + + // Identical per-column gaps still compare equal, and so does an absent-vs-absent pair. + expect( + columnRenderLayoutsEqual( + { count: 3, gap: 24, widths: [100, 100, 300], gaps: [24, 96], equalWidth: false }, + { count: 3, gap: 24, widths: [100, 100, 300], gaps: [24, 96], equalWidth: false }, + ), + ).toBe(true); + expect( + columnRenderLayoutsEqual( + { count: 3, gap: 24, widths: [100, 100, 300], equalWidth: false }, + { count: 3, gap: 24, widths: [100, 100, 300], equalWidth: false }, + ), ).toBe(true); + + // Equal mode has no per-column gaps to compare, so stray ones must not split a region. + expect(columnRenderLayoutsEqual({ count: 2, gap: 24, gaps: [24] }, { count: 2, gap: 24, gaps: [96] })).toBe(true); + }); + + it('compares the gutters that are drawn, not the authored arrays', () => { + const explicit = (gaps?: number[]): ColumnLayout => ({ + count: 3, + gap: 48, + widths: [100, 100, 300], + equalWidth: false, + ...(gaps ? { gaps } : {}), + }); + + // Spelling the scalar gap out per column renders identically, so it must NOT split a region. + // Comparing the authored arrays instead would see `undefined` vs `[48, 48]` and split, resetting + // the later section to column 0 mid-page. + expect(columnRenderLayoutsEqual(explicit(), explicit([48, 48]))).toBe(true); + // Same for a negative gutter, which geometry floors at 0 exactly as 0 does. + expect(columnRenderLayoutsEqual(explicit([48, 0]), explicit([48, -10]))).toBe(true); + + // A SHORT array falls back to the scalar gap for the gutter it omits — it does not mean 0. These + // two render differently and must split; padding the authored array with 0 would call them equal. + expect(columnRenderLayoutsEqual(explicit([20]), explicit([20, 0]))).toBe(false); + expect(columnRenderLayoutsEqual(explicit([20]), explicit([20, 48]))).toBe(true); + + // Surplus gaps beyond count-1 are discarded by resolution, so they must not split either. + expect(columnRenderLayoutsEqual(explicit([48, 48, 999]), explicit([48, 48]))).toBe(true); }); it('distinguishes explicit vs equal mode and different resolved widths', () => { diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index 44294cfcbd..f2be923ffb 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -386,14 +386,23 @@ export function columnLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): boolean ); } +/** + * The gutters a layout will actually be drawn with: `gaps[i]` positionally when present, the scalar + * gap otherwise, each floored at 0. Mirrors `normalizeColumnLayout` so render equality stays exactly + * as discriminating as the geometry it stands in for. + */ +function effectiveColumnGaps(columns: ColumnLayout, count: number): number[] { + const gap = Math.max(0, columns.gap ?? 0); + const authored = Array.isArray(columns.gaps) ? columns.gaps : []; + return Array.from({ length: Math.max(0, count - 1) }, (_, i) => Math.max(0, authored[i] ?? gap)); +} + /** * Render equality: true when two column configs produce the SAME rendered layout even if their raw * fields differ. Compares the canonical render form for today's renderer (resolved mode + count, - * scalar gap, withSeparator, and in explicit mode the sliced widths) and deliberately ignores raw - * `equalWidth` and the surplus count/widths that resolution discards. Per-column `gaps` are - * intentionally ignored until geometry/separators consume them (step 4), so a gaps-only authored - * delta does not split regions or invalidate the normalized-columns cache before it becomes - * paint-significant. Use for region/cache change detection so e.g. `{num:4, widths:[a,b]}` vs + * scalar gap, withSeparator, and in explicit mode the sliced widths and per-column `gaps`) and + * deliberately ignores raw `equalWidth` and the surplus count/widths that resolution discards. + * Use for region/cache change detection so e.g. `{num:4, widths:[a,b]}` vs * `{num:2, widths:[a,b]}`, or `equalWidth:true` vs an omitted equalWidth, do not split into * separate regions. (SD-2629) */ @@ -411,6 +420,23 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo if (mode === 'explicit') { const ra = resolveColumnLayout(a); const rb = resolveColumnLayout(b); + // Per-column gaps ARE paint-significant. `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. + // This comparison used to skip them, on the note that nothing consumed them yet; SD-2629 step 4 + // made that false, and while it was skipped two sections differing only in their per-column gaps + // compared equal — no region split, no cache invalidation, and the later section laid out with + // the earlier one's gutters. + // + // Compare the gutters that will actually be DRAWN, not the authored arrays. `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 are equal in cases that + // render differently (a short `[20]` vs `[20, 0]`) and differ in cases that render identically + // (an omitted array vs one spelling out the scalar gap, or a negative gap vs 0). Deriving the + // effective gutters the way `normalizeColumnLayout` does keeps this predicate exactly as + // discriminating as the geometry it is standing in for. + if (!widthsEqual(effectiveColumnGaps(a, resolveColumnCount(a)), effectiveColumnGaps(b, resolveColumnCount(b)))) { + return false; + } if (!widthsEqual(ra.widths, rb.widths)) return false; } return true; diff --git a/packages/layout-engine/layout-engine/src/index.test.ts b/packages/layout-engine/layout-engine/src/index.test.ts index ae5fd6ed12..6e8a6d0cf3 100644 --- a/packages/layout-engine/layout-engine/src/index.test.ts +++ b/packages/layout-engine/layout-engine/src/index.test.ts @@ -7315,7 +7315,7 @@ describe('requirePageBoundary edge cases', () => { expect(right.width).toBeCloseTo(365.4); }); - it('keeps the current explicit column after a manual column break when only later per-column gaps differ', () => { + it('opens a new region after a manual column break when later per-column gaps differ', () => { const toExplicitColumns: FlowBlock = { kind: 'sectionBreak', id: 'sb-explicit', @@ -7366,15 +7366,17 @@ describe('requirePageBoundary edge cases', () => { expect(p2.x).toBeCloseTo(50); expect(p3.x).toBeCloseTo(expectedSecondColumnX); - expect(p4.x).toBeCloseTo(expectedSecondColumnX); - expect(page.columnRegions).toHaveLength(2); - expect(page.columnRegions?.[1]?.columns).toEqual({ + // Per-column gaps drive geometry, so the delta is paint-significant and opens its own region + // carrying the NEW gaps. It used to compare equal, and p4 was laid out with the old gutters. + expect(page.columnRegions).toHaveLength(3); + expect(page.columnRegions?.at(-1)?.columns).toEqual({ count: 3, gap: 48, widths: [100, 100, 300], - gaps: [48, 48], + gaps: [48, 96], equalWidth: false, }); + expect(p4.x).toBeCloseTo(50); }); it('does not balance the final page for explicit custom-width columns', () => { diff --git a/packages/layout-engine/layout-engine/src/section-breaks.test.ts b/packages/layout-engine/layout-engine/src/section-breaks.test.ts index 303cd16706..e2e7c9a08e 100644 --- a/packages/layout-engine/layout-engine/src/section-breaks.test.ts +++ b/packages/layout-engine/layout-engine/src/section-breaks.test.ts @@ -141,7 +141,7 @@ describe('scheduleSectionBreak', () => { expect(result.state.pendingColumns).toEqual({ count: 2, gap: 48 }); }); - it('does not trigger mid-page region change for explicit gaps-only changes before geometry uses gaps', () => { + it('triggers a mid-page region change for an explicit gaps-only delta', () => { const state = createSectionState({ activeColumns: { count: 3, gap: 48, widths: [100, 100, 300], gaps: [48, 48], equalWidth: false }, }); @@ -152,7 +152,10 @@ describe('scheduleSectionBreak', () => { const result = scheduleSectionBreak(block, state, BASE_MARGINS); - expect(result.decision.forceMidPageRegion).toBe(false); + // Per-column gaps drive geometry, so a gaps-only delta moves every column after the first + // and needs its own region. It used to compare equal, and the new section was then laid out + // with the previous one's gutters. + expect(result.decision.forceMidPageRegion).toBe(true); expect(result.state.pendingColumns).toEqual({ count: 3, gap: 48, From 71ff97ae4a5c8efbb166eea0bd0c6478a9971ee8 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 03:34:46 +0300 Subject: [PATCH 13/20] fix(layout): order a balanced page by column, not by fragment x 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. --- .../src/column-balancing.test.ts | 211 +++++++++++++++++- .../layout-engine/src/column-balancing.ts | 95 +++++++- 2 files changed, 293 insertions(+), 13 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/column-balancing.test.ts b/packages/layout-engine/layout-engine/src/column-balancing.test.ts index e49901ca8c..e184d4fe03 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.test.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.test.ts @@ -322,7 +322,27 @@ function createMeasure(kind: string, lineHeights: number[]): { kind: string; lin } describe('balanceSectionOnPage', () => { - type TestFragment = { blockId: string; x: number; y: number; width: number; kind: string }; + type TestFragment = { + blockId: string; + x: number; + y: number; + width: number; + kind: string; + columnIndex?: number; + }; + + /** + * Read the balanced page back the way a person does — each column top to bottom, columns in fill + * order — and return the document indices in that sequence. Asserting on this rather than on exact + * y values keeps the test about ORDER, so it does not break when the balancer legitimately picks a + * different split point. + */ + function readingOrder(fragments: TestFragment[], firstColumnX: number): number[] { + return fragments + .map((fragment, index) => ({ index, column: fragment.x === firstColumnX ? 0 : 1, y: fragment.y })) + .sort((a, b) => a.column - b.column || a.y - b.y) + .map((entry) => entry.index); + } /** Build a fragment + section mapping for section-scoped tests. */ function buildSectionFixture( @@ -420,6 +440,195 @@ describe('balanceSectionOnPage', () => { expect(fragments.map((f) => f.y)).toEqual([top, top + 20, top + 40, top, top + 20, top + 40]); }); + it('keeps document order when fragments in one column do not share an x', () => { + // Document order was reconstructed by sorting on raw x, on the premise that every fragment in a + // column starts at the same place. A negative w:ind, a float offset, or a right-aligned wide + // table all break that premise, and the balanced x/y are written back in whatever order the sort + // produced — so the page is REORDERED, not merely laid out oddly. + const top = 96; + const LEFT = 96; + const RIGHT = 432; // margin 96 + column 288 + gap 48 + // Paragraphs 0-3 fill the left column; #1 is indented 20px and #2 by a hair. 4-5 spilled right. + const placements = [ + { x: LEFT, y: top }, + { x: LEFT + 20, y: top + 20 }, + { x: LEFT + 1e-7, y: top + 40 }, + { x: LEFT, y: top + 60 }, + { x: RIGHT, y: top }, + { x: RIGHT, y: top + 20 }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ blockId: id, x: placement.x, y: placement.y, width: 288, kind: 'para' }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // Reading the page back gives document order. Sorting on x would have put #1 (x = 116) after + // #0, #2 and #3, so paragraph 1 would surface in the wrong place entirely. + expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4, 5]); + // Both columns are actually used, or the assertion above proves nothing. + expect(new Set(fragments.map((f) => f.x)).size).toBe(2); + }); + + it('keeps document order for an over-wide table whose x sits outside its column', () => { + // resolveTableFrame right-aligns an over-wide table inside its column, which puts its origin at + // a NEGATIVE offset — outside the column, and left of every fragment in the column before it. + const top = 96; + const LEFT = 96; + const RIGHT = 432; + const placements: TestFragment[] = [ + { blockId: '', x: LEFT, y: top, width: 288, kind: 'para' }, + { blockId: '', x: LEFT, y: top + 20, width: 288, kind: 'para' }, + { blockId: '', x: LEFT, y: top + 40, width: 288, kind: 'para' }, + // In the SECOND column, but right-aligned and 500 wide, so resolveTableFrame gives it + // 432 + (288 - 500) = 220 — an origin that lands INSIDE the first column's span. Nothing about + // its box says which column owns it; only the columnIndex the engine recorded does. + { blockId: '', x: 220, y: top, width: 500, kind: 'table', columnIndex: 1 }, + { blockId: '', x: RIGHT, y: top + 20, width: 288, kind: 'para' }, + { blockId: '', x: RIGHT, y: top + 40, width: 288, kind: 'para' }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ ...placement, blockId: id }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // It keeps its place in reading order. Sorting on x would have pulled it forward, because 220 is + // lower than every other fragment's x on the page. + expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4, 5]); + expect(new Set(fragments.map((f) => f.x)).size).toBe(2); + }); + + it('recovers document order from a shuffled fragment array', () => { + // The paginator's array order is not a contract, so the ordinal/y ordering has to stand on its + // own rather than leaning on the stability of an already-sorted input. + const top = 96; + const LEFT = 96; + const RIGHT = 432; + const byIndex = [ + { x: LEFT, y: top }, + { x: LEFT, y: top + 20 }, + { x: LEFT, y: top + 40 }, + { x: RIGHT, y: top }, + { x: RIGHT, y: top + 20 }, + { x: RIGHT, y: top + 40 }, + ]; + const shuffled = [3, 0, 5, 2, 4, 1]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + shuffled.forEach((documentIndex) => { + const id = `s2-b${documentIndex}`; + fragments.push({ blockId: id, ...byIndex[documentIndex], width: 288, kind: 'para' }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // Read the page back and recover the document indices, which the blockIds carry. + const recovered = fragments + .map((fragment) => ({ + n: Number(fragment.blockId.slice('s2-b'.length)), + column: fragment.x === LEFT ? 0 : 1, + y: fragment.y, + })) + .sort((a, b) => a.column - b.column || a.y - b.y) + .map((entry) => entry.n); + expect(recovered).toEqual([0, 1, 2, 3, 4, 5]); + }); + + it('resolves columns against the page margin, not the page origin', () => { + // Fragment x is absolute; column geometry is content-relative. A left margin wide enough to be + // mistaken for a column offset catches the conversion being dropped. + const top = 96; + const MARGIN = 300; + const LEFT = MARGIN; // column 0 + const RIGHT = MARGIN + 288 + 48; // column 1 + const placements = [ + { x: LEFT, y: top }, + { x: LEFT, y: top + 20 }, + { x: LEFT, y: top + 40 }, + { x: RIGHT, y: top }, + { x: RIGHT, y: top + 20 }, + { x: RIGHT, y: top + 40 }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ blockId: id, x: placement.x, y: placement.y, width: 288, kind: 'para' }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: MARGIN }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4, 5]); + expect(new Set(fragments.map((f) => f.x)).size).toBe(2); + }); + it('balances the target section and returns the tallest balanced column bottom', () => { // 6 equal paragraphs in a 2-col section → 3+3 balanced, tallest col ends at top + 3×20 = top + 60. const top = 96; diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index 6fe1dd848c..43a934d126 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -6,7 +6,13 @@ * matching Microsoft Word's behavior. */ -import { getColumnGeometry, getColumnX, hasGenuinelyUnequalExplicitColumnWidths } from '@superdoc/contracts'; +import { + findColumnContaining, + getColumnAtX, + getColumnGeometry, + getColumnX, + hasGenuinelyUnequalExplicitColumnWidths, +} from '@superdoc/contracts'; import type { BaseDirection } from '@superdoc/contracts'; // ============================================================================ @@ -794,19 +800,84 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu precedingHeight: precedingHeightBeforeTable, }); - // Order fragments in document order: by current column, then by y within each column. During - // unbalanced layout the paginator fills column 0 top-to-bottom, then column 1, etc. — so column - // order followed by y preserves the original sequence. + // Order fragments in document order: by the column each one occupies, then by y within it. During + // unbalanced layout the paginator fills column 0 top-to-bottom, then column 1, etc., so column + // ordinal followed by y reproduces the original sequence — and the balanced x/y are written back + // onto the fragments in exactly this order, so getting it wrong reorders the page rather than + // just laying it out oddly. // - // Which way "column order" runs across the page is direction-relative. In an RTL section column 0 - // is the RIGHT one, so document order DESCENDS in x; sorting ascending there would feed the - // balancer the trailing column first and silently scramble the balanced page's reading order, - // since the balanced x/y are then written back onto the fragments in this order. - const columnOrder = - sectionColumns.direction === 'rtl' ? (a: number, b: number) => b - a : (a: number, b: number) => a - b; + // Resolve the ordinal from the fragment rather than sorting on raw `x`. Sorting on x assumes every + // fragment in a column shares one origin, which is false for a negative `w:ind`, a float offset, + // or a right-aligned over-wide table — and a difference of 1e-7 is enough to swap two paragraphs. + // The ordinal is also direction-free: it is the fill order, so it needs no RTL special case, where + // an x comparison needs one because column 0 sits on the right and document order descends in x. + const preBalanceGeometry = getColumnGeometry({ + count: columnCount, + gap: columnGap, + width: columnWidth, + ...(Array.isArray(sectionColumns.widths) ? { widths: sectionColumns.widths } : {}), + ...(Array.isArray(sectionColumns.gaps) ? { gaps: sectionColumns.gaps } : {}), + ...(sectionColumns.direction !== undefined ? { direction: sectionColumns.direction } : {}), + ...(sectionColumns.contentWidth !== undefined ? { contentWidth: sectionColumns.contentWidth } : {}), + }); + const originX = args.margins.left; + const clampOrdinal = (value: number): number => Math.max(0, Math.min(columnCount - 1, Math.floor(value))); + + /** + * Which column a fragment occupies, as a fill-order ordinal. ALWAYS a number: the result is a sort + * key, and a key that is sometimes absent would leave the comparator mixing two different metrics, + * which is not a total order — `Array.prototype.sort` is then free to return different orders for + * the same input, and it does differ between engines. + */ + const ordinalOf = (fragment: BalancingFragment): number => { + // The engine's own record, where it kept one. Tables carry it; ordinary paragraphs do not. + const recorded = (fragment as { columnIndex?: number }).columnIndex; + if (typeof recorded === 'number' && Number.isFinite(recorded)) return clampOrdinal(recorded); + + // The origin is the column's left edge for ordinary content, and it stays right for content + // that merely OVERFLOWS its column, which an overlap comparison gets wrong once the columns are + // unequal enough for the spill to cover more than the column it came from. + const byOrigin = findColumnContaining(preBalanceGeometry, fragment.x, originX); + if (byOrigin !== null) return byOrigin; + + // Origin outside every column: a negative `w:ind` or a float offset hung it into the gutter. The + // box still lies mostly in the column that owns it. + const span = Number.isFinite(fragment.width) && fragment.width > 0 ? fragment.width : 0; + const left = fragment.x - originX; + let best: number | null = null; + let bestOverlap = 0; + for (const col of preBalanceGeometry) { + const overlap = Math.min(left + span, col.x + col.width) - Math.max(left, col.x); + if (overlap > bestOverlap) { + bestOverlap = overlap; + best = col.index; + } + } + // Touching no column at all (fully inside a gutter, or off the strip): fall back to the + // hit-testing walk, which clamps and therefore always names one. + return best ?? clampOrdinal(getColumnAtX(preBalanceGeometry, fragment.x, originX)); + }; + + // Order fragments in document order: by the column each one occupies, then by y within it. During + // unbalanced layout the paginator fills column 0 top-to-bottom, then column 1, etc., so column + // ordinal followed by y reproduces the original sequence — and the balanced x/y are written back + // onto the fragments in exactly this order, so getting it wrong REORDERS the page rather than + // merely laying it out oddly. + // + // Sorting on raw x, as this did, assumes every fragment in a column shares one origin. A negative + // `w:ind`, a float offset, or a right-aligned over-wide table each break that, and a difference of + // 1e-7 was enough to swap two paragraphs. The ordinal is also direction-free — it IS the fill + // order — where an x comparison needed an RTL special case because column 0 sits on the right. + const ordinals = new Map(); + sectionFragments.forEach((fragment) => ordinals.set(fragment, ordinalOf(fragment))); + const inputOrder = new Map(); + sectionFragments.forEach((fragment, index) => inputOrder.set(fragment, index)); const ordered = [...sectionFragments].sort((a, b) => { - if (a.x !== b.x) return columnOrder(a.x, b.x); - return a.y - b.y; + const byColumn = (ordinals.get(a) ?? 0) - (ordinals.get(b) ?? 0); + if (byColumn !== 0) return byColumn; + if (a.y !== b.y) return a.y - b.y; + // Same column, same y: keep the order they arrived in, so the sort is total and stable. + return (inputOrder.get(a) ?? 0) - (inputOrder.get(b) ?? 0); }); // Treat each fragment as its own block for binary-search balancing. Grouping From e8f70b971e096c558d45c13e252f07ad89d7a4ef Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 17:13:41 +0300 Subject: [PATCH 14/20] fix(contracts): stop an unreachable scalar gap from splitting an explicit 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 --- .../contracts/src/column-layout.test.ts | 59 +++++++++++++++++++ .../contracts/src/column-layout.ts | 27 ++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index d2b88ed957..a55d2f5ec9 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -436,6 +436,65 @@ describe('columnRenderLayoutsEqual (SD-2629)', () => { expect(columnRenderLayoutsEqual(explicit([48, 48, 999]), explicit([48, 48]))).toBe(true); }); + it('ignores a scalar gap that explicit per-column gaps have made unreachable', () => { + const explicit = (gap: number, gaps?: number[]): ColumnLayout => ({ + count: 3, + gap, + widths: [100, 100, 300], + equalWidth: false, + ...(gaps ? { gaps } : {}), + }); + + // `gaps` supplies BOTH gutters, so geometry never reaches the scalar fallback and these two draw + // the same three columns in the same places. The scalar comparison used to run ahead of the mode + // branch and split anyway — a mid-page region break plus a cache flush over a value nothing + // read, which restarts the following content in column 0. + expect(columnRenderLayoutsEqual(explicit(24, [30, 40]), explicit(900, [30, 40]))).toBe(true); + + // The moment the array stops supplying a gutter, the scalar IS that gutter and has to + // discriminate again. + expect(columnRenderLayoutsEqual(explicit(24, [30]), explicit(900, [30]))).toBe(false); + expect(columnRenderLayoutsEqual(explicit(24), explicit(900))).toBe(false); + + // Equal mode reads the scalar twice over — as every gutter, and as what normalization subtracts + // before dividing the content area — so it still splits there. + expect(columnRenderLayoutsEqual({ count: 3, gap: 24 }, { count: 3, gap: 40 })).toBe(false); + }); + + it('still splits explicit sub-pixel widths on the scalar gap alone', () => { + // The one route by which the scalar reaches explicit WIDTHS rather than the gutters: normalize + // floors a fabricated width at 1px, and collapses to a single full-width column once the usable + // width falls to epsilon — both keyed on the sign of `contentWidth - gap * (count - 1)`, which + // the scalar moves. It can only bite when an authored width is ITSELF sub-pixel, because at 1px + // or more the floor and the collapse are no-ops, which is why the two tests above are unaffected. + // + // Measured at contentWidth 720: `[0.5, 0.5, 0.5]` renders as three 1px columns under gap 24 and + // as three 0.5px ones under gap 999, and at 1e-5 the second collapses to one 720px column. So + // these pairs must NOT compare equal, even though `gaps` supplies every gutter. + const subPixel = (gap: number): ColumnLayout => ({ + count: 3, + gap, + widths: [0.5, 0.5, 0.5], + equalWidth: false, + gaps: [1, 1], + }); + expect(columnRenderLayoutsEqual(subPixel(24), subPixel(999))).toBe(false); + // Same scalar on both sides is still equal — the guard keys on the gap differing, not on the + // widths being small. + expect(columnRenderLayoutsEqual(subPixel(24), subPixel(24))).toBe(true); + + // A whole-pixel width is immune, which is the property that keeps this off every real document: + // no `w:col/@w` a document can author lands under 1px except at absurd twip values. + const wholePixel = (gap: number): ColumnLayout => ({ + count: 3, + gap, + widths: [1, 1, 1], + equalWidth: false, + gaps: [1, 1], + }); + expect(columnRenderLayoutsEqual(wholePixel(24), wholePixel(999))).toBe(true); + }); + it('distinguishes explicit vs equal mode and different resolved widths', () => { expect( columnRenderLayoutsEqual({ count: 2, gap: 24, widths: [192, 384], equalWidth: false }, { count: 2, gap: 24 }), diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index f2be923ffb..6bf16563be 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -412,7 +412,6 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo const mode = resolveColumnMode(a); if (mode !== resolveColumnMode(b)) return false; if (resolveColumnCount(a) !== resolveColumnCount(b)) return false; - if ((a.gap ?? 0) !== (b.gap ?? 0)) return false; if (Boolean(a.withSeparator) !== Boolean(b.withSeparator)) return false; // Direction IS paint-significant: it decides which side column 0 lands on, so two layouts that // differ only here must split regions and invalidate the normalized-columns cache. @@ -438,6 +437,32 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo return false; } if (!widthsEqual(ra.widths, rb.widths)) return false; + // The one way the scalar still reaches explicit WIDTHS: `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 (under ~15 twips), 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, refuse to call such a pair equal + // at all: `{widths: [0.5, 0.5, 0.5], gaps: [1, 1]}` renders as three 1px columns under one scalar + // gap and three 0.5px columns under another, and at 1e-5 the second collapses to one full-width + // column. Costs nothing on any real document. + const hasSubPixelWidth = (resolved: ColumnLayout): boolean => (resolved.widths ?? []).some((w) => w < 1); + if ((hasSubPixelWidth(ra) || hasSubPixelWidth(rb)) && (a.gap ?? 0) !== (b.gap ?? 0)) return false; + } else if ((a.gap ?? 0) !== (b.gap ?? 0)) { + // Equal mode reads the scalar gap twice over — it IS every gutter, and `normalizeColumnLayout` + // subtracts the total from the content area before dividing it, so it sets the column width as + // well. Nothing else can stand in for it here. + // + // Explicit mode deliberately does NOT compare it on its own. There the scalar is only the + // fallback for a gutter `gaps` does not supply, and `effectiveColumnGaps` above already folds it + // in at exactly that position — so a layout whose `gaps` spell out every gutter renders + // identically no matter what the scalar says, and comparing it separately split a region and + // invalidated the normalized-columns cache over a value nothing read. (The scalar can still + // reach explicit widths through the sub-pixel `Math.max(1, …)` floor, which turns on the sign of + // `contentWidth - gap * (count - 1)`; that is content-width-dependent, and this predicate is + // documented as content-width-INDEPENDENT — the widths comparison does not model the floor + // either.) + return false; } return true; } From 161f852220580884b00e7d3fe3a892d1614b600a Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 17:14:01 +0300 Subject: [PATCH 15/20] fix(layout): decide which column owns a fragment from more than its origin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/column-balancing.test.ts | 263 ++++++++++++++++++ .../layout-engine/src/column-balancing.ts | 73 ++++- 2 files changed, 325 insertions(+), 11 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/column-balancing.test.ts b/packages/layout-engine/layout-engine/src/column-balancing.test.ts index e184d4fe03..f0f6c69eb1 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.test.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.test.ts @@ -329,6 +329,7 @@ describe('balanceSectionOnPage', () => { width: number; kind: string; columnIndex?: number; + height?: number; }; /** @@ -535,6 +536,231 @@ describe('balanceSectionOnPage', () => { expect(new Set(fragments.map((f) => f.x)).size).toBe(2); }); + it('keeps document order for an over-wide anchored table that recorded no column', () => { + // The test above leans on the columnIndex the paginator recorded. An ANCHORED table has none: + // `createAnchoredTableFragment` never sets the field, so a floating over-wide table reaches the + // geometric fallback with nothing but its box. Answering from the box's ORIGIN gets it wrong, + // because resolveTableFrame's right-aligned placement puts that origin inside an EARLIER column + // — and `end` is the default justification for any w:bidiVisual table, so this is the common + // shape, not an exotic one. Its trailing edge is what still lands on its own column's edge. + const top = 96; + const LEFT = 96; + const RIGHT = 432; + const placements: TestFragment[] = [ + { blockId: '', x: LEFT, y: top, width: 288, kind: 'para' }, + { blockId: '', x: LEFT, y: top + 20, width: 288, kind: 'para' }, + { blockId: '', x: LEFT, y: top + 40, width: 288, kind: 'para' }, + // Identical to the recorded-column case (432 + (288 - 500) = 220) with the record removed. + { blockId: '', x: 220, y: top, width: 500, kind: 'table' }, + { blockId: '', x: RIGHT, y: top + 20, width: 288, kind: 'para' }, + { blockId: '', x: RIGHT, y: top + 40, width: 288, kind: 'para' }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ ...placement, blockId: id }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + // Containment on the origin put it in column 0 and hoisted it above the two paragraphs that + // precede it there. + expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4, 5]); + expect(new Set(fragments.map((f) => f.x)).size).toBe(2); + }); + + it('updates a recorded column when balancing moves the fragment to another one', () => { + // The canonical last page of a balanced section: the paginator filled column 0 top to bottom and + // left column 1 empty, so everything it recorded a column for recorded column 0. Balancing then + // moves the tail into column 1 — and the record has to follow, because two consumers now treat + // it as authoritative. Ordering above prefers it over any geometry, and the DOM painter's + // separator gate reads it to decide which columns hold content, so a table left claiming column + // 0 while painted in column 1 can suppress a separator Word draws. + const top = 96; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + for (let i = 0; i < 5; i += 1) { + const id = `p${i}`; + fragments.push({ blockId: id, x: 96, y: top + i * 20, width: 288, kind: 'para' }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 0); + } + // layout-table.ts stamps columnIndex on a flow table; this one was laid out in column 0. + fragments.push({ blockId: 'tbl', x: 96, y: top + 100, width: 288, kind: 'table', height: 20, columnIndex: 0 }); + measureMap.set('tbl', createMeasure('table', [])); + blockSectionMap.set('tbl', 0); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 0, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + const table = fragments.find((f) => f.blockId === 'tbl')!; + expect(table.x).toBe(432); + expect(table.columnIndex).toBe(1); + // A paragraph the paginator recorded nothing for still records nothing: the presence of a value + // is the engine's own evidence of ownership, and the separator gate reads it as such. + expect(fragments.filter((f) => f.blockId !== 'tbl').every((f) => f.columnIndex === undefined)).toBe(true); + }); + + describe('resolving which column owns a fragment', () => { + // Every case here uses EQUAL columns, because `hasGenuinelyUnequalExplicitColumnWidths` makes + // balancing skip a genuinely unequal explicit section outright — an unequal-width fixture would + // return null and prove nothing. + // + // 200px columns over a 420px content area with a 20px gutter: column 0 spans 96..296 absolute, + // column 1 spans 316..516. Each case builds four 20px fragments, two per column, and replaces + // one of them with the fragment under test. Correct ordinals always read back as [0, 1, 2, 3]; + // balancing splits the page 2/2, so a misresolved ordinal hoists the fragment past a sibling and + // shows up as a swap rather than merely an odd position. + const TOP = 96; + const LEFT = 96; + const COL1 = 316; + + function orderWith(slot: number, special: TestFragment): number[] { + const base: TestFragment[] = [ + { blockId: '', x: LEFT, y: TOP, width: 200, kind: 'para' }, + { blockId: '', x: LEFT, y: TOP + 20, width: 200, kind: 'para' }, + { blockId: '', x: COL1, y: TOP, width: 200, kind: 'para' }, + { blockId: '', x: COL1, y: TOP + 20, width: 200, kind: 'para' }, + ]; + base[slot] = { ...special, y: base[slot].y }; + + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + base.forEach((fragment, i) => { + const id = `s2-b${i}`; + fragments.push({ ...fragment, blockId: id }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 20, width: 200, contentWidth: 420 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: LEFT }, + topMargin: TOP, + columnWidth: 200, + availableHeight: 60, + measureMap, + }); + expect(result).not.toBeNull(); + return readingOrder(fragments, LEFT); + } + + it('takes the column the engine recorded over anything the geometry says', () => { + // A footnote body is placed in its own note band, so its x can name one column while the flow + // column that owns it is another (contracts/index.ts calls the field "distinct from visual + // x"). Here the box sits exactly on column 0's leading edge — the strongest signal geometry + // has — while the record says column 1. The record has to win, or a note band's ordering comes + // from where it was drawn rather than what it belongs to. + expect(orderWith(2, { blockId: '', x: LEFT, y: 0, width: 200, kind: 'para', columnIndex: 1 })).toEqual([ + 0, 1, 2, 3, + ]); + }); + + it('reads a table as wide as the whole content area from its leading edge', () => { + // A table at column 0's left edge that spans the entire content area ENDS exactly on column + // 1's trailing edge. The trailing-edge rule below would therefore claim it for column 1, which + // is why the leading edge has to be asked first: content overflows rightward from its own + // column's start, so a leading-edge match settles ownership on its own. + expect(orderWith(1, { blockId: '', x: LEFT, y: 0, width: 420, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); + }); + + it('reads a right-aligned over-wide table from its trailing edge', () => { + // resolveTableFrame places an over-wide table that justifies to `end` at + // `col.x + (col.width - width)`: 316 + (200 - 600) = -84, so it starts left of the page's + // content area entirely. Overlap cannot settle this one — the table covers 200px of column 0 + // and 200px of column 1, an exact tie that resolves to the EARLIER column and pulls the table + // ahead of the paragraphs it follows. Its trailing edge still lands on column 1's, and `end` + // is the default justification for any w:bidiVisual table, so this is the common shape. + expect(orderWith(2, { blockId: '', x: -84, y: 0, width: 600, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); + }); + + it('reads a centred over-wide table from the column it covers, not the one it starts in', () => { + // Centred, an over-wide table lands on NEITHER column edge: 316 + (200 - 400) / 2 = 216, ending + // at 616. Its origin sits inside column 0, so containment names column 0 — and that is why + // containment only counts when the box FITS the column it starts in. It does not here, so + // overlap decides, and the table covers 200px of column 1 against 80px of column 0. + expect(orderWith(2, { blockId: '', x: 216, y: 0, width: 400, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); + }); + }); + + it('still reads an indented fragment from the column its origin sits in', () => { + // The trailing-edge rule must not swallow the case it was added beside. A paragraph indented by + // a `w:ind` matches NEITHER column edge, and it is the origin that identifies it — so long as + // the box still fits the column it starts in, which is what an indent leaves behind. + const top = 96; + const LEFT = 96; + const RIGHT = 432; + const placements: TestFragment[] = [ + { blockId: '', x: LEFT, y: top, width: 288, kind: 'para' }, + // Indented 36px inside column 0 and 88px short of its trailing edge, so it lands on NEITHER + // column edge and the origin is all there is to go on. Its own column is still the only one + // it overlaps. + { blockId: '', x: LEFT + 36, y: top + 20, width: 200, kind: 'para' }, + { blockId: '', x: LEFT, y: top + 40, width: 288, kind: 'para' }, + // The same indent in column 1. + { blockId: '', x: RIGHT + 36, y: top, width: 200, kind: 'para' }, + { blockId: '', x: RIGHT, y: top + 20, width: 288, kind: 'para' }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ ...placement, blockId: id }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 2, gap: 48, width: 288, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 288, + availableHeight: 60, + measureMap, + }); + + expect(result).not.toBeNull(); + expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4]); + }); + it('recovers document order from a shuffled fragment array', () => { // The paginator's array order is not a contract, so the ordinal/y ordering has to stand on its // own rather than leaning on the stability of an already-sorted input. @@ -901,6 +1127,43 @@ describe('balanceSectionOnPage', () => { ...extra, }); + it('moves the recorded column of a split half to the column it lands in', () => { + // Column ordering trusts a fragment's own `columnIndex` ahead of any geometry, so a record + // that contradicts the placement is worse than none. The spread that builds the second half + // copies the FIRST half's column, and the half is then placed one column further on — so a + // later balancing pass would read the stale record and sort the half back into the column it + // was moved out of, undoing the split it is the other half of. + const { fragments, measureMap, blockSectionMap } = straddleFixture(); + // The paginator records the column for a footnote body and for tables, not for an ordinary + // paragraph; C carries one here to stand for the fragments that do. + const c = fragments.find((f) => f.blockId === 'C')!; + c.columnIndex = 0; + + expect(balance(fragments, measureMap, blockSectionMap)).not.toBeNull(); + + const [c1, c2] = (fragments.filter((f) => f.blockId === 'C') as SplitFragment[]).sort( + (a, b) => (a.fromLine ?? 0) - (b.fromLine ?? 0), + ); + expect(c1.x).toBe(96); + expect(c2.x).toBe(COL1_X); + // Each half's record now names the column it is actually in. + expect(c1.columnIndex).toBe(0); + expect(c2.columnIndex).toBe(1); + }); + + it('does not invent a recorded column for a half that never had one', () => { + // The separator gate reads the same field and treats a value as the engine's own evidence of + // ownership, so filling it in for a paragraph that carried nothing would be asserting more + // than the paginator ever knew. + const { fragments, measureMap, blockSectionMap } = straddleFixture(); + + expect(balance(fragments, measureMap, blockSectionMap)).not.toBeNull(); + + const halves = fragments.filter((f) => f.blockId === 'C') as SplitFragment[]; + expect(halves.length).toBe(2); + expect(halves.every((half) => half.columnIndex === undefined)).toBe(true); + }); + it('splits a straddling paragraph at a line boundary so columns balance', () => { const { fragments, measureMap, blockSectionMap } = straddleFixture(); diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index 43a934d126..841b2eafc2 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -822,6 +822,10 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu }); const originX = args.margins.left; const clampOrdinal = (value: number): number => Math.max(0, Math.min(columnCount - 1, Math.floor(value))); + // Sub-pixel tolerance for "this edge IS that column's edge". Column x values reach fragments + // through `getColumnX`, so unindented content matches exactly and the slack only absorbs float + // drift — it stays two orders of magnitude below the smallest indent a document can author. + const EDGE_EPSILON = 0.01; /** * Which column a fragment occupies, as a fill-order ordinal. ALWAYS a number: the result is a sort @@ -834,27 +838,53 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu const recorded = (fragment as { columnIndex?: number }).columnIndex; if (typeof recorded === 'number' && Number.isFinite(recorded)) return clampOrdinal(recorded); - // The origin is the column's left edge for ordinary content, and it stays right for content - // that merely OVERFLOWS its column, which an overlap comparison gets wrong once the columns are - // unequal enough for the spill to cover more than the column it came from. - const byOrigin = findColumnContaining(preBalanceGeometry, fragment.x, originX); - if (byOrigin !== null) return byOrigin; - - // Origin outside every column: a negative `w:ind` or a float offset hung it into the gutter. The - // box still lies mostly in the column that owns it. const span = Number.isFinite(fragment.width) && fragment.width > 0 ? fragment.width : 0; const left = fragment.x - originX; + const right = left + span; + + // A box on a column's LEADING edge is that column's, fit or no fit. That covers ordinary + // content, and it covers content WIDER than its column, which overflows rightward from that + // same edge in either direction. It has to be asked before any overlap test, because an + // over-wide box can cover more of a wide neighbour than of the narrow column it came from. + for (const col of preBalanceGeometry) { + if (Math.abs(left - col.x) <= EDGE_EPSILON) return col.index; + } + + // A box on a column's TRAILING edge is that column's too, and it is a different question, not a + // mirror of the one above. `resolveTableFrame` places an over-wide table that justifies to `end` + // at `col.x + (col.width - width)` — a NEGATIVE offset from its own column — 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, and once it is + // wide enough its overlap outvotes its own column too. An anchored table reaches this at all + // because `createAnchoredTableFragment` records no `columnIndex` for the step above to read. + for (const col of preBalanceGeometry) { + if (Math.abs(right - (col.x + col.width)) <= EDGE_EPSILON) return col.index; + } + + // Neither edge lands on a column edge, so an indent moved the origin. Containment is right while + // the box still FITS the column it starts in, which is what an indent leaves behind. Demanding + // the fit is what stops this from claiming a SHIFTED origin: a centred over-wide table also + // begins inside an earlier column, and there the origin is no evidence of ownership at all. + const byOrigin = findColumnContaining(preBalanceGeometry, fragment.x, originX); + if (byOrigin !== null) { + const originColumn = preBalanceGeometry.find((col) => col.index === byOrigin); + if (originColumn && right <= originColumn.x + originColumn.width + EDGE_EPSILON) return byOrigin; + } + + // What is left is a box whose origin cannot be trusted: centred over-wide content, or an origin + // hung into a gutter by a negative `w:ind` or a float offset. It still lies mostly in the column + // that owns it. Ties go to the earliest column in fill order. let best: number | null = null; let bestOverlap = 0; for (const col of preBalanceGeometry) { - const overlap = Math.min(left + span, col.x + col.width) - Math.max(left, col.x); + const overlap = Math.min(right, col.x + col.width) - Math.max(left, col.x); if (overlap > bestOverlap) { bestOverlap = overlap; best = col.index; } } - // Touching no column at all (fully inside a gutter, or off the strip): fall back to the - // hit-testing walk, which clamps and therefore always names one. + // Touching no column at all (zero width, wholly inside a gutter, or off the strip): fall back to + // the hit-testing walk, which clamps and therefore always names one. return best ?? clampOrdinal(getColumnAtX(preBalanceGeometry, fragment.x, originX)); }; @@ -966,6 +996,18 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu f.x = columnX(col); f.y = colCursors[col]; f.width = columnWidth; + // Balancing MOVES fragments between columns, so a `columnIndex` the paginator wrote before the + // move now names the column the fragment left. Three consumers trust that record ahead of any + // geometry — `ordinalOf` above, the painter's separator gate, and `determineTableColumn` for hit + // testing — so a stale one is worse than none: an in-flow table balanced out of column 0 kept + // reporting column 0, which left its new column reading as empty and suppressed a rule Word + // draws. Rewrite it where the fragment had one; do not invent one where it did not, for the same + // reason the split branch below does not — an invented value is evidence the fragment never + // carried, and the gate would act on it. + const recordedColumn = (f as { columnIndex?: number }).columnIndex; + if (typeof recordedColumn === 'number' && Number.isFinite(recordedColumn)) { + (f as { columnIndex?: number }).columnIndex = col; + } // SD-3359: apply a line-boundary split chosen by the balancer. The first // half keeps the leading lines in this column; a cloned second half carries // the remaining lines to the top of the next column — the same @@ -990,6 +1032,15 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu continuesFromPrev: true, continuesOnNext: originalContinuesOnNext, } as BalancingFragment; + // The half lands in the NEXT column, so a `columnIndex` carried over by the spread would still + // name the FIRST half's column — and column ordering trusts that record ahead of any geometry, + // so a later balancing pass would sort this half back into the column it left. Rewrite it + // where the source had one; do not invent one where it did not, because the separator gate + // reads the same field and an invented value is evidence it never had. + const carriedColumn = (f as { columnIndex?: number }).columnIndex; + if (typeof carriedColumn === 'number' && Number.isFinite(carriedColumn)) { + (secondHalf as { columnIndex?: number }).columnIndex = col + 1; + } // Remeasured fragments render their own `lines` wholesale (fromLine/toLine are // ignored by the resolve stage then), so the halves must each carry ONLY their // slice or both columns render the entire paragraph. From 760df12e2a910e5c17f04c708ff0397b8f67417a Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 17:19:25 +0300 Subject: [PATCH 16/20] fix(layout): record the flow column on an anchored table fragment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../layout-engine/layout-engine/src/index.ts | 4 +- .../layout-engine/src/layout-paragraph.ts | 2 +- .../layout-engine/src/layout-table.test.ts | 44 ++++++++++++++++++- .../layout-engine/src/layout-table.ts | 16 +++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/index.ts b/packages/layout-engine/layout-engine/src/index.ts index 692f670f17..73935ec5da 100644 --- a/packages/layout-engine/layout-engine/src/index.ts +++ b/packages/layout-engine/layout-engine/src/index.ts @@ -3920,7 +3920,9 @@ function* layoutDocumentSteps( const anchorX = tableBlock.anchor?.offsetH ?? columnX(state); floatManager.registerTable(tableBlock, tableMeasure, anchorY, state.columnIndex, state.page.number); - state.page.fragments.push(createAnchoredTableFragment(tableBlock, tableMeasure, anchorX, anchorY)); + state.page.fragments.push( + createAnchoredTableFragment(tableBlock, tableMeasure, anchorX, anchorY, state.columnIndex), + ); } } diff --git a/packages/layout-engine/layout-engine/src/layout-paragraph.ts b/packages/layout-engine/layout-engine/src/layout-paragraph.ts index f20843c160..b7f8c4dd9a 100644 --- a/packages/layout-engine/layout-engine/src/layout-paragraph.ts +++ b/packages/layout-engine/layout-engine/src/layout-paragraph.ts @@ -878,7 +878,7 @@ export function layoutParagraphBlock(ctx: ParagraphLayoutContext, anchors?: Para ) : columnX(state); - const fragment = createAnchoredTableFragment(entry.block, entry.measure, anchorX, anchorY); + const fragment = createAnchoredTableFragment(entry.block, entry.measure, anchorX, anchorY, state.columnIndex); state.page.fragments.push(fragment); registeredAnchoredTablePlacements.set(entry.block.id, { page: state.page, diff --git a/packages/layout-engine/layout-engine/src/layout-table.test.ts b/packages/layout-engine/layout-engine/src/layout-table.test.ts index d2600c57f8..617177eb1e 100644 --- a/packages/layout-engine/layout-engine/src/layout-table.test.ts +++ b/packages/layout-engine/layout-engine/src/layout-table.test.ts @@ -4,7 +4,7 @@ import type { BlockId, TableAttrs, TableBlock, TableFragment, TableMeasure } from '@superdoc/contracts'; import { describe, expect, it } from 'bun:test'; -import { layoutTableBlock } from './layout-table.js'; +import { createAnchoredTableFragment, layoutTableBlock } from './layout-table.js'; /** * Creates a dummy table fragment for test scenarios where prior page content is needed. @@ -4954,3 +4954,45 @@ describe('layoutTableBlock', () => { }); }); }); + +describe('createAnchoredTableFragment', () => { + const block = createMockTableBlock(1); + const measure = createMockTableMeasure([500], [40]); + + it('records the flow column it was given', () => { + // An anchored table's `x` does not identify its column. `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 for any `w:bidiVisual` table — so + // an over-wide table BEGINS inside an earlier column without ever having left its own. + // Measured on 2 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 had to guess + // the column from that origin and all three guessed wrong — balancing's fragment ordering, + // `determineTableColumn` for hit testing, and the painter's column-separator gate. + const fragment = createAnchoredTableFragment(block, measure, 220, 300, 1); + + expect(fragment.columnIndex).toBe(1); + expect(fragment.isAnchored).toBe(true); + expect(fragment.x).toBe(220); + }); + + it('omits the record entirely when the caller has no column context', () => { + // Absent is NOT column 0. The separator gate reads this field ahead of any geometry, so a + // fabricated 0 would be evidence the fragment never carried — and on a page whose text never + // left the first column that is exactly the difference between a rule and no rule. + const fragment = createAnchoredTableFragment(block, measure, 220, 300); + + expect('columnIndex' in fragment).toBe(false); + expect(fragment.columnIndex).toBeUndefined(); + }); + + it('rejects a column that is not a usable ordinal', () => { + // Same reasoning as above, one step further out: NaN and Infinity reach here from arithmetic on + // a malformed anchor, and writing either would make the field unusable rather than absent. + expect(createAnchoredTableFragment(block, measure, 0, 0, Number.NaN).columnIndex).toBeUndefined(); + expect(createAnchoredTableFragment(block, measure, 0, 0, Number.POSITIVE_INFINITY).columnIndex).toBeUndefined(); + // A fractional or negative ordinal is floored into range rather than dropped, because it still + // names a column the caller meant. + expect(createAnchoredTableFragment(block, measure, 0, 0, 1.9).columnIndex).toBe(1); + expect(createAnchoredTableFragment(block, measure, 0, 0, -3).columnIndex).toBe(0); + }); +}); diff --git a/packages/layout-engine/layout-engine/src/layout-table.ts b/packages/layout-engine/layout-engine/src/layout-table.ts index 9994b5d013..8d5dec9de2 100644 --- a/packages/layout-engine/layout-engine/src/layout-table.ts +++ b/packages/layout-engine/layout-engine/src/layout-table.ts @@ -1946,12 +1946,23 @@ export function layoutTableBlock(context: TableLayoutContext): void { /** * Create a table fragment for an anchored/floating table at its computed position. * Called by the layout engine after the float manager computes the table's position. + * + * `columnIndex` is the flow column the table belongs to, which for an anchored table is NOT + * recoverable from `x`. `resolveAnchoredGraphicX` places one that justifies to `end` at + * `col.x + (col.width - width)`, a negative offset from its own column whenever the table is wider + * than it — and `end` is the default for any `w:bidiVisual` table — so an over-wide table BEGINS + * inside an earlier column without ever having left its own. Every consumer that has to infer the + * column from coordinates gets that wrong: it reorders a balanced page, it answers the wrong column + * for a click, and it decides whether a column separator is drawn. The in-flow table paths have + * always recorded the field for exactly this reason; the anchored ones did not, which is why the + * inference existed at all. */ export function createAnchoredTableFragment( block: TableBlock, measure: TableMeasure, x: number, y: number, + columnIndex?: number, ): TableFragment { const metadata = generateFragmentMetadata(measure, block, 0, block.rows.length, 0); @@ -1967,6 +1978,11 @@ export function createAnchoredTableFragment( height: measure.totalHeight ?? 0, metadata, sourceAnchor: block.sourceAnchor, + // Optional so a caller with no column context omits it rather than asserting column 0, which + // the separator gate would read as content the page does not have. + ...(typeof columnIndex === 'number' && Number.isFinite(columnIndex) + ? { columnIndex: Math.max(0, Math.floor(columnIndex)) } + : {}), }; applyTableFragmentPmRange(fragment, block, measure); return fragment; From e7af399af24027214395c576c4f6918543441da1 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 17:28:40 +0300 Subject: [PATCH 17/20] docs(contracts): stop the gap comment contradicting the sub-pixel guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- packages/layout-engine/contracts/src/column-layout.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index 6bf16563be..11720f2b14 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -457,11 +457,11 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo // fallback for a gutter `gaps` does not supply, and `effectiveColumnGaps` above already folds it // in at exactly that position — so a layout whose `gaps` spell out every gutter renders // identically no matter what the scalar says, and comparing it separately split a region and - // invalidated the normalized-columns cache over a value nothing read. (The scalar can still - // reach explicit widths through the sub-pixel `Math.max(1, …)` floor, which turns on the sign of - // `contentWidth - gap * (count - 1)`; that is content-width-dependent, and this predicate is - // documented as content-width-INDEPENDENT — the widths comparison does not model the floor - // either.) + // invalidated the normalized-columns cache over a value nothing read. The one route by which the + // scalar still reaches explicit WIDTHS — normalize's sub-pixel `Math.max(1, …)` floor and its + // epsilon collapse, both keyed on the sign of `contentWidth - gap * (count - 1)` — is handled by + // the `hasSubPixelWidth` guard in the explicit branch above, not waved off here: a width of 1px + // or more makes both no-ops, which is exactly the threshold that guard tests. return false; } return true; From 3c2b2ab5182759ac56a74d0f211b467158e06f58 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:05:54 +0300 Subject: [PATCH 18/20] docs(layout): name the fragment kinds that actually record a column MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../layout-engine/layout-engine/src/column-balancing.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index 841b2eafc2..50202b9d81 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -834,7 +834,11 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu * the same input, and it does differ between engines. */ const ordinalOf = (fragment: BalancingFragment): number => { - // The engine's own record, where it kept one. Tables carry it; ordinary paragraphs do not. + // The engine's own record, where it kept one. Only a few fragment kinds do: tables + // (`layout-table.ts`, five sites), footnote bodies, and a paragraph ONLY when it is a collapsed + // split-line-break anchor carrier (`layout-paragraph.ts`, under `collapseSplitLineBreakCarrier`) + // — a narrow document shape, not the ordinary paragraph. The rules below therefore carry almost + // every fragment. const recorded = (fragment as { columnIndex?: number }).columnIndex; if (typeof recorded === 'number' && Number.isFinite(recorded)) return clampOrdinal(recorded); From 4b286346c797ba92062f1d994fddb3b4d9559b3f Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:25:37 +0300 Subject: [PATCH 19/20] fix(layout): trust a fragment's origin by its width, not by its edges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/column-balancing.test.ts | 142 +++++++++++++----- .../layout-engine/src/column-balancing.ts | 79 +++++----- 2 files changed, 149 insertions(+), 72 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/column-balancing.test.ts b/packages/layout-engine/layout-engine/src/column-balancing.test.ts index f0f6c69eb1..f9bcf36939 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.test.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.test.ts @@ -536,13 +536,13 @@ describe('balanceSectionOnPage', () => { expect(new Set(fragments.map((f) => f.x)).size).toBe(2); }); - it('keeps document order for an over-wide anchored table that recorded no column', () => { - // The test above leans on the columnIndex the paginator recorded. An ANCHORED table has none: - // `createAnchoredTableFragment` never sets the field, so a floating over-wide table reaches the - // geometric fallback with nothing but its box. Answering from the box's ORIGIN gets it wrong, - // because resolveTableFrame's right-aligned placement puts that origin inside an EARLIER column - // — and `end` is the default justification for any w:bidiVisual table, so this is the common - // shape, not an exotic one. Its trailing edge is what still lands on its own column's edge. + it('keeps document order for an over-wide anchored table by the column it recorded', () => { + // Same shape as the test above, reached the other way: an ANCHORED table. + // `createAnchoredTableFragment` used to record no column at all, which left a floating over-wide + // table to be placed from its box alone — and its box cannot say which column owns it, because + // resolveTableFrame's `end` placement (the default for any w:bidiVisual table) puts its origin + // inside an EARLIER column while its trailing edge sits on its own. The factory now writes + // `state.columnIndex`, so the record answers it. const top = 96; const LEFT = 96; const RIGHT = 432; @@ -550,8 +550,8 @@ describe('balanceSectionOnPage', () => { { blockId: '', x: LEFT, y: top, width: 288, kind: 'para' }, { blockId: '', x: LEFT, y: top + 20, width: 288, kind: 'para' }, { blockId: '', x: LEFT, y: top + 40, width: 288, kind: 'para' }, - // Identical to the recorded-column case (432 + (288 - 500) = 220) with the record removed. - { blockId: '', x: 220, y: top, width: 500, kind: 'table' }, + // 432 + (288 - 500) = 220, an origin that lands inside the FIRST column's span. + { blockId: '', x: 220, y: top, width: 500, kind: 'table', columnIndex: 1 }, { blockId: '', x: RIGHT, y: top + 20, width: 288, kind: 'para' }, { blockId: '', x: RIGHT, y: top + 40, width: 288, kind: 'para' }, ]; @@ -579,8 +579,8 @@ describe('balanceSectionOnPage', () => { }); expect(result).not.toBeNull(); - // Containment on the origin put it in column 0 and hoisted it above the two paragraphs that - // precede it there. + // Without the record, containment on the origin puts it in column 0 and hoists it above the two + // paragraphs that precede it there. expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4, 5]); expect(new Set(fragments.map((f) => f.x)).size).toBe(2); }); @@ -689,30 +689,49 @@ describe('balanceSectionOnPage', () => { ]); }); - it('reads a table as wide as the whole content area from its leading edge', () => { - // A table at column 0's left edge that spans the entire content area ENDS exactly on column - // 1's trailing edge. The trailing-edge rule below would therefore claim it for column 1, which - // is why the leading edge has to be asked first: content overflows rightward from its own - // column's start, so a leading-edge match settles ownership on its own. + it('reads a table as wide as the whole content area from its own column', () => { + // A table at column 0's left edge spanning the entire content area. Its origin IS column 0's + // origin, so containment answers it without needing to reason about the overhang at all. expect(orderWith(1, { blockId: '', x: LEFT, y: 0, width: 420, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); }); - it('reads a right-aligned over-wide table from its trailing edge', () => { - // resolveTableFrame places an over-wide table that justifies to `end` at - // `col.x + (col.width - width)`: 316 + (200 - 600) = -84, so it starts left of the page's - // content area entirely. Overlap cannot settle this one — the table covers 200px of column 0 - // and 200px of column 1, an exact tie that resolves to the EARLIER column and pulls the table - // ahead of the paragraphs it follows. Its trailing edge still lands on column 1's, and `end` - // is the default justification for any w:bidiVisual table, so this is the common shape. - expect(orderWith(2, { blockId: '', x: -84, y: 0, width: 600, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); + it('resolves an over-wide table from its record, because its box cannot', () => { + // These two boxes are THE SAME box. resolveTableFrame places a 600px `end`-justified table in + // column 1 at 316 + (200 - 600) = -84, spanning content-relative -180..420. A 400px table + // centred in column 1 sits at 216, spanning 120..420. Both end on column 1's trailing edge; + // both begin inside column 0; and for the pair below they even overlap the two columns + // identically. Nothing about either box distinguishes it from a box of the same shape whose + // owner is column 0 — see the content-area-wide case further down, which really does belong to + // column 0 while ending on a later column's trailing edge. Only the record settles it. + expect( + orderWith(2, { blockId: '', x: -84, y: 0, width: 600, kind: 'table', height: 20, columnIndex: 1 }), + ).toEqual([0, 1, 2, 3]); + expect( + orderWith(2, { blockId: '', x: 216, y: 0, width: 400, kind: 'table', height: 20, columnIndex: 1 }), + ).toEqual([0, 1, 2, 3]); + }); + + it('reads an outdented paragraph from the column it covers, not the one its origin fell into', () => { + // A negative `w:ind w:left` moves the origin OUT of its column and widens the fragment by the + // same amount. An outdent larger than the gutter therefore lands the origin inside the + // PREVIOUS column: a 50px outdent in column 1 gives x 266 (316 - 50) with width 250 + // (200 + 50), and 266 is inside column 0's span of 96..296. Containment alone names column 0 + // and pulls the paragraph ahead of column 0's own content. + // + // This is what the width gate is for. 250 does not fit column 0's 200, so the origin is not + // evidence of ownership, and the overlap vote decides: 30px of column 0 against the whole + // 200px of column 1. + expect(orderWith(2, { blockId: '', x: 266, y: 0, width: 250, kind: 'para' })).toEqual([0, 1, 2, 3]); }); - it('reads a centred over-wide table from the column it covers, not the one it starts in', () => { - // Centred, an over-wide table lands on NEITHER column edge: 316 + (200 - 400) / 2 = 216, ending - // at 616. Its origin sits inside column 0, so containment names column 0 — and that is why - // containment only counts when the box FITS the column it starts in. It does not here, so - // overlap decides, and the table covers 200px of column 1 against 80px of column 0. - expect(orderWith(2, { blockId: '', x: 216, y: 0, width: 400, kind: 'table', height: 20 })).toEqual([0, 1, 2, 3]); + it('keeps a right-aligned float paragraph in the column its origin sits in', () => { + // `layout-paragraph.ts` re-points a `floatAlignment: 'right'` fragment at + // `columnX + (columnWidth - maxLineWidth)` and does NOT reduce its width, so a 50px line in a + // 200px column yields x 246 with width still 200 — an origin inside its own column and a right + // edge 150px past it. This is why the origin must NOT be gated on the box fitting its column: + // gating it sent this to an overlap vote, which sees 50px in column 0 against 130px in column + // 1 and moves the paragraph to the end of the page. + expect(orderWith(1, { blockId: '', x: LEFT + 150, y: 0, width: 200, kind: 'para' })).toEqual([0, 1, 2, 3]); }); }); @@ -725,13 +744,12 @@ describe('balanceSectionOnPage', () => { const RIGHT = 432; const placements: TestFragment[] = [ { blockId: '', x: LEFT, y: top, width: 288, kind: 'para' }, - // Indented 36px inside column 0 and 88px short of its trailing edge, so it lands on NEITHER - // column edge and the origin is all there is to go on. Its own column is still the only one - // it overlaps. - { blockId: '', x: LEFT + 36, y: top + 20, width: 200, kind: 'para' }, + // A real `w:ind w:left` of 36px: the origin moves in by 36 and the width comes DOWN by 36, so + // the fragment lands on neither column edge and the origin is all there is to go on. + { blockId: '', x: LEFT + 36, y: top + 20, width: 252, kind: 'para' }, { blockId: '', x: LEFT, y: top + 40, width: 288, kind: 'para' }, // The same indent in column 1. - { blockId: '', x: RIGHT + 36, y: top, width: 200, kind: 'para' }, + { blockId: '', x: RIGHT + 36, y: top, width: 252, kind: 'para' }, { blockId: '', x: RIGHT, y: top + 20, width: 288, kind: 'para' }, ]; const fragments: TestFragment[] = []; @@ -761,6 +779,60 @@ describe('balanceSectionOnPage', () => { expect(readingOrder(fragments, LEFT)).toEqual([0, 1, 2, 3, 4]); }); + it('keeps a content-area-wide centred box in the column it was centred in', () => { + // The counter-example that makes a trailing-edge rule unusable. Three 192px columns over a 624px + // content area with 24px gutters sit at 0 / 216 / 432. A box the full width of the content area, + // centred in column 0, is placed at 0 + (192 - 624) / 2 = -216 and so spans -216..408 — and 408 + // is EXACTLY column 1's trailing edge (216 + 192). Reading the trailing edge therefore hands a + // column-0 box to column 1. It is not a coincidence of these numbers: 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. + // + // The origin is outside every column here, so the overlap vote decides, and the box covers + // column 0 and column 1 equally — 192px each — which the vote breaks toward the earlier column. + const top = 96; + const COLS = [96, 312, 528]; + const placements: TestFragment[] = [ + { blockId: '', x: COLS[0], y: top, width: 192, kind: 'para' }, + { blockId: '', x: -120, y: top + 20, width: 624, kind: 'table', height: 20 }, + { blockId: '', x: COLS[1], y: top, width: 192, kind: 'para' }, + { blockId: '', x: COLS[1], y: top + 20, width: 192, kind: 'para' }, + { blockId: '', x: COLS[2], y: top, width: 192, kind: 'para' }, + { blockId: '', x: COLS[2], y: top + 20, width: 192, kind: 'para' }, + ]; + const fragments: TestFragment[] = []; + const measureMap = new Map }>(); + const blockSectionMap = new Map(); + placements.forEach((placement, i) => { + const id = `s2-b${i}`; + fragments.push({ ...placement, blockId: id }); + measureMap.set(id, createMeasure('paragraph', [20])); + blockSectionMap.set(id, 2); + }); + + const result = balanceSectionOnPage({ + fragments, + sectionIndex: 2, + sectionColumns: { count: 3, gap: 24, width: 192, contentWidth: 624 }, + sectionHasExplicitColumnBreak: false, + blockSectionMap, + margins: { left: 96 }, + topMargin: top, + columnWidth: 192, + availableHeight: 40, + measureMap, + }); + + expect(result).not.toBeNull(); + // Three columns, so read the page by ascending x then y rather than through the two-column + // helper above. + const reading = fragments + .map((fragment, index) => ({ index, x: fragment.x, y: fragment.y })) + .sort((a, b) => a.x - b.x || a.y - b.y) + .map((entry) => entry.index); + expect(reading).toEqual([0, 1, 2, 3, 4, 5]); + }); + it('recovers document order from a shuffled fragment array', () => { // The paginator's array order is not a contract, so the ordinal/y ordering has to stand on its // own rather than leaning on the stability of an already-sorted input. diff --git a/packages/layout-engine/layout-engine/src/column-balancing.ts b/packages/layout-engine/layout-engine/src/column-balancing.ts index 50202b9d81..3dbb898653 100644 --- a/packages/layout-engine/layout-engine/src/column-balancing.ts +++ b/packages/layout-engine/layout-engine/src/column-balancing.ts @@ -822,66 +822,71 @@ export function balanceSectionOnPage(args: BalanceSectionOnPageArgs): { maxY: nu }); const originX = args.margins.left; const clampOrdinal = (value: number): number => Math.max(0, Math.min(columnCount - 1, Math.floor(value))); - // Sub-pixel tolerance for "this edge IS that column's edge". Column x values reach fragments - // through `getColumnX`, so unindented content matches exactly and the slack only absorbs float - // drift — it stays two orders of magnitude below the smallest indent a document can author. - const EDGE_EPSILON = 0.01; + // Tolerance for "this box is no wider than that column". Widths reach fragments as the column + // width itself, so the slack only absorbs float drift (measured worst case ~5e-13px across + // awkward page/margin/count combinations). One twip is 96/1440 = 0.067px, so this stays well + // under the smallest width difference a document can express. + const WIDTH_EPSILON = 0.01; /** * Which column a fragment occupies, as a fill-order ordinal. ALWAYS a number: the result is a sort * key, and a key that is sometimes absent would leave the comparator mixing two different metrics, * which is not a total order — `Array.prototype.sort` is then free to return different orders for * the same input, and it does differ between engines. + * + * Geometry alone CANNOT settle this for a box wider than its column, and it is worth being exact + * about why, because every plausible repair trades one wrong answer for another. `resolveTableFrame` + * places an over-wide table that justifies to `end` at `col.x + (col.width - width)`, so the box + * runs from an earlier column's left edge to 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, runs between exactly those same two edges — and the owner is the EARLIER column. + * Same span, same per-column overlaps, opposite owners. The origin, the trailing edge and the + * overlap vote are each provably unable to tell those two apart, so the only thing that can is + * what the engine recorded: `columnIndex` is asked first, and is now written for anchored tables + * as well as in-flow ones. + * + * For everything else the question is only whether the origin can be trusted, and the test that + * answers it is the box's WIDTH, not its edges. A box no wider than the column its origin sits in + * was placed inside that column, wherever in it the origin ended up — ordinary content, a + * `w:ind` indent, and a `floatAlignment` right/centre paragraph, which `layout-paragraph.ts` + * re-points at `columnX + (columnWidth - maxLineWidth)` while leaving its width at the full column + * width. A box WIDER than that column may instead have been pulled left out of its own column: a + * negative `w:ind` widens the fragment by the outdent, so an outdent bigger than the gutter lands + * the origin inside the PREVIOUS column and containment names that one. + * + * Gating on the box's right EDGE staying inside the column instead of on its width was tried and + * was worse: it rejects the right-aligned float too, and the overlap vote then hands a short + * right-aligned line to whichever neighbour its overhang covers more of, reordering the page. */ const ordinalOf = (fragment: BalancingFragment): number => { // The engine's own record, where it kept one. Only a few fragment kinds do: tables - // (`layout-table.ts`, five sites), footnote bodies, and a paragraph ONLY when it is a collapsed - // split-line-break anchor carrier (`layout-paragraph.ts`, under `collapseSplitLineBreakCarrier`) - // — a narrow document shape, not the ordinary paragraph. The rules below therefore carry almost - // every fragment. + // (`layout-table.ts`, five sites, plus `createAnchoredTableFragment` now), footnote bodies, and a + // paragraph ONLY when it is a collapsed split-line-break anchor carrier (`layout-paragraph.ts`, + // under `collapseSplitLineBreakCarrier`) — a narrow document shape, not the ordinary paragraph. + // The rule below therefore still carries almost every fragment. const recorded = (fragment as { columnIndex?: number }).columnIndex; if (typeof recorded === 'number' && Number.isFinite(recorded)) return clampOrdinal(recorded); const span = Number.isFinite(fragment.width) && fragment.width > 0 ? fragment.width : 0; const left = fragment.x - originX; - const right = left + span; - - // A box on a column's LEADING edge is that column's, fit or no fit. That covers ordinary - // content, and it covers content WIDER than its column, which overflows rightward from that - // same edge in either direction. It has to be asked before any overlap test, because an - // over-wide box can cover more of a wide neighbour than of the narrow column it came from. - for (const col of preBalanceGeometry) { - if (Math.abs(left - col.x) <= EDGE_EPSILON) return col.index; - } - - // A box on a column's TRAILING edge is that column's too, and it is a different question, not a - // mirror of the one above. `resolveTableFrame` places an over-wide table that justifies to `end` - // at `col.x + (col.width - width)` — a NEGATIVE offset from its own column — 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, and once it is - // wide enough its overlap outvotes its own column too. An anchored table reaches this at all - // because `createAnchoredTableFragment` records no `columnIndex` for the step above to read. - for (const col of preBalanceGeometry) { - if (Math.abs(right - (col.x + col.width)) <= EDGE_EPSILON) return col.index; - } - // Neither edge lands on a column edge, so an indent moved the origin. Containment is right while - // the box still FITS the column it starts in, which is what an indent leaves behind. Demanding - // the fit is what stops this from claiming a SHIFTED origin: a centred over-wide table also - // begins inside an earlier column, and there the origin is no evidence of ownership at all. + // The column the origin sits in, so long as the box is narrow enough to have been placed there. const byOrigin = findColumnContaining(preBalanceGeometry, fragment.x, originX); if (byOrigin !== null) { const originColumn = preBalanceGeometry.find((col) => col.index === byOrigin); - if (originColumn && right <= originColumn.x + originColumn.width + EDGE_EPSILON) return byOrigin; + if (originColumn && span <= originColumn.width + WIDTH_EPSILON) return byOrigin; } - // What is left is a box whose origin cannot be trusted: centred over-wide content, or an origin - // hung into a gutter by a negative `w:ind` or a float offset. It still lies mostly in the column - // that owns it. Ties go to the earliest column in fill order. + // Either the origin sits in no column at all — an outdent or float offset smaller than the + // gutter hung it in there — or the box is too wide for the column the origin landed in, so that + // origin is not evidence of ownership. It still lies mostly in the column that owns it. Ties go + // to the earliest column in fill order, but only approximately: two mathematically equal + // overlaps derived from a content width that does not divide evenly can differ in their last + // bits and settle either way. let best: number | null = null; let bestOverlap = 0; for (const col of preBalanceGeometry) { - const overlap = Math.min(right, col.x + col.width) - Math.max(left, col.x); + const overlap = Math.min(left + span, col.x + col.width) - Math.max(left, col.x); if (overlap > bestOverlap) { bestOverlap = overlap; best = col.index; From d7c496457a0baf7304a4f420e9213d23763a2939 Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Wed, 2 Sep 2026 18:52:23 +0300 Subject: [PATCH 20/20] fix(contracts): fall a hole in the per-column gaps back to the scalar MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../contracts/src/column-layout.test.ts | 24 +++++++++++++++++++ .../contracts/src/column-layout.ts | 18 ++++++++++++-- 2 files changed, 40 insertions(+), 2 deletions(-) diff --git a/packages/layout-engine/contracts/src/column-layout.test.ts b/packages/layout-engine/contracts/src/column-layout.test.ts index a55d2f5ec9..0c485c483d 100644 --- a/packages/layout-engine/contracts/src/column-layout.test.ts +++ b/packages/layout-engine/contracts/src/column-layout.test.ts @@ -461,6 +461,30 @@ describe('columnRenderLayoutsEqual (SD-2629)', () => { expect(columnRenderLayoutsEqual({ count: 3, gap: 24 }, { count: 3, gap: 40 })).toBe(false); }); + it('falls a hole in the gaps array back to the scalar, not to NaN', () => { + // `Math.max(0, undefined)` is NaN, and NaN is not nullish, so `buildColumnGeometry`'s + // `gaps?.[i] ?? gap` could not rescue it: the hole propagated into every later column's x. + // A hole is forbidden by `gaps?: number[]` and nothing in the repo builds a gaps array yet, so + // this is a guard for the day the importer starts projecting `w:space` per column. + const holed = normalizeColumnLayout( + { count: 3, gap: 40, widths: [200, 200, 200], equalWidth: false, gaps: [30, undefined] } as ColumnLayout, + 720, + ); + expect(holed.gaps).toEqual([30, 40]); + const geometry = getColumnGeometry(holed); + expect(geometry.map((col) => col.x)).toEqual([0, 230, 470]); + expect(geometry.every((col) => Number.isFinite(col.x))).toBe(true); + + // Which also means the hole now renders identically to spelling the scalar out, so the render + // predicate must call the two equal rather than splitting a region over an unreachable value. + expect( + columnRenderLayoutsEqual( + { count: 3, gap: 40, widths: [200, 200, 200], equalWidth: false, gaps: [30, undefined] } as ColumnLayout, + { count: 3, gap: 40, widths: [200, 200, 200], equalWidth: false, gaps: [30, 40] }, + ), + ).toBe(true); + }); + it('still splits explicit sub-pixel widths on the scalar gap alone', () => { // The one route by which the scalar reaches explicit WIDTHS rather than the gutters: normalize // floors a fabricated width at 1px, and collapses to a single full-width column once the usable diff --git a/packages/layout-engine/contracts/src/column-layout.ts b/packages/layout-engine/contracts/src/column-layout.ts index 11720f2b14..712e448c2f 100644 --- a/packages/layout-engine/contracts/src/column-layout.ts +++ b/packages/layout-engine/contracts/src/column-layout.ts @@ -219,9 +219,19 @@ export function normalizeColumnLayout( // that monotonicity to tell a mirrored strip from an upright one, so a negative gap wide enough to // pull a column back behind its predecessor would make an LTR layout answer hit tests as if it // were RTL. + // A HOLE in the array falls back to the scalar, exactly as `buildColumnGeometry`'s `gaps?.[i] ?? + // gap` and `effectiveColumnGaps` already do for a SHORT one. `Math.max(0, undefined)` is NaN, and + // NaN is not nullish, so the geometry fallback below could not catch it: `{gap: 40, gaps: [30, + // undefined]}` normalized to `gaps: [30, NaN]` and painted `col2.x = NaN`, i.e. a column with no + // position at all. `gaps?: number[]` forbids a hole under TypeScript and nothing in this repo + // constructs a `gaps` array yet, so this is unreachable today — and it stops being unreachable the + // day the importer starts projecting `w:cols/w:col/@w:space` per column. A non-finite entry takes + // the same fallback, since the reason to distrust it is identical. const gaps = explicitWidths.length > 0 && Array.isArray(input?.gaps) - ? input.gaps.slice(0, Math.max(0, count - 1)).map((value) => Math.max(0, value)) + ? input.gaps + .slice(0, Math.max(0, count - 1)) + .map((value) => (Number.isFinite(value) ? Math.max(0, value) : Math.max(0, gap))) : undefined; const width = widths.reduce((max, value) => Math.max(max, value), 0); @@ -461,7 +471,11 @@ export function columnRenderLayoutsEqual(a?: ColumnLayout, b?: ColumnLayout): bo // scalar still reaches explicit WIDTHS — normalize's sub-pixel `Math.max(1, …)` floor and its // epsilon collapse, both keyed on the sign of `contentWidth - gap * (count - 1)` — is handled by // the `hasSubPixelWidth` guard in the explicit branch above, not waved off here: a width of 1px - // or more makes both no-ops, which is exactly the threshold that guard tests. + // or more makes both no-ops, which is exactly the threshold that guard tests. That guard is + // COMPLETE rather than merely in scope — the epsilon collapse needs the MAXIMUM authored width + // at or under the epsilon, and both epsilons live in the tree are below 1px (1e-4 in + // `layout-engine/src/index.ts`, 1e-2 in `layout-bridge/src/incrementalLayout.ts`), so that route + // also implies a sub-pixel width. There is no path the guard misses. return false; } return true;