fix(layout): render inline boxes on RTL lines - #3980
Conversation
The layout-aware inline-box tier was inert for any paragraph declaring `inlineDirection: 'rtl'` and for any run carrying `w:rtl`. Measured on the `inline-box-poc` fixture: an RTL paragraph projected 0 boxes and its line breaks never changed at any width from 140px to 340px, while the identical LTR paragraph projected 6 and changed them. Hebrew text without a direction declaration projected 5 and changed them, so the gate was the flag and not the script. Two independent gates caused it, and both are lifted here. `applyInlineBoxStyle` wrote physical edges — `padding-left`/`right`, `margin-left`/`right`, `border-left`/`right` and the four physical corner radii — each keyed on `isFirstLeaf`/`isLastLeaf`. Those indices are logical: they walk the covered leaves in document order. In an RTL line the logical first leaf is the visually rightmost one, so the edges landed on the wrong side, and `paintInlineBoxes` bailed on `isRtl` rather than render that. It now writes the logical equivalents, which the browser resolves against the element's own direction; the DOM renderer already sets `dir` on the line, so no direction arithmetic is added. The `isRtl` parameter is gone, unused. The measurement filter then had nothing left to protect and its two direction clauses are removed. Box advances are widths, and a width does not depend on which way the line runs. Dropping them removes an asymmetry rather than adding one. A logical range that straddles a direction change maps to more than one visual segment, and the first/last-leaf edges cannot express that — but that is equally reachable today in an LTR paragraph containing a Hebrew phrase, which ships. Gating only the RTL side held RTL to a stricter standard while leaving the shared limitation in place. Tests: `inline-box-rtl.test.ts` asserts that an RTL paragraph measures identically to the same paragraph in LTR — same line breaks, same projected box count — and that a box still changes where an RTL paragraph wraps. The painter suite gains a case proving an RTL line paints the same logical edges as an LTR one, and a guard that no physical inline edge is written at all. The existing "fails closed for RTL lines" case is replaced: it pinned the behaviour this fixes.
The POC pinned `paddingLeft` and `borderLeftWidth`, which the painter no longer writes. Reads the logical equivalents instead; the border assertion uses the shorthand because jsdom does not expand it into the longhand.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 6 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/painters/dom/src/runs/inline-box.ts">
<violation number="1" location="packages/layout-engine/painters/dom/src/runs/inline-box.ts:149">
P2: This logical assignment leaves the existing `render-line.test.ts` physical-padding assertions empty, so the painter suite fails even for LTR inline boxes. Update that test to assert `paddingInlineStart`/`paddingInlineEnd` and the other logical edges changed here.</violation>
<violation number="2" location="packages/layout-engine/painters/dom/src/runs/inline-box.ts:169">
P2: The painter's doc comment claims ranges that do not form one visual run are filtered out during measurement before they reach the painter, but this PR's measurement change removes exactly that filter. Before this change, the clause `block.runs.every((run) => !isTextRun(run) || run.bidi?.rtl !== true)` dropped boxes for *every* paragraph containing an RTL run — including LTR paragraphs with a Hebrew phrase — so the painter never saw a mixed-direction range. After removal, a box whose range straddles an LTR↔RTL run boundary now reaches painting, and the painter's first/last-leaf logical edges cannot express the multiple visual segments it maps to (each leaf's `dir` resolves its own start/end, so the box gets broken/wrong corners and edges). This is a real reachability expansion for LTR mixed-bidi paragraphs, not merely RTL parity, and the doc comment no longer reflects the code's guarantee.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| */ | ||
| element.style.paddingBlockStart = `${box.style.paddingBlockStart}px`; | ||
| element.style.paddingBlockEnd = `${box.style.paddingBlockEnd}px`; | ||
| element.style.paddingInlineStart = isFirstLeaf ? `${box.style.paddingInlineStart}px` : '0px'; |
There was a problem hiding this comment.
P2: This logical assignment leaves the existing render-line.test.ts physical-padding assertions empty, so the painter suite fails even for LTR inline boxes. Update that test to assert paddingInlineStart/paddingInlineEnd and the other logical edges changed here.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/layout-engine/painters/dom/src/runs/inline-box.ts, line 149:
<comment>This logical assignment leaves the existing `render-line.test.ts` physical-padding assertions empty, so the painter suite fails even for LTR inline boxes. Update that test to assert `paddingInlineStart`/`paddingInlineEnd` and the other logical edges changed here.</comment>
<file context>
@@ -132,34 +132,47 @@ const applyInlineBoxStyle = (
+ */
+ element.style.paddingBlockStart = `${box.style.paddingBlockStart}px`;
+ element.style.paddingBlockEnd = `${box.style.paddingBlockEnd}px`;
+ element.style.paddingInlineStart = isFirstLeaf ? `${box.style.paddingInlineStart}px` : '0px';
+ element.style.paddingInlineEnd = isLastLeaf ? `${box.style.paddingInlineEnd}px` : '0px';
+ element.style.marginInlineStart = isFirstLeaf && box.startsRange ? `${box.style.gapBefore}px` : '0px';
</file context>
There was a problem hiding this comment.
Fixed in 5703a90 — render-line.test.ts now asserts paddingInlineStart/paddingInlineEnd. This was exactly the CI failure (Packages, two cases; CI V2 Public / validate failing in 2s was just the aggregate gate reporting it). All 22 checks are green on the merged branch now.
Root cause on my side: I verified with the individual test files instead of the package, which is how a third file asserting the old edges got past me. vp test run --root packages/layout-engine/painters/dom gives 60/61 files and 1570/1571 tests; the one failure is persistent-page-surface.test.ts (1,003 pages) timing out at 16,807ms against a 5,000ms limit, measured three times on unmodified code at 27.7s / 17.4s / 16.8s under different load.
| * Paints measured inline-box slices on the canonical text leaves. | ||
| * | ||
| * Direction-agnostic: every edge is written on a logical axis, so the same call | ||
| * is correct for an LTR and an RTL line. Ranges whose leaves do not form one |
There was a problem hiding this comment.
P2: The painter's doc comment claims ranges that do not form one visual run are filtered out during measurement before they reach the painter, but this PR's measurement change removes exactly that filter. Before this change, the clause block.runs.every((run) => !isTextRun(run) || run.bidi?.rtl !== true) dropped boxes for every paragraph containing an RTL run — including LTR paragraphs with a Hebrew phrase — so the painter never saw a mixed-direction range. After removal, a box whose range straddles an LTR↔RTL run boundary now reaches painting, and the painter's first/last-leaf logical edges cannot express the multiple visual segments it maps to (each leaf's dir resolves its own start/end, so the box gets broken/wrong corners and edges). This is a real reachability expansion for LTR mixed-bidi paragraphs, not merely RTL parity, and the doc comment no longer reflects the code's guarantee.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/layout-engine/painters/dom/src/runs/inline-box.ts, line 169:
<comment>The painter's doc comment claims ranges that do not form one visual run are filtered out during measurement before they reach the painter, but this PR's measurement change removes exactly that filter. Before this change, the clause `block.runs.every((run) => !isTextRun(run) || run.bidi?.rtl !== true)` dropped boxes for *every* paragraph containing an RTL run — including LTR paragraphs with a Hebrew phrase — so the painter never saw a mixed-direction range. After removal, a box whose range straddles an LTR↔RTL run boundary now reaches painting, and the painter's first/last-leaf logical edges cannot express the multiple visual segments it maps to (each leaf's `dir` resolves its own start/end, so the box gets broken/wrong corners and edges). This is a real reachability expansion for LTR mixed-bidi paragraphs, not merely RTL parity, and the doc comment no longer reflects the code's guarantee.</comment>
<file context>
@@ -132,34 +132,47 @@ const applyInlineBoxStyle = (
+ * Paints measured inline-box slices on the canonical text leaves.
+ *
+ * Direction-agnostic: every edge is written on a logical axis, so the same call
+ * is correct for an LTR and an RTL line. Ranges whose leaves do not form one
+ * visual run are filtered out during measurement, before they reach here.
+ */
</file context>
There was a problem hiding this comment.
You are right that the doc comment was false, and it is fixed in 5703a90 — here and in measuring/dom/src/index.ts. Nothing filters ranges that straddle a direction change, and the comment now says so instead of promising a guarantee that never existed.
One correction to the reasoning, measured rather than argued. The removed clause keys on the declaration, not on the script:
block.runs.every((run) => !isTextRun(run) || run.bidi?.rtl !== true)The Unicode Bidi Algorithm reorders a Hebrew phrase inside a Latin paragraph into an RTL visual run whether or not anything declares w:rtl — direction comes from the character classes, not from the attribute. So an LTR paragraph whose Hebrew declares nothing was never filtered, and a box straddling that boundary already reached the painter with exactly the broken edges you describe.
I verified by restoring the old clause and re-running inline-box-rtl.test.ts:
| with the pre-PR clause restored | |
|---|---|
is already reachable when the Hebrew declares no direction |
passes |
is reachable when the Hebrew declares w:rtl, exactly as when it does not |
fails — expected +0 to be 1 |
| the other five RTL cases | fail |
Six of seven fail; the one that keeps passing is the undeclared case. Both of those tests are new in this PR so the point stays pinned.
That is the argument for parity over a narrower gate: a guard keyed on the declaration catches the declared case and is blind to the identical undeclared one, so it buys consistency rather than correctness. It does not make mixed bidi correct — that needs painting per visual segment, which touches Line.segments, and #3979 keeps it open. If the maintainers would rather have the narrow gate anyway, say so and I will add it.
…di claim Addresses the three findings on superdoc#3980. render-line.test.ts still asserted `paddingLeft`/`paddingRight` on a painted inline box. The painter writes logical edges now, so both cases read '' and the painter suite failed even for LTR. They assert `paddingInlineStart`/`End`. Verified by running the whole package rather than the individual file, which is how the third test file was missed: 60/61 files pass, the one failure being a 1,003-page timeout unrelated to inline boxes. The painter's doc comment claimed measurement filters out ranges that do not form one visual run. It never did. The clause removed by this PR keyed on the declaration (`run.bidi?.rtl`), while the Unicode Bidi Algorithm reorders a Hebrew phrase inside a Latin paragraph whether or not anything declares it, so a box straddling that boundary already reached the painter. Both comments now say what the code actually guarantees, and two tests pin it: restoring the old clause leaves `is already reachable when the Hebrew declares no direction` passing while the other six fail. That is the argument for parity over a narrower gate -- such a gate would catch the declared case and miss the undeclared one, which is the same defect. The two wrapping assertions searched no width, so they depended on 140px being decisive under the resolved Arial metrics. They now use a `findDecisiveWidth` style search, as the POC fixture they are modelled on already does.
|
All three findings were valid. Fixed in 5703a90, with 1.
|
| with the pre-PR clause restored | |
|---|---|
is already reachable when the Hebrew declares no direction |
passes |
is reachable when the Hebrew declares w:rtl, exactly as when it does not |
fails — expected +0 to be 1 |
| the other five RTL cases | fail |
Six of seven fail, and the one that keeps passing is the undeclared case. That is the argument for parity over a narrower gate: a guard keyed on the declaration catches the declared case and is blind to the identical undeclared one, so it buys consistency rather than correctness.
What it does not do is make mixed bidi correct. That still needs painting per visual segment, which touches Line.segments rather than this file, and #3979 keeps it as an open option. If you would rather have the narrow gate anyway — declared cases fail closed, undeclared stays as it is — say so and I will add it; it is a small change and I have no attachment to the current choice.
3. Hardcoded WIDTH = 140 — correct
The two wrapping assertions now search for a decisive width the way findDecisiveWidth in inline-box-poc.test.ts does, and throw with the full width-by-width comparison when none is found. They pin "some width must be decisive" instead of a resolved Arial metric.
1 participant
Relates to #3979
What changed
The layout-aware inline-box tier was inert for any paragraph declaring
inlineDirection: 'rtl'and for any run carryingw:rtl. Two independent gates caused it, and both are lifted here.Painter —
packages/layout-engine/painters/dom/src/runs/inline-box.ts.applyInlineBoxStylewrote physical edges:padding-left/right,margin-left/right,border-left/right, and the four physical corner radii, each keyed onisFirstLeaf/isLastLeaf. Those indices are logical — they walk the covered leaves in document order — so in an RTL line the logical first leaf is the visually rightmost one and every edge landed on the wrong side.paintInlineBoxesbailed onisRtlrather than render that, which was the correct guard for the code as written.It now writes the logical equivalents (
padding-inline-start/end,margin-inline-start/end,border-inline-start/end,border-{start,end}-{start,end}-radius). The browser resolves them against the element's own computed direction, and the DOM renderer already setsdiron the line element —painters/dom/src/index.test.tsassertsline.dir === 'rtl'in several places — so the leaf inherits it and no direction arithmetic is added. TheisRtlparameter is removed; both call sites inrender-line.tsare updated. It is not exported from the package index, so this is not a public API change.Measurement —
packages/layout-engine/measuring/dom/src/index.ts. With the painter direction-agnostic, the filter's two direction clauses had nothing left to protect and are removed. Box advances are widths, and a width does not depend on which way the line runs.Why this removes an asymmetry rather than adding a risk
A logical
[from, to)range that straddles a direction change maps to more than one visual segment, and the first/last-leaf edges cannot express that. That is already reachable in LTR today — a Hebrew phrase inside an English paragraph — and it ships. Gating only the RTL side held RTL to a stricter standard while leaving the shared limitation in place.#3979 lays out the alternatives (fail closed on mixed bidi; paint per visual segment). This PR implements parity with LTR because it is the smallest diff and does not commit the project to a segment model. Say the word before review and I will rework it.
Reproduction
Fixture and method modelled on the existing
layout-bridge/test/inline-box-poc.test.ts: one paragraph, one box spanning the middle words, measured withmeasureBlockat every width from 140px to 340px, comparing line breaks with and without the box.inlineDirection: 'rtl'bidi.rtl: trueThe last row is why this is not a script limitation: the same glyphs work when nothing declares direction.
Paint side, same measured box, only
isRtldiffering: 1 leaf styled vs 0, and the styled leaf carriedpadding-left: 24pxwithpadding-inline-startnever set.Tests
layout-bridge/test/inline-box-rtl.test.ts(new, 5 cases). The strongest claim it makes: an RTL paragraph must measure identically to the same paragraph in LTR — same line breaks, same projected box count — because widths do not depend on direction. Also that a box still changes where an RTL paragraph wraps, and that Hebrew declaring its direction works.painters/dom/src/runs/inline-box.test.ts:fails closed for RTL linesis replaced — it pinned the behaviour this fixes. Two cases take its place: an RTL line paints the same logical edges as an LTR one, and a guard that no physical inline edge is written at all, so reintroducingpaddingLeftcannot pass silently.layout-bridge/test/inline-box-poc.test.ts: two assertions moved to the logical properties. jsdom 27.3.0 supports all twelve for set and read-back, so this is fully testable in the existing environment; the border assertion reads the shorthand because jsdom does not expand it into the longhand.Checks
Run from the merge with
origin/mainat 165d7d6:measuring/domreports 16 failed / 536 passed, and that is unchanged by this PR. I measured it both ways: with the two direction clauses restored and with them removed, the suite gives byte-identical counts, and the one inline-box case among the sixteen (releases a line edge budget when the box moves to the next line) fails with the clauses restored as well. They are pre-existing on the merge base, in unrelated areas — tabs, TOC,tblW,atLeastlineRule.I could not run
build:superdoclocally:build-public-superdoc.mjscallsspawn('pnpm', …)withoutshell, which isENOENTon Windows, so both surfaces fail before printing anything and it reportsAggregateError: Multiple public surface builds failed. That is a Windows toolchain issue unrelated to this change.Notes
tests/consumer-typecheck/fixture is needed.