From ea1e7f6a4fbb6310ba75b47ee298bc02a3ed15cc Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 2 Jun 2026 21:07:05 -0300 Subject: [PATCH 1/6] feat(super-converter): resolve style-based cell shading (pct/auto) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rewrite resolveShadingFillColor to the full CT_Shd model (ECMA-376 §17.3.5): nil/none yields no background; a pattern (pctNN/solid) blends the foreground over the fill; the auto sentinel resolves to white (fill) and black (foreground). parseTableCell resolves style-based shading through it, so a table style's pct10/auto band renders as the gray Word paints instead of being dropped. (SD-2969) --- .../core/layout-adapter/converters/table.ts | 26 +++++----- .../v1/core/super-converter/helpers.d.ts | 3 ++ .../v1/core/super-converter/helpers.js | 49 ++++++++++++++++--- .../v1/core/super-converter/helpers.test.js | 41 ++++++++++++++++ 4 files changed, 100 insertions(+), 19 deletions(-) diff --git a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts index f084386e84..0e4cb49c0a 100644 --- a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts +++ b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts @@ -45,6 +45,7 @@ import { } from '../attributes/index.js'; import { pickNumber, twipsToPx } from '../utilities.js'; import { hydrateTableStyleAttrs } from './table-styles.js'; +import { resolveShadingFillColor } from '@converter/helpers.js'; import { collectTrackedChangeFromMarks } from '../marks/index.js'; import { annotateBlockWithTrackedChange, shouldHideTrackedNode } from '../tracked-changes.js'; import { @@ -308,18 +309,21 @@ const parseTableCell = (args: ParseTableCellArgs): TableCell | null => { } } } - // Fall back to resolved style shading if no inline background + // Fall back to resolved style shading if no inline background. Resolve the fill + // base (explicit hex → theme fill), then apply the OOXML shading pattern + // (pctNN/solid) via the shared resolver so a table style's `pct10`/`auto` shading + // (e.g. a tblStylePr firstRow band) becomes the gray Word paints, instead of being + // dropped. (SD-2969) if (!cellBackgroundColor && resolvedTcProps?.shading) { - const { fill, themeFill, themeFillTint, themeFillShade } = resolvedTcProps.shading; - const normalizedFill = normalizeShadingColor(fill); - if (normalizedFill) { - cellBackgroundColor = normalizedFill; - } else if (themeFill && context.themeColors) { - const resolved = resolveThemeColorValue(themeFill, themeFillTint, themeFillShade, context.themeColors); - const normalizedTheme = normalizeShadingColor(resolved); - if (normalizedTheme) { - cellBackgroundColor = normalizedTheme; - } + const { fill, color, val, themeFill, themeFillTint, themeFillShade } = resolvedTcProps.shading; + let fillBase = normalizeShadingColor(fill); + if (!fillBase && themeFill && context.themeColors) { + const resolvedTheme = resolveThemeColorValue(themeFill, themeFillTint, themeFillShade, context.themeColors); + fillBase = normalizeShadingColor(resolvedTheme); + } + const resolved = resolveShadingFillColor({ val, color, fill: fillBase ?? fill }); + if (resolved) { + cellBackgroundColor = resolved.startsWith('#') ? resolved : `#${resolved}`; } } diff --git a/packages/super-editor/src/editors/v1/core/super-converter/helpers.d.ts b/packages/super-editor/src/editors/v1/core/super-converter/helpers.d.ts index f72c7f1804..5b99926369 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/helpers.d.ts +++ b/packages/super-editor/src/editors/v1/core/super-converter/helpers.d.ts @@ -41,6 +41,9 @@ export function getDocxHighlightKeywordFromHex(hexColor: any): any; export function normalizeHexColor(hex: any): any; export function isValidHexColor(color: any): boolean; export function rgbToHex(rgb: any): string; +export function resolveShadingFillColor( + shading: { val?: string; color?: string; fill?: string } | null | undefined, +): string | null; export function ptToTwips(pt: any): number; export function twipsToPt(twips: any): number; export function getLineHeightValueString( diff --git a/packages/super-editor/src/editors/v1/core/super-converter/helpers.js b/packages/super-editor/src/editors/v1/core/super-converter/helpers.js index a18381b3d8..641085bc2c 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/helpers.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/helpers.js @@ -519,6 +519,7 @@ const rgbToHex = (rgb) => { }; const DEFAULT_SHADING_FOREGROUND_COLOR = '#000000'; +const DEFAULT_SHADING_FILL_COLOR = '#FFFFFF'; const hexToRgb = (hex) => { const normalized = normalizeHexColor(hex); @@ -549,21 +550,53 @@ const blendHexColors = (backgroundHex, foregroundHex, foregroundRatio) => { return `${toByte(r)}${toByte(g)}${toByte(b)}`; }; +/** + * Resolve an OOXML shading element (CT_Shd, ECMA-376 §17.3.5) to a concrete CSS + * hex fill WITHOUT a leading '#', or null when the shading produces no background. + * + * The shading model paints a pattern (`w:val`) of the foreground color (`w:color`) + * over the fill background (`w:fill`). The `auto` sentinel resolves to white for a + * fill and black for a foreground, so `pct10` + `color="auto"` + `fill="auto"` is + * 10% black over white ≈ light gray (#E6E6E6) — which is what Word renders. + * + * Theme colors (themeFill/themeColor) are resolved by callers that own the theme + * palette and passed back in as explicit hex; this helper handles explicit hex and + * the `auto` sentinel only. + * + * @param {{ val?: string, color?: string, fill?: string } | null | undefined} shading + * @returns {string | null} Hex without '#', or null for "no background". + */ const resolveShadingFillColor = (shading) => { if (!shading || typeof shading !== 'object') return null; - const fill = normalizeHexColor(shading.fill); - if (!fill) return null; - const val = typeof shading.val === 'string' ? shading.val.trim().toLowerCase() : ''; + if (val === 'nil' || val === 'none') return null; + + // The `auto` fill sentinel means "automatic background" (white). A missing or + // non-hex fill yields no explicit color. + const fillIsAuto = typeof shading.fill === 'string' && shading.fill.trim().toLowerCase() === 'auto'; + const fillHex = fillIsAuto ? null : normalizeHexColor(shading.fill); + const pctMatch = val.match(/^pct(\d{1,3})$/); - if (!pctMatch) return fill; + const isPattern = Boolean(pctMatch) || val === 'solid'; + + // No pattern (clear / unspecified): the fill IS the background. An automatic or + // missing fill therefore means "no background". + if (!isPattern) return fillHex; + + // Pattern (pctNN or solid): blend the foreground over the fill base. An automatic + // fill base resolves to white; an automatic or absent foreground resolves to black. + const baseHex = fillHex ?? DEFAULT_SHADING_FILL_COLOR; + const colorIsAuto = + typeof shading.color !== 'string' || shading.color.trim() === '' || shading.color.trim().toLowerCase() === 'auto'; + const foregroundHex = colorIsAuto + ? DEFAULT_SHADING_FOREGROUND_COLOR + : (normalizeHexColor(shading.color) ?? DEFAULT_SHADING_FOREGROUND_COLOR); - const pct = Number.parseInt(pctMatch[1], 10); - if (!Number.isFinite(pct) || pct < 0 || pct > 100) return fill; + const pct = pctMatch ? Number.parseInt(pctMatch[1], 10) : 100; // solid = 100% foreground + if (!Number.isFinite(pct) || pct < 0 || pct > 100) return fillHex; - const foreground = normalizeHexColor(shading.color) ?? DEFAULT_SHADING_FOREGROUND_COLOR; - return blendHexColors(fill, foreground, pct / 100) ?? fill; + return blendHexColors(baseHex, foregroundHex, pct / 100) ?? fillHex; }; const getLineHeightValueString = (lineHeight, defaultUnit, lineRule = '', isObject = false) => { diff --git a/packages/super-editor/src/editors/v1/core/super-converter/helpers.test.js b/packages/super-editor/src/editors/v1/core/super-converter/helpers.test.js index e86cdab2e5..6ea1d23b5a 100644 --- a/packages/super-editor/src/editors/v1/core/super-converter/helpers.test.js +++ b/packages/super-editor/src/editors/v1/core/super-converter/helpers.test.js @@ -10,6 +10,7 @@ import { dataUriToArrayBuffer, detectImageType, eighthPointsToPixels, + resolveShadingFillColor, } from './helpers.js'; import { getFallbackImageNameFromDataUri } from './helpers/mediaHelpers.js'; @@ -601,3 +602,43 @@ describe('eighthPointsToPixels', () => { }); }); }); + +describe('resolveShadingFillColor', () => { + it('returns null when there is no shading', () => { + expect(resolveShadingFillColor(null)).toBeNull(); + expect(resolveShadingFillColor(undefined)).toBeNull(); + }); + + it('returns the explicit fill for a clear pattern (no blend)', () => { + expect(resolveShadingFillColor({ val: 'clear', color: 'auto', fill: 'DCE6F1' })).toBe('DCE6F1'); + }); + + it('treats an automatic fill as no background when there is no pattern', () => { + expect(resolveShadingFillColor({ val: 'clear', color: 'auto', fill: 'auto' })).toBeNull(); + expect(resolveShadingFillColor({ fill: 'auto' })).toBeNull(); + }); + + it('returns null for an explicit no-shading pattern (nil/none)', () => { + expect(resolveShadingFillColor({ val: 'nil', fill: 'DCE6F1' })).toBeNull(); + expect(resolveShadingFillColor({ val: 'none', fill: 'DCE6F1' })).toBeNull(); + }); + + // SD-2969: a pattern over the automatic sentinel must resolve to gray (Word renders it gray), not garbage/white. + it('resolves pct10 over auto fill and color to light gray', () => { + // 10% black painted over automatic (white) ≈ #E6E6E6 + expect(resolveShadingFillColor({ val: 'pct10', color: 'auto', fill: 'auto' })).toBe('E6E6E6'); + }); + + it('resolves pct35 over explicit black-on-white to mid gray', () => { + expect(resolveShadingFillColor({ val: 'pct35', color: '000000', fill: 'FFFFFF' })).toBe('A6A6A6'); + }); + + it('blends a percentage pattern over an explicit fill and color', () => { + // 50% black over white = #808080 + expect(resolveShadingFillColor({ val: 'pct50', color: '000000', fill: 'FFFFFF' })).toBe('808080'); + }); + + it('treats a solid pattern as 100% foreground color', () => { + expect(resolveShadingFillColor({ val: 'solid', color: 'FF0000', fill: 'FFFFFF' })).toBe('FF0000'); + }); +}); From d1a44577e565f33602cd7bb7b5dac0e53d86a536 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 2 Jun 2026 21:07:41 -0300 Subject: [PATCH 2/6] fix(layout-engine): keepNext heading reserves table first row In a keepNext chain anchored to a table, reserve only the first row's height (not the full table) so the table starts on the page and splits, matching Word. Mirrors the paragraph anchor's first-line reservation; previously a heading plus a tall splittable table advanced together and left a large gap. (SD-3345) --- .../layout-engine/src/index.test.ts | 62 +++++++++++++++++++ .../layout-engine/layout-engine/src/index.ts | 20 +++++- 2 files changed, 79 insertions(+), 3 deletions(-) diff --git a/packages/layout-engine/layout-engine/src/index.test.ts b/packages/layout-engine/layout-engine/src/index.test.ts index 14c53262a4..2ae1da636a 100644 --- a/packages/layout-engine/layout-engine/src/index.test.ts +++ b/packages/layout-engine/layout-engine/src/index.test.ts @@ -5825,6 +5825,68 @@ describe('requirePageBoundary edge cases', () => { expect(pageContainsBlock(layout.pages[0], 'body')).toBe(true); }); + it('uses first ROW height (not full table) for a splittable table anchor so the chain starts and the table splits (SD-3345)', () => { + // A heading (keepNext) immediately followed by a tall splittable table. The + // heading + table FIRST ROW fits in the remaining space, but heading + the WHOLE + // table does not. Word starts the table here and splits it; reserving the full + // table height would push the heading + table wholly to the next page (large gap). + const filler: FlowBlock = { + kind: 'paragraph', + id: 'filler', + runs: [{ text: 'filler', fontFamily: 'Arial', fontSize: 12 }], + attrs: {}, + }; + const heading: FlowBlock = { + kind: 'paragraph', + id: 'heading', + runs: [{ text: 'Heading', fontFamily: 'Arial', fontSize: 24 }], + attrs: { keepNext: true }, + }; + const table = { + kind: 'table', + id: 'tbl', + rows: Array.from({ length: 4 }, (_unused, i) => ({ + id: `r${i}`, + cells: [ + { + id: `c${i}`, + blocks: [{ kind: 'paragraph', id: `p${i}`, runs: [{ text: 'x', fontFamily: 'Arial', fontSize: 10 }] }], + }, + ], + })), + } as unknown as TableBlock; + + const fillerMeasure: ParagraphMeasure = { kind: 'paragraph', lines: [makeLine(50)], totalHeight: 50 }; + const headingMeasure: ParagraphMeasure = { kind: 'paragraph', lines: [makeLine(20)], totalHeight: 20 }; + // 4 rows × 15px = 60px total; first row 15px. Cells carry a single measured line + // so the table-start preflight can render at least one row on the current page. + const tableMeasure = { + kind: 'table', + rows: Array.from({ length: 4 }, () => ({ + cells: [{ paragraph: { kind: 'paragraph', lines: [makeLine(15)], totalHeight: 15 }, width: 100, height: 15 }], + height: 15, + })), + columnWidths: [100], + totalWidth: 100, + totalHeight: 60, + } as unknown as TableMeasure; + + // Content area = 100px. After filler(50), 50px remain. + // - heading(20) + firstRow(15) = 35 <= 50 → chain fits → start on this page. + // - heading(20) + fullTable(60) = 80 > 50 → would advance without the fix. + // (80 <= 100 content height, so the blank-page guard does not suppress the advance.) + const options: LayoutOptions = { + pageSize: { w: 400, h: 160 }, + margins: { top: 30, right: 30, bottom: 30, left: 30 }, + }; + + const layout = layoutDocument([filler, heading, table], [fillerMeasure, headingMeasure, tableMeasure], options); + + // Heading and the table both start on page 0 (the table then splits across pages). + expect(pageContainsBlock(layout.pages[0], 'heading')).toBe(true); + expect(pageContainsBlock(layout.pages[0], 'tbl')).toBe(true); + }); + it('reclaims trailing spacing when both filler and chain starter have contextualSpacing', () => { // Both filler and chain starter have contextualSpacing + same style. // The trailing spacing should be reclaimed, making room for the chain. diff --git a/packages/layout-engine/layout-engine/src/index.ts b/packages/layout-engine/layout-engine/src/index.ts index 00cb67ea06..488f66a327 100644 --- a/packages/layout-engine/layout-engine/src/index.ts +++ b/packages/layout-engine/layout-engine/src/index.ts @@ -438,13 +438,27 @@ function calculateChainHeight( totalHeight += interParagraphSpacing + anchorHeight; } else { - // Non-paragraph anchor (table, image, etc.): use full height - // No contextual spacing applies to non-paragraph blocks + // Non-paragraph anchor (table, image, etc.). + // No contextual spacing applies to non-paragraph blocks. // Skip anchored tables - they're positioned out of flow and don't consume flow height // (consistent with shouldSkipAnchoredTable guard in legacy keepNext path) const isAnchoredTable = anchorBlock.kind === 'table' && (anchorBlock as TableBlock).anchor?.isAnchored === true; if (!isAnchoredTable) { - totalHeight += prevSpacingAfter + getMeasureHeight(anchorBlock, anchorMeasure); + // For a table anchor, only require the FIRST ROW to stay with the chain, not + // the full table. The keepNext contract keeps the heading with the table's + // start; the table itself splits across pages (SD-3345). Reserving the full + // height pushed a heading + tall splittable table wholly to the next page, + // leaving a large gap, where Word starts the table here and splits it. This + // mirrors the paragraph anchor's first-line optimization (SD-1282). A table + // whose first row cannot split is still handled by the table-start preflight. + let anchorHeight = getMeasureHeight(anchorBlock, anchorMeasure); + if (anchorBlock.kind === 'table' && anchorMeasure.kind === 'table' && anchorMeasure.rows.length > 0) { + const firstRowHeight = anchorMeasure.rows[0]?.height; + if (typeof firstRowHeight === 'number' && Number.isFinite(firstRowHeight) && firstRowHeight > 0) { + anchorHeight = firstRowHeight; + } + } + totalHeight += prevSpacingAfter + anchorHeight; } } } From a8e71ee8427da73e1983a76d229672ade027b44f Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 2 Jun 2026 21:07:44 -0300 Subject: [PATCH 3/6] =?UTF-8?q?feat(painter):=20add=20ECMA-376=20=C2=A717.?= =?UTF-8?q?4.66=20cell-border=20conflict=20resolution?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add resolveBorderConflict and isPresentBorder to collapse two cells' shared-edge borders to the single displayed border: if either side is nil/none the opposing border wins; otherwise greater weight (lines x style number) wins, with ties broken by style precedence, then color brightness, then reading order. Pure helper with unit tests; wired into the table painter in the following commit. --- .../dom/src/table/border-utils.test.ts | 40 +++++++++ .../painters/dom/src/table/border-utils.ts | 83 +++++++++++++++++++ 2 files changed, 123 insertions(+) diff --git a/packages/layout-engine/painters/dom/src/table/border-utils.test.ts b/packages/layout-engine/painters/dom/src/table/border-utils.test.ts index 7d11407077..ebadd10b2d 100644 --- a/packages/layout-engine/painters/dom/src/table/border-utils.test.ts +++ b/packages/layout-engine/painters/dom/src/table/border-utils.test.ts @@ -22,6 +22,7 @@ import { hasExplicitCellBorders, swapTableBordersLR, swapCellBordersLR, + resolveBorderConflict, } from './border-utils.js'; describe('applyBorder', () => { @@ -522,3 +523,42 @@ describe('swapCellBordersLR', () => { expect(swapCellBordersLR(undefined)).toBeUndefined(); }); }); + +describe('resolveBorderConflict (ECMA-376 §17.4.66)', () => { + const D9 = { style: 'single' as const, width: 1.333, color: '#BDD7EE' }; + + it('collapses two identical borders to one (symmetric → no doubling)', () => { + // The M&A checklist case: adjacent cells both specify the same border. + const winner = resolveBorderConflict(D9, { ...D9 }); + expect(winner).toMatchObject({ style: 'single', color: '#BDD7EE' }); + }); + + it('keeps the present border when the opposing side is none (asymmetric → no dropped border)', () => { + // The it1007 case: header has a bottom border, the body cell below has no top. + const headerBottom = { style: 'single' as const, width: 1, color: '#000000' }; + expect(resolveBorderConflict(undefined, headerBottom)).toEqual(headerBottom); + expect(resolveBorderConflict({ style: 'none' }, headerBottom)).toEqual(headerBottom); + expect(resolveBorderConflict(headerBottom, undefined)).toEqual(headerBottom); + }); + + it('returns undefined when neither side has a border', () => { + expect(resolveBorderConflict(undefined, undefined)).toBeUndefined(); + expect(resolveBorderConflict({ style: 'none' }, { style: 'none' })).toBeUndefined(); + expect(resolveBorderConflict({ style: 'single', width: 0, color: '#000' }, undefined)).toBeUndefined(); + }); + + it('the heavier-weight border wins (double over single)', () => { + const single = { style: 'single' as const, width: 1, color: '#000000' }; + const dbl = { style: 'double' as const, width: 1, color: '#000000' }; + // weight: single = 1×1 = 1, double = 2×3 = 6 → double wins + expect(resolveBorderConflict(single, dbl)).toEqual(dbl); + expect(resolveBorderConflict(dbl, single)).toEqual(dbl); + }); + + it('on equal weight + identical style, the darker color wins', () => { + const dark = { style: 'single' as const, width: 1, color: '#000000' }; + const light = { style: 'single' as const, width: 1, color: '#FFFFFF' }; + // brightness(R+B+2G): dark=0 < light=1020 → dark wins + expect(resolveBorderConflict(light, dark)).toEqual(dark); + }); +}); diff --git a/packages/layout-engine/painters/dom/src/table/border-utils.ts b/packages/layout-engine/painters/dom/src/table/border-utils.ts index 6dee312c35..bf4f4f808f 100644 --- a/packages/layout-engine/painters/dom/src/table/border-utils.ts +++ b/packages/layout-engine/painters/dom/src/table/border-utils.ts @@ -183,6 +183,89 @@ export const resolveTableBorderValue = ( return borderValueToSpec(fallback); }; +// Border "number" per ECMA-376 §17.4.66 (only the realistic styles; unknown → 1). +const BORDER_STYLE_NUMBER: Partial> = { + single: 1, + thick: 2, + double: 3, + dotted: 4, + dashed: 5, + dotDash: 6, + dotDotDash: 7, + triple: 8, + wave: 18, + doubleWave: 19, +}; +// Number of drawn lines per style (single=1, double=2, triple=3, …). +const BORDER_STYLE_LINES: Partial> = { + single: 1, + thick: 1, + double: 2, + dotted: 1, + dashed: 1, + dotDash: 1, + dotDotDash: 1, + triple: 3, + wave: 1, + doubleWave: 2, +}; + +export const isPresentBorder = (b?: BorderSpec): b is BorderSpec => + !!b && b.style !== undefined && b.style !== 'none' && (b.width === undefined || b.width > 0); + +const borderWeight = (b: BorderSpec): number => + (BORDER_STYLE_LINES[b.style as BorderStyle] ?? 1) * (BORDER_STYLE_NUMBER[b.style as BorderStyle] ?? 1); + +const colorBrightness = (color: string | undefined, formula: (r: number, g: number, bl: number) => number): number => { + const hex = (color ?? '#000000').replace('#', ''); + if (hex.length < 6) return 0; + const r = parseInt(hex.slice(0, 2), 16); + const g = parseInt(hex.slice(2, 4), 16); + const bl = parseInt(hex.slice(4, 6), 16); + return formula(r, g, bl); +}; + +/** + * OOXML cell-border conflict resolution (ECMA-376 §17.4.66). + * + * With zero cell spacing, two cells sharing an edge each specify a border; the spec + * collapses them to a SINGLE displayed border: + * 1. If either side is nil/none/absent, the opposing (present) border is displayed. + * 2. Otherwise the border with greater weight wins, where + * weight = (#lines in the style) × (style number). + * 3. Equal weight → the style higher on the precedence list (single first) wins. + * 4. Identical style → the color with the smaller brightness (R+B+2G, then B+2G, then + * G) wins; finally the first border (reading order) wins. + * + * @param a - One side's border (the owning cell's, e.g. the lower/right cell) + * @param b - The opposing side's border (e.g. the upper/left neighbor) + * @returns The single BorderSpec to display, or undefined if neither is present. + */ +export const resolveBorderConflict = (a?: BorderSpec, b?: BorderSpec): BorderSpec | undefined => { + const pa = isPresentBorder(a); + const pb = isPresentBorder(b); + if (!pa && !pb) return undefined; + if (!pa) return b; + if (!pb) return a; + const wa = borderWeight(a); + const wb = borderWeight(b); + if (wa !== wb) return wa > wb ? a : b; + const na = BORDER_STYLE_NUMBER[a.style as BorderStyle] ?? 99; + const nb = BORDER_STYLE_NUMBER[b.style as BorderStyle] ?? 99; + if (na !== nb) return na < nb ? a : b; + const formulas: Array<(r: number, g: number, bl: number) => number> = [ + (r, g, bl) => r + bl + 2 * g, + (_r, g, bl) => bl + 2 * g, + (_r, g) => g, + ]; + for (const f of formulas) { + const ba = colorBrightness(a.color, f); + const bb = colorBrightness(b.color, f); + if (ba !== bb) return ba < bb ? a : b; + } + return a; +}; + /** * Creates a border overlay element for a table fragment. * From fd8661b7911043a91bb890df7b2ff83494992dfc Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Tue, 2 Jun 2026 21:08:10 -0300 Subject: [PATCH 4/6] fix(table): collapsed cell-border rendering with tblPrEx and gridAfter MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Render w:tblPrEx row-level borders (merged per edge over the table borders) and resolve collapsed shared edges with single-owner ownership plus the §17.4.66 winner, so adjacent cell borders draw exactly once (no doubling) and asymmetric or borderless edges keep their line (no drop). Handle trailing w:gridAfter columns: the rightmost real cell owns the right border, the resize overlay skips the degenerate spacer, and a cell spanning past the next row's coverage owns its full-width bottom while that row suppresses its top. (SD-3345, SD-2969) --- packages/layout-engine/contracts/src/index.ts | 8 + .../dom/src/table/renderTableFragment.test.ts | 55 +++ .../dom/src/table/renderTableFragment.ts | 34 +- .../dom/src/table/renderTableRow.test.ts | 374 +++++++++++++++++- .../painters/dom/src/table/renderTableRow.ts | 199 ++++++++-- .../layout-adapter/converters/table.test.ts | 58 +++ .../core/layout-adapter/converters/table.ts | 12 + 7 files changed, 711 insertions(+), 29 deletions(-) diff --git a/packages/layout-engine/contracts/src/index.ts b/packages/layout-engine/contracts/src/index.ts index e12ab7ea53..4b9d00afbc 100644 --- a/packages/layout-engine/contracts/src/index.ts +++ b/packages/layout-engine/contracts/src/index.ts @@ -760,6 +760,14 @@ export type TableRowAttrs = { value: number; rule?: 'auto' | 'atLeast' | 'exact' | string; }; + /** + * Row-level border override from OOXML `w:tblPrEx/w:tblBorders` (§17.4.61). + * Table property exceptions override the table-level borders for this row + * only. Rows without a `tblPrEx` border block leave this undefined and fall + * through to the table's borders. Resolved (eighth-points → px) by the v1 + * layout-adapter; the painter merges it over the table borders per edge. + */ + borders?: TableBorders; }; export type TableRow = { diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts index b77aa3edd4..79d5d1d529 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts @@ -577,6 +577,61 @@ describe('renderTableFragment', () => { }); }); + // Build a one-row measure whose single cell occupies only column 0, leaving + // the last grid column as an empty trailing gridAfter spacer. + const spacerMeasure = (spacerWidth: number): TableMeasure => ({ + kind: 'table', + rows: [ + { + cells: [ + { + paragraph: { kind: 'paragraph', lines: [], totalHeight: 20 }, + width: 100, + height: 20, + gridColumnStart: 0, + colSpan: 1, + }, + ], + height: 20, + }, + ], + columnWidths: [100, spacerWidth], + totalWidth: 100 + spacerWidth, + totalHeight: 20, + }); + + const renderSpacerTable = (spacerWidth: number): Element => + renderTableFragment({ + doc, + fragment: createTestTableFragment([ + { index: 0, x: 0, width: 100, minWidth: 25, resizable: true }, + { index: 1, x: 100, width: spacerWidth, minWidth: 25, resizable: true }, + ]), + context, + block: createTestTableBlock(), + measure: spacerMeasure(spacerWidth), + cellSpacingPx: 0, + effectiveColumnWidths: [100, spacerWidth], + renderLine: () => doc.createElement('div'), + applyFragmentFrame: () => {}, + applySdtDataset: () => {}, + applyStyles: () => {}, + }); + + it('omits the resize boundary for a degenerate trailing gridAfter spacer column (SD-3345)', () => { + // Spacer is narrower than its own min width → degenerate → its left-edge + // resize boundary is suppressed so it does not crowd the table-edge handle. + const parsed = JSON.parse(renderSpacerTable(7).getAttribute('data-table-boundaries')!); + expect(parsed.segments[1]).toEqual([]); + }); + + it('keeps the resize boundary for a normal-width trailing column (control)', () => { + // Same shape but the trailing column meets its min width → not degenerate → + // the boundary is kept. Proves the suppression is gated on degeneracy. + const parsed = JSON.parse(renderSpacerTable(40).getAttribute('data-table-boundaries')!); + expect(parsed.segments[1].length).toBeGreaterThan(0); + }); + it('should embed row boundary metadata when rowBoundaries are present', () => { const block = createTestTableBlock(); const measure = createTestTableMeasure(); diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts index 4a0faf4db7..cacc298f3a 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts @@ -313,6 +313,28 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement // Track which column boundaries exist in this row const boundariesInRow = new Set(); + // Columns occupied by a cell in this row. Used to detect a trailing + // w:gridAfter spacer column this row leaves empty. + const occupiedCols = new Set(); + for (const cellMeasure of rowMeasure.cells) { + const s = cellMeasure.gridColumnStart ?? 0; + const sp = cellMeasure.colSpan ?? 1; + for (let c = s; c < s + sp; c++) occupiedCols.add(c); + } + // A degenerate trailing gridAfter spacer (last column, unoccupied this row, + // narrower than its own min width) sits a few px from the table edge. Emitting + // a resize boundary at its left edge crowds the table-edge handle and reads as a + // doubled border on hover, so skip that boundary for this row (SD-3345). + const lastColIndex = columnCount - 1; + const lastColMeta = fragment.metadata.columnBoundaries[lastColIndex]; + const skipTrailingSpacerBoundary = + lastColIndex > 0 && + !occupiedCols.has(lastColIndex) && + !!lastColMeta && + typeof lastColMeta.width === 'number' && + typeof lastColMeta.minWidth === 'number' && + lastColMeta.width < lastColMeta.minWidth; + for (const cellMeasure of rowMeasure.cells) { const startCol = cellMeasure.gridColumnStart ?? 0; const colSpan = cellMeasure.colSpan ?? 1; @@ -323,8 +345,10 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement if (startCol > 0) { boundariesInRow.add(startCol); } - // End boundary (right edge of cell) - if (endCol < columnCount) { + // End boundary (right edge of cell), unless it lands on a degenerate + // trailing gridAfter spacer (its left edge is the table edge for practical + // purposes, handled by the table-edge handle). + if (endCol < columnCount && !(skipTrailingSpacerBoundary && endCol === lastColIndex)) { boundariesInRow.add(endCol); } } @@ -428,6 +452,9 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement y, rowMeasure, row: block.rows[r], + prevRow: r > 0 ? block.rows[r - 1] : undefined, + prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, + nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, totalRows: block.rows.length, tableBorders, columnWidths: effectiveColumnWidths, @@ -595,6 +622,9 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement y, rowMeasure, row: block.rows[r], + prevRow: r > 0 ? block.rows[r - 1] : undefined, + prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, + nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, totalRows: block.rows.length, tableBorders, columnWidths: effectiveColumnWidths, diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts index 4c1661ced4..fdcfe7bb89 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts @@ -126,7 +126,7 @@ describe('renderTableRow', () => { expect(call.borders?.right).toBeDefined(); }); - it('does not paint interior right border for explicit cell borders in collapsed mode', () => { + it('draws its own interior right border when the right neighbor is borderless (asymmetric, no doubling)', () => { renderTableRow( createDeps({ rowIndex: 0, @@ -165,12 +165,14 @@ describe('renderTableRow', () => { ); expect(renderTableCellMock).toHaveBeenCalledTimes(2); - const firstCall = renderTableCellMock.mock.calls[0][0] as { - borders?: { right?: unknown; left?: unknown }; - }; + const firstCall = renderTableCellMock.mock.calls[0][0] as { borders?: { right?: unknown; left?: unknown } }; + const secondCall = renderTableCellMock.mock.calls[1][0] as { borders?: { left?: unknown } }; - expect(firstCall.borders?.right).toBeUndefined(); + // Asymmetric edge: only cell-1 declares a right border (cell-2 is borderless), so cell-1 + // owns and paints it on itself; cell-2 does NOT redraw the shared edge, so it's drawn once. + expect(firstCall.borders?.right).toBeDefined(); expect(firstCall.borders?.left).toBeDefined(); + expect(secondCall.borders?.left).toBeUndefined(); }); it('does not paint interior bottom border for explicit cell borders in collapsed mode on non-final row', () => { @@ -228,6 +230,368 @@ describe('renderTableRow', () => { expect(call.borders?.bottom).toBeDefined(); }); + describe('row-level border override (w:tblPrEx)', () => { + const D9 = { style: 'single' as const, width: 1, color: '#D9D9D9' }; + const rowBorderOverride = { top: D9, right: D9, bottom: D9, left: D9, insideH: D9, insideV: D9 }; + + it('applies the row border override over the table borders for that row', () => { + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 1, + cellSpacingPx: 0, + columnWidths: [100], + rowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + row: { + id: 'row-1', + cells: [{ id: 'cell-1', blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }], + attrs: { borders: rowBorderOverride }, + }, + }) as never, + ); + + const call = getRenderedCellCall(); + // Single cell touches all edges; the row override (#D9D9D9) wins over the + // default table borders (#000000) on every side. + expect(call.borders?.top).toEqual(D9); + expect(call.borders?.right).toEqual(D9); + expect(call.borders?.bottom).toEqual(D9); + expect(call.borders?.left).toEqual(D9); + }); + + it('draws the row override even when the table borders are explicitly none (FWC form rows)', () => { + const none = { none: true as const }; + renderTableRow( + createDeps({ + rowIndex: 1, + totalRows: 3, + cellSpacingPx: 0, + columnWidths: [100], + tableBorders: { top: none, right: none, bottom: none, left: none, insideH: none, insideV: none }, + rowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + row: { + id: 'row-1', + cells: [{ id: 'cell-1', blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }], + attrs: { borders: rowBorderOverride }, + }, + }) as never, + ); + + const call = getRenderedCellCall(); + // Table says borderless, but this row's tblPrEx draws #D9D9D9. Top is an + // interior edge (row 1 of 3) → resolves from the override's insideH. + expect(call.borders?.top).toEqual(D9); + expect(call.borders?.left).toEqual(D9); + }); + + it('leaves table borders unchanged for a row without an override (callout row)', () => { + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 1, + cellSpacingPx: 0, + columnWidths: [100], + rowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + row: { + id: 'row-1', + cells: [{ id: 'cell-1', blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }], + // no attrs.borders — falls through to the table borders + }, + }) as never, + ); + + const call = getRenderedCellCall(); + // Regression guard: absent a row override, the cell still paints the table + // border (#000000), not the #D9D9D9 override from the other tests. + expect(call.borders?.top).toEqual({ style: 'single', width: 1, color: '#000000' }); + }); + }); + + describe('trailing gridAfter columns (w:gridAfter)', () => { + it('draws the right border on the rightmost real cell when gridAfter pads the grid', () => { + // FWC form pattern: two content columns + one trailing gridAfter spacer column. + // The rightmost real cell (col 1) does not reach totalCols (3), but it is the + // row's right edge, so it must own the table/row right border (Word behaviour). + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 1, + cellSpacingPx: 0, + columnWidths: [100, 100, 20], // col 2 is the empty gridAfter spacer + rowMeasure: { + height: 20, + cells: [ + { width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }, + { width: 100, height: 20, gridColumnStart: 1, colSpan: 1, rowSpan: 1 }, + ], + }, + row: { + id: 'row-1', + cells: [ + { id: 'c0', blocks: [{ kind: 'paragraph', id: 'p0', runs: [] }] }, + { id: 'c1', blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }, + ], + }, + }) as never, + ); + + expect(renderTableCellMock).toHaveBeenCalledTimes(2); + const rightCell = renderTableCellMock.mock.calls[1][0] as { borders?: { right?: unknown } }; + // The default table borders include a single right border; the rightmost + // real cell now owns it despite the trailing gridAfter column. + expect(rightCell.borders?.right).toBeDefined(); + }); + }); + + describe('collapsed cell-border conflict resolution (ECMA-376 §17.4.66, SD-3345)', () => { + const B = { style: 'single' as const, width: 1.333, color: '#BDD7EE' }; + const allSides = { top: B, right: B, bottom: B, left: B }; + + it('draws a shared interior vertical edge once (no doubling) for adjacent cells with identical borders', () => { + // The M&A checklist case: no table-level borders, every cell has all 4 sides. + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 2, + cellSpacingPx: 0, + columnWidths: [100, 100], + tableBorders: undefined, + rowMeasure: { + height: 20, + cells: [ + { width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }, + { width: 100, height: 20, gridColumnStart: 1, colSpan: 1, rowSpan: 1 }, + ], + }, + row: { + id: 'r', + cells: [ + { id: 'c0', attrs: { borders: allSides }, blocks: [{ kind: 'paragraph', id: 'p0', runs: [] }] }, + { id: 'c1', attrs: { borders: allSides }, blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }, + ], + }, + }) as never, + ); + const left = renderTableCellMock.mock.calls[0][0] as { borders?: { right?: unknown } }; + const right = renderTableCellMock.mock.calls[1][0] as { borders?: { left?: unknown } }; + // Single-owner: the left cell's interior RIGHT is suppressed; the right cell draws + // the shared edge as its LEFT (the §17.4.66 winner). So the edge is drawn exactly once. + expect(left.borders?.right).toBeUndefined(); + expect(right.borders?.left).toBeDefined(); + }); + + it('keeps BOTH side borders on a cell whose neighbors are borderless (SD-3345 RTL start/end)', () => { + // The RTL tcBorders fixture in logical space: the first cell declares left (start) AND + // right (end) borders; the other cells are borderless. Both must stay on that cell — + // after the downstream RTL swap they become visual-right (start) and visual-left (end), + // not move onto a borderless neighbor. (Regression: single-owner used to delegate the + // interior right, dropping the cell's own end border.) + const start = { style: 'single' as const, width: 2, color: '#FF0000' }; + const end = { style: 'single' as const, width: 2, color: '#0000FF' }; + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 1, + cellSpacingPx: 0, + columnWidths: [100, 100, 100], + tableBorders: undefined, + rowMeasure: { + height: 20, + cells: [ + { width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }, + { width: 100, height: 20, gridColumnStart: 1, colSpan: 1, rowSpan: 1 }, + { width: 100, height: 20, gridColumnStart: 2, colSpan: 1, rowSpan: 1 }, + ], + }, + row: { + id: 'r', + cells: [ + { + id: 'c0', + attrs: { borders: { left: start, right: end } }, + blocks: [{ kind: 'paragraph', id: 'p0', runs: [] }], + }, + { id: 'c1', attrs: {}, blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }, + { id: 'c2', attrs: {}, blocks: [{ kind: 'paragraph', id: 'p2', runs: [] }] }, + ], + }, + }) as never, + ); + const c0 = renderTableCellMock.mock.calls[0][0] as { borders?: { left?: unknown; right?: unknown } }; + expect(c0.borders?.left).toMatchObject({ color: '#FF0000' }); + expect(c0.borders?.right).toMatchObject({ color: '#0000FF' }); + }); + + it('keeps the above cell bottom border when this cell has no top (asymmetric → no dropped line)', () => { + // The it1007 regression case: header has a bottom border, the body cell below has no top. + const black = { style: 'single' as const, width: 1, color: '#000000' }; + renderTableRow( + createDeps({ + rowIndex: 1, + totalRows: 2, + cellSpacingPx: 0, + columnWidths: [100], + tableBorders: undefined, + rowMeasure: { height: 20, cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }] }, + row: { + id: 'r1', + // body cell: borders on left/right/bottom but NOT top + cells: [ + { + id: 'cb', + attrs: { borders: { left: black, right: black, bottom: black } }, + blocks: [{ kind: 'paragraph', id: 'pb', runs: [] }], + }, + ], + }, + prevRow: { + id: 'r0', + cells: [ + { + id: 'ch', + attrs: { borders: { top: black, left: black, right: black, bottom: black } }, + blocks: [{ kind: 'paragraph', id: 'ph', runs: [] }], + }, + ], + }, + prevRowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + }) as never, + ); + const cell = renderTableCellMock.mock.calls[0][0] as { borders?: { top?: unknown } }; + // This cell has no top border, but the cell above has a bottom border → §17.4.66 + // rule 1 keeps the present border, drawn here as this cell's top. + expect(cell.borders?.top).toMatchObject({ style: 'single', color: '#000000' }); + }); + + it('paints the above cell bottom border on a fully BORDERLESS row below it (SD-2969 clause-header)', () => { + // The SD-2969 case: a clause-header row has top+bottom borders; the row directly + // below it has NO border attribute at all. Single-owner gives the shared edge to + // the lower cell, so without the §17.4.66 fallback the header's bottom border is + // dropped entirely (the cell above suppressed its own bottom, the borderless cell + // below never drew it). + const black = { style: 'single' as const, width: 1, color: '#000000' }; + renderTableRow( + createDeps({ + rowIndex: 1, + totalRows: 2, + cellSpacingPx: 0, + columnWidths: [100], + tableBorders: undefined, + rowMeasure: { height: 8, cells: [{ width: 100, height: 8, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }] }, + row: { + id: 'r1', + // borderless cell: no `borders` attribute at all + cells: [{ id: 'cs', attrs: {}, blocks: [{ kind: 'paragraph', id: 'ps', runs: [] }] }], + }, + prevRow: { + id: 'r0', + cells: [ + { + id: 'ch', + attrs: { borders: { top: black, left: black, right: black, bottom: black } }, + blocks: [{ kind: 'paragraph', id: 'ph', runs: [] }], + }, + ], + }, + prevRowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + }) as never, + ); + const cell = renderTableCellMock.mock.calls[0][0] as { borders?: { top?: unknown } }; + expect(cell.borders?.top).toMatchObject({ style: 'single', color: '#000000' }); + }); + + it('draws its own bottom border when the next row leaves a gridAfter gap under it (SD-3345 callout corner)', () => { + // SD-3345 23_notification: the callout cell spans the full grid (gridSpan), but the + // row below has a gridAfter so its real cells do not reach the callout's rightmost + // column. Single-owner would defer the callout's bottom to the row below, which then + // stops short of the right edge → a gap at the bottom-right corner. The callout must + // draw its own bottom across the uncovered span instead. + const blue = { style: 'single' as const, width: 1, color: '#342D8C' }; + renderTableRow( + createDeps({ + rowIndex: 0, + totalRows: 2, + cellSpacingPx: 0, + columnWidths: [100, 100], + tableBorders: undefined, + // full-width callout cell (spans both columns) + rowMeasure: { height: 20, cells: [{ width: 200, height: 20, gridColumnStart: 0, colSpan: 2, rowSpan: 1 }] }, + row: { + id: 'r0', + cells: [ + { + id: 'callout', + attrs: { borders: { top: blue, left: blue, right: blue, bottom: blue } }, + blocks: [{ kind: 'paragraph', id: 'p0', runs: [] }], + }, + ], + }, + // next row covers only col0 (col1 is a gridAfter spacer → not a real cell) + nextRowMeasure: { + height: 20, + cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }], + }, + }) as never, + ); + const cell = renderTableCellMock.mock.calls[0][0] as { borders?: { bottom?: unknown } }; + expect(cell.borders?.bottom).toMatchObject({ style: 'single', color: '#342D8C' }); + }); + + it('suppresses this row top border when the cell above spans past it (gridAfter) so the edge is drawn once', () => { + // The companion to the case above: when the spanning cell owns the shared bottom edge, + // the narrower row below must NOT also draw its top, or the two adjacent cell divs stack + // into a doubled line (this painter has no border-collapse). (SD-3345 callout) + const blue = { style: 'single' as const, width: 1, color: '#342D8C' }; + renderTableRow( + createDeps({ + rowIndex: 1, + totalRows: 2, + cellSpacingPx: 0, + columnWidths: [100, 100], + tableBorders: undefined, + // this row covers only col0 (col1 is its gridAfter spacer) + rowMeasure: { height: 20, cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }] }, + row: { + id: 'r1', + cells: [{ id: 'opt', attrs: {}, blocks: [{ kind: 'paragraph', id: 'po', runs: [] }] }], + }, + // the cell above spans BOTH columns and has a bottom border (it owns the shared edge) + prevRow: { + id: 'r0', + cells: [ + { + id: 'callout', + attrs: { borders: { top: blue, left: blue, right: blue, bottom: blue } }, + blocks: [{ kind: 'paragraph', id: 'p0', runs: [] }], + }, + ], + }, + prevRowMeasure: { + height: 20, + cells: [{ width: 200, height: 20, gridColumnStart: 0, colSpan: 2, rowSpan: 1 }], + }, + }) as never, + ); + const cell = renderTableCellMock.mock.calls[0][0] as { borders?: { top?: unknown } } | undefined; + expect(cell?.borders?.top).toBeUndefined(); + }); + }); + describe('RTL table (isRtl)', () => { it('mirrors cell x positions so first logical column is on the right', () => { renderTableRow( diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts index d3b2a117f2..a289f389da 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts @@ -14,7 +14,9 @@ import { resolveTableCellBorders, borderValueToSpec, resolveTableBorderValue, + resolveBorderConflict, hasExplicitCellBorders, + isPresentBorder, swapCellBordersLR, } from './border-utils.js'; import { getTableCellGridBounds, type TableCellGridPosition } from './grid-geometry.js'; @@ -32,6 +34,25 @@ type CellBorderResolutionArgs = { cellSpacingPx: number; continuesFromPrev: boolean; continuesOnNext: boolean; + /** Borders of the cell directly above (previous row, same grid column) for §17.4.66 conflict resolution. */ + aboveCellBorders?: CellBorders; + /** Borders of the cell directly to the left (same row, previous grid column). */ + leftCellBorders?: CellBorders; + /** Borders of the cell directly to the right (same row, next grid column), for asymmetric-edge ownership. */ + rightCellBorders?: CellBorders; + /** + * True when the next row's real cells do not reach this cell's right edge (e.g. the next + * row has a `w:gridAfter` spacer while this cell spans into it). The cell below then can't + * own the shared bottom edge across the uncovered span, so this cell must draw its own + * bottom border or the line stops short at the bottom-right corner. (SD-3345) + */ + nextRowLeavesRightGap?: boolean; + /** + * True when the cell ABOVE spans past this cell's row right edge (this row has a gridAfter + * relative to it). The spanning cell owns the shared bottom edge and draws it, so this cell + * must suppress its top border to avoid a doubled line. (SD-3345) + */ + deferTopToAboveCell?: boolean; }; const hasAnyResolvedBorder = (borders: CellBorders): boolean => @@ -52,14 +73,78 @@ const resolveRenderedCellBorders = ({ cellSpacingPx, continuesFromPrev, continuesOnNext, + aboveCellBorders, + leftCellBorders, + rightCellBorders, + nextRowLeavesRightGap, + deferTopToAboveCell, }: CellBorderResolutionArgs): CellBorders | undefined => { const hasExplicitBorders = hasExplicitCellBorders(cellBorders); + const cellBounds = getTableCellGridBounds(cellPosition); + const touchesTopBoundary = cellBounds.touchesTopEdge || continuesFromPrev; + // The bottom is a real boundary either when this is the last row / a fragment break, OR + // when the next row's real cells don't reach this cell's right edge (a gridAfter spacer + // under a spanning cell): the row below can't own the shared edge across the uncovered + // span, so this (spanning) cell owns and draws its full-width bottom. The row below then + // suppresses its top there (see `deferTopToAboveCell`) so the edge is drawn exactly once — + // this painter has no border-collapse, so two cells drawing it would stack into a doubled + // line, not overlap. (SD-3345) + const touchesBottomBoundary = cellBounds.touchesBottomEdge || continuesOnNext || nextRowLeavesRightGap === true; + + // A shared interior edge in the collapsed model is owned by the lower/right cell, so a + // border defined ONLY by the neighbor above/left must still be painted here — even when + // this cell has no border of its own — or the line is dropped entirely (the neighbor + // suppressed its own edge under single-owner). (SD-2969: a bordered clause-header row + // above a fully borderless spacer row.) + const hasInteriorNeighborBorder = + (!touchesTopBoundary && !deferTopToAboveCell && isPresentBorder(aboveCellBorders?.bottom)) || + (!cellBounds.touchesLeftEdge && isPresentBorder(leftCellBorders?.right)); + + // Collapsed model (zero cell spacing): single-owner positioning, where the value at a + // shared interior edge is the ECMA-376 §17.4.66 winner of the two adjacent cell borders. + // This draws a shared edge exactly ONCE (no doubling) while keeping the present border on + // an asymmetric edge (no dropped line). Runs whenever this cell OR a neighbor above/left + // defines a border, so `cb` defaults to {} for the borderless case (resolveBorderConflict + // (undefined, x) === x). Interior right/bottom are owned by the neighbor to the right/below; + // outer edges use the cell border (which beats the table border), falling back to the table + // border. Works whether or not table-level borders exist. (SD-3345, SD-2969) + if (cellSpacingPx === 0 && (hasExplicitBorders || hasInteriorNeighborBorder)) { + const cb = (cellBorders ?? {}) as CellBorders; + return { + top: touchesTopBoundary + ? resolveTableBorderValue(cb.top, tableBorders?.top) + : deferTopToAboveCell + ? undefined + : (resolveBorderConflict(cb.top, aboveCellBorders?.bottom) ?? borderValueToSpec(tableBorders?.insideH)), + // Vertical interior edges: when BOTH adjacent cells declare a border, the right cell + // owns it (draws its left as the §17.4.66 winner) so the edge is painted once (no + // doubling). When only ONE side declares a border (asymmetric, no doubling risk) that + // cell draws it on ITS OWN side — so an RTL cell's end (logical-right) border stays on + // the cell after the left/right swap instead of moving onto a borderless neighbor. (SD-3345) + left: cellBounds.touchesLeftEdge + ? resolveTableBorderValue(cb.left, tableBorders?.left) + : isPresentBorder(cb.left) + ? (resolveBorderConflict(cb.left, leftCellBorders?.right) ?? borderValueToSpec(tableBorders?.insideV)) + : isPresentBorder(leftCellBorders?.right) + ? undefined + : borderValueToSpec(tableBorders?.insideV), + right: cellBounds.touchesRightEdge + ? resolveTableBorderValue(cb.right, tableBorders?.right) + : isPresentBorder(cb.right) && !isPresentBorder(rightCellBorders?.left) + ? cb.right + : undefined, + bottom: touchesBottomBoundary ? resolveTableBorderValue(cb.bottom, tableBorders?.bottom) : undefined, + }; + } + if (hasBordersAttribute && !hasExplicitBorders) { return undefined; } if (!tableBorders) { + // Separate mode (non-zero cell spacing) with explicit borders, or no table borders + // at all: there is no shared-edge conflict, so draw every specified border. return hasExplicitBorders ? { top: cellBorders.top, @@ -70,27 +155,8 @@ const resolveRenderedCellBorders = ({ : undefined; } - const cellBounds = getTableCellGridBounds(cellPosition); - const touchesTopBoundary = cellBounds.touchesTopEdge || continuesFromPrev; - const touchesBottomBoundary = cellBounds.touchesBottomEdge || continuesOnNext; - if (hasExplicitBorders) { - if (cellSpacingPx === 0) { - // Collapsed model: avoid double interior borders by using single-owner sides. - // Keep explicit top/left (or table fallbacks), and only render right/bottom on table edges. - // Assumes shared interior edges specify the same border on both adjacent cells (e.g. Google Docs - // round-trips this way). Asymmetric (only one cell’s side set) may miss a line until we add conflict resolution. - return { - top: resolveTableBorderValue(cellBorders.top, touchesTopBoundary ? tableBorders.top : tableBorders.insideH), - right: cellBounds.touchesRightEdge ? resolveTableBorderValue(cellBorders.right, tableBorders.right) : undefined, - bottom: touchesBottomBoundary ? resolveTableBorderValue(cellBorders.bottom, tableBorders.bottom) : undefined, - left: resolveTableBorderValue( - cellBorders.left, - cellBounds.touchesLeftEdge ? tableBorders.left : tableBorders.insideV, - ), - }; - } - + // Separate mode (cellSpacingPx > 0) with table-level borders present. return { top: resolveTableBorderValue(cellBorders.top, touchesTopBoundary ? tableBorders.top : tableBorders.insideH), right: resolveTableBorderValue(cellBorders.right, cellBounds.touchesRightEdge ? tableBorders.right : undefined), @@ -142,6 +208,11 @@ type TableRowRenderDependencies = { rowMeasure: TableRowMeasure; /** Row data (cells, attributes), or undefined for empty rows */ row?: TableRow; + /** Previous (above) row data + measure, for collapsed-border conflict resolution (§17.4.66). */ + prevRow?: TableRow; + prevRowMeasure?: TableRowMeasure; + /** Next (below) row measure, to detect a gridAfter gap under a spanning cell (SD-3345). */ + nextRowMeasure?: TableRowMeasure; /** Total number of rows in the table (for border resolution) */ totalRows: number; /** Table-level borders (for resolving cell borders) */ @@ -254,6 +325,9 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { y, rowMeasure, row, + prevRow, + prevRowMeasure, + nextRowMeasure, totalRows, tableBorders, columnWidths, @@ -279,6 +353,26 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { const totalCols = columnWidths.length; + // Effective right grid edge for THIS row's border ownership. A row with a + // trailing w:gridAfter reserves empty columns past its last cell (FWC forms do + // this), so the rightmost real cell never reaches `totalCols` and the + // single-owner model would drop the row's right border. Word draws the right + // border at the rightmost cell, treating gridAfter columns as outside the box. + // Use the last occupied column as the right edge; for rows without gridAfter + // this equals totalCols (no change). + const rowRightEdgeCol = rowMeasure.cells.length + ? Math.min(totalCols, Math.max(...rowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1)))) + : totalCols; + + // Row-level border override (OOXML w:tblPrEx/w:tblBorders, §17.4.61). When this + // row carries its own borders, they override the table borders for this row only, + // merged per edge so unspecified sides still inherit the table. Rows without an + // override paint with the table borders unchanged (no behavior change). + const rowBorderOverride = row?.attrs?.borders; + const effectiveTableBorders: TableBorders | undefined = rowBorderOverride + ? { ...(tableBorders ?? {}), ...rowBorderOverride } + : tableBorders; + /** * Calculates the horizontal position (x-coordinate) for a cell based on its grid column index. * @@ -359,6 +453,44 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { return width; }; + // Find the borders of the cell in `cells` that occupies grid column `gridCol`, using + // the row's measure to map cell index → grid span (handles colspan). Used to fetch the + // above/left neighbor's borders for §17.4.66 collapsed-border conflict resolution. + const findCellBordersAtColumn = ( + cells: TableRow['cells'] | undefined, + measureCells: TableRowMeasure['cells'] | undefined, + gridCol: number, + ): CellBorders | undefined => { + if (!cells || !measureCells) return undefined; + for (let i = 0; i < measureCells.length; i++) { + const start = measureCells[i].gridColumnStart ?? i; + const span = measureCells[i].colSpan ?? 1; + if (gridCol >= start && gridCol < start + span) return cells[i]?.attrs?.borders; + } + return undefined; + }; + + // Right edge (exclusive grid column) of the cell occupying `gridCol` in `measureCells`. + const findCellRightEdgeAtColumn = ( + measureCells: TableRowMeasure['cells'] | undefined, + gridCol: number, + ): number | undefined => { + if (!measureCells) return undefined; + for (let i = 0; i < measureCells.length; i++) { + const start = measureCells[i].gridColumnStart ?? i; + const span = measureCells[i].colSpan ?? 1; + if (gridCol >= start && gridCol < start + span) return start + span; + } + return undefined; + }; + + // Rightmost grid column (exclusive) covered by the next row's REAL cells. When a spanning + // cell's right edge exceeds this, the next row has a gridAfter spacer beneath it and can't + // own the shared bottom edge across the uncovered span. (SD-3345) + const nextRowMaxCol = nextRowMeasure?.cells?.length + ? Math.max(...nextRowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1))) + : Infinity; + for (let cellIndex = 0; cellIndex < rowMeasure.cells.length; cellIndex += 1) { const cellMeasure = rowMeasure.cells[cellIndex]; const cell = row?.cells?.[cellIndex]; @@ -381,9 +513,27 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { gridColumnStart, colSpan, totalRows, - totalCols, + // Use the row's effective right edge so the rightmost cell owns the right + // border even when trailing w:gridAfter columns pad the grid (§17.4.55). + totalCols: rowRightEdgeCol, }; + // Neighbor borders for §17.4.66 collapsed-border conflict resolution: the cell above + // (previous row, same grid column) and the cell to the left (same row, previous column). + const aboveCellBorders = findCellBordersAtColumn(prevRow?.cells, prevRowMeasure?.cells, gridColumnStart); + const leftCellBorders = + gridColumnStart > 0 ? findCellBordersAtColumn(row?.cells, rowMeasure.cells, gridColumnStart - 1) : undefined; + // The cell to the right (same row, the column just past this cell's span) — used to keep + // an asymmetric vertical edge on the owning cell instead of moving it to the neighbor. + const rightCellBorders = findCellBordersAtColumn(row?.cells, rowMeasure.cells, gridColumnStart + colSpan); + // This cell spans past the next row's real cells (gridAfter spacer beneath its right edge). + const nextRowLeavesRightGap = gridColumnStart + colSpan > nextRowMaxCol; + // Conversely, the cell ABOVE spans past THIS row's right edge (this row has a gridAfter + // relative to it). The spanning cell then owns the full shared edge and draws its own + // bottom, so this cell must NOT also draw its top, or the edge doubles. (SD-3345) + const aboveCellRightEdge = findCellRightEdgeAtColumn(prevRowMeasure?.cells, gridColumnStart); + const deferTopToAboveCell = aboveCellRightEdge !== undefined && aboveCellRightEdge > rowRightEdgeCol; + // Resolve borders using logical positions, then swap output for RTL. // The resolver uses touchesLeftEdge/touchesRightEdge which are LOGICAL edges. // For RTL, logical left = visual right, so we swap the resolved CSS properties @@ -391,11 +541,16 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { const resolvedBorders = resolveRenderedCellBorders({ cellBorders: cellBordersAttr, hasBordersAttribute, - tableBorders, + tableBorders: effectiveTableBorders, cellPosition, cellSpacingPx, continuesFromPrev: continuesFromPrev === true, continuesOnNext: continuesOnNext === true, + aboveCellBorders, + leftCellBorders, + rightCellBorders, + nextRowLeavesRightGap, + deferTopToAboveCell, }); // RTL: swap resolved left↔right so CSS properties match visual edges const finalBorders = isRtl && resolvedBorders ? swapCellBordersLR(resolvedBorders) : resolvedBorders; diff --git a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.test.ts b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.test.ts index 4765298947..27b9e32471 100644 --- a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.test.ts +++ b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.test.ts @@ -179,6 +179,64 @@ describe('table converter', () => { expect(result.rows[0].cells[0].paragraph.kind).toBe('paragraph'); }); + it('resolves w:tblPrEx row borders onto the row attrs, leaving rows without them untouched', () => { + // FWC form pattern (SD-3345): the table declares tblBorders=none, but form + // rows carry a w:tblPrEx/w:tblBorders override (#D9D9D9). The converter stores + // it raw (eighth-points) under tableRowProperties.tblPrExBorders; the adapter + // must resolve it to typed row.attrs.borders. The callout row has no override. + const D9 = { val: 'single', color: '#D9D9D9', themeColor: 'background1', themeShade: 'D9', size: 4, space: 0 }; + const node: PMNode = { + type: 'table', + attrs: { borders: {} }, // table-level borders are none/empty + content: [ + { + type: 'tableRow', + attrs: { tableRowProperties: { someFlag: true } }, // callout row: no tblPrExBorders + content: [ + { type: 'tableCell', content: [{ type: 'paragraph', content: [{ type: 'text', text: 'callout' }] }] }, + ], + }, + { + type: 'tableRow', + attrs: { + tableRowProperties: { + tblPrExBorders: { top: D9, left: D9, bottom: D9, right: D9, insideH: D9, insideV: D9 }, + }, + }, + content: [ + { + type: 'tableCell', + content: [{ type: 'paragraph', content: [{ type: 'text', text: 'First name(s)' }] }], + }, + ], + }, + ], + }; + + const result = tableNodeToBlock( + node, + mockBlockIdGenerator, + mockPositionMap, + 'Arial', + 16, + undefined, + undefined, + undefined, + undefined, + mockParagraphConverter, + ) as TableBlock; + + // Callout row: no tblPrEx → no resolved row borders (falls through to table). + expect(result.rows[0].attrs?.borders).toBeUndefined(); + + // Form row: tblPrEx resolved to typed borders (#D9D9D9, eighth-points → px). + const rowBorders = result.rows[1].attrs?.borders; + expect(rowBorders).toBeDefined(); + expect(rowBorders?.top).toMatchObject({ style: 'single', color: '#D9D9D9' }); + expect((rowBorders?.top as { width: number }).width).toBeGreaterThan(0); + expect(rowBorders?.insideH).toMatchObject({ style: 'single', color: '#D9D9D9' }); + }); + it('does not emit imported gridBefore/gridAfter placeholder cells into TableBlock rows', () => { const node: PMNode = { type: 'table', diff --git a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts index 0e4cb49c0a..b6d2d44e41 100644 --- a/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts +++ b/packages/super-editor/src/editors/v1/core/layout-adapter/converters/table.ts @@ -720,11 +720,23 @@ const parseTableRow = (args: ParseTableRowArgs): TableRow | null => { const rowProps = rowNode.attrs?.tableRowProperties; const rowHeight = normalizeRowHeight(rowProps as Record | undefined); + + // Row-level border override from w:tblPrEx/w:tblBorders (ECMA-376 §17.4.61). + // The converter stores these raw (eighth-points) under tableRowProperties.tblPrExBorders. + // tblPrEx overrides the table's borders for this row only; rows without it fall + // through to the table borders (callout rows in FWC forms stay borderless this way). + const tblPrExBordersRaw = (rowProps as Record | undefined)?.tblPrExBorders; + const rowBorders = + tblPrExBordersRaw && typeof tblPrExBordersRaw === 'object' + ? extractTableBorders(tblPrExBordersRaw as Record, { unit: 'eighthPoints' }) + : undefined; + const attrs: TableRowAttrs | undefined = rowProps && typeof rowProps === 'object' ? { tableRowProperties: rowProps as Record, ...(rowHeight ? { rowHeight } : {}), + ...(rowBorders ? { borders: rowBorders } : {}), } : rowHeight ? { rowHeight } From 668fcba2f9f1bbb32cd2122ea9d6cc310f8fe082 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 3 Jun 2026 12:49:11 -0300 Subject: [PATCH 5/6] fix(table): honor explicit nil borders at collapsed shared edges MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two §17.4.66 cell-border conflict cases in the single-owner collapsed model were resolved against the wrong precedence, both visible in sd-1279-tables.docx vs Word: - A style-bordered row above a row whose tblPrEx override suppresses the shared horizontal edge (insideH none) lost its closing border: the lower row owns that edge but draws nothing, and the upper row's border came from the table style (no cell tcBorder for the neighbor path to pick up). The grid no longer closed. The upper row now draws its own insideH to close the grid when the next row's override suppresses the edge (present beats none). - A divider between two cells that BOTH set the shared vertical edge to w:val="nil" was still drawn because the resolver fell back to the table style insideV, treating an explicit nil the same as an unset side. An explicit nil on both adjacent cells now suppresses the divider, while a merely-unset side still inherits insideH/insideV. Adds isExplicitNoneBorder to distinguish authored nil from unset. Scope verified across the layout corpus: only sd-1279-tables changes rendering (the only doc with a present interior border plus authored nil on both sides of a shared edge). Adds 5 regression tests. --- .../painters/dom/src/table/border-utils.ts | 13 +++ .../dom/src/table/renderTableFragment.ts | 2 + .../dom/src/table/renderTableRow.test.ts | 110 ++++++++++++++++++ .../painters/dom/src/table/renderTableRow.ts | 51 +++++++- 4 files changed, 173 insertions(+), 3 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/table/border-utils.ts b/packages/layout-engine/painters/dom/src/table/border-utils.ts index bf4f4f808f..1285d0aae0 100644 --- a/packages/layout-engine/painters/dom/src/table/border-utils.ts +++ b/packages/layout-engine/painters/dom/src/table/border-utils.ts @@ -213,6 +213,19 @@ const BORDER_STYLE_LINES: Partial> = { export const isPresentBorder = (b?: BorderSpec): b is BorderSpec => !!b && b.style !== undefined && b.style !== 'none' && (b.width === undefined || b.width > 0); +/** + * True when a border is EXPLICITLY set to none/nil (`w:val="nil"`/`"none"`), as opposed to + * simply unset/absent. The distinction matters for shared interior edges (§17.4.66): an + * explicit none on BOTH adjacent cells suppresses the divider, while an unset side inherits + * the table's insideH/insideV. Accepts either a CellBorders BorderSpec (`{style:'none'}`) or + * a TableBorderValue (`{none:true}`). + */ +export const isExplicitNoneBorder = (b?: unknown): boolean => { + if (!b || typeof b !== 'object') return false; + const r = b as Record; + return r.style === 'none' || r.none === true; +}; + const borderWeight = (b: BorderSpec): number => (BORDER_STYLE_LINES[b.style as BorderStyle] ?? 1) * (BORDER_STYLE_NUMBER[b.style as BorderStyle] ?? 1); diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts index cacc298f3a..4a7d79beba 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts @@ -454,6 +454,7 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement row: block.rows[r], prevRow: r > 0 ? block.rows[r - 1] : undefined, prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, + nextRow: r < block.rows.length - 1 ? block.rows[r + 1] : undefined, nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, totalRows: block.rows.length, tableBorders, @@ -624,6 +625,7 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement row: block.rows[r], prevRow: r > 0 ? block.rows[r - 1] : undefined, prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, + nextRow: r < block.rows.length - 1 ? block.rows[r + 1] : undefined, nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, totalRows: block.rows.length, tableBorders, diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts index fdcfe7bb89..f2eefb55f1 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts @@ -126,6 +126,67 @@ describe('renderTableRow', () => { expect(call.borders?.right).toBeDefined(); }); + // SD-3028: a tblPrEx override on the row BELOW that suppresses the shared horizontal edge + // (insideH none/nil) means the lower cell — which owns that interior edge in the single-owner + // model — won't draw it. When the upper row's border comes from the table/style (no cell + // tcBorder for the neighbor path to pick up), the grid bottom would be dropped. §17.4.66 + // (present beats none) requires the upper row to close the grid by drawing its own insideH. + const collapsedStyleRow = (overrides: Record = {}) => + createDeps({ + rowIndex: 0, + totalRows: 6, + cellSpacingPx: 0, + ...overrides, + }); + const noneBorder = { none: true }; + const allNoneOverride = { + id: 'row-next', + attrs: { + borders: { + top: noneBorder, + bottom: noneBorder, + left: noneBorder, + right: noneBorder, + insideH: noneBorder, + insideV: noneBorder, + }, + }, + cells: [{ id: 'cell-next', blocks: [{ kind: 'paragraph', id: 'pn', runs: [] }] }], + }; + + it('closes the grid by drawing its own bottom when the next row suppresses the shared edge (tblPrEx none)', () => { + renderTableRow(collapsedStyleRow({ nextRow: allNoneOverride }) as never); + + expect(renderTableCellMock).toHaveBeenCalledTimes(1); + const call = getRenderedCellCall(); + expect(call.borders?.bottom).toBeDefined(); + expect((call.borders?.bottom as { style?: string })?.style).toBe('single'); + }); + + it('does not draw an interior bottom when the next row has no override (lower cell owns it, no doubling)', () => { + renderTableRow(collapsedStyleRow() as never); + + expect(renderTableCellMock).toHaveBeenCalledTimes(1); + const call = getRenderedCellCall(); + expect(call.borders?.bottom).toBeUndefined(); + }); + + it('does not draw an interior bottom when the next row override keeps a present shared edge', () => { + renderTableRow( + collapsedStyleRow({ + nextRow: { + id: 'row-next', + attrs: { borders: { insideH: { style: 'single', width: 1, color: '#D9D9D9' } } }, + cells: [{ id: 'cell-next', blocks: [{ kind: 'paragraph', id: 'pn', runs: [] }] }], + }, + }) as never, + ); + + expect(renderTableCellMock).toHaveBeenCalledTimes(1); + const call = getRenderedCellCall(); + expect(call.borders?.bottom).toBeUndefined(); + }); + it('draws its own interior right border when the right neighbor is borderless (asymmetric, no doubling)', () => { renderTableRow( createDeps({ @@ -175,6 +236,55 @@ describe('renderTableRow', () => { expect(secondCall.borders?.left).toBeUndefined(); }); + // SD-3028: "some borders" tables set a cell's shared vertical edge to w:val="nil" on BOTH + // sides to remove a divider, while the table style (TableGrid) defines insideV. The explicit + // nil on both cells must suppress the divider (§17.4.66); it must NOT fall back to insideV. + const twoCellRow = (cell0Borders: unknown, cell1Borders: unknown) => + createDeps({ + rowIndex: 0, + totalRows: 1, + cellSpacingPx: 0, + columnWidths: [100, 100], + rowMeasure: { + height: 20, + cells: [ + { width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }, + { width: 100, height: 20, gridColumnStart: 1, colSpan: 1, rowSpan: 1 }, + ], + }, + row: { + id: 'row-1', + cells: [ + { id: 'cell-1', attrs: { borders: cell0Borders }, blocks: [{ kind: 'paragraph', id: 'p1', runs: [] }] }, + { id: 'cell-2', attrs: { borders: cell1Borders }, blocks: [{ kind: 'paragraph', id: 'p2', runs: [] }] }, + ], + }, + }); + const noneSpec = { style: 'none' as const, width: 0 }; + const bottomOnly = { + top: noneSpec, + right: noneSpec, + bottom: { style: 'single' as const, width: 1, color: '#000000' }, + left: noneSpec, + }; + + it('suppresses an interior vertical divider when BOTH adjacent cells set the shared edge to nil', () => { + renderTableRow(twoCellRow(bottomOnly, bottomOnly) as never); + + expect(renderTableCellMock).toHaveBeenCalledTimes(2); + const secondCall = renderTableCellMock.mock.calls[1][0] as { borders?: { left?: unknown } }; + expect(secondCall.borders?.left).toBeUndefined(); + }); + + it('still inherits the table insideV divider when only ONE side is explicitly nil (the other is unset)', () => { + // cell-2 left = nil, cell-1 right = unset (inherits insideV, present) -> §17.4.66 present wins. + renderTableRow(twoCellRow({ bottom: { style: 'single', width: 1, color: '#000000' } }, bottomOnly) as never); + + expect(renderTableCellMock).toHaveBeenCalledTimes(2); + const secondCall = renderTableCellMock.mock.calls[1][0] as { borders?: { left?: unknown } }; + expect(secondCall.borders?.left).toBeDefined(); + }); + it('does not paint interior bottom border for explicit cell borders in collapsed mode on non-final row', () => { const explicit = { top: { style: 'single' as const, width: 2, color: '#123456' }, diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts index a289f389da..01c7886c92 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts @@ -17,6 +17,7 @@ import { resolveBorderConflict, hasExplicitCellBorders, isPresentBorder, + isExplicitNoneBorder, swapCellBordersLR, } from './border-utils.js'; import { getTableCellGridBounds, type TableCellGridPosition } from './grid-geometry.js'; @@ -53,6 +54,13 @@ type CellBorderResolutionArgs = { * must suppress its top border to avoid a doubled line. (SD-3345) */ deferTopToAboveCell?: boolean; + /** + * True when the row BELOW has a tblPrEx border override that suppresses its shared horizontal + * edge (insideH none/nil). The lower cell owns that edge but won't draw it, so a present + * table/style border on THIS row must be drawn here to close the grid (§17.4.61/§17.4.66). + * (SD-3028) + */ + nextRowSuppressesSharedTop?: boolean; }; const hasAnyResolvedBorder = (borders: CellBorders): boolean => @@ -78,6 +86,7 @@ const resolveRenderedCellBorders = ({ rightCellBorders, nextRowLeavesRightGap, deferTopToAboveCell, + nextRowSuppressesSharedTop, }: CellBorderResolutionArgs): CellBorders | undefined => { const hasExplicitBorders = hasExplicitCellBorders(cellBorders); @@ -116,7 +125,13 @@ const resolveRenderedCellBorders = ({ ? resolveTableBorderValue(cb.top, tableBorders?.top) : deferTopToAboveCell ? undefined - : (resolveBorderConflict(cb.top, aboveCellBorders?.bottom) ?? borderValueToSpec(tableBorders?.insideH)), + : (resolveBorderConflict(cb.top, aboveCellBorders?.bottom) ?? + // Both sides not present: an explicit nil on BOTH adjacent cells suppresses the + // shared horizontal edge (§17.4.66); only inherit the table insideH when at least + // one side is merely unset. (SD-3028) + (isExplicitNoneBorder(cb.top) && isExplicitNoneBorder(aboveCellBorders?.bottom) + ? undefined + : borderValueToSpec(tableBorders?.insideH))), // Vertical interior edges: when BOTH adjacent cells declare a border, the right cell // owns it (draws its left as the §17.4.66 winner) so the edge is painted once (no // doubling). When only ONE side declares a border (asymmetric, no doubling risk) that @@ -128,7 +143,12 @@ const resolveRenderedCellBorders = ({ ? (resolveBorderConflict(cb.left, leftCellBorders?.right) ?? borderValueToSpec(tableBorders?.insideV)) : isPresentBorder(leftCellBorders?.right) ? undefined - : borderValueToSpec(tableBorders?.insideV), + : // Both sides not present: an explicit nil on BOTH adjacent cells suppresses the + // divider (§17.4.66); only fall back to the table insideV when at least one side + // is merely unset (and would inherit it). (SD-3028) + isExplicitNoneBorder(cb.left) && isExplicitNoneBorder(leftCellBorders?.right) + ? undefined + : borderValueToSpec(tableBorders?.insideV), right: cellBounds.touchesRightEdge ? resolveTableBorderValue(cb.right, tableBorders?.right) : isPresentBorder(cb.right) && !isPresentBorder(rightCellBorders?.left) @@ -181,10 +201,16 @@ const resolveRenderedCellBorders = ({ const baseBorders = resolveTableCellBorders(tableBorders, cellPosition); + // The row below owns this interior bottom edge, but if its tblPrEx override suppresses it + // (insideH none), draw this row's own present interior horizontal border so the grid still + // closes. (SD-3028) + const insideHSpec = borderValueToSpec(tableBorders.insideH); + const interiorBottom = nextRowSuppressesSharedTop && isPresentBorder(insideHSpec) ? insideHSpec : baseBorders.bottom; + return { top: touchesTopBoundary ? borderValueToSpec(tableBorders.top) : baseBorders.top, right: baseBorders.right, - bottom: touchesBottomBoundary ? borderValueToSpec(tableBorders.bottom) : baseBorders.bottom, + bottom: touchesBottomBoundary ? borderValueToSpec(tableBorders.bottom) : interiorBottom, left: baseBorders.left, }; }; @@ -211,6 +237,9 @@ type TableRowRenderDependencies = { /** Previous (above) row data + measure, for collapsed-border conflict resolution (§17.4.66). */ prevRow?: TableRow; prevRowMeasure?: TableRowMeasure; + /** Next (below) row data, to detect a row-level border override that suppresses the shared + * horizontal edge so the current row closes the grid itself (§17.4.61/§17.4.66). */ + nextRow?: TableRow; /** Next (below) row measure, to detect a gridAfter gap under a spanning cell (SD-3345). */ nextRowMeasure?: TableRowMeasure; /** Total number of rows in the table (for border resolution) */ @@ -327,6 +356,7 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { row, prevRow, prevRowMeasure, + nextRow, nextRowMeasure, totalRows, tableBorders, @@ -373,6 +403,20 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { ? { ...(tableBorders ?? {}), ...rowBorderOverride } : tableBorders; + // When the NEXT row carries a tblPrEx override that suppresses its shared horizontal edge + // (insideH = none/nil), the lower cell — which owns that edge in the single-owner model — + // won't draw it, and a table/style-derived border above (no cell tcBorder for the SD-2969 + // neighbor path to pick up) would be dropped. Per §17.4.66 a present border beats the + // none, so THIS row must close the grid by drawing its own interior bottom. Gated on the + // next row actually having an override, so unoverridden tables are unchanged (no doubling). + // (SD-3028) + const nextRowBorderOverride = nextRow?.attrs?.borders; + const nextRowEffectiveInsideH = nextRowBorderOverride + ? ({ ...(tableBorders ?? {}), ...nextRowBorderOverride } as TableBorders).insideH + : undefined; + const nextRowSuppressesSharedTop = + nextRowBorderOverride !== undefined && !isPresentBorder(borderValueToSpec(nextRowEffectiveInsideH)); + /** * Calculates the horizontal position (x-coordinate) for a cell based on its grid column index. * @@ -551,6 +595,7 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { rightCellBorders, nextRowLeavesRightGap, deferTopToAboveCell, + nextRowSuppressesSharedTop, }); // RTL: swap resolved left↔right so CSS properties match visual edges const finalBorders = isRtl && resolvedBorders ? swapCellBordersLR(resolvedBorders) : resolvedBorders; From 8f7db8310bb2d24256364d804dccda122763f117 Mon Sep 17 00:00:00 2001 From: Tadeu Tupinamba Date: Wed, 3 Jun 2026 13:11:49 -0300 Subject: [PATCH 6/6] fix(table): stop doubled borders on rowspan continuation rows In the single-owner collapsed model, two geometry helpers decide who owns a shared interior edge by walking ONE row's measure: `rowRightEdgeCol` (the row's rightmost occupied column, used for right-border ownership and the gridAfter right edge) and `nextRowMaxCol` (used by the SD-3345 `nextRowLeavesRightGap` check). A row's measure only lists cells that START in that row, so on a w:vMerge (rowspan) continuation row the columns held by a cell spanning from above look empty. Both helpers then undercounted, which: - made a leftmost cell on a continuation row look like the rightmost column, so it drew the table/style insideV as its right border (doubling the gray column's left), and - made a rowspan-covered column below a spanning cell look like a gridAfter gap, so the spanning cell drew its own bottom while the cell below still drew its top (doubling the horizontal divider). Visible in sd-1797-autofit-tables.docx (PCI assessment template): the gray "Identify/Describe" column showed 2px interior lines where Word draws 1px. Fix: compute per-row occupied grid columns INCLUDING cells that span into the row via rowspan, and feed that to both helpers. Tables without rowspans are unchanged (occupancy equals the row's own cells), and genuine gridAfter gaps still draw the spanning cell's bottom. Verified zero doubled edges across the fixture (112 cells) and adds 3 regression tests (incl. SD-3345 preserved). --- .../dom/src/table/renderTableFragment.ts | 23 ++++++++ .../dom/src/table/renderTableRow.test.ts | 54 +++++++++++++++++++ .../painters/dom/src/table/renderTableRow.ts | 34 +++++++++--- 3 files changed, 105 insertions(+), 6 deletions(-) diff --git a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts index 4a7d79beba..9200c1ada9 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableFragment.ts @@ -435,6 +435,25 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement return r?.height ?? 0; }); + // Per-row rightmost occupied grid column (exclusive), INCLUDING cells that span into a row + // via w:vMerge (rowspan) from an earlier row. A single row's measure only lists the cells + // that START in that row, so on a rowspan-continuation row the columns held by a spanning + // cell look empty. The single-owner edge helpers (rowRightEdgeCol / nextRowMaxCol) would + // then undercount and treat a leftmost cell as the rightmost column (drawing an interior + // right border) or treat a covered column as a gridAfter gap (drawing an interior bottom), + // doubling the shared edge. Counting rowspan occupancy keeps those edges single-owned. + // (SD-1797) + const rowOccupiedRightCols: number[] = new Array(measure.rows.length).fill(0); + measure.rows.forEach((rowM, r) => { + for (const c of rowM?.cells ?? []) { + const right = (c.gridColumnStart ?? 0) + (c.colSpan ?? 1); + const lastRow = Math.min(measure.rows.length - 1, r + (c.rowSpan ?? 1) - 1); + for (let rr = r; rr <= lastRow; rr += 1) { + if (right > rowOccupiedRightCols[rr]) rowOccupiedRightCols[rr] = right; + } + } + }); + // First row starts after space before table content (space between table border and first row) let y = cellSpacingPx; @@ -456,6 +475,8 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, nextRow: r < block.rows.length - 1 ? block.rows[r + 1] : undefined, nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, + rowOccupiedRightCol: rowOccupiedRightCols[r], + nextRowOccupiedRightCol: rowOccupiedRightCols[r + 1], totalRows: block.rows.length, tableBorders, columnWidths: effectiveColumnWidths, @@ -627,6 +648,8 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement prevRowMeasure: r > 0 ? measure.rows[r - 1] : undefined, nextRow: r < block.rows.length - 1 ? block.rows[r + 1] : undefined, nextRowMeasure: r < block.rows.length - 1 ? measure.rows[r + 1] : undefined, + rowOccupiedRightCol: rowOccupiedRightCols[r], + nextRowOccupiedRightCol: rowOccupiedRightCols[r + 1], totalRows: block.rows.length, tableBorders, columnWidths: effectiveColumnWidths, diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts index f2eefb55f1..7062f0569b 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.test.ts @@ -285,6 +285,60 @@ describe('renderTableRow', () => { expect(secondCall.borders?.left).toBeDefined(); }); + // SD-1797: a single row's measure only lists cells that START in it, so on a w:vMerge + // (rowspan) continuation row the columns held by a cell spanning from above look empty. + // `rowOccupiedRightCol` / `nextRowOccupiedRightCol` count that occupancy so the single-owner + // edge ownership doesn't misfire (a leftmost cell drawing a right border, or a covered column + // mistaken for a gridAfter gap) and double the shared edge. + const sparseRow = (overrides: Record = {}) => + createDeps({ + rowIndex: 2, + totalRows: 6, + cellSpacingPx: 0, + columnWidths: [100, 100, 100, 100], + rowMeasure: { height: 20, cells: [{ width: 100, height: 20, gridColumnStart: 0, colSpan: 1, rowSpan: 1 }] }, + row: { id: 'row-x', cells: [{ id: 'c', blocks: [{ kind: 'paragraph', id: 'p', runs: [] }] }] }, + ...overrides, + }); + + it('does not draw a right border on a leftmost cell of a rowspan-continuation row', () => { + // Columns 1-3 are covered by a vMerge cell spanning from above (rowOccupiedRightCol = 4), + // so this leftmost cell is NOT the rightmost column and must not draw insideV as its right. + renderTableRow(sparseRow({ rowOccupiedRightCol: 4 }) as never); + + const call = getRenderedCellCall(); + expect(call.borders?.right).toBeUndefined(); + expect(call.borders?.left).toBeDefined(); + }); + + it('does not treat a rowspan-covered column below a spanning cell as a gridAfter gap', () => { + // A cell spanning all 4 columns; the row below is fully covered (occupancy 4) -> no gap, + // so the spanning cell does NOT draw its own bottom (the cell below owns the shared edge). + renderTableRow( + sparseRow({ + rowMeasure: { height: 20, cells: [{ width: 400, height: 20, gridColumnStart: 0, colSpan: 4, rowSpan: 1 }] }, + nextRowOccupiedRightCol: 4, + }) as never, + ); + + const call = getRenderedCellCall(); + expect(call.borders?.bottom).toBeUndefined(); + }); + + it('still treats a genuine gridAfter gap as a bottom boundary (SD-3345 preserved)', () => { + // The cell spans all 4 columns but the row below only reaches column 2 (real gridAfter gap), + // so the spanning cell must draw its own bottom across the uncovered span. + renderTableRow( + sparseRow({ + rowMeasure: { height: 20, cells: [{ width: 400, height: 20, gridColumnStart: 0, colSpan: 4, rowSpan: 1 }] }, + nextRowOccupiedRightCol: 2, + }) as never, + ); + + const call = getRenderedCellCall(); + expect(call.borders?.bottom).toBeDefined(); + }); + it('does not paint interior bottom border for explicit cell borders in collapsed mode on non-final row', () => { const explicit = { top: { style: 'single' as const, width: 2, color: '#123456' }, diff --git a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts index 01c7886c92..6db797374d 100644 --- a/packages/layout-engine/painters/dom/src/table/renderTableRow.ts +++ b/packages/layout-engine/painters/dom/src/table/renderTableRow.ts @@ -242,6 +242,16 @@ type TableRowRenderDependencies = { nextRow?: TableRow; /** Next (below) row measure, to detect a gridAfter gap under a spanning cell (SD-3345). */ nextRowMeasure?: TableRowMeasure; + /** + * Rightmost occupied grid column (exclusive) for THIS row, counting cells that span into it + * via w:vMerge (rowspan) from an earlier row. Falls back to this row's own cells when absent. + * Prevents a leftmost cell on a rowspan-continuation row from being treated as the rightmost + * column. (SD-1797) + */ + rowOccupiedRightCol?: number; + /** Same as {@link rowOccupiedRightCol} for the NEXT row, so a rowspan continuation below is + * not mistaken for a gridAfter gap (which would double the shared bottom edge). (SD-1797) */ + nextRowOccupiedRightCol?: number; /** Total number of rows in the table (for border resolution) */ totalRows: number; /** Table-level borders (for resolving cell borders) */ @@ -358,6 +368,8 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { prevRowMeasure, nextRow, nextRowMeasure, + rowOccupiedRightCol, + nextRowOccupiedRightCol, totalRows, tableBorders, columnWidths, @@ -390,9 +402,14 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { // border at the rightmost cell, treating gridAfter columns as outside the box. // Use the last occupied column as the right edge; for rows without gridAfter // this equals totalCols (no change). - const rowRightEdgeCol = rowMeasure.cells.length - ? Math.min(totalCols, Math.max(...rowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1)))) - : totalCols; + // Prefer the rowspan-aware occupied width (counts cells spanning into this row via vMerge); + // fall back to this row's own cells when the caller doesn't provide it. (SD-1797, SD-3345) + const rowRightEdgeCol = + rowOccupiedRightCol != null && rowOccupiedRightCol > 0 + ? Math.min(totalCols, rowOccupiedRightCol) + : rowMeasure.cells.length + ? Math.min(totalCols, Math.max(...rowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1)))) + : totalCols; // Row-level border override (OOXML w:tblPrEx/w:tblBorders, §17.4.61). When this // row carries its own borders, they override the table borders for this row only, @@ -531,9 +548,14 @@ export const renderTableRow = (deps: TableRowRenderDependencies): void => { // Rightmost grid column (exclusive) covered by the next row's REAL cells. When a spanning // cell's right edge exceeds this, the next row has a gridAfter spacer beneath it and can't // own the shared bottom edge across the uncovered span. (SD-3345) - const nextRowMaxCol = nextRowMeasure?.cells?.length - ? Math.max(...nextRowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1))) - : Infinity; + // Rowspan-aware occupied width of the next row (counts cells spanning into it); fall back to + // the next row's own cells. A covered column must not look like a gridAfter gap. (SD-1797) + const nextRowMaxCol = + nextRowOccupiedRightCol != null && nextRowOccupiedRightCol > 0 + ? nextRowOccupiedRightCol + : nextRowMeasure?.cells?.length + ? Math.max(...nextRowMeasure.cells.map((c) => (c.gridColumnStart ?? 0) + (c.colSpan ?? 1))) + : Infinity; for (let cellIndex = 0; cellIndex < rowMeasure.cells.length; cellIndex += 1) { const cellMeasure = rowMeasure.cells[cellIndex];