fix(layout): align painter run expansion with measurer for literal tabs (SD-3266)#3518
fix(layout): align painter run expansion with measurer for literal tabs (SD-3266)#3518tupizz wants to merge 2 commits into
Conversation
36956c8 to
1ef0a07
Compare
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
b2291ed to
ab909d7
Compare
ab909d7 to
5d2358e
Compare
…bs (SD-3266)
The measurer expanded text runs containing literal U+0009 into a sequence
[text, tab, text] before measurement; the painter only expanded inline
newlines. Line.fromRun/toRun returned by the measurer therefore indexed a
9-run expanded array, but the painter sliced the same indices against the
7-run unexpanded block.runs — silently dropping 41 PM characters from the
first paragraph of imported Word redlines (the visible "text appears
missing" symptom in SD-3266).
Fix:
- Promote the measurer's inline-tab expansion into a shared
expandRunsForInlineTabs helper in contracts/run-helpers.ts.
- Chain it after expandRunsForInlineNewlines in every painter pre-expansion
site (paragraph/renderParagraphContent.ts at 2 sites, render-line.ts,
renderer.ts table-cell cache).
- Synthetic TabRuns produced from a literal \t inside a tracked-change
text run carry fromLiteralTab=true plus the propagated trackedChange
metadata. The measurer's tab-run handler short-circuits to a 2-space
glyph advance (no tabStopCursor change). The painter's tab-run module
renders these as a " " (two-space) <span class="superdoc-tab
superdoc-tab--literal"> with applyTrackedChangeDecorations applied so
the strikethrough/insertion-underline crosses the placeholder.
- Non-revision inline tabs (TOC-style "Chapter 1\t42") keep the legacy
tab-stop advance + leader behavior — fromLiteralTab is set only when
the source text run carries a tracked-change mark.
SD-2939 integration: the .superdoc-tab class on the literal-tab span
already participates in Caio's "Show formatting marks" toggle — when
the toggle is ON, the existing
`.superdoc-show-formatting-marks .superdoc-tab::after { content: "→" }`
CSS overlays a centered arrow on the 2-space strut. No separate render
mechanism added.
Sidebar balloon: replaced the single-string display helper in
CommentDialog.vue with displayTrackedTextSegments() that splits the
deleted/inserted text on \t into {kind:'text'|'arrow'} segments. Arrow
segments render inside <span class="tracked-change-tab-arrow"> styled
with --sd-formatting-mark-color (falls back to --sd-ui-action, then
Word's blue #4a86e8) — matches Word's revision-balloon convention of
showing a blue → for tab characters while the surrounding deletion text
stays red.
Round-trip: PM document is never modified — expandRunsForInlineTabs is
a pure transform that lives only in the layout pipeline. The literal
\t stays on the trackDelete-marked text node in PM, and export writes
<w:delText>[\t]</w:delText> verbatim. Verified end-to-end on the
SD-3266 fixture.
Tests:
- contracts: 247/247 (5 new SD-3266 cases for the helper)
- measuring-dom: 347/347
- painter-dom: 1136/1136
- pm-adapter: 1853/1853
- superdoc: 1116/1116
Two renderer-hanging-indent fixtures replaced literal \t with a space:
they hand-rolled Line.segments with runIndex:0 that depended on no
expansion, and the test intent (positioning, not tab semantics) is
unchanged.
5d2358e to
174bd02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d2358eed6
ℹ️ 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".
| ...(isInRevision | ||
| ? { | ||
| fromLiteralTab: true, | ||
| // Propagate tracked-change metadata so the painter can paint | ||
| // the strikethrough/underline across the synthesized glyph. | ||
| trackedChange: textRun.trackedChange, | ||
| } | ||
| : {}), |
There was a problem hiding this comment.
Preserve measured width for non-revision literal tabs
When a literal \t is not inside a tracked change, this creates a normal TabRun rather than fromLiteralTab. The measurer mutates its own synthesized TabRun with width, but the painter expands the original text again, producing a fresh normal TabRun with no measured width; because tab runs do not emit their own LineSegment, a trailing/standalone non-revision literal tab falls back to zero width in renderPositionedTabRun. This collapses visible underlined/signature-style tabs encoded as literal U+0009 unless there is following content whose segment supplies an x-position.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
working on fixing this issue
There was a problem hiding this comment.
cubic analysis
1 issue found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/layout-engine/measuring/dom/src/index.ts">
<violation number="1" location="packages/layout-engine/measuring/dom/src/index.ts:1536">
P2: `segStart` uses `currentLine.toChar` which is a character offset into the *previous* `toRun`, not the current tab run. For a tab run (1 char), the segment should be `[0, 1)` and `toChar` should be `1`, matching the normal tab path below. When a literal tab appears mid-line (after text), this produces out-of-bounds segment indices for downstream rendering.</violation>
</file>
Linked issue analysis
Linked issue: SD-3266: Bug: Imported Word redlines can render paragraph text as missing
| Status | Acceptance criteria | Notes |
|---|---|---|
| ✅ | Imported DOCX review paragraphs with dense tracked changes render readable full text without sections appearing missing relative to Word | PR realigns measurer and painter expansions so run indices no longer drop content (root cause described). Before/after screenshots and narrative are included; layout code changes in measuring/painter show explicit handling to avoid tab-stop jumps that previously hid text. |
| ✅ | Tracked delete placeholder tab content from imported Word documents is treated as placeholder display text, not as real tab-stop layout instructions | New expandRunsForInlineTabs synthesizes TabRun.fromLiteralTab for tracked-change text; measurer measures a compact glyph width and does not advance tabStopCursor; painter renders a 2-space placeholder span with tracked-change decorations. |
| The source DOCX from IT-1100 reproduces correctly in the latest SuperDoc build | PR includes a test plan and states live verification with the root-cause fixture/customer DOCX and provides before/after screenshots, but the description does not explicitly state the IT-1100 artifact was independently validated in CI; manual verification steps are provided rather than an automated acceptance test tied to that exact file. | |
| ✅ | Regression coverage exists for imported Word tracked-changes paragraphs with placeholder revision content | New unit tests were added for expandRunsForInlineTabs and test-suite counts in the PR indicate passing suites; also adjustments to renderer tests were made to accommodate the new expansion behavior. |
Reply with feedback, questions, or to request a fix.
Fix all with cubic | Re-trigger cubic
…D-3266) Trailing/standalone literal U+0009 outside tracked changes (signature lines like "Sign:____\t") collapsed to zero width because the measurer's run.width mutation lived on a TabRun the painter never sees: expandRunsForInlineTabs runs independently in both stages, producing fresh TabRun instances. Combined with tab runs not emitting their own LineSegment, the painter had no path to recover the advance. - Always set fromLiteralTab on synthesized tabs; gate trackedChange on revision so the compact 2-space strut still only applies to revisions. - Emit a LineSegment from the measurer's normal-tab handler when fromLiteralTab is true, so segmentsByRun carries the measured advance to the painter. - Painter literal-tab branch sets explicit box width from the segment for non-revision tabs and inherits the underline mark, restoring signature-line rendering.
Summary
Imported Word redlines containing literal
\tinside<w:delText>(e.g. Orbital Copilot's[\t]placeholders) rendered with chunks of paragraph text silently missing, plus a huge inline tab-stop gap where the deletion sat.Linear: SD-3266
Root cause: the measurer expanded text runs containing
\tinto[text, tab, text]for measurement; the painter only expanded inline newlines. The measurer'sLine.fromRun/toRuntherefore indexed a 9-run expanded array, but the painter sliced the same indices against the 7-run unexpandedblock.runs— silently dropping 41 PM characters of the affected paragraph.This PR makes the painter expand identically, then renders the resulting tab as a 2-space placeholder inline (matching Word's body-view convention) so the deletion strikethrough has something to paint across and the body text flows naturally instead of hitting the paragraph's tab stop.
Before / After
What changed
contracts/src/run-helpers.ts— new sharedexpandRunsForInlineTabs(runs, tabStops, indent). Returns the input array unchanged when nothing needs expanding. Tags synthesised TabRuns withfromLiteralTab: trueonly when the originating text run carries a tracked-change mark, so TOC-styleChapter 1\t42paragraphs keep their real tab-stop + leader behaviour.contracts/src/index.ts— addedfromLiteralTab?: boolean,trackedChange?: TrackedChangeMeta,fontFamily?: string,fontSize?: numbertoTabRunso the painter can render the synthesized glyph with the right typography (the line container usesfont-size: 0for whitespace control).measuring/dom/src/index.ts— replaced the inline\texpansion with the shared helper; added a short-circuit in the tab-run handler that givesfromLiteralTabtabs a 2-space advance and leavestabStopCursoralone (no jump to the next paragraph tab stop).painters/dom/src/renderer.ts— chainedexpandRunsForInlineTabsafterexpandRunsForInlineNewlinesat all 5 expansion sites; fixedblock.runs[runIndex]→expandedBlock.runs[runIndex]in the in-line iteration loop (the actual index-alignment bug); added afromLiteralTabbranch in both tab-rendering paths that emits a 2-space<span class="superdoc-tab superdoc-tab--literal">withapplyTrackedChangeDecorationsfor the strikethrough/insertion-underline.superdoc/CommentsLayer/CommentDialog.vue— replaced the single-string display helper withdisplayTrackedTextSegments()that splits the deleted/inserted text on\tinto{ kind: 'text' | 'arrow' }segments. Arrow segments render inside<span class="tracked-change-tab-arrow">styled with--sd-formatting-mark-color(falls back to--sd-ui-action, then Word's blue#4a86e8) — matches Word's revision-balloon convention of showing a blue→for tab characters while the surrounding deletion text stays red.SD-2939 integration
The
.superdoc-tabclass on the literal-tab span makes it automatically pick up Caio's "Show formatting marks" toggle from SD-2939 — no new mechanism added:[ ]— two-space strikethrough strut between deletion brackets[→]— existing.superdoc-tab::after { content: "→" }overlay paints the arrow centered on the spanLive-verified with
superdocdev.editor.presentationEditor.setShowFormattingMarks(true/false).Round-trip preservation
The PM document is never modified —
expandRunsForInlineTabsis a pure transform that lives only in the layout pipeline. The literal\tstays on the trackDelete-marked text node in PM, and export writes<w:delText>[\t]</w:delText>verbatim.Verified end-to-end:
text: "[\t]"withtrackDelete{ id: "...", sourceId: "103" }.deletedTextkeeps raw\t(used bycommentsExporter.js:49to writecustom:trackedDeletedTextback unchanged).word/document.xmlbyte-scan: exactly one<w:delText>element, content'[\t]', exactly one U+0009 character document-wide,w:id/w:author/w:dateall preserved from the import.Tests
@superdoc/contractsexpandRunsForInlineTabs)@superdoc/measuring-dom@superdoc/painter-dom@superdoc/pm-adaptersuperdocsuper-editorTwo
renderer-hanging-indent.test.tsfixtures had hand-rolledrunIndex: 0segments that depended on no expansion; replaced literal\twith a space (test intent is positioning, not tab semantics).Test plan
pnpm install,pnpm dev.[ ]as a compact 2-space placeholder where the deleted[\t]lived — no missing text, no inline tab-stop gap.Show formatting marks— body switches to[→]overlay; sidebar arrow unchanged.[+ blue→+]for each tab.pnpm --filter @superdoc/contracts test,pnpm --filter @superdoc/measuring-dom test,pnpm --filter @superdoc/painter-dom test,pnpm --filter @superdoc/pm-adapter test,pnpm --filter superdoc testall pass.word/document.xml→<w:delText>[\t]</w:delText>survives byte-perfect.