Skip to content

Commit 766f82e

Browse files
artem-harbourArtem Nistuleycaio-pizzol
authored
fix: align rtl toolbar state and alignment writes with ooxml bidi (#3244)
* fix: align rtl toolbar state and alignment writes with ooxml bidi * test(rtl-alignment): cover helper roundtrip + toolbar writer + doc-api export Adds three coverage layers for the SD-3094 writer-side mirror: - paragraph-alignment unit (23 tests): direct coverage of the new helper pair (mapDisplayAlignmentToStoredJustification + mapStoredJustificationToDisplayAlignment), including the absent-value default and a display->stored->display roundtrip for every alignment. - Toolbar behavior (4 tests): clicking each align button on an RTL paragraph writes the expected mirrored OOXML justification (verified via the doc-api). - Doc-API story (5 tests): format.paragraph.setAlignment on an RTL paragraph exports the expected w:jc value (left->right, right->left, center->center, justify->both); LTR baseline unchanged. Fixture rtl-paragraph-alignment.docx is duplicated from the SD-3093 behavior tests so this PR's CI can run standalone; it will collapse to the SD-3093 copy when the two PRs merge. * test(rtl-alignment): cover section-bidi-only edge case (ECMA-376 §17.6.1) Adds regression coverage for the spec rule that section-level w:bidi must NOT cascade to paragraph w:jc interpretation. Section bidi affects only section-level properties (page numbers, columns), not paragraph alignment. The fixture has a section with <w:bidi/> but two paragraphs without paragraph-level <w:bidi/>. Tests verify that clicking Align Left or Align Right on these paragraphs stores the display value unchanged (no mirror), since the paragraph itself is LTR per spec. * test(rtl-alignment): cover headings, tables, multi-paragraph selection, and indent Expands SD-3094 behavior coverage across additional surfaces that share the alignment-writer code path: - Headings (Heading1/Heading2 with w:bidi): Align Left/Right mirrors the stored OOXML the same way it does for body paragraphs. - Table cells with RTL direction (w:bidiVisual): Align Left on a Hebrew cell paragraph stores w:jc=right. - Multi-paragraph selection across three RTL paragraphs: Align Left applies the writer mirror per-paragraph, all three end up stored as w:jc=right. - Indent + RTL: Increase Indent on an RTL paragraph updates the paragraph properties (smoke test against the toolbar indent button). Lists + RTL alignment was investigated but deferred: it needs a hand-crafted OOXML fixture with explicit numbering + paragraph-level bidi, which is a bigger scope and better tracked separately. --------- Co-authored-by: Artem Nistuley <artem@superdoc.dev> Co-authored-by: Caio Pizzol <caio@superdoc.dev>
1 parent fa0cfc5 commit 766f82e

20 files changed

Lines changed: 1158 additions & 30 deletions

‎packages/super-editor/src/editors/v1/core/helpers/paragraph-alignment.js‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
/**
2+
* Maps display alignment (UI-facing physical left/right/center/justify) to
3+
* stored OOXML paragraph justification, honoring RTL paragraph direction.
4+
*
5+
* @param {'left' | 'center' | 'right' | 'justify'} alignment
6+
* @param {boolean} isRtl
7+
* @returns {'left' | 'center' | 'right' | 'both'}
8+
*/
9+
export function mapDisplayAlignmentToStoredJustification(alignment, isRtl) {
10+
if (alignment === 'justify') return 'both';
11+
if (!isRtl) return alignment;
12+
if (alignment === 'left') return 'right';
13+
if (alignment === 'right') return 'left';
14+
return alignment;
15+
}
16+
17+
/**
18+
* Maps stored OOXML paragraph justification to display alignment, honoring
19+
* RTL paragraph direction. When justification is absent, returns the
20+
* visual default by direction.
21+
*
22+
* @param {string | null | undefined} justification
23+
* @param {boolean} isRtl
24+
* @returns {'left' | 'center' | 'right' | 'justify'}
25+
*/
26+
export function mapStoredJustificationToDisplayAlignment(justification, isRtl) {
27+
if (!justification) return isRtl ? 'right' : 'left';
28+
if (justification === 'both') return 'justify';
29+
if (!isRtl) return /** @type {'left' | 'center' | 'right' | 'justify'} */ (justification);
30+
if (justification === 'left') return 'right';
31+
if (justification === 'right') return 'left';
32+
return /** @type {'left' | 'center' | 'right' | 'justify'} */ (justification);
33+
}

‎packages/super-editor/src/editors/v1/core/helpers/paragraph-alignment.test.js‎

Lines changed: 105 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,105 @@
1+
import { describe, it, expect } from 'vitest';
2+
import {
3+
mapDisplayAlignmentToStoredJustification,
4+
mapStoredJustificationToDisplayAlignment,
5+
} from './paragraph-alignment.js';
6+
7+
// SD-3094: per ECMA-376 §17.3.1.13, `w:jc="left"` is the leading edge of the
8+
// paragraph, not the physical left. In RTL paragraphs (`w:bidi`), leading is
9+
// the right side. The two helpers below own the display ↔ stored translation
10+
// so the editor can keep its UI in physical terms while the model stays in the
11+
// spec's logical terms.
12+
13+
describe('mapDisplayAlignmentToStoredJustification', () => {
14+
describe('LTR paragraphs (isRtl=false)', () => {
15+
it('passes left/right/center through unchanged', () => {
16+
expect(mapDisplayAlignmentToStoredJustification('left', false)).toBe('left');
17+
expect(mapDisplayAlignmentToStoredJustification('right', false)).toBe('right');
18+
expect(mapDisplayAlignmentToStoredJustification('center', false)).toBe('center');
19+
});
20+
21+
it('maps justify to OOXML both', () => {
22+
expect(mapDisplayAlignmentToStoredJustification('justify', false)).toBe('both');
23+
});
24+
});
25+
26+
describe('RTL paragraphs (isRtl=true)', () => {
27+
it('mirrors left to right', () => {
28+
expect(mapDisplayAlignmentToStoredJustification('left', true)).toBe('right');
29+
});
30+
31+
it('mirrors right to left', () => {
32+
expect(mapDisplayAlignmentToStoredJustification('right', true)).toBe('left');
33+
});
34+
35+
it('keeps center unchanged', () => {
36+
expect(mapDisplayAlignmentToStoredJustification('center', true)).toBe('center');
37+
});
38+
39+
it('maps justify to OOXML both (no direction-flip)', () => {
40+
expect(mapDisplayAlignmentToStoredJustification('justify', true)).toBe('both');
41+
});
42+
});
43+
44+
it('justify always maps to both regardless of direction', () => {
45+
expect(mapDisplayAlignmentToStoredJustification('justify', false)).toBe('both');
46+
expect(mapDisplayAlignmentToStoredJustification('justify', true)).toBe('both');
47+
});
48+
});
49+
50+
describe('mapStoredJustificationToDisplayAlignment', () => {
51+
describe('LTR paragraphs (isRtl=false)', () => {
52+
it('passes left/right/center through unchanged', () => {
53+
expect(mapStoredJustificationToDisplayAlignment('left', false)).toBe('left');
54+
expect(mapStoredJustificationToDisplayAlignment('right', false)).toBe('right');
55+
expect(mapStoredJustificationToDisplayAlignment('center', false)).toBe('center');
56+
});
57+
58+
it('maps both to justify', () => {
59+
expect(mapStoredJustificationToDisplayAlignment('both', false)).toBe('justify');
60+
});
61+
62+
it('defaults to left when justification is absent', () => {
63+
expect(mapStoredJustificationToDisplayAlignment(null, false)).toBe('left');
64+
expect(mapStoredJustificationToDisplayAlignment(undefined, false)).toBe('left');
65+
expect(mapStoredJustificationToDisplayAlignment('', false)).toBe('left');
66+
});
67+
});
68+
69+
describe('RTL paragraphs (isRtl=true)', () => {
70+
it('mirrors stored left to display right', () => {
71+
expect(mapStoredJustificationToDisplayAlignment('left', true)).toBe('right');
72+
});
73+
74+
it('mirrors stored right to display left', () => {
75+
expect(mapStoredJustificationToDisplayAlignment('right', true)).toBe('left');
76+
});
77+
78+
it('keeps center unchanged', () => {
79+
expect(mapStoredJustificationToDisplayAlignment('center', true)).toBe('center');
80+
});
81+
82+
it('maps both to justify (no direction-flip)', () => {
83+
expect(mapStoredJustificationToDisplayAlignment('both', true)).toBe('justify');
84+
});
85+
86+
it('defaults to right when justification is absent', () => {
87+
expect(mapStoredJustificationToDisplayAlignment(null, true)).toBe('right');
88+
expect(mapStoredJustificationToDisplayAlignment(undefined, true)).toBe('right');
89+
expect(mapStoredJustificationToDisplayAlignment('', true)).toBe('right');
90+
});
91+
});
92+
93+
// Roundtrip: writing then reading must return the original display value.
94+
describe('roundtrip display → stored → display', () => {
95+
for (const isRtl of [false, true]) {
96+
for (const display of /** @type {const} */ (['left', 'center', 'right', 'justify'])) {
97+
it(`${display} on ${isRtl ? 'RTL' : 'LTR'} survives a write+read cycle`, () => {
98+
const stored = mapDisplayAlignmentToStoredJustification(display, isRtl);
99+
const readBack = mapStoredJustificationToDisplayAlignment(stored, isRtl);
100+
expect(readBack).toBe(display);
101+
});
102+
}
103+
}
104+
});
105+
});

‎packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/executor.test.ts‎

Lines changed: 109 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ const mockedDeps = vi.hoisted(() => ({
3434
applyDirectMutationMeta: vi.fn(),
3535
applyTrackedMutationMeta: vi.fn(),
3636
mapBlockNodeType: vi.fn(),
37+
calculateResolvedParagraphProperties: vi.fn((_editor: Editor, node: any) => node?.attrs?.paragraphProperties ?? {}),
3738
}));
3839

3940
vi.mock('../helpers/index-cache.js', () => ({
@@ -71,6 +72,10 @@ vi.mock('../helpers/node-address-resolver.js', () => ({
7172
candidate.nodeType === 'tableCell',
7273
}));
7374

75+
vi.mock('../../extensions/paragraph/resolvedPropertiesCache.js', () => ({
76+
calculateResolvedParagraphProperties: mockedDeps.calculateResolvedParagraphProperties,
77+
}));
78+
7479
// Register built-in executors once
7580
beforeAll(() => {
7681
registerBuiltInExecutors();
@@ -81,6 +86,9 @@ beforeEach(() => {
8186
mockedDeps.getRevision.mockReturnValue('0');
8287
mockedDeps.incrementRevision.mockReturnValue('1');
8388
mockedDeps.mapBlockNodeType.mockReturnValue(undefined);
89+
mockedDeps.calculateResolvedParagraphProperties.mockImplementation((_editor: Editor, node: any) => {
90+
return node?.attrs?.paragraphProperties ?? {};
91+
});
8492
});
8593

8694
// ---------------------------------------------------------------------------
@@ -3015,6 +3023,107 @@ describe('executeStyleApply: collapsed-range no-op guard', () => {
30153023
expect(result).toEqual({ changed: false });
30163024
expect(tr.addMark).not.toHaveBeenCalled();
30173025
});
3026+
3027+
it('mirrors left/right alignment for RTL paragraphs when applying format.alignment', () => {
3028+
const tr = {
3029+
setNodeMarkup: vi.fn(),
3030+
doc: {
3031+
nodesBetween: vi.fn((from: number, to: number, callback: (node: any, pos: number) => void) => {
3032+
callback(
3033+
{
3034+
isTextblock: true,
3035+
attrs: {
3036+
paragraphProperties: {
3037+
rightToLeft: true,
3038+
justification: 'left',
3039+
},
3040+
},
3041+
nodeSize: 8,
3042+
},
3043+
5,
3044+
);
3045+
}),
3046+
},
3047+
} as any;
3048+
3049+
const editor = {} as Editor;
3050+
const target = makeTarget({
3051+
op: 'style.apply' as any,
3052+
absFrom: 6,
3053+
absTo: 10,
3054+
}) as any;
3055+
const step: StyleApplyStep = {
3056+
op: 'style.apply',
3057+
id: 'step-rtl-align',
3058+
ref: 'test-ref',
3059+
args: { alignment: 'left' as any },
3060+
};
3061+
3062+
const result = executeStyleApply(editor, tr, target, step, { map: (pos: number) => pos } as any);
3063+
3064+
expect(result).toEqual({ changed: true });
3065+
expect(tr.setNodeMarkup).toHaveBeenCalledWith(
3066+
5,
3067+
undefined,
3068+
expect.objectContaining({
3069+
paragraphProperties: expect.objectContaining({
3070+
rightToLeft: true,
3071+
justification: 'right',
3072+
}),
3073+
}),
3074+
);
3075+
});
3076+
3077+
it('uses resolved RTL from style cascade when applying format.alignment', () => {
3078+
mockedDeps.calculateResolvedParagraphProperties.mockReturnValueOnce({ rightToLeft: true });
3079+
const tr = {
3080+
setNodeMarkup: vi.fn(),
3081+
doc: {
3082+
resolve: vi.fn((pos: number) => ({ pos })),
3083+
nodesBetween: vi.fn((from: number, to: number, callback: (node: any, pos: number) => void) => {
3084+
callback(
3085+
{
3086+
isTextblock: true,
3087+
attrs: {
3088+
paragraphProperties: {
3089+
rightToLeft: false,
3090+
justification: 'left',
3091+
},
3092+
},
3093+
nodeSize: 8,
3094+
},
3095+
5,
3096+
);
3097+
}),
3098+
},
3099+
} as any;
3100+
3101+
const editor = {} as Editor;
3102+
const target = makeTarget({
3103+
op: 'style.apply' as any,
3104+
absFrom: 6,
3105+
absTo: 10,
3106+
}) as any;
3107+
const step: StyleApplyStep = {
3108+
op: 'style.apply',
3109+
id: 'step-rtl-align-resolved',
3110+
ref: 'test-ref',
3111+
args: { alignment: 'left' as any },
3112+
};
3113+
3114+
const result = executeStyleApply(editor, tr, target, step, { map: (pos: number) => pos } as any);
3115+
3116+
expect(result).toEqual({ changed: true });
3117+
expect(tr.setNodeMarkup).toHaveBeenCalledWith(
3118+
5,
3119+
undefined,
3120+
expect.objectContaining({
3121+
paragraphProperties: expect.objectContaining({
3122+
justification: 'right',
3123+
}),
3124+
}),
3125+
);
3126+
});
30183127
});
30193128

30203129
// ---------------------------------------------------------------------------

‎packages/super-editor/src/editors/v1/document-api-adapters/plan-engine/executor.ts‎

Lines changed: 15 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -39,7 +39,7 @@ import type {
3939
} from './executor-registry.types.js';
4040
import { getStepExecutor } from './executor-registry.js';
4141
import { planError } from './errors.js';
42-
import { ALIGNMENT_TO_JUSTIFICATION } from './paragraphs-wrappers.js';
42+
import { mapAlignmentToJustificationForParagraph } from './paragraphs-wrappers.js';
4343
import { closeHistory } from 'prosemirror-history';
4444
import { yUndoPluginKey } from 'y-prosemirror';
4545
import { checkRevision, getRevision } from './revision-tracker.js';
@@ -53,6 +53,7 @@ import { mapBlockNodeType } from '../helpers/node-address-resolver.js';
5353
import { resolveWithinScope, scopeByRange } from '../helpers/adapter-utils.js';
5454
import { normalizeReplacementText } from './replacement-normalizer.js';
5555
import { getWordChanges } from './word-diff.js';
56+
import { calculateResolvedParagraphProperties } from '../../extensions/paragraph/resolvedPropertiesCache.js';
5657
import { Fragment, Slice } from 'prosemirror-model';
5758
import type { Mark as ProseMirrorMark, MarkType, Node as ProseMirrorNode, NodeType } from 'prosemirror-model';
5859
import type { Transaction } from 'prosemirror-state';
@@ -1067,16 +1068,19 @@ export function executeTextDelete(
10671068
return { changed: true };
10681069
}
10691070

1070-
// ALIGNMENT_TO_JUSTIFICATION imported from paragraphs-wrappers.js
1071-
10721071
/**
10731072
* Applies alignment to the paragraph node(s) that contain the given range.
10741073
* Uses the same mechanism as paragraphsSetAlignmentWrapper: updates
10751074
* paragraphProperties.justification via tr.setNodeMarkup.
10761075
*/
1077-
function applyAlignmentToRange(tr: Transaction, absFrom: number, absTo: number, alignment: string): boolean {
1078-
const justification = ALIGNMENT_TO_JUSTIFICATION[alignment as keyof typeof ALIGNMENT_TO_JUSTIFICATION];
1079-
if (!justification) return false;
1076+
function applyAlignmentToRange(
1077+
editor: Editor,
1078+
tr: Transaction,
1079+
absFrom: number,
1080+
absTo: number,
1081+
alignment: string,
1082+
): boolean {
1083+
if (!alignment) return false;
10801084

10811085
let changed = false;
10821086
const doc = tr.doc;
@@ -1086,6 +1090,9 @@ function applyAlignmentToRange(tr: Transaction, absFrom: number, absTo: number,
10861090
if (!node.isTextblock) return;
10871091

10881092
const existing = (node.attrs as Record<string, unknown>).paragraphProperties as Record<string, unknown> | undefined;
1093+
const paragraphPos = typeof tr.doc.resolve === 'function' ? tr.doc.resolve(pos) : null;
1094+
const resolved = calculateResolvedParagraphProperties(editor, node, paragraphPos as any);
1095+
const justification = mapAlignmentToJustificationForParagraph(alignment as any, resolved?.rightToLeft === true);
10891096
const currentJustification = existing?.justification;
10901097

10911098
if (currentJustification === justification) return;
@@ -1145,7 +1152,7 @@ export function executeStyleApply(
11451152
}
11461153

11471154
if (step.args.alignment) {
1148-
changed = applyAlignmentToRange(tr, absFrom, absTo, step.args.alignment) || changed;
1155+
changed = applyAlignmentToRange(editor, tr, absFrom, absTo, step.args.alignment) || changed;
11491156
}
11501157

11511158
return { changed };
@@ -1305,7 +1312,7 @@ export function executeSpanStyleApply(
13051312
}
13061313

13071314
if (step.args.alignment) {
1308-
changed = applyAlignmentToRange(tr, absFrom, absTo, step.args.alignment) || changed;
1315+
changed = applyAlignmentToRange(editor, tr, absFrom, absTo, step.args.alignment) || changed;
13091316
}
13101317

13111318
return { changed };

0 commit comments

Comments
 (0)