feat(pm-adapter): preserve run-level bidi/script metadata on TextRun (SD-2781)#3203
Open
caio-pizzol wants to merge 4 commits intomainfrom
Open
feat(pm-adapter): preserve run-level bidi/script metadata on TextRun (SD-2781)#3203caio-pizzol wants to merge 4 commits intomainfrom
caio-pizzol wants to merge 4 commits intomainfrom
Conversation
…(SD-2781) Adds two preservation-only fields to the layout text-run contract, kept on separate axes per ECMA's own categorization: - TextRun.bidi (RunBidiContext): direction signals only - run rtl flag now, embedding (w:dir) and override (w:bdo) wired in Wave 1c. - TextRun.script (RunScriptContext): complex-script flag + per-script language metadata (default / complexScript / eastAsian) per §17.3.2.20. Both populated by pm-adapter from raw run properties when present. Wave 1a does not render either; Wave 1b will gate the formatting-stack selection on script.complexScript, Wave 1c will read bidi.embedding/override. Why now: ECMA puts direction (rtl, dir, bdo) and script formatting (cs, lang/@bidi) in different categories. Lumping them under one bidi field would collapse the axes and lie about the schema. Adding both fields now means Wave 1b/1c don't have to introduce both the data path and the rendering at once. The RunScriptContext.language field expanded from a single string to a structured object with three optional tags. No production consumer reads the field yet (preservation-only since #3184), so the shape change is safe. Tests: - bidi populated only on rtl, script populated only on cs/lang - explicit rtl=false preserved (a meaningful override) - the three lang tags land on separate fields - axis non-collapse: rtl never leaks into script, cs never leaks into bidi
superdoc-bot
Bot
added
review: thorough
review: careful
and removed
review: thorough
labels
…TextRun The previous commit added `bidi?: RunBidiContext` and `script?: RunScriptContext` to TextRun in contracts/src/index.ts but only re-exported the type names; they were not in local type scope, so `tsc --project` failed: src/index.ts(324,10): error TS2304: Cannot find name 'RunBidiContext'. src/index.ts(330,12): error TS2304: Cannot find name 'RunScriptContext'. Vitest's transform doesn't enforce type-only imports the way tsc does, so the unit tests passed even though the build was broken. CI build would have caught it; the missing piece was running `pnpm build` locally before pushing. Adds the two names to the existing local `import type` statement alongside ParagraphDirectionContext (same pattern, same line 19). Also adds two integration tests in pm-adapter/src/integration.test.ts that exercise the full PM -> FlowBlock conversion through the unmocked applyInlineRunProperties pipeline. The previous unit tests in common.test.ts mock computeRunAttrs, which is why they couldn't catch shape-level regressions in the contracts package. The integration tests prove a real PM doc with raw runProperties (rtl/cs/lang on a run-wrapper node) produces a TextRun with populated bidi/script, and that runs without signals don't gain empty objects.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b7cf49dae8
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
Repo convention puts the ticket reference in the `describe` label only
(e.g., `describe('SD-1333: ...')`). Trims the redundant `SD-2781:` prefix
from the block comment, keeps the non-obvious "Wave 1a preserves these
signals; nothing renders them yet" note.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
…ssign on token runs Two codex findings on PR #3203: 1. Cascade-leak: applyInlineRunProperties was receiving cascade-resolved runProperties from runNodeChildrenToRuns and populating bidi/script from them. Style-inherited runs ended up with bidi/script metadata they did not have inline, making preservation indistinguishable from direct formatting and bloating the layout tree on every styled run. Fix: thread the raw inline runProperties through InlineConverterParams (alongside the existing cascade-resolved runProperties). applyInlineRunProperties gains a fourth parameter that bidi/script populate from. When the caller doesn't opt in (no inline parameter), no metadata is attached - safer default than reading the cascaded view. 2. Token-run drop: generic-token.ts called applyInlineRunProperties without reassigning the return value. Since the helper builds a new object via spread, all merged fields (including the SD-2781 bidi/script) were lost for page-number / total-page-count token runs inside an rtl run wrapper. Fix: change the local to `let` and reassign. Also forwards inlineRunProperties through. no-break-hyphen.ts (another caller) updated for consistency. Tests added: - 3 unit tests in common.test.ts proving bidi/script populate from inlineRunProperties only, never from cascade-resolved runProperties - Existing 7 SD-2781 unit tests updated to pass inlineRunProperties (the new opt-in source) - 1 integration test proving bidi/script propagate to page-number tokens inside an rtl run wrapper All 1800 pm-adapter tests, 12644 super-editor tests, and 1201 layout-bridge tests pass. Contracts + pm-adapter both build clean.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds preservation-only run-level direction and script metadata to the layout text-run contract, kept on separate axes per ECMA's own categorization.
What changes
Two fields on `TextRun`:
Both populated by pm-adapter in `applyInlineRunProperties` from raw `RunProperties` when present. Empty contexts are not attached, so the layout tree doesn't bloat for runs with no signals.
Why typed split
ECMA-376 puts direction (`rtl`, `dir`, `bdo`) and script formatting (`cs`, `lang/@bidi`) in different categories.
Lumping them under one `bidi` field would collapse the axes and lie about the schema. The non-collapse test exercises this directly: setting `rtl: true, cs: true` produces `bidi.rtl === true` and `script.complexScript === true` with no leakage.
Why now
Wave 1b will consume `script.complexScript` to gate the formatting-stack selection; Wave 1c will consume `bidi.embedding` and `bidi.override`. Adding the data path now means each wave doesn't have to introduce both the data and the rendering at once.
Type shape change
`RunScriptContext.language` expanded from `string` to `{ default?, complexScript?, eastAsian? }`. No production consumer reads the field yet (preservation-only since #3184), so the change is safe.
What this PR does NOT do
Validation