From 95cf92dee80c9281da68728674905462211cd51e Mon Sep 17 00:00:00 2001 From: Nathaniel-260 Date: Tue, 1 Sep 2026 08:59:03 +0300 Subject: [PATCH 01/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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/11] 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);