Skip to content

Commit 7364827

Browse files
artem-harbourArtem Nistuleycaio-pizzol
authored
feat: rtl for tables (#3227)
* feat: rtl for tables * fix(rtl-tables): emit logical resize delta and document right-edge index convention Two review-fix changes on the table-RTL stability work: 1. resize-move / resize-end events now emit the LOGICAL delta (the value applied to newWidths), restoring the pre-PR contract for external listeners (logging, analytics, undo metadata). The visual delta still lives on dragState.constrainedDelta for the in-flight preview guideline at line 591. LTR consumers see no behavior change; RTL inner-boundary consumers no longer see a sign-flipped payload. 2. Added a load-bearing comment at the right-edge boundary push explaining why the reported columnIndex is 0 in RTL and columns.length - 1 in LTR. Downstream consumers that need 'is this the outer edge?' should key off type === 'right-edge', not on the numeric column index. * fix(rtl-tables): scope right-edge resize affectedColumns + add spec-coverage fixtures Two changes: 1. dispatchResizeTransaction now passes isRightEdge and constrains affectedColumns to [columnIndex] on right-edge drags. Pre-fix, [columnIndex, columnIndex + 1] unconditionally rewrote the next column's per-cell w:tcW with the grid value, destroying authored divergent tcW on merged or width-overridden cells. LTR was unaffected because columnIndex + 1 was past the last column; in RTL the right-edge handle maps to column 0 so column 1 cells were real targets. updateCellColwidths's tableCellProperties.cellWidth write is unconditional, so the value goes through even when the new value matches the old. 2. Four ECMA-376 §17.4-aligned Word fixtures + a spec-coverage behavior suite: - rtl-table-col1-tcw-divergent.docx (column 1 cells have tcW=2400 diverging from grid=1200; contract-guard test asserts a right-edge drag leaves column 1 attrs byte-identical) - rtl-table-gridspan.docx (§17.4.17 gridSpan=2 cell in row 0) - rtl-table-vmerge.docx (§17.4.84 vMerge restart/continue across 3 rows; test currently fixme'd pending DOM-target investigation) - rtl-table-tblind.docx (§17.4.51 tblInd indent; test asserts table indents from the page's RIGHT edge in RTL per spec wording 'right edge in a right-to-left table') - rtl-table-tcborders-startend.docx (§17.4.66 cell with logical start/end borders; test fixme'd pending border DOM target) Two of the four spec-coverage tests pass and pin the gridSpan + tblInd RTL semantics. The other two are skipped with explicit TODO comments naming what needs DOM-target investigation; the fixtures are still useful substrate for visual-regression coverage. * fix(rtl-tables): apply rtl default table alignment * fix(rtl-tables): map tblBorders start/end to physical sides with rtl-aware fallback * fix(rtl-tables): map tcMar start/end to physical padding for rtl tables * fix(rtl-tables): preserve tcBorders start/end for rtl table cell border rendering * fix(rtl-tables): render outer table left/right borders in separate mode without cellSpacin * test(behavior): table tab navigation coverage * fix(rtl-tables): scope rtl arrow cell navigation to bidiVisual tables * fix(rtl-tables): fix RTL style padding, Shift+Arrow edge, and border typing * test(tables): align rtl tblInd spec assertion with right-anchored bidiVisual behavior * fix(rtl-tables): restore legacy border-style alias normalization + tighten tblInd test Two follow-up fixes from round-2 review: 1. `normalizeLegacyBorderStyle` removal (e11a430) dropped the case normalization for legacy persisted-cell border `val` strings. `convertBorderSpec` passes `val` through unchanged, so lowercase/alias forms (`dot`, `dotdash`, `dotdotdash`, `doublewave`) would have landed on the painter unrecognized. Re-add the normalizer as a private helper in table.ts and apply it in the legacy fallback path before `extractCellBorders`. Adds a unit test covering the alias mapping. 2. `rtl-table-spec-coverage.spec.ts` tblInd assertion was weakened to just `rightGap < leftGap` - true for any right-anchored RTL table, regardless of whether the 1-inch tblInd was actually applied. Adds a magnitude check (`leftGap - rightGap > 40px`) so the test fails if tblInd is silently ignored. The other round-2 reviewer findings were investigated and dropped: - "tcBorders/tcMar double-mirror" was a false positive: per Wave 1a's axis-separation rule, `bidiVisual` only flips cell ORDER, not the paragraph inline direction. The pm-adapter and painter swaps are both keyed off cell-paragraph direction, so they don't compound. Manual comparison against Word on all 5 R2 fixtures confirms parity. - "Legacy direction priority can flip explicit ltr" is unreachable per the SD-2777 pm-adapter pairing invariant test. - Arrow-nav assertion tightening would need targeted runtime verification that's out of scope for this incremental commit; left for follow-up. All 1828 pm-adapter tests pass including the new alias normalization test. * fix(rtl-tables): keep start/end LTR-default in pm-adapter to avoid double-mirror Per §17.4.33 "start (Table Cell Leading Edge Border) ... left for LTR tables, right for RTL tables" and §17.4.12 "end (Trailing Edge) ... right for LTR, left for RTL", the visual side for tcBorders / tblBorders / tcMar start/end is determined by *table* direction (bidiVisual), not paragraph direction. Round-2 pre-swapping these in pm-adapter based on `isRtl` was correct about the direction, but the DOM painter (renderTableRow.swapCellBordersLR, renderTableFragment.applyBorder swap, renderTableCell.ts paddingLeft/Right swap) also mirrors L<->R for RTL tables. Result: double-mirror — start landed on the wrong visual edge. Verified in the dev app loading rtl-table-tcborders-startend.docx: - Before fix: cell at visual right had border-left=RED (start) and border-right=BLUE (end) — wrong per §17.4.33. - After fix: border-right=RED (start, visual right) and border-left=BLUE (end, visual left) — matches Word and the spec. Changes: - extractTableBorders, extractCellBorders, extractCellPadding: drop the isRtl-driven swap. Always map start -> .left, end -> .right (LTR default). The `options` param is retained for backwards-compat callers but isRtl is no longer read. - table.ts: same fix for the resolved tableCellProperties.borders path. - Updated borders.test.ts and table.test.ts to assert the new LTR-default contract (and reference the painter as the single source of RTL mirror). The painter remains the single owner of RTL visual mirroring keyed off the table's `bidiVisual` flag. 1828 pm-adapter tests + 1070 painter-dom tests pass. * test(rtl-tables): expand RTL coverage for resize, tcBorders sides, gridSpan clicks Three additions filling test-coverage gaps surfaced by audit: TableResizeOverlay.test.js (was 1456 lines, ZERO rtl/bidiVisual mentions): 6 new tests under "RTL (bidiVisual) handling": - parses rtl:true from data-table-boundaries metadata - defaults rtl to false when omitted - right-edge boundary in RTL targets column index 0 (not last column) - right-edge in LTR targets columns.length-1 (regression guard) - inner boundary visual X is mirrored from logical X in RTL - right-edge X in RTL equals the table content width Catches: silent column-0 cellWidth rewrites on right-edge drag, inner boundary X-coord sign inversion, mismatched logical vs visual handle position in bidiVisual tables. rtl-table-tcborders-startend.spec.ts: tightened the width-only assertion to also check start (RED) lands on cell border-right and end (BLUE) on border-left per ECMA-376 §17.4.33 + §17.4.12. Width-only would have silently passed a double-mirror regression. rtl-table-spec-coverage.spec.ts: - Activated the previously fixme'd tcBorders color test now that the pm-adapter / painter mirror contract is settled. Fixed the DOM target (cell wrapper is the absolutely-positioned div, no .superdoc-table-cell class). Comment updated to cite the spec. - Added click-target coverage for gridSpan=2 cell: clicking the visually-rightmost merged cell lands the cursor inside that cell's paragraph. vMerge fixme left disabled (merged-span DOM target still not mapped) and comment-range RTL coverage deferred to SD-2771 Wave 3. 12785 super-editor tests + 5 of 6 touched Playwright specs pass on chromium (vMerge fixme skipped as designed). * test(rtl-tables): add asymmetric tblBorders / tcMar / gridBefore-After fixtures Three Word-native fixtures pinning ECMA-376 sides where the visual direction flips with table direction: - tblBorders/start (RED) end (BLUE) on visual right/left (section 17.4.38) - tcMar/start (480 dxa) end (60 dxa) on visual right/left (section 17.4.41) - gridBefore=1 and gridAfter=1 gaps on visual right/left (section 17.4.14/15) Tests assert the spec-mandated side per row, so a regression that double-mirrors or skips the mirror fails here instead of silently producing the LTR rendering. The tcMar test is fixme'd: importing tableCellProperties.cellMargins preserves marginStart/marginEnd, but the style-engine projection that writes top-level cellMargins only handles top/bottom. Test comment points at the projection step (follow-up under SD-2771). R2 corpus upload pending (wrangler token refresh). * test(rtl-tables): fix tcMar spec reference to section 17.4.68 Direct cell w:tcMar is section 17.4.68 (Single Table Cell Margins, child of w:tcPr). Section 17.4.41 is tblCellMar (Table Cell Margin Exceptions, child of w:tblPrEx) - a different element. Also reword the fixme comment to avoid paragraph-direction language. tcMar start/end follow table direction (same governance as tblBorders/start/end per section 17.4.33/12 and the leading-edge rule in section 17.4.15), not paragraph bidi. * test(rtl-tables): annotate tcMar fixme with AIDEV-NOTE issue anchor Per comment-policy.md, temporary/workaround comments need an AIDEV-NOTE anchor with a removal condition and issue id. Convert the TODO at the tcMar fixme to AIDEV-NOTE: temporary with SD-2771 as the gating issue. --------- Co-authored-by: Artem Nistuley <artem@superdoc.dev> Co-authored-by: Caio Pizzol <caio@superdoc.dev>
1 parent fd1a740 commit 7364827

38 files changed

Lines changed: 2198 additions & 101 deletions

packages/layout-engine/contracts/src/index.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -620,10 +620,16 @@ export type TableCellAttrs = {
620620
tableCellProperties?: Record<string, unknown>;
621621
};
622622

623+
export type TablePropertiesAttrs = {
624+
rightToLeft?: boolean;
625+
[key: string]: unknown;
626+
};
627+
623628
export type TableAttrs = {
624629
borders?: TableBorders;
625630
borderCollapse?: 'collapse' | 'separate';
626631
cellSpacing?: CellSpacing;
632+
tableProperties?: TablePropertiesAttrs;
627633
sdt?: SdtMetadata;
628634
containerSdt?: SdtMetadata;
629635
[key: string]: unknown;

packages/layout-engine/layout-engine/src/layout-table.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -181,16 +181,24 @@ export function resolveTableFrame(
181181
attrs: TableBlock['attrs'],
182182
): { x: number; width: number } {
183183
const width = resolveRenderedTableWidth(columnWidth, tableWidth, attrs);
184-
const justification = typeof attrs?.justification === 'string' ? attrs.justification : undefined;
184+
const explicitJustification = typeof attrs?.justification === 'string' ? attrs.justification : undefined;
185+
const isRtlTable = attrs?.tableProperties?.rightToLeft === true;
186+
const effectiveJustification = explicitJustification ?? (isRtlTable ? 'end' : undefined);
187+
const tableIndent = getTableIndentWidth(attrs);
185188

186-
if (justification === 'center') {
189+
if (effectiveJustification === 'center') {
187190
return { x: baseX + (columnWidth - width) / 2, width };
188191
}
189-
if (justification === 'right' || justification === 'end') {
190-
return { x: baseX + (columnWidth - width), width };
192+
if (effectiveJustification === 'right' || effectiveJustification === 'end') {
193+
const rightAlignedX = baseX + (columnWidth - width);
194+
// In bidiVisual tables with no explicit jc, Word keeps right alignment
195+
// and applies tblInd from the right edge.
196+
if (explicitJustification == null && isRtlTable && tableIndent !== 0) {
197+
return { x: rightAlignedX - tableIndent, width };
198+
}
199+
return { x: rightAlignedX, width };
191200
}
192201

193-
const tableIndent = getTableIndentWidth(attrs);
194202
return applyTableIndent(baseX, width, tableIndent, columnWidth);
195203
}
196204

packages/layout-engine/layout-engine/src/resolve-table-frame.test.ts

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -113,6 +113,29 @@ describe('resolveTableFrame', () => {
113113
const result = resolveTableFrame(0, 500, 600, { justification: 'end' } as TableAttrs);
114114
expect(result).toEqual({ x: -100, width: 600 });
115115
});
116+
117+
it('defaults bidiVisual tables without jc to right alignment', () => {
118+
const result = resolveTableFrame(0, 500, 300, {
119+
tableProperties: { rightToLeft: true },
120+
} as TableAttrs);
121+
expect(result).toEqual({ x: 200, width: 300 });
122+
});
123+
124+
it('applies tblInd from the right edge for bidiVisual tables without jc', () => {
125+
const result = resolveTableFrame(0, 500, 300, {
126+
tableIndent: { width: 40 },
127+
tableProperties: { rightToLeft: true },
128+
} as TableAttrs);
129+
expect(result).toEqual({ x: 160, width: 300 });
130+
});
131+
132+
it('keeps explicit left justification for bidiVisual tables', () => {
133+
const result = resolveTableFrame(0, 500, 300, {
134+
justification: 'left',
135+
tableProperties: { rightToLeft: true },
136+
} as TableAttrs);
137+
expect(result).toEqual({ x: 0, width: 300 });
138+
});
116139
});
117140

118141
describe('with pct tableWidth', () => {

packages/layout-engine/painters/dom/src/table/renderTableFragment.test.ts

Lines changed: 34 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,40 @@ describe('renderTableFragment', () => {
135135
expect(element.dataset.pmEnd).toBe('34');
136136
});
137137

138+
it('applies outer left/right borders in separate mode even when cellSpacing is unset or zero', () => {
139+
const block = createTestTableBlock();
140+
block.attrs = {
141+
borderCollapse: 'separate',
142+
borders: {
143+
left: { style: 'single', width: 2, color: '#ff0000' },
144+
right: { style: 'single', width: 3, color: '#0000ff' },
145+
},
146+
};
147+
148+
const element = renderTableFragment({
149+
doc,
150+
fragment: createTestTableFragment(),
151+
context,
152+
block,
153+
measure: createTestTableMeasure(),
154+
cellSpacingPx: 0,
155+
effectiveColumnWidths: [100],
156+
renderLine: () => doc.createElement('div'),
157+
applyFragmentFrame: () => {
158+
// Intentionally empty for test mock
159+
},
160+
applySdtDataset: () => {
161+
// Intentionally empty for test mock
162+
},
163+
applyStyles: () => {
164+
// Intentionally empty for test mock
165+
},
166+
});
167+
168+
expect(element.style.borderLeftWidth).toBe('2px');
169+
expect(element.style.borderRightWidth).toBe('3px');
170+
});
171+
138172
describe('merged-cell border ownership', () => {
139173
it('renders the outer right border for a merged header cell in collapsed mode', () => {
140174
const block: TableBlock = {

packages/layout-engine/painters/dom/src/table/renderTableFragment.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -306,6 +306,7 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement
306306
min: boundary.minWidth,
307307
r: boundary.resizable ? 1 : 0,
308308
})),
309+
rtl: isRtl,
309310
// Add segments for each column boundary (segments where resize handle should appear)
310311
segments: boundarySegments.map((segs, colIndex) =>
311312
segs.map((seg) => ({
@@ -337,7 +338,7 @@ export const renderTableFragment = (deps: TableRenderDependencies): HTMLElement
337338
}
338339

339340
const borderCollapse = block.attrs?.borderCollapse ?? (block.attrs?.cellSpacing != null ? 'separate' : 'collapse');
340-
if (borderCollapse === 'separate' && block.attrs?.cellSpacing && tableBorders) {
341+
if (borderCollapse === 'separate' && tableBorders) {
341342
applyBorder(container, 'Top', borderValueToSpec(tableBorders.top));
342343
applyBorder(container, 'Right', borderValueToSpec(isRtl ? tableBorders.left : tableBorders.right));
343344
applyBorder(container, 'Bottom', borderValueToSpec(tableBorders.bottom));

packages/layout-engine/pm-adapter/src/attributes/borders.test.ts

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -399,6 +399,46 @@ describe('SD-2343 - no double conversion for pre-converted px widths', () => {
399399
const result = convertBorderSpec({ val: 'single', size: 8 }, { unit: 'eighthPoints' });
400400
expect(result?.width).toBeCloseTo(1.3333, 4);
401401
});
402+
403+
it('maps logical start/end to left/right in LTR when physical sides are missing', () => {
404+
const input = {
405+
start: { val: 'single', size: 2, color: 'FF0000' },
406+
end: { val: 'double', size: 3, color: '0000FF' },
407+
};
408+
409+
const result = extractTableBorders(input, { isRtl: false });
410+
expect(result?.left?.style).toBe('single');
411+
expect((result?.left as { color?: string })?.color).toBe('#FF0000');
412+
expect(result?.right?.style).toBe('double');
413+
expect((result?.right as { color?: string })?.color).toBe('#0000FF');
414+
});
415+
416+
it('maps logical start/end as LTR-default regardless of isRtl flag (painter handles RTL mirror)', () => {
417+
const input = {
418+
start: { val: 'single', size: 2, color: 'FF0000' },
419+
end: { val: 'double', size: 3, color: '0000FF' },
420+
};
421+
422+
// Per §17.4.12 + §17.4.33 the visual side flips with table direction,
423+
// but the painter's swapTableBordersLR does that mirror once. pm-adapter
424+
// pre-swapping would double-mirror, so isRtl is no longer read here.
425+
const result = extractTableBorders(input, { isRtl: true });
426+
expect(result?.left?.style).toBe('single');
427+
expect((result?.left as { color?: string })?.color).toBe('#FF0000');
428+
expect(result?.right?.style).toBe('double');
429+
expect((result?.right as { color?: string })?.color).toBe('#0000FF');
430+
});
431+
432+
it('keeps explicit physical sides over logical start/end', () => {
433+
const input = {
434+
left: { val: 'single', size: 1, color: '00AA00' },
435+
start: { val: 'double', size: 5, color: 'FF0000' },
436+
};
437+
438+
const result = extractTableBorders(input, { isRtl: false });
439+
expect(result?.left?.style).toBe('single');
440+
expect((result?.left as { color?: string })?.color).toBe('#00AA00');
441+
});
402442
});
403443

404444
describe('extractCellBorders', () => {
@@ -452,6 +492,47 @@ describe('extractCellBorders', () => {
452492
expect(result?.top).toBeDefined();
453493
expect(result?.bottom).toEqual({ style: 'none', width: 0 });
454494
});
495+
496+
it('maps start/end to left/right in LTR when physical sides are missing', () => {
497+
const input = {
498+
borders: {
499+
start: { val: 'single', size: 2, color: 'FF0000' },
500+
end: { val: 'single', size: 3, color: '0000FF' },
501+
},
502+
};
503+
const result = extractCellBorders(input, { isRtl: false });
504+
expect(result?.left).toMatchObject({ style: 'single', width: 2, color: '#FF0000' });
505+
expect(result?.right).toMatchObject({ style: 'single', width: 3, color: '#0000FF' });
506+
});
507+
508+
it('maps start/end as LTR-default regardless of isRtl flag (painter handles RTL mirror)', () => {
509+
const input = {
510+
borders: {
511+
start: { val: 'single', size: 2, color: 'FF0000' },
512+
end: { val: 'single', size: 3, color: '0000FF' },
513+
},
514+
};
515+
// Per §17.4.12/33, end/start visual side flips with table direction, but
516+
// the painter's swapCellBordersLR is the single source of that mirror.
517+
// pm-adapter pre-swapping would double-mirror.
518+
const result = extractCellBorders(input, { isRtl: true });
519+
expect(result?.left).toMatchObject({ style: 'single', width: 2, color: '#FF0000' });
520+
expect(result?.right).toMatchObject({ style: 'single', width: 3, color: '#0000FF' });
521+
});
522+
523+
it('keeps explicit physical left/right over logical start/end', () => {
524+
const input = {
525+
borders: {
526+
left: { val: 'single', size: 7, color: '00FF00' },
527+
right: { val: 'single', size: 8, color: 'FFFF00' },
528+
start: { val: 'single', size: 2, color: 'FF0000' },
529+
end: { val: 'single', size: 3, color: '0000FF' },
530+
},
531+
};
532+
const result = extractCellBorders(input, { isRtl: true });
533+
expect(result?.left).toMatchObject({ style: 'single', width: 7, color: '#00FF00' });
534+
expect(result?.right).toMatchObject({ style: 'single', width: 8, color: '#FFFF00' });
535+
});
455536
});
456537

457538
describe('invalid inputs', () => {
@@ -536,6 +617,54 @@ describe('extractCellPadding', () => {
536617
left: 12.75,
537618
});
538619
});
620+
621+
it('maps marginStart/marginEnd to left/right in LTR when physical sides are missing', () => {
622+
const input = {
623+
cellMargins: {
624+
marginStart: 11,
625+
marginEnd: 22,
626+
},
627+
};
628+
const result = extractCellPadding(input, { isRtl: false });
629+
expect(result).toEqual({
630+
left: 11,
631+
right: 22,
632+
});
633+
});
634+
635+
it('maps marginStart/marginEnd as LTR-default regardless of isRtl flag (painter handles RTL mirror)', () => {
636+
const input = {
637+
cellMargins: {
638+
marginStart: 11,
639+
marginEnd: 22,
640+
},
641+
};
642+
// renderTableCell.ts mirrors paddingLeft <-> paddingRight when the
643+
// table is bidiVisual. pm-adapter must therefore keep marginStart/End
644+
// mapped to LTR-default (start->left, end->right) - otherwise the
645+
// painter double-mirrors and start padding lands on the visual left.
646+
const result = extractCellPadding(input, { isRtl: true });
647+
expect(result).toEqual({
648+
left: 11,
649+
right: 22,
650+
});
651+
});
652+
653+
it('keeps explicit physical left/right over logical marginStart/marginEnd', () => {
654+
const input = {
655+
cellMargins: {
656+
left: 33,
657+
right: 44,
658+
marginStart: 11,
659+
marginEnd: 22,
660+
},
661+
};
662+
const result = extractCellPadding(input, { isRtl: true });
663+
expect(result).toEqual({
664+
left: 33,
665+
right: 44,
666+
});
667+
});
539668
});
540669

541670
describe('invalid inputs', () => {

0 commit comments

Comments
 (0)