fix(layout): never wrap at a trailing space that only precedes an explicit break - #3948
Conversation
…licit break A space that overflows the measure right before a <w:br/> (or the paragraph end) used to wrap onto its own line, and the break then closed that line — rendering a spurious empty line that Word does not show (three .superdoc-line elements instead of two). Word never measures trailing spaces for line fitting: they hang past the text margin, so a space immediately before a hard break can never be a wrap point. Guard both measurers: the primary DOM measurer (both the all-space-segment and the empty-token overflow branches) and the incremental remeasurer, which otherwise reintroduces the phantom line on re-layout. The new condition only engages when every run between the space and the next explicit break (or the paragraph end) contributes no non-space content, so general word wrapping is untouched. Fixes superdoc#3946 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Qodo reviews are paused for this user.Troubleshooting steps vary by plan Learn more → On a Teams plan? Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center? |
|
All contributors have signed the CLA ✍️ ✅ |
There was a problem hiding this comment.
1 issue found across 4 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:2776">
P2: The new `spacesHangBeforeBreak` treats ANY `kind === 'break'` run (including page/column breaks) as a line-closing break, but the rest of measuring/dom (lines 1566, 2803) uses `isLineBreakRun(run) || (run.kind === 'break' && breakType === 'line')` for that predicate. The mirror implementation in layout-bridge's `trailingSpacesHang` correctly uses `isLineBreakRun(nextRun)`, which only accepts `break` with `breakType === 'line'`. This divergence means a trailing space that overflows before a page/column break hangs in the primary measurer but wraps in the incremental remeasure, so the phantom-line bug #3946 can reappear during reflow for non-line breaks.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| const spacesHangBeforeBreak = (startRunIndex: number): boolean => { | ||
| for (let i = startRunIndex + 1; i < runsToProcess.length; i++) { | ||
| const nextRun = runsToProcess[i] as Run; | ||
| if (nextRun.kind === 'lineBreak' || nextRun.kind === 'break') return true; |
There was a problem hiding this comment.
P2: The new spacesHangBeforeBreak treats ANY kind === 'break' run (including page/column breaks) as a line-closing break, but the rest of measuring/dom (lines 1566, 2803) uses isLineBreakRun(run) || (run.kind === 'break' && breakType === 'line') for that predicate. The mirror implementation in layout-bridge's trailingSpacesHang correctly uses isLineBreakRun(nextRun), which only accepts break with breakType === 'line'. This divergence means a trailing space that overflows before a page/column break hangs in the primary measurer but wraps in the incremental remeasure, so the phantom-line bug #3946 can reappear during reflow for non-line breaks.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/layout-engine/measuring/dom/src/index.ts, line 2776:
<comment>The new `spacesHangBeforeBreak` treats ANY `kind === 'break'` run (including page/column breaks) as a line-closing break, but the rest of measuring/dom (lines 1566, 2803) uses `isLineBreakRun(run) || (run.kind === 'break' && breakType === 'line')` for that predicate. The mirror implementation in layout-bridge's `trailingSpacesHang` correctly uses `isLineBreakRun(nextRun)`, which only accepts `break` with `breakType === 'line'`. This divergence means a trailing space that overflows before a page/column break hangs in the primary measurer but wraps in the incremental remeasure, so the phantom-line bug #3946 can reappear during reflow for non-line breaks.</comment>
<file context>
@@ -2760,6 +2760,29 @@ async function measureParagraphBlock(
+ const spacesHangBeforeBreak = (startRunIndex: number): boolean => {
+ for (let i = startRunIndex + 1; i < runsToProcess.length; i++) {
+ const nextRun = runsToProcess[i] as Run;
+ if (nextRun.kind === 'lineBreak' || nextRun.kind === 'break') return true;
+ if (isVanishedRun(nextRun)) continue;
+ const isPlainTextRun = !nextRun.kind || nextRun.kind === 'text';
</file context>
…ntics Precompute the trailing-space suffix classification lazily in one backward pass per measured paragraph (and per remeasure call), so overflow checks stay O(1) even when trailing spaces span many independently styled runs; the remeasure cache also records each run's all-space tail start so the per-character check no longer rescans the run text. Document why the two measurers intentionally differ on non-line breaks: the primary measurer closes the current line for every 'break' kind, so a space that only precedes a page/column break is line-trailing there (covered by a new test), while remeasurement treats those runs as zero-width passthroughs that do not close the line, so only true line breaks qualify. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
1 existing issue remains and 1 new issue found across 3 files (changes from recent commits).
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/layout-bridge/src/remeasure.ts">
<violation number="1" location="packages/layout-engine/layout-bridge/src/remeasure.ts:1807">
P1: When a paragraph contains a MathRun and an overflowing space triggers `trailingSpacesHang`, `computeTrailingSpacesHangCache` throws because MathRun has no `text` property. Exclude MathRun from `isTextRun` or from this cache before reading `run.text`; otherwise incremental remeasurement fails for affected paragraphs.</violation>
</file>
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
| for (let i = runs.length - 1; i >= 0; i -= 1) { | ||
| const run = runs[i]; | ||
| if (isTextRun(run)) { | ||
| let lastNonSpace = run.text.length - 1; |
There was a problem hiding this comment.
P1: When a paragraph contains a MathRun and an overflowing space triggers trailingSpacesHang, computeTrailingSpacesHangCache throws because MathRun has no text property. Exclude MathRun from isTextRun or from this cache before reading run.text; otherwise incremental remeasurement fails for affected paragraphs.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/layout-engine/layout-bridge/src/remeasure.ts, line 1807:
<comment>When a paragraph contains a MathRun and an overflowing space triggers `trailingSpacesHang`, `computeTrailingSpacesHangCache` throws because MathRun has no `text` property. Exclude MathRun from `isTextRun` or from this cache before reading `run.text`; otherwise incremental remeasurement fails for affected paragraphs.</comment>
<file context>
@@ -1781,6 +1781,55 @@ export function remeasureParagraph(
+ for (let i = runs.length - 1; i >= 0; i -= 1) {
+ const run = runs[i];
+ if (isTextRun(run)) {
+ let lastNonSpace = run.text.length - 1;
+ while (lastNonSpace >= 0 && run.text[lastNonSpace] === ' ') lastNonSpace -= 1;
+ tailAllSpaceFrom[i] = lastNonSpace + 1;
</file context>
|
recheck |
What
A single trailing space immediately before
<w:br/>rendered a spurious empty line whenever that space happened to land at the line-wrap boundary — three.superdoc-lineelements instead of two, while Word renders the same paragraph without the empty line (#3946).Mechanism
Word never measures trailing spaces for line fitting: they hang past the text margin, so a space immediately before a hard break (which is at a line end by definition) can never force a wrap. In the primary DOM measurer (
measuring/dom/src/index.ts,measureParagraphBlock) the overflow check for a space token didn't know what follows the space, so whencurrentLine.width + spaceWidthexceeded the measure it closed the line and opened a new one containing only the space. The explicit-break handler then closed that space-only line as a real line — the phantom empty line. Both entry paths reproduce it: a space as the tail of a text run (emptysplit(' ')token branch) and a space as its own run (all-space-segment branch).The incremental remeasurer (
layout-bridge/src/remeasure.ts) has the mirror bug: on overflow at' 'it consumed the space at zero width but still force-closed the line, so re-layout would reintroduce the phantom line even after the primary measurer was fixed.Fix
A space is not a wrap point when every run between it and the next explicit break (or the paragraph end) contributes no non-space content — it stays on the current line and hangs, and the break then closes that line. Both measurers get the same guard (
spacesHangBeforeBreak/trailingSpacesHang); general word wrapping is untouched because the condition only engages for line-trailing spaces.One known sub-pixel inaccuracy: for a justified line closed by a soft break, the primary measurer charges the hanging space's width (~4px) into the line width, producing marginal space compression where Word purely overhangs. That is visually imperceptible and strictly better than the phantom blank line.
Testing
measuring/dom531/531,layout-bridge1655/1655,painters/dom1545/1545,layout-engine/tests(integration + parity) 275/275.Fixes #3946