feat: rtl for tables#3227
Conversation
874e31e to
688659e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
1c72d03 to
460369c
Compare
9900ff0 to
fc32d1d
Compare
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @artem-harbour - the resize handle metadata + RTL drag flip math are clean, and the click-fallback / Tab tests pin the interaction surface well :)
Important
looks right for the resize/click/nav surface this PR scopes. 4 pre-existing rendering bugs surfaced when i instrumented the new fixtures against Word; they're out of scope here but block the "RTL tables done" claim.
bugs: bidiVisual tables default left-aligned (Word right-aligns), outer L/R table borders never render, cell padding ignores start/end margins, cell-level <w:tcBorders><w:start>/<w:end> dropped entirely. test coverage is the bigger gap (arrow keys in RTL cells, Tab/Shift+Tab boundary cases, resize positive-width assertions, layout/visual baselines for the new fixtures).
i pushed a follow-up commit scoping right-edge resize affectedColumns to one column (avoids rewriting column 1 cell tcW), plus 5 ECMA §17.4 fixtures in R2 as rendering/sd-2810-rtl-table-*.docx.
screenshot: Word vs SuperDoc side-by-side on rtl-table-tcborders-startend.docx (Word renders red start border on visual right + blue end border on visual left; SuperDoc renders neither).
agent context
{
"schema": "review_handoff_v1",
"review_id": "pr-3227",
"findings": [
{
"id": "rtl-table-position",
"severity": "important",
"anchors": [{"file": "packages/layout-engine/layout-engine/src/layout-table.ts", "line": 184, "quote": "const justification = typeof attrs?.justification === 'string' ? attrs.justification : undefined;"}],
"claim": "resolveTableFrame ignores rightToLeft when justification is absent, so bidiVisual tables with no explicit w:jc default to left-aligned. Word defaults to right-aligned (start-aligned in RTL).",
"failure_mode": "User opens a bidiVisual table; SuperDoc places it on the left of the page, Word places it on the right.",
"verification": [{"command": "pnpm dev (port 9095) + load rendering/sd-2810-rtl-table-tblind.docx + measure fragment leftGap/rightGap vs page", "result_summary": "leftGap=193px, rightGap=383px (table closer to left); Word screenshot shows table on right with rightGap smaller than leftGap.", "status": "executed"}],
"ruled_out": [{"hypothesis": "Section-level w:bidi cascades to table alignment", "why_ruled_out": "Fixture has no section bidi; spec §17.6.1 says section bidi only affects section-level properties, not paragraph or table alignment."}],
"handoff": {"recommended_start": "layout-table.ts:177 resolveTableFrame", "suggested_fix": "After reading justification, derive effective = justification ?? (rightToLeft ? 'end' : undefined) and use that for the right-aligned branch.", "expected_files": ["packages/layout-engine/layout-engine/src/layout-table.ts", "packages/layout-engine/layout-engine/src/layout-table.test.ts"], "avoid_files": ["packages/layout-engine/painters/dom/src/table/renderTableFragment.ts"], "do_not_repeat": []},
"verification_after_fix": [{"command": "playwright test tables/rtl-table-spec-coverage.spec.ts", "expected": "new assertion: tableRect.right is within ~96px of pageRect.right on rtl-table-tblind.docx"}]
},
{
"id": "rtl-table-outer-borders",
"severity": "important",
"anchors": [{"file": "packages/layout-engine/painters/dom/src/table/renderTableFragment.ts", "line": 343, "quote": "applyBorder(container, 'Right', borderValueToSpec(isRtl ? tableBorders.left : tableBorders.right));"}],
"claim": "Outer left/right table borders never render on bidiVisual tables. The swap reads tableBorders.left/right but the importer may store start/end keys, leaving the swap pulling undefined.",
"failure_mode": "User opens a bidiVisual table with full tblBorders; SuperDoc shows only top/bottom + inner borders, Word renders all four outer sides.",
"verification": [{"command": "pnpm dev + load rendering/sd-2810-rtl-table-vmerge.docx + visual compare to Word", "result_summary": "Word renders left and right outer borders; SuperDoc does not.", "status": "executed"}],
"ruled_out": [{"hypothesis": "Cell-level borders are masking the table-level borders", "why_ruled_out": "Cells have no per-side borders defined; the comment at line 178 notes 'cells handle all borders' but cell-render is not applying them either."}],
"handoff": {"recommended_start": "super-converter/v3/handlers/w/tblBorders and the tableBorders key normalization", "suggested_fix": "Either normalize importer output to left/right physical keys, or change the swap to read tableBorders.start/end.", "expected_files": ["packages/layout-engine/painters/dom/src/table/renderTableFragment.ts", "packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tblBorders"], "avoid_files": [], "do_not_repeat": []},
"verification_after_fix": [{"command": "playwright test tables/rtl-table-spec-coverage.spec.ts", "expected": "new assertion: container has non-zero borderLeftWidth and borderRightWidth on rtl-table-vmerge.docx"}]
},
{
"id": "rtl-cell-padding",
"severity": "important",
"anchors": [{"file": "packages/layout-engine/style-engine/src/ooxml/index.ts", "line": 0, "quote": "cellMargins.marginStart / marginEnd resolution"}],
"claim": "Cells in bidiVisual tables render with less padding than Word. cellMargins.marginStart/marginEnd are not mapped to physical padding for RTL.",
"failure_mode": "User opens a bidiVisual table; SuperDoc cells look cramped vs Word's padded cells.",
"verification": [{"command": "visual compare rtl-table-col1-tcw-divergent.docx in SuperDoc vs Word", "result_summary": "Word shows ~8px padding all sides; SuperDoc shows minimal padding.", "status": "executed"}],
"ruled_out": [],
"handoff": {"recommended_start": "style-engine cellMargins resolver + painter cell padding application", "suggested_fix": "Map marginStart/marginEnd to marginLeft/marginRight based on table rightToLeft.", "expected_files": ["packages/layout-engine/style-engine/src/ooxml/index.ts", "packages/layout-engine/painters/dom/src/table/renderTableCell.ts"], "avoid_files": [], "do_not_repeat": []},
"verification_after_fix": [{"command": "playwright test tables/rtl-table-spec-coverage.spec.ts", "expected": "computed cell padding-left/padding-right match marginEnd/marginStart values respectively on bidiVisual tables"}]
},
{
"id": "rtl-tcborders-startend-dropped",
"severity": "important",
"anchors": [{"file": "packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tcBorders/tcBorders-translator.js", "line": 0, "quote": "tcBorders w:start / w:end parsing"}],
"claim": "Cell-level <w:tcBorders><w:start> and <w:end> render as no borders on bidiVisual tables. Fixture has start=#FF0000 24sz, end=#0000FF 24sz; SuperDoc paints neither.",
"failure_mode": "User opens a bidiVisual table cell with start/end borders; Word shows them on the correct visual sides (start=right, end=left in RTL), SuperDoc shows none.",
"verification": [{"command": "pnpm dev + load rendering/sd-2810-rtl-table-tcborders-startend.docx + getComputedStyle on cell + walk ancestors for any colored border", "result_summary": "No element in the cell subtree has border-left-color or border-right-color matching #FF0000 or #0000FF; Word screenshot shows both clearly.", "status": "executed"}],
"ruled_out": [{"hypothesis": "Painter renders borders as absolutely-positioned overlay divs", "why_ruled_out": "queried all div descendants with position:absolute + non-default background-color inside the fragment; result was empty array."}],
"handoff": {"recommended_start": "tcBorders-translator.js + renderTableCell.ts border application", "suggested_fix": "Confirm start/end are preserved through the parser, then map them to visual edges using table rightToLeft in the painter.", "expected_files": ["packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/tcBorders/tcBorders-translator.js", "packages/layout-engine/painters/dom/src/table/renderTableCell.ts"], "avoid_files": ["packages/layout-engine/painters/dom/src/table/renderTableFragment.ts"], "do_not_repeat": []},
"verification_after_fix": [{"command": "un-fixme tables/rtl-table-spec-coverage.spec.ts cell-borders test", "expected": "cell on visual right has border-right-color rgb(255,0,0); cell on visual left has border-left-color rgb(0,0,255)"}]
},
{
"id": "test-coverage-corner-cases",
"severity": "nit",
"anchors": [{"file": "tests/behavior/tests/tables", "line": 0, "quote": "table interaction suite"}],
"claim": "Corner cases of table interaction in RTL are unverified: arrow-key navigation between cells, Tab/Shift+Tab from last-visual-cell boundary, positive width assertion after right-edge drag.",
"failure_mode": "Regressions in RTL interaction won't be caught by the existing suite.",
"verification": [{"command": "grep tests/behavior/tests/tables for ArrowLeft/ArrowRight/ArrowUp/ArrowDown", "result_summary": "No specs exercise arrow-key navigation in tables under RTL context.", "status": "executed"}],
"ruled_out": [],
"handoff": {"recommended_start": "tests/behavior/tests/tables - new specs against the 5 R2 fixtures", "suggested_fix": "Add specs for ArrowLeft/Right cell navigation in RTL, Tab from last visual cell, resize positive-width assertion. Land layout-compare + visual baselines for the 5 new corpus entries.", "expected_files": ["tests/behavior/tests/tables", "tests/visual/tests/rendering", "tests/layout"], "avoid_files": [], "do_not_repeat": []},
"verification_after_fix": [{"command": "playwright test tables", "expected": "new specs pass against current bidiVisual rendering after the 4 rendering bugs above are addressed"}]
}
]
}8aaf2fe to
4b3f567
Compare
…3263) * refactor: read paragraph direction from directionContext (SD-2777) Migrates Phase A paragraph consumers onto Wave 1a's resolver context (SD-2776). Adds getParagraphInlineDirection helper in @superdoc/contracts that reads directionContext.inlineDirection first, with a compat fallback to legacy attrs.direction / attrs.dir / attrs.rtl / paragraphProperties.rightToLeft until SD-2778 collapses the duplicates. Behavior is unchanged because pm-adapter still mirrors directionContext.inlineDirection onto the legacy fields, so hash output stays identical. Phase A scope (consumers, paragraph-axis only): - layout-bridge: paragraph-hash-utils, cache (paragraph + cell sites) - layout-resolved: versionSignature (block + cell sites) - painter-dom: renderer (block + cell hash sites) - super-editor: listBoundaryNavigationPlugin Out of scope (deferred): - Table-axis consumers (renderTableFragment / renderTableCell / renderTableRow). Wait for PR #3227 to settle since it overlaps these. - SD-2779 rename of the rtl-paragraph DomPainter feature module. Kept separate to avoid diff noise. - ParagraphNodeView.inferParagraphRtlFromRuns - heuristic violates the resolver's no-infer-from-runs rule; removing it is a behavior change. * chore(comments): apply comment-policy fixes to Phase A - Drop paraphrase from three `// Direction` section labels in layout-bridge. The label now matches the surrounding `// Shading` / `// Tabs` neighbors; the helper's name + JSDoc already document the read order. - Reshape the compat-fallback comment on getParagraphInlineDirection into the policy's canonical `AIDEV-NOTE: compat-fallback - <trigger>. Retire once <condition>.` form per comment-policy.md. No behavior change. * test(pm-adapter): pin direction <-> directionContext.inlineDirection invariant (SD-2777) Verifies the load-bearing property of the Phase A migration: pm-adapter must write `paragraphAttrs.direction` and `paragraphAttrs.directionContext.inlineDirection` as either the same value or both undefined. This is what makes `getParagraphInlineDirection(attrs)` produce a hash byte-identical to the legacy `attrs.direction` read on FlowBlock attrs. Layout-compare against 462 R2 docs returned 0 changed snapshots; this test codifies the invariant so a future change to the writer can't silently break hash stability. Also rename `getParagraphInlineDirection` test 'returns undefined when directionContext is present with no inlineDirection' to 'falls back past directionContext when inlineDirection is null' so the title matches the assertion. * refactor: migrate remaining FlowBlock attrs direction reads to helper (SD-2777) Catches four `attrs?.direction === 'rtl'` reads the original audit's regex missed (the optional-chain form `attrs?.direction` didn't match `attrs\.direction`): - painters/dom/src/renderer.ts:3182, 3289 - render-path isRtl lookups - painters/dom/src/features/rtl-paragraph/rtl-styles.ts - isRtlParagraph helper - super-editor/.../CaretGeometry.ts - caret RTL adjustment Behavior is unchanged: the pm-adapter invariant test added in the previous commit proves `direction` and `directionContext.inlineDirection` are always paired (or both undefined). These call sites resolve to the same value via the helper as they did via the legacy scalar. Caught via codex-bot review comment on PR #3263 plus a broader grep for `paragraphProperties.rightToLeft` reads. Two upstream consumers remain out of scope: `headless-toolbar/helpers/ paragraph.ts` and `extensions/text-align/text-align.js` read `paragraphProperties.rightToLeft` at the style-resolved layer, upstream of pm-adapter where directionContext doesn't exist yet.
…dex 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.
…overage 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.
…de without cellSpacin
4b3f567 to
09fa2ed
Compare
…ghten 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.
…uble-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.
…idSpan 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).
…r 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).
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.
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.
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @artem-harbour! the RTL table work is in good shape :) - verified the new fixtures against dev.
i pushed follow-ups: dropped the double-mirror in pm-adapter (painter is now the sole RTL swap), restored a missing legacy border-style normalizer, and added asymmetric fixtures + TableResizeOverlay RTL tests.
the tcMar fixme exposes a real importer gap (style-engine projection drops marginStart/marginEnd) - tracked under SD-2771.
lgtm.
|
🎉 This PR is included in vscode-ext v2.3.0-next.132 |
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.87 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.130 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.102 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.86 |
|
🎉 This PR is included in superdoc v1.30.0-next.84 The release is available on GitHub release |
#3272) * fix(rtl-tables): drop double-mirror for logical cell margins (SD-3134) Two paths needed alignment with the borders fix from #3227: 1. getTableCellMargins (super-converter) only iterated left/right/top/bottom, so inline w:tcMar/w:start and w:end never projected to top-level cellMargins. Extend the iteration to add marginStart/marginEnd, preserving the OOXML logical names. 2. convertCellMarginsToPx (pm-adapter table-styles) still pre-swapped marginStart/marginEnd for RTL via an isRtlTable param. DomPainter already mirrors padding for bidiVisual tables, so the table-style hydration path was double-mirroring. Remove the pre-swap and the param; pm-adapter now stores logical start/end LTR-default. Same rule the borders fix encodes: importer/style-engine preserve OOXML logical names, pm-adapter maps logical to LTR-default physical, DomPainter performs the single visual RTL mirror. Flip the bad RTL unit test that was locking in the swap. Unfixme the behavior regression test at tests/behavior/tests/tables/ rtl-bidivisual-asymmetric.spec.ts. Verified: - pm-adapter tests: 201 pass (table-styles + borders + table) - super-converter tests: 2889 pass - Playwright RTL bidiVisual asymmetric spec: 12 pass across chromium/firefox/webkit (tcMar test now passes for real, no fixme) * fix(rtl-tables): resolve tcMar precedence over table-level defaults (SD-3134) Follow-up to the original fix in this PR. A second-round review caught a case the original change still got wrong: when a table has tblCellMar with physical w:left/w:right defaults and a cell has inline tcMar with logical w:start/w:end, the cell-level exception was silently dropped. Root cause: the importer stored physical defaults in result.left/right and logical inline values in result.marginStart/marginEnd. Downstream extractCellPadding gave physical precedence (padding.left was already non-null), so the inline w:start/w:end never reached the painter. Per ECMA-376 section 17.4.68: "This setting [tcMar], if present, shall override the table cell margins from the table-level cell margins". Rewrite getTableCellMargins to resolve precedence inside the importer: cell-level inline (physical or logical) wins over table-level defaults (physical or logical). Output is a single physical-shaped object with LTR-default semantic; DomPainter still owns the visual RTL mirror. Verified with a Word-native fixture (table-level w:tblCellMar w:left=40 and w:right=40 plus cell-level w:tcMar w:start=480 and w:end=60) loaded in the dev app: - before: paddingLeft=2.667, paddingRight=2.667 (table defaults; bug) - after: paddingLeft=32, paddingRight=4 (cell exception; correct) Tests: - pm-adapter: 213 pass - super-converter: 2889 pass - behavior spec rtl-bidivisual-asymmetric: 15 pass across chromium / firefox / webkit (new precedence test + 4 existing ones) * refactor(rtl-tables): drop dead defensive branch + freshen test comment Comment-audit follow-up on SD-3134: - readMargin had a defensive branch for a nested { value: { value, type } } shape, but no super-converter or layout-engine translator emits that shape. Per CLAUDE.md (do not add error handling for scenarios that cannot happen) the branch and its vague "some translators emit" comment are removed. - The regression note on the round-1 tcMar behavior test described the round-1 importer output (marginStart/marginEnd keys), which no longer matches the round-2 output (resolved physical left/right). Refreshed so future agents do not look for keys that no longer exist. No behavior change. Tests still pass.
…uperdoc-dev#3263) * refactor: read paragraph direction from directionContext (SD-2777) Migrates Phase A paragraph consumers onto Wave 1a's resolver context (SD-2776). Adds getParagraphInlineDirection helper in @superdoc/contracts that reads directionContext.inlineDirection first, with a compat fallback to legacy attrs.direction / attrs.dir / attrs.rtl / paragraphProperties.rightToLeft until SD-2778 collapses the duplicates. Behavior is unchanged because pm-adapter still mirrors directionContext.inlineDirection onto the legacy fields, so hash output stays identical. Phase A scope (consumers, paragraph-axis only): - layout-bridge: paragraph-hash-utils, cache (paragraph + cell sites) - layout-resolved: versionSignature (block + cell sites) - painter-dom: renderer (block + cell hash sites) - super-editor: listBoundaryNavigationPlugin Out of scope (deferred): - Table-axis consumers (renderTableFragment / renderTableCell / renderTableRow). Wait for PR superdoc-dev#3227 to settle since it overlaps these. - SD-2779 rename of the rtl-paragraph DomPainter feature module. Kept separate to avoid diff noise. - ParagraphNodeView.inferParagraphRtlFromRuns - heuristic violates the resolver's no-infer-from-runs rule; removing it is a behavior change. * chore(comments): apply comment-policy fixes to Phase A - Drop paraphrase from three `// Direction` section labels in layout-bridge. The label now matches the surrounding `// Shading` / `// Tabs` neighbors; the helper's name + JSDoc already document the read order. - Reshape the compat-fallback comment on getParagraphInlineDirection into the policy's canonical `AIDEV-NOTE: compat-fallback - <trigger>. Retire once <condition>.` form per comment-policy.md. No behavior change. * test(pm-adapter): pin direction <-> directionContext.inlineDirection invariant (SD-2777) Verifies the load-bearing property of the Phase A migration: pm-adapter must write `paragraphAttrs.direction` and `paragraphAttrs.directionContext.inlineDirection` as either the same value or both undefined. This is what makes `getParagraphInlineDirection(attrs)` produce a hash byte-identical to the legacy `attrs.direction` read on FlowBlock attrs. Layout-compare against 462 R2 docs returned 0 changed snapshots; this test codifies the invariant so a future change to the writer can't silently break hash stability. Also rename `getParagraphInlineDirection` test 'returns undefined when directionContext is present with no inlineDirection' to 'falls back past directionContext when inlineDirection is null' so the title matches the assertion. * refactor: migrate remaining FlowBlock attrs direction reads to helper (SD-2777) Catches four `attrs?.direction === 'rtl'` reads the original audit's regex missed (the optional-chain form `attrs?.direction` didn't match `attrs\.direction`): - painters/dom/src/renderer.ts:3182, 3289 - render-path isRtl lookups - painters/dom/src/features/rtl-paragraph/rtl-styles.ts - isRtlParagraph helper - super-editor/.../CaretGeometry.ts - caret RTL adjustment Behavior is unchanged: the pm-adapter invariant test added in the previous commit proves `direction` and `directionContext.inlineDirection` are always paired (or both undefined). These call sites resolve to the same value via the helper as they did via the legacy scalar. Caught via codex-bot review comment on PR superdoc-dev#3263 plus a broader grep for `paragraphProperties.rightToLeft` reads. Two upstream consumers remain out of scope: `headless-toolbar/helpers/ paragraph.ts` and `extensions/text-align/text-align.js` read `paragraphProperties.rightToLeft` at the style-resolved layer, upstream of pm-adapter where directionContext doesn't exist yet.
* 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>
superdoc-dev#3272) * fix(rtl-tables): drop double-mirror for logical cell margins (SD-3134) Two paths needed alignment with the borders fix from superdoc-dev#3227: 1. getTableCellMargins (super-converter) only iterated left/right/top/bottom, so inline w:tcMar/w:start and w:end never projected to top-level cellMargins. Extend the iteration to add marginStart/marginEnd, preserving the OOXML logical names. 2. convertCellMarginsToPx (pm-adapter table-styles) still pre-swapped marginStart/marginEnd for RTL via an isRtlTable param. DomPainter already mirrors padding for bidiVisual tables, so the table-style hydration path was double-mirroring. Remove the pre-swap and the param; pm-adapter now stores logical start/end LTR-default. Same rule the borders fix encodes: importer/style-engine preserve OOXML logical names, pm-adapter maps logical to LTR-default physical, DomPainter performs the single visual RTL mirror. Flip the bad RTL unit test that was locking in the swap. Unfixme the behavior regression test at tests/behavior/tests/tables/ rtl-bidivisual-asymmetric.spec.ts. Verified: - pm-adapter tests: 201 pass (table-styles + borders + table) - super-converter tests: 2889 pass - Playwright RTL bidiVisual asymmetric spec: 12 pass across chromium/firefox/webkit (tcMar test now passes for real, no fixme) * fix(rtl-tables): resolve tcMar precedence over table-level defaults (SD-3134) Follow-up to the original fix in this PR. A second-round review caught a case the original change still got wrong: when a table has tblCellMar with physical w:left/w:right defaults and a cell has inline tcMar with logical w:start/w:end, the cell-level exception was silently dropped. Root cause: the importer stored physical defaults in result.left/right and logical inline values in result.marginStart/marginEnd. Downstream extractCellPadding gave physical precedence (padding.left was already non-null), so the inline w:start/w:end never reached the painter. Per ECMA-376 section 17.4.68: "This setting [tcMar], if present, shall override the table cell margins from the table-level cell margins". Rewrite getTableCellMargins to resolve precedence inside the importer: cell-level inline (physical or logical) wins over table-level defaults (physical or logical). Output is a single physical-shaped object with LTR-default semantic; DomPainter still owns the visual RTL mirror. Verified with a Word-native fixture (table-level w:tblCellMar w:left=40 and w:right=40 plus cell-level w:tcMar w:start=480 and w:end=60) loaded in the dev app: - before: paddingLeft=2.667, paddingRight=2.667 (table defaults; bug) - after: paddingLeft=32, paddingRight=4 (cell exception; correct) Tests: - pm-adapter: 213 pass - super-converter: 2889 pass - behavior spec rtl-bidivisual-asymmetric: 15 pass across chromium / firefox / webkit (new precedence test + 4 existing ones) * refactor(rtl-tables): drop dead defensive branch + freshen test comment Comment-audit follow-up on SD-3134: - readMargin had a defensive branch for a nested { value: { value, type } } shape, but no super-converter or layout-engine translator emits that shape. Per CLAUDE.md (do not add error handling for scenarios that cannot happen) the branch and its vague "some translators emit" comment are removed. - The regression note on the round-1 tcMar behavior test described the round-1 importer output (marginStart/marginEnd keys), which no longer matches the round-2 output (resolved physical left/right). Refreshed so future agents do not look for keys that no longer exist. No behavior change. Tests still pass.
Linear: SD-2810
Phase 5 (RTL Table Stability) is now completed for RTL table interaction and roundtrip reliability.
This PR does not introduce RTL tables from scratch; core support was already in place.
The main work here was stabilization and regression coverage:
bidiVisualpipeline behavior (<w:bidiVisual/>↔tableProperties.rightToLeft)Overall result: RTL tables are now covered by explicit tests across rendering, interaction, and export paths, with resize/drag behavior stabilized and regression-protected.