feat: rtl for body, lists, header/footer#3187
Conversation
|
The changes look fine against ECMA-376. Quick verification of what they emit:
Status: PASS No spec violations found in the changed handlers. The |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @artem-harbour! good consolidation across phases :)
Important
needs work, two things inline + two out-of-diff notes.
section w:bidi propagates to paragraph inline direction (§17.6.1 says it shouldn't), and run-level <w:rtl/> exports inline even when inherited from a style. each is one-spot, both inline.
two more out-of-diff but worth flagging before merge:
packages/document-api/src/format/inline-run-patch.ts:318—runAttribute('iCs', ...)lacks theitalicCsalias thatbCshas on line 317. since the new w:iCs translator writes PM attritalicCs, document-api consumers reading or writingiCswon't see freshly-imported italic. one-line fix: add'italicCs'as 5th arg.packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js:168-169— companion CS keys (fontSizeCs,boldCs,italicCs) drop out ofrunPropertiesInlineKeyson user override of a style-provided base (fontSize,bold,italic). result: user-edited size/bold/italic on mixed-script content writes<w:sz>only, drops<w:szCs>. CS chars in the same run silently render at the original style size.
agent context
{
"schema": "review_handoff_v1",
"review_id": "pr-3187-r1",
"findings": [
{
"id": "f2-section-bidi-propagates-to-paragraph",
"severity": "important",
"anchors": [{ "file": "packages/layout-engine/pm-adapter/src/attributes/paragraph.ts", "line": 102 }],
"claim": "section w:bidi propagates to paragraph inline direction. ECMA §17.6.1: section bidi 'only affects section-level properties, and does not affect the layout of text within the contents of this section.' Word renders Latin paragraphs in RTL sections LTR; v5 renders them RTL.",
"verification": [{ "command": "upload section-bidi-only fixture in v5 dev server", "result_summary": "all 4 Latin paragraph lines render dir='rtl', textAlign 'right', direction 'rtl'; expected dir=null, ltr", "status": "executed" }],
"handoff": {
"recommended_start": "packages/layout-engine/pm-adapter/src/attributes/paragraph.ts:92-105",
"suggested_fix": "drop the section fallback (and the run-content inference fallback) from resolveEffectiveParagraphDirection. paragraph w:bidi or its style cascade through docDefaults is the only valid signal. when no signal is present, leave inlineDirection undefined and let the browser apply UAX #9 P2/P3 via the missing dir attribute."
},
"verification_after_fix": [{ "command": "reload section-bidi-only fixture", "expected": "Latin paragraphs render dir=null, computed direction ltr, text-align left" }]
},
{
"id": "f1-forced-rtl-export-bypasses-style-gating",
"severity": "important",
"anchors": [{ "file": "packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js", "line": 344 }],
"claim": "shouldExport short-circuits style/override gating for rtl. any run whose runProperties.rtl is set always exports inline <w:rtl/>, even when rtl was inherited from a paragraph or character style. on roundtrip, every Hebrew/Arabic doc with style-set rtl gets inline <w:rtl/> flattened onto every run.",
"verification": [{ "command": "read r-translator.js:343-348", "result_summary": "first clause forces export; second clause (which gates by styleKeys/overrideKeys) is unreachable for key='rtl' when set", "status": "advisory" }],
"handoff": {
"recommended_start": "packages/super-editor/src/editors/v1/core/super-converter/v3/handlers/w/r/r-translator.js:343",
"suggested_fix": "drop the short-circuit. let rtl flow through the second clause, which gates by inlineKeys/overrideKeys membership. that respects the style cascade and matches how other keys are handled."
},
"verification_after_fix": [{ "command": "import a doc that defines a character style with w:rtl, re-export, inspect word/document.xml", "expected": "runs that inherit rtl from the style do NOT have inline <w:rtl/>; only runs with overrideKeys-flagged rtl do" }]
}
],
"cross_cutting": [
{
"id": "f4-icscs-document-api-alias-missing",
"severity": "important",
"anchors": [{ "file": "packages/document-api/src/format/inline-run-patch.ts", "line": 318 }],
"claim": "document-api `iCs` registration lacks the `italicCs` alias that `bCs` has on line 317. The new w:iCs translator writes PM attr `italicCs`, but document-api still reads/writes `iCs`. Roundtrip via the public API loses italic on freshly-imported docs.",
"verification": [{ "command": "grep -n 'runAttribute' packages/document-api/src/format/inline-run-patch.ts", "result_summary": "line 317 'bCs' has 5th-arg alias 'boldCs'; line 318 'iCs' has no 5th arg", "status": "executed" }],
"handoff": {
"suggested_fix": "runAttribute('iCs', 'boolean', 'w:iCs', schemaBooleanOrNull(), 'italicCs')"
}
},
{
"id": "f6-cs-companion-dropped-on-override",
"severity": "important",
"anchors": [{ "file": "packages/super-editor/src/editors/v1/extensions/run/calculateInlineRunPropertiesPlugin.js", "line": 168 }],
"claim": "COMPANION_INLINE_KEYS treats fontSizeCs/boldCs/italicCs as not-new when their base keys (fontSize/bold/italic) exist in the resolved style cascade. overrideKeysFromInlineProps only filters styleKeys, which never contains the CS companion. result: companion never reaches runPropertiesInlineKeys/runPropertiesOverrideKeys, so the export drops it on user override.",
"verification": [{ "command": "trace logic in calculateInlineRunPropertiesPlugin.js:140-180", "result_summary": "user overrides style fontSize=12 to 24 -> inlineProps={fontSize:24, fontSizeCs:24} -> inlineKeys=['fontSize'] (fontSizeCs treated companion-of-existing) -> overrideKeys=['fontSize'] (fontSizeCs not in styleKeys) -> exported XML has w:sz=24 only, w:szCs dropped", "status": "advisory" }]
}
]
}319d3b6 to
2a7cb6a
Compare
2a7cb6a to
6869011
Compare
…rrides Two related export-gating bugs surfaced when reviewing PR #3187 round 2: 1. r-translator force-pushed 'rtl' into candidateKeys whenever runProperties.rtl was set, regardless of whether rtl was actually inline. Combined with the plugin (below), runs that just referenced a character/paragraph style with w:rtl had inline <w:rtl/> flattened onto every export. Per ECMA Annex I, run rtl participates in the style cascade; exports must preserve the style reference. 2. calculateInlineRunPropertiesPlugin's overrideKeysFromInlineProps tagged any styleKey present in resolved inlineProps as an override - even when the value matched the style's value. After mark/cascade resolution that's essentially every styled key. Compare values instead so a key only counts as an override when the user genuinely overrode it. 3. hasChangedStyleComparableProps gated on styleKeys.includes(k), but styleKeys only tracks the run's rStyle - not paragraph style or docDefaults inheritance. Runs whose fontSize came from docDefaults/paragraph style and that the user then overrode would lose the CS companion (fontSizeCs) on export. Compare against existingStyleComparableProps (the full cascade). Tests: - style-inherited-rtl-roundtrip: locks fix #1 (style/inline-bold/inline-rtl) - iCs-alias-roundtrip: confirms <w:iCs/> survives import->export via the italicCs PM mark alias - cs-companion-on-override-roundtrip: locks fix #2 (paragraph-style override) and adds a docDefaults override case for fix #3
caio-pizzol
left a comment
There was a problem hiding this comment.
hey @artem-harbour! thanks for addressing last round.
i pushed a follow-up commit (c1b37da) with two fixes plus locking tests.
first: style-inherited <w:rtl/> was still flattening into inline rPr on every export, because round 2 left a candidateKeys.push('rtl') block in r-translator and the inline-props plugin was tagging cascade-resolved values as overrides whenever they showed up in inlineProps (which is basically always).
second: CS-companion finding - hasChangedStyleComparableProps gated on styleKeys, but styleKeys only tracks the run's rStyle, so docDefaults/paragraph-style-inherited fontSize overrides dropped the companion. compares against the full cascade now.
lgtm
Comment audit on c1b37da: - r-translator.js: reduce the rtl no-force-push comment to one AIDEV-NOTE - calculateInlineRunPropertiesPlugin.js: anchor the override and cascade rules with AIDEV-NOTE so the simpler-but-wrong shapes don't get reintroduced - test headers: drop the round/commit references that will rot once #3187 merges; keep the rule + spec citation
…egression) Broadening hasChangedStyleComparableProps to compare against the full style cascade (existingStyleComparableProps) rather than just the run's rStyle re-introduced the SD-2517 regression: zero-edit round-trip injected w:rFonts into style-inherited heading runs. Mark round-trip is lossy for fontFamily (per-script eastAsia/cs entries get flattened to ascii). The broader check then fired on every style-inherited run on every transaction, dragging mark keys into runPropertiesInlineKeys and serializing them as inline w:rFonts on export. Reverts only the hasChangedStyleComparableProps part of c1b37da. The overrideKeysFromInlineProps value-comparison fix stays - that one is what prevents style-inherited <w:rtl/> from flattening, and it doesn't trigger on zero-edit round-trips because the values match the style. The docDefaults-override test still passes because that path goes through hasNewInlineProps, not hasChangedStyleComparableProps.
|
🎉 This PR is included in @superdoc-dev/mcp v0.3.0-next.71 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.113 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.3.0-next.115 |
|
🎉 This PR is included in superdoc v1.30.0-next.68 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.8.0-next.87 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.8.0-next.69 |
|
🎉 This PR is included in superdoc-cli v0.9.0 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.32.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/mcp v0.4.0 The release is available on GitHub release |
|
🎉 This PR is included in @superdoc-dev/react v1.3.0 The release is available on GitHub release |
|
🎉 This PR is included in vscode-ext v2.4.0 |
Includes changes from previous PRs (+ rtl for header/footer):
#3113
#3160
Linear: SD-2808 (Body Interaction Reliability)
rtlusage from the layout path and standardized ondirectionas the single source of truth.w:valandvalsupport).CaretGeometry0)Home/Endhandling in vertical navigation and stabilized related behavior tests.dir/directionwhen returning to LTR.Note:
Linear: SD-2932 (RTL List Support)
pm-adapter(start/end-> physical sides) and applied RTL mirroring viamirrorIndentForRtl().resolveMarkerIndent()inshared/common/list-marker-utils.ts.renderer.ts) and resolved path (resolveParagraph.ts).Tab/Shift+Tablevel changes in RTL lists.ArrowLeft/ArrowRightandShift+Arrowselection behavior inside RTL list lines.ensureBidiIndentPxhelper and related public export/tests noise.changeListLevelinline-indent deletion as non-blocking for validated RTL list scenarios.ParagraphNodeViewmarker-path item as out of scope for user-facing rendering (DomPainter is the visual path).Linear: SD-2809 (Header/Footer Interaction Reliability)
RTL reliability for header/footer plus RTL direction/export fixes.
Shift+Arrowselection expectations.runProperties.rightToLeft/runProperties.rtl) for visual rendering; ParagraphNodeView infers RTL from runs (runProperties.rightToLeft/runProperties.rtl) for hidden PM typing direction.dir, CSSdirection, and logical margins (margin-inline-*) through the paste parse path (parseAttrs.js).paragraph bidi -> run inference -> section -> docDefaults -> LTR.runProperties.rtl -> w:rPr/w:rtl) and added roundtrip regression coverage for footer field scenarios.Fixed RTL run-format roundtrip gaps across edit -> export for Word compatibility.
fontSize -> fontSizeCsbold -> boldCsitalic -> italicCs*Csvalues from persisting after edits by treating them as mark-derived in inline run-property recalculation.w:iCsmapping to use canonicalitalicCskey (instead of legacyiCs).runProperties.iCsis normalized toitalicCsiCsstill exports asw:iCsw:b+w:bCsandw:i+w:iCsare emitted togetheriCspayloads still exportw:iCs