Skip to content

Commit a81be59

Browse files
authored
docs(direction): document visual mirror rule + drop dead isRtl args (SD-3134) (#3273)
Cleanup PR stacked on the SD-3134 fix. No behavior change. - Add a "Visual mirror rule (table-visual RTL axis)" section to pm-adapter/src/direction/README.md. Spells out the three-condition gate (table-scoped + logical-side language + painter mirrors), enumerates the OOXML properties it covers (tblBorders/tcBorders/ tcMar start/end + bidiVisual cell order + gridBefore/After), and the properties it does NOT cover (paragraph w:bidi, run w:rtl/dir/ bdo, w:textDirection, numbering w:start, editing-side visual-to- logical mapping). Future bidiVisual-axis features should follow the same pattern instead of re-introducing pre-mirrors. - Remove dead isRtl args at three call sites in pm-adapter/src/ converters/table.ts. The receivers in borders.ts already ignore the flag (the comments at lines 313 and 369 said so); the call sites were misleading. Removing them eliminates the trap where a future agent reads the call and assumes isRtl flows through. - Tighten the existing "ignored" comments on extractCellBorders, extractCellPadding, and BorderConversionOptions to be definite ("isRtl is IGNORED") and to reference direction/README.md. Extractor signatures keep the optional _options param for now to avoid churning ~6 test invocations across the three "regardless of isRtl flag" tests. A broader API trim can happen later. Tests: pm-adapter 213 pass (table-styles + borders + table). Stacked on caio-pizzol/SD-3134; retarget to main after that lands.
1 parent a9ab6f1 commit a81be59

3 files changed

Lines changed: 165 additions & 101 deletions

File tree

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

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@ const MAX_BORDER_SIZE_PX = 100; // Reasonable maximum
2525
type BorderConversionUnit = 'px' | 'eighthPoints';
2626
type BorderConversionOptions = {
2727
unit?: BorderConversionUnit;
28+
// isRtl is IGNORED here. pm-adapter stores logical sides as LTR-default;
29+
// DomPainter mirrors for bidiVisual tables. See direction/README.md
30+
// "Visual mirror rule". Retained as optional for older call sites.
2831
isRtl?: boolean;
2932
};
3033

@@ -269,8 +272,8 @@ export function extractTableBorders(
269272
// Logical start/end fallback when physical counterpart is missing. Map as
270273
// LTR-default (start->left, end->right). The DOM painter handles the RTL
271274
// visual mirror once via swapTableBordersLR / swapCellBordersLR keyed off
272-
// the table's bidiVisual flag (§17.4.12 + §17.4.33). Pre-swapping here
273-
// would double-mirror.
275+
// the table's bidiVisual flag (table borders: §17.4.36/13; cell borders:
276+
// §17.4.33/12). Pre-swapping here would double-mirror.
274277
if (borders.left == null) {
275278
assignConverted('left', bordersInput.start);
276279
}
@@ -310,9 +313,11 @@ type CellBorderExtractionOptions = {
310313

311314
export function extractCellBorders(
312315
cellAttrs: Record<string, unknown>,
313-
// options retained for backwards-compatible call sites; no longer reads isRtl
314-
// since pm-adapter maps start/end as LTR-default and the painter's
315-
// swapCellBordersLR handles the RTL mirror (§17.4.12 + §17.4.33).
316+
// isRtl on the options object is IGNORED. pm-adapter stores start/end as
317+
// LTR-default physical sides; the painter's swapCellBordersLR is the single
318+
// visual mirror for bidiVisual tables (§17.4.1 + §17.4.33/12).
319+
// See pm-adapter/src/direction/README.md "Visual mirror rule".
320+
// The optional param is retained for older call sites; do not read it.
316321
_options?: CellBorderExtractionOptions,
317322
): CellBorders | undefined {
318323
if (!cellAttrs?.borders) return undefined;
@@ -366,9 +371,12 @@ type CellPaddingExtractionOptions = {
366371

367372
export function extractCellPadding(
368373
cellAttrs: Record<string, unknown>,
369-
// options retained for backwards-compat; no longer reads isRtl. pm-adapter
370-
// maps marginStart/End as LTR-default; renderTableCell mirrors paddingLeft
371-
// <-> paddingRight for RTL tables (§17.4.42 + §17.4.13 on cell margins).
374+
// isRtl on the options object is IGNORED. pm-adapter maps marginStart/End
375+
// as LTR-default physical sides; renderTableCell mirrors paddingLeft <->
376+
// paddingRight for bidiVisual tables
377+
// (§17.4.1 + §17.4.42 + §17.4.68 + §17.4.35/10).
378+
// See pm-adapter/src/direction/README.md "Visual mirror rule".
379+
// The optional param is retained for older call sites; do not read it.
372380
_options?: CellPaddingExtractionOptions,
373381
): BoxSpacing | undefined {
374382
const cellMargins = cellAttrs?.cellMargins;

packages/layout-engine/pm-adapter/src/converters/table.ts

Lines changed: 4 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -563,7 +563,7 @@ const parseTableCell = (args: ParseTableCellArgs): TableCell | null => {
563563
}
564564
// Logical start/end fallback (LTR-default). The painter's
565565
// swapCellBordersLR is the single source of the RTL visual mirror
566-
// (§17.4.12 + §17.4.33). Pre-swapping here would double-mirror.
566+
// for cell borders (§17.4.33/12). Pre-swapping here would double-mirror.
567567
if (resolvedBorders.left == null) {
568568
const spec = convertResolvedCellBorder(resolvedBordersData.start);
569569
if (spec) resolvedBorders.left = spec;
@@ -597,19 +597,14 @@ const parseTableCell = (args: ParseTableCellArgs): TableCell | null => {
597597
filteredLegacyBorders[side] = { ...b, val: normalizeLegacyBorderStyle(b.val) };
598598
}
599599
}
600-
const fallback = extractCellBorders(
601-
{ borders: filteredLegacyBorders },
602-
{ isRtl: tableProperties?.rightToLeft === true },
603-
);
600+
const fallback = extractCellBorders({ borders: filteredLegacyBorders });
604601
if (fallback) {
605602
cellAttrs.borders = fallback;
606603
}
607604
}
608605

609606
const padding =
610-
extractCellPadding(cellNode.attrs ?? {}, {
611-
isRtl: tableProperties?.rightToLeft === true,
612-
}) ?? (defaultCellPadding ? { ...defaultCellPadding } : undefined);
607+
extractCellPadding(cellNode.attrs ?? {}) ?? (defaultCellPadding ? { ...defaultCellPadding } : undefined);
613608
if (padding) cellAttrs.padding = padding;
614609

615610
const verticalAlign = cellNode.attrs?.verticalAlign;
@@ -972,9 +967,8 @@ export function tableNodeToBlock(
972967
};
973968

974969
const borderSource = getBorderSource();
975-
const isRtlTable = tablePropertiesForCascade?.rightToLeft === true;
976970
const tableBorders: TableBorders | undefined = borderSource
977-
? extractTableBorders(borderSource.borders, { unit: borderSource.unit, isRtl: isRtlTable })
971+
? extractTableBorders(borderSource.borders, { unit: borderSource.unit })
978972
: undefined;
979973
if (tableBorders) tableAttrs.borders = tableBorders;
980974

Lines changed: 145 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -1,34 +1,27 @@
1-
# Direction Resolver
2-
3-
The shared direction model for SuperDoc. Computes a typed
4-
`ParagraphDirectionContext` once per paragraph during pm-adapter
5-
conversion. Downstream consumers (DomPainter, layout-bridge, hit
6-
testing) read from the resolved context — they do not re-derive
7-
direction from raw attributes.
8-
9-
## The core principle: orthogonal axes, no auto-inheritance
10-
11-
OOXML expresses direction along several independent axes. Most do
12-
NOT propagate to paragraph inline direction.
13-
14-
Per ECMA-376:
15-
16-
1. **Section `w:bidi` (§17.6.1)** affects section chrome only —
17-
page numbers, columns, gutters. It does NOT make paragraphs RTL.
18-
2. **Paragraph `w:bidi` (§17.3.1.6)** affects paragraph-level
19-
properties only — indent, justification, tab stops, text
20-
direction. It does NOT reorder text within the paragraph.
21-
3. **Table `w:bidiVisual` (§17.4.1)** affects cell ordering and
22-
table-level properties only. It does NOT make cell paragraphs
23-
RTL.
24-
4. **Writing mode `w:textDirection` (§17.3.1.41)** is the one
25-
direction property that DOES inherit across containers — a
26-
paragraph inherits its cell's writing mode, then the section's,
27-
then horizontal-tb default.
28-
29-
The resolver chain enforces these rules by construction. A
30-
contributor cannot accidentally make a downstream consumer infer
31-
paragraph direction from section bidi.
1+
# Direction Model
2+
3+
This module computes typed direction contexts during pm-adapter conversion.
4+
5+
Consumers read the resolved context from layout attrs. They do not re-derive
6+
direction from raw OOXML attributes.
7+
8+
## Core Rule: Keep Direction Axes Separate
9+
10+
OOXML has several direction-related properties. They are not interchangeable.
11+
12+
- Section `w:bidi` (§17.6.1) affects section chrome: page numbers, columns,
13+
and gutters. It does not make paragraphs RTL.
14+
- Paragraph `w:bidi` (§17.3.1.6) affects paragraph-level properties: indent,
15+
justification, tab stops, and text direction. It does not reorder text
16+
inside the paragraph.
17+
- Table `w:bidiVisual` (§17.4.1) affects table visual order and table-level
18+
properties. It does not make cell paragraphs RTL.
19+
- Writing mode `w:textDirection` (§17.3.1.41, §17.4.72) controls text flow.
20+
It can inherit across containers. A paragraph inherits its cell writing mode,
21+
then its section writing mode, then the `horizontal-tb` default.
22+
23+
The resolver chain keeps those axes separate. A paragraph direction consumer
24+
should not infer inline direction from section RTL or table visual RTL.
3225

3326
## Public API
3427

@@ -44,7 +37,7 @@ import {
4437
} from '@superdoc/pm-adapter/direction';
4538
```
4639

47-
Each resolver consumes its parent's context and returns its own:
40+
Each resolver consumes its parent context and returns its own:
4841

4942
```ts
5043
const sectionContext = resolveSectionDirection(sectPr);
@@ -59,36 +52,38 @@ const paragraphContext = resolveParagraphDirection(
5952

6053
The resolved `paragraphContext` carries:
6154

62-
- `inlineDirection: 'ltr' | 'rtl' | undefined` — paragraph inline
63-
base direction. Undefined when no explicit `w:bidi` is set in
64-
the paragraph or its style cascade. Consumers should omit the
65-
`dir` attribute when undefined and let the browser apply the
66-
Unicode Bidi Algorithm.
67-
- `writingMode: 'horizontal-tb' | 'vertical-rl' | 'vertical-lr'`
68-
text flow direction, inherited from the cell, then the section,
69-
then default.
55+
- `inlineDirection: 'ltr' | 'rtl' | undefined`
56+
57+
Paragraph inline base direction. It is undefined when no explicit `w:bidi`
58+
is set in the paragraph or its style cascade. Consumers should omit `dir`
59+
when this is undefined and let the browser apply the Unicode Bidi Algorithm.
60+
61+
- `writingMode: 'horizontal-tb' | 'vertical-rl' | 'vertical-lr'`
62+
63+
Text flow direction. It inherits from the cell, then the section, then the
64+
default.
7065

71-
## How downstream consumers read the context
66+
## Reading the Context
7267

73-
The resolved context is written onto `ParagraphAttrs.directionContext`
74-
during pm-adapter conversion. Downstream code reads it without
75-
importing from pm-adapter (which would violate the package boundary):
68+
pm-adapter writes the resolved context to `ParagraphAttrs.directionContext`.
69+
Downstream code reads that value from attrs. It should not import pm-adapter
70+
direction resolvers directly.
7671

7772
```ts
7873
// in layout-bridge or DomPainter
7974
const inline = block.attrs.directionContext?.inlineDirection;
8075
const writingMode = block.attrs.directionContext?.writingMode;
8176
```
8277

83-
For convenience, the legacy `ParagraphAttrs.direction` scalar
84-
(inline direction only) is also populated for consumers that only
85-
need that one field.
78+
`ParagraphAttrs.direction` is also populated for consumers that only need the
79+
inline-direction scalar.
8680

87-
## Logical-to-physical helpers
81+
## Logical-to-Physical Helpers
8882

89-
OOXML uses logical sides (`start`, `end`) that flip based on
90-
direction. CSS uses physical sides (`left`, `right`). Don't ask
91-
"is this RTL?" and map inline — use the helpers:
83+
OOXML uses logical sides such as `start` and `end`. CSS uses physical sides
84+
such as `left` and `right`.
85+
86+
Use the helpers instead of mapping sides inline:
9287

9388
```ts
9489
import { resolveLogicalAlignment, resolveLogicalIndent } from
@@ -104,35 +99,102 @@ const physicalIndent = resolveLogicalIndent(
10499
);
105100
```
106101

107-
## What this module does NOT do
108-
109-
- It does NOT infer paragraph base direction from run content.
110-
Per UAX #9 P2/P3, paragraph base direction without explicit
111-
`w:bidi` comes from the first strong character — and the
112-
browser already implements that natively when `dir` is
113-
omitted. SuperDoc does not need a server-side classifier.
114-
- It does NOT resolve complex-script formatting selection
115-
(`bCs`/`iCs`/`szCs`/`rFonts/@cs`). That's `RunScriptContext`
116-
and is implemented in Wave 1b.
117-
- It does NOT handle bidi controls (`w:bdo`/`w:dir`). That's
118-
Wave 1c.
119-
- It does NOT render vertical text. Wave 4 expands the writing
120-
mode enum and adds layout for vertical line boxes.
121-
122-
## Why the resolver chain matters
123-
124-
Before this module, several files each computed direction from
125-
raw attributes:
126-
127-
- `pm-adapter/src/attributes/paragraph.ts`
128-
`resolveEffectiveParagraphDirection` had a fallback cascade
129-
through `sectionDirection` (ECMA §17.6.1 violation) and a
130-
majority-of-runs heuristic (UAX #9 disagreement).
131-
- `layout-bridge/src/position-hit.ts` — conflated `textDirection`
132-
(writing mode) with `direction` (inline direction).
133-
- DomPainter, table-cell mirroring, and other sites each had their
134-
own ad-hoc direction detection, sometimes disagreeing.
135-
136-
Centralizing the model fixes the violations by construction and
137-
gives future RTL features (complex-script typography, visual RTL
138-
tables, vertical text, bidi controls) a single source of truth.
102+
## Out of Scope
103+
104+
This module does not infer paragraph base direction from run content. When
105+
`w:bidi` is absent, UAX #9 P2/P3 derives base direction from the first strong
106+
character. Browsers already do this when `dir` is omitted.
107+
108+
This module also does not resolve:
109+
110+
- Complex-script formatting selection (`bCs`, `iCs`, `szCs`, `rFonts/@cs`).
111+
That belongs to `RunScriptContext`.
112+
- Bidi controls (`w:bdo` §17.3.2.3 and `w:dir` §17.3.2.8).
113+
- Vertical text layout. The writing-mode enum carries the data; layout support
114+
is separate work.
115+
116+
## Table Visual Mirror
117+
118+
`w:bidiVisual` is separate from paragraph direction. It controls how table
119+
geometry and table-scoped sides appear visually.
120+
121+
For `w:bidiVisual`, upstream layers keep logical sides in LTR-default form:
122+
123+
- `start -> left`
124+
- `end -> right`
125+
126+
DomPainter applies the visual RTL mirror once at paint time.
127+
128+
Do not pre-mirror these values in the importer, style-engine, or pm-adapter.
129+
If an upstream layer chooses `left` or `right` based on table RTL, DomPainter
130+
will mirror the value again.
131+
132+
The rule applies when all three are true:
133+
134+
1. The OOXML property is table-scoped or cell-scoped. Examples: `w:tbl`,
135+
`w:tblPr`, `w:tblPrEx`, `w:tr`, `w:trPr`, `w:tc`, or `w:tcPr`.
136+
2. The property uses logical side language: `start`, `end`,
137+
leading/trailing, or table cell order.
138+
3. DomPainter already applies the `w:bidiVisual` visual mirror for that
139+
property.
140+
141+
### Covered
142+
143+
- `w:tblBorders/start`, `w:tblBorders/end` (§17.4.38, §17.4.36/13)
144+
- `w:tcBorders/start`, `w:tcBorders/end` (§17.4.66, §17.4.33/12)
145+
- `w:tblCellMar/start`, `w:tblCellMar/end` (§17.4.42, §17.4.41;
146+
start/end children at §17.4.34/11 and §17.4.35/10)
147+
- `w:tcMar/start`, `w:tcMar/end` (§17.4.68, §17.4.35/10)
148+
- Table cell visual order under `w:bidiVisual` (§17.4.1)
149+
- `w:gridBefore` and `w:gridAfter` placement (§17.4.15, §17.4.14)
150+
151+
### Not Covered
152+
153+
- Paragraph `w:bidi` (§17.3.1.6). Paragraph alignment and indent follow
154+
paragraph inline direction, not table direction.
155+
- Run `w:rtl` (§17.3.2.30), `w:dir` (§17.3.2.8), and `w:bdo` (§17.3.2.3).
156+
These are inline bidi controls, not table visual mirroring.
157+
- `w:textDirection` (§17.3.1.41, §17.4.72). This is writing mode, not a
158+
mirror.
159+
- Numeric `w:start` values in numbering (§17.9.25) or page numbering
160+
(§17.6.12). These are starting values, not sides.
161+
- Editing-side visual-to-logical mapping: table resize, cursor navigation, and
162+
hit testing. Those paths need RTL awareness as an inverse mapping from visual
163+
coordinates to logical structure.
164+
165+
## Why This Exists
166+
167+
Several older paths computed direction from raw attributes independently. They
168+
did not always agree.
169+
170+
- `pm-adapter/src/attributes/paragraph.ts` used section direction as a fallback
171+
for paragraph direction. That violates §17.6.1.
172+
- `layout-bridge/src/position-hit.ts` conflated writing mode with inline
173+
direction.
174+
- Table border and margin paths pre-mirrored `w:bidiVisual` sides before
175+
DomPainter mirrored them again.
176+
177+
Centralizing the direction model keeps consumers on one set of rules. It also
178+
gives future RTL work a clear place to plug in without adding another local
179+
direction heuristic.
180+
181+
## Quick Checks
182+
183+
Use these searches before adding a new direction-aware path:
184+
185+
```bash
186+
# Suspicious: upstream table-side pre-mirroring.
187+
rg "rightToLeft.*\\?.*'(left|right)'|rightToLeft.*\\?.*\\\"(left|right)\\\"" \
188+
packages/layout-engine/pm-adapter/src packages/super-editor/src/editors/v1/core/super-converter
189+
190+
# Review: downstream consumers reading raw direction fields.
191+
rg "sectionDirection|rightToLeft" \
192+
packages/layout-engine/layout-bridge/src packages/layout-engine/painters/dom/src
193+
194+
# Suspicious: painter importing direction logic from upstream packages.
195+
rg "@superdoc/(pm-adapter|style-engine)" packages/layout-engine/painters/dom/src
196+
```
197+
198+
Resolver files under `pm-adapter/src/direction/` are expected to read raw
199+
direction fields. The checks above are for new local direction decisions
200+
outside the resolver.

0 commit comments

Comments
 (0)