fix(layout): place the first column on the right in RTL sections - #3953
fix(layout): place the first column on the right in RTL sections#3953Nathaniel-260 wants to merge 5 commits into
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 9 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Code Review by Qodo
🔴 High 1. No producer wires w:bidi into ColumnLayout.direction
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| /** | ||
| * Section page direction, from `w:sectPr/w:bidi`. Decides which side the FIRST column sits on: | ||
| * `'ltr'` (default) fills left to right, `'rtl'` fills right to left, matching Word. | ||
| * | ||
| * Per ECMA-376 §17.6.1 a section's `w:bidi` governs section-level chrome — page numbers, gutters | ||
| * and columns — and is independent of the paragraph inline direction (§17.3.1.6). It is carried | ||
| * here, on the column layout itself, because `getColumnGeometry` is the single source every | ||
| * column consumer reads for positioning (fill, hit testing, separators, balancing, floating | ||
| * anchors, footnotes); threading the axis alongside the widths keeps those consumers from having | ||
| * to re-derive it, and keeps them from disagreeing. | ||
| * | ||
| * Absent means `'ltr'`. Every existing producer therefore keeps its current geometry unchanged. | ||
| */ | ||
| direction?: BaseDirection; |
There was a problem hiding this comment.
🔴 High
4. No producer wires w:bidi into columnlayout.direction 🐞 Bug ≡ Correctness
The consumer-side RTL geometry pipeline only activates when ColumnLayout.direction is explicitly set, but no production path bridges the existing section-level pageDirection/w:sectPr/w:bidi signal into SectionBreakBlock.columns.direction; only hand-built tests set direction: 'rtl'. Consequently, real imported Hebrew or Arabic multi-column documents retain an undefined direction that defaults to LTR, so the first column remains on the left and the reported selection bug (#3952) is not fixed for actual users.
Agent Prompt
## Issue description
RTL column mirroring and its related geometry behavior are opt-in through `ColumnLayout.direction`, but the production import/conversion pipeline never assigns the resolved section bidi value to that field. Real sections carrying `w:sectPr/w:bidi` therefore reach layout with an undefined direction, which defaults to LTR, so the fix for #3952 works only for manually constructed test data rather than actual Hebrew or Arabic documents.
## Issue Context
`SectionDirectionContext.pageDirection` is the existing section-level signal documented as being resolved from `w:sectPr/w:bidi` and feeding section chrome such as columns. However, section layout snapshots and scheduling only carry `SectionBreakBlock.columns`; section processing clones that object, and normalization copies `input.direction` only if it was already populated.
`SectionsAdapter.setSectionDirection` is declared as a document-API interface, but no production implementation was found in this repository. The only identified OOXML section-properties parser, the test helper `readSectPr`, reads `<w:cols>` into `{count, gap}` but does not read `<w:bidi>` or set the column direction, and the documented resolver chain under `pm-adapter/src/direction/` was not found. Existing RTL column tests manually supply `direction: 'rtl'`, so they do not validate the document-to-flow-block path.
Wire the resolved section page direction into `SectionBreakBlock.columns.direction` when section-break and column metadata are produced, preserve it through section snapshots and normalization, and add an end-to-end test that begins with a section carrying `w:sectPr/w:bidi` rather than a hand-built `ColumnLayout`. If the production OOXML wiring exists elsewhere, link it explicitly in the PR description and cover it with an integration test; otherwise, treat the import-pipeline change as required before the feature reaches users.
## Fix Focus Areas
- packages/layout-engine/contracts/src/direction-context.ts[40-52]
- packages/layout-engine/contracts/src/index.ts[1994-2036]
- packages/layout-engine/contracts/src/index.ts[2890-2903]
- packages/layout-engine/layout-engine/src/section-props.ts[25-28]
- packages/layout-engine/tests/src/test-helpers/to-flow-blocks.ts[147-188]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
I have read the CLA Document and I hereby sign the CLA |
A section carrying `w:sectPr/w:bidi` fills its columns left to right, so the first paragraph of a Hebrew two-column section lands in the LEFT column. Word puts it on the right (ECMA-376 §17.6.1), and `SectionDirectionContext` already documents `pageDirection` as governing columns -- but no function in the column geometry ever received a direction. `ColumnLayout` now carries an optional `direction`, and `buildColumnGeometry` mirrors the strip about the CONTENT AREA when it is `'rtl'`. Indices stay in fill order, so every consumer that walks columns 0..n-1 keeps filling in document order and only the painted x changes; fill, hit testing, separators, balancing, floating anchors and footnotes all follow from that single source. The mirror axis is the content area and not the strip's own span because explicit widths are not scaled to fill it -- a strip that underfills must end up against the right margin with the slack on the left. Four consumers needed direction awareness of their own, and each failed silently without it: - `getColumnAtX` walked the geometry assuming x ascends with the index. - `toBalancingColumns` rebuilt the layout field by field and dropped the axis, so the balanced last page of an RTL section laid out left to right while every earlier page of the same section laid out right to left. - Footnote column attribution broke on the first match under the same ascending assumption, collapsing a page's notes into column 0: the left column's notes printed under the right column and its own note area stayed empty. - The DOM painter's separator gate read "content past the separator" as "content to the right", so a section whose content never left the first column drew a line Word does not draw. Absent `direction`, every path is byte-identical to before: verified across 46,080 comparisons of geometry and hit testing over 960 LTR configurations and 6 content widths.
…s end to end Follow-up to the RTL column-order fix in this branch, addressing review feedback on superdoc#3953. `balanceSectionOnPage` reconstructed document order by sorting the page's fragments on ASCENDING x, on the premise that the paginator fills column 0 first. That premise inverts under this branch: in an RTL section column 0 is the RIGHT column, so document order DESCENDS in x. The balancer consumed the trailing column first and wrote the balanced x/y back in that order, which scrambles the reading order of a balanced page rather than merely mirroring it. Measured on a 2-column RTL page of 6 paragraphs: x came back as [432, 96, 96, 96, 432, 432] instead of [432, 432, 432, 96, 96, 96]. The sort is now direction-relative. The existing RTL balancing test could not catch this because its fixture places every fragment at the same x, which makes the ascending sort a stable no-op. Two smaller geometry corrections: - A single column is mirrored too. The old guard skipped `count < 2`, so an explicit one-column section that underfills the content area stayed pinned to the LEFT margin, contradicting the axis rule the multi-column path applies. It is a provable no-op whenever the column fills the area, so equal-mode `count: 1` is byte-identical. - Per-column `gaps` are clamped to >= 0, matching the scalar `gap` above. OOXML cannot express a negative gutter (`w:space` is unsigned), but a hand-built layout could, and a gap negative enough to pull a column behind its predecessor would make an upright LTR strip answer hit tests as if it were mirrored. `ColumnLayoutForAnchor` and `ParagraphAnchorsContext.columns` now declare `direction` and `contentWidth`. Runtime was already correct because every caller passes a full normalized layout, but neither would have produced a type error if a future edit dropped the fields -- the exact failure mode that made the `toBalancingColumns` fix necessary. Coverage. Three paths in the previous commit survived mutation: `toBalancingColumns` dropping both spreads, the footnote column boundary reverted to its LTR-only form, and `determineColumn` in position-hit, which had no RTL coverage at all. Each now has a test that fails without its fix, and position-hit also covers three columns, which nothing exercised before. `tests/src/test-helpers/to-flow-blocks.ts` reads `w:sectPr/w:bidi` (ST_OnOff, so a bare element means on) and sets `columns.direction`, which makes `section-breaks-rtl-columns.test.ts` an end-to-end check from OOXML section properties down to fragment x. This is a TEST adapter and does not reach real documents: the production PM/OOXML adapter is not in this repository. It does double as a precise reference for what that adapter must do. Adds a consumer-typecheck fixture for the new public `ColumnLayout.direction`, reachable from outside the package through `Layout.columns`.
939524e to
99cc234
Compare
|
Thanks — both findings were right, and both are addressed in 99cc234. High — no producer wires
So the branch is the complete engine half, proven end to end through an adapter that doubles as a reference for what the external one must do. I have asked the maintainers for the seam where Medium — no consumer fixture. Added Your review also prompted a re-audit that found something neither of us had flagged: Three more paths that survived mutation now have tests that do not: |
…trails Review follow-up on superdoc#3953. The RTL branch of the separator gate tested the fragment's LEFT edge, the same edge the LTR branch tests, which leaves the two asymmetric for anything wider than a column. `page.items` carries anchored drawings alongside column content, so a page-relative watermark or logo sits at `x = 0` spanning the page. Going right it is never past the separator; going left, a left-edge test always puts it past. An RTL section with `w:sep="1"` whose text all fits in the first column therefore drew a separator on the strength of the watermark alone -- a line Word does not draw, which is exactly what this gate exists to prevent. Each branch now tests the edge that trails in its own fill direction: the left edge going right, the right edge going left.
|
Third round of review follow-up — one more real defect, in code this PR added. A QA pass over the combined change found that the RTL branch of the separator gate tested the fragment's left edge, the same edge the LTR branch tests. That leaves the two branches asymmetric for anything wider than a column, and So an RTL section with The same pass verified the rest numerically against the source and found nothing else:
One pre-existing issue surfaced that is not part of this PR and is direction-independent, noted only so it is not lost: |
… not by an edge The column-separator gate asks whether a LATER column holds content, because Word draws no line next to an empty column. It answered by comparing a fragment edge against the separator x, choosing whichever edge trails in the fill direction. No edge can answer that question. Content wider than its column does not sit inside it, and `resolveTableFrame` places an over-wide table at a NEGATIVE offset from its column whenever the table is right-aligned or centred -- and `end` is the default justification for any bidiVisual table. So in an RTL section a wide table starts left of its own column and ends past the separator, while never having left the later column: both of its edges lie on the wrong side, and so does its origin. A negative `w:ind` puts a paragraph's origin in the gutter with the same effect. Use `fragment.columnIndex` instead -- the engine's own record of the owning column, written for paragraphs and tables as they are laid out, and documented as the field to trust "when overflow crosses margins". Geometry is the fallback for a fragment carrying no such record, and it is containment rather than `getColumnAtX` because containment can answer "no column": that is what keeps page-anchored objects out of the gate, a full-width watermark belonging to none. `findColumnContaining` is the new contracts helper for that fallback, the strict counterpart to `getColumnAtX`, which must clamp because a click has to select something. Its spans are half-open so that columns authored with no gutter do not both claim the boundary they share -- the boundary is exactly where the later column's content begins, and an inclusive bound would give it to the earlier column in LTR but not in RTL, making the two directions disagree. The painter's private separator helper now returns the geometry rather than bare x positions, so each separator stays paired with the column it follows instead of relying on array-index alignment. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
…ts origin Review follow-up on superdoc#3953. The separator gate reads `fragment.columnIndex` first and falls back to geometry, but the fallback tested containment of the fragment's ORIGIN, and the previous commit's rationale assumed the engine records `columnIndex` for paragraphs. It does not: the paginator writes it for tables (layout-table.ts) and for footnote bodies, and nowhere for an ordinary paragraph fragment. Paragraphs therefore always reach the fallback. That matters because a paragraph's origin can sit outside its own column. A negative `w:ind` hangs it into the gutter, and containment then answers "no column" -- so a later column holding only an outdented paragraph registered as empty and its separator was suppressed, a line Word draws. The same shape applies to an over-wide right-aligned or centred table, which `resolveTableFrame` places at a negative offset from its column. Attribution is now by overlap: the column whose span the fragment covers most, ties going to the earliest in fill order. Anything at least as wide as the whole content area still belongs to no column, which is what keeps page-anchored objects out of the gate -- a full-width watermark overlaps every column without being content of any, and counting it would draw a separator on a page whose text never left the first column. Both directions are covered: an outdented paragraph alone in a later column now draws its separator, and the watermark case still does not.
|
Valid, and it went further than the report — thanks. Fixed in You were right that production paragraph fragments do not carry So paragraphs always take the fallback, which means it has to be right on its own rather than as a rare backstop. Containment of the origin is not right: a negative Rather than populating The one property the fallback still has to keep is the ability to answer "no column", since that is what excludes page-anchored objects: a full-width watermark overlaps every column without being content of any, and counting it would draw a separator on a page whose text never left the first column. So anything at least as wide as the content area is attributed to no column. That rule also catches an over-wide table reaching the fallback without a recorded column, where Both directions are covered by tests now, and both were mutation-checked: the outdented-paragraph case fails with the containment form restored, and the watermark case fails with the earlier left-edge form restored.
|
There was a problem hiding this comment.
1 issue found across 2 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/painters/dom/src/renderer.ts">
<violation number="1" location="packages/layout-engine/painters/dom/src/renderer.ts:1083">
P2: When explicit column widths overflow the content area, this guard treats a full-width anchored object as column content if its width is smaller than the overfull geometry span. Pass the page `contentWidth` into `columnOwningSpan` and reject spans at least that wide before overlap attribution.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| const span = Number.isFinite(width) && width > 0 ? width : 0; | ||
| const contentStart = Math.min(...geometry.map((col) => col.x)); | ||
| const contentEnd = Math.max(...geometry.map((col) => col.x + col.width)); | ||
| if (span >= contentEnd - contentStart) return null; |
There was a problem hiding this comment.
P2: When explicit column widths overflow the content area, this guard treats a full-width anchored object as column content if its width is smaller than the overfull geometry span. Pass the page contentWidth into columnOwningSpan and reject spans at least that wide before overlap attribution.
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/renderer.ts, line 1083:
<comment>When explicit column widths overflow the content area, this guard treats a full-width anchored object as column content if its width is smaller than the overfull geometry span. Pass the page `contentWidth` into `columnOwningSpan` and reject spans at least that wide before overlap attribution.</comment>
<file context>
@@ -1054,6 +1054,46 @@ function svgEffectColor(value: TextEffectColor): string | undefined {
+ const span = Number.isFinite(width) && width > 0 ? width : 0;
+ const contentStart = Math.min(...geometry.map((col) => col.x));
+ const contentEnd = Math.max(...geometry.map((col) => col.x + col.width));
+ if (span >= contentEnd - contentStart) return null;
+
+ let best: number | null = null;
</file context>
1 participant
Fixes #3952.
The problem
A section carrying
w:sectPr/w:bidifills its columns left to right, so the first paragraph of a two-column Hebrew section lands in the left column. Word puts it on the right (ECMA-376 §17.6.1).SectionDirectionContextalready documentspageDirectionas governing "section-level chrome only (page numbers, columns, gutters)" — butColumnGeometrymeasuresxfrom the content-area left edge and no function in the column geometry ever received a direction. The axis was documented and never consumed.It reads as a selection bug to users, because selection follows the fill order: drag from the column a Hebrew reader starts in (the right one) into the left one, and everything already highlighted below the anchor disappears. #3952 has the measurements.
The change
ColumnLayoutgains an optionaldirection, andbuildColumnGeometrymirrors the strip when it is'rtl'.Two decisions worth reviewing:
Mirror, don't reverse.
indexstays the FILL order and only the paintedxmoves. Every consumer that walks columns0..n-1keeps filling in document order, and sincegetColumnGeometryis the single source for positioning, fill, hit testing, separators, balancing, floating anchors and footnotes all follow from one place.The mirror axis is the content area, not the strip. Explicit widths are deliberately not scaled to fill the content area, so a strip of explicit columns can underfill it. Mirroring about its own span would leave it pinned to the left margin and merely swap the columns inside it — the page would still read as left-aligned.
NormalizedColumnLayouttherefore carries thecontentWidthit was normalized against; when absent, the mirror falls back to the span, which is exact whenever the columns fill the area (always so in equal mode). A single column is mirrored on the same rule: a no-op when it fills the area, right-aligned when an explicit one does not.Five consumers that failed silently
None of these produce a type error when the axis is dropped, which is why each has a test that fails without its fix:
getColumnAtXxascends with the index.balanceSectionOnPage[432, 96, 96, 96, 432, 432]instead of[432, 432, 432, 96, 96, 96].toBalancingColumnsincrementalLayout)DomPainter)ColumnLayoutForAnchorandParagraphAnchorsContext.columnsnow declaredirection/contentWidthas well. Runtime was already correct — every caller passes a full normalized layout — but neither would have produced a type error if a future edit dropped the fields, which is exactly howtoBalancingColumnswent wrong.Per-column
gapsare also clamped to>= 0, matching the scalargapbeside them. OOXML cannot express a negative gutter (w:spaceis unsigned), but a hand-built layout can, and a gap negative enough to pull a column behind its predecessor would make an upright LTR strip answer hit tests as if it were mirrored.The separator gate needed a second pass (
b9e7e51)The gate asks whether a later column holds content, because Word draws no line beside an empty
column. The first two attempts both answered it by comparing a fragment edge against the
separator x, picking whichever edge trails in the fill direction. No edge can answer that question.
Content wider than its column does not sit inside it, and
resolveTableFrameplaces an over-widetable at a negative offset from its column whenever the table is right-aligned or centred — and
endis the default justification for anybidiVisualtable. So in an RTL section a wide tablestarts left of its own column and ends past the separator, while never having left the later column:
both edges lie on the wrong side, and so does its origin. A negative
w:inddoes the same to aparagraph, putting its origin in the gutter.
Measured on the repo's own fixture geometry (816px page, 96px margins, gap 48, separator at 408): a
wide right-aligned RTL table alone in the later column lands at
x = -116, width = 500, and thetrailing-edge test suppressed a line Word draws. The earlier left-edge form had the mirror failure —
a page-anchored watermark at
x = 0satisfied it and drew a line Word does not draw.The fix stops guessing from coordinates and reads
fragment.columnIndex, the engine's own record ofthe owning column, documented as the field to trust "when overflow crosses margins".
Corrected in
2438bd9: that record is written for tables (layout-table.ts) and for footnotebodies, and not for ordinary paragraph fragments — so paragraphs always reach the geometry
fallback, and the fallback has to be right on its own. Containment of the fragment's origin is
not: a negative
w:indhangs a paragraph into the gutter, putting its origin outside its owncolumn, so a later column holding only an outdented paragraph registered as empty and its separator
was suppressed. Attribution is now by overlap — the column whose span the fragment covers most,
ties to the earliest in fill order.
Anything at least as wide as the whole content area still belongs to no column. That is the part
that keeps page-anchored objects out of the gate:
page.itemscarries them, a full-width watermarkoverlaps every column without being content of any, and counting it would draw a separator on a page
whose text never left the first column. The same rule catches an over-wide table arriving without a
recorded column, where
nullis also the safe answer — it can only suppress a separator, neverinvent one.
findColumnContainingremains incontractsas the strict counterpart togetColumnAtX, whichmust clamp because a click has to select something. Its spans are half-open so that columns authored
with no gutter (
w:space="0") do not both claim the boundary they share — the boundary is exactlywhere the later column's content begins, and an inclusive bound would hand it to the earlier column
in LTR but not in RTL, making the two directions disagree.
Backward compatibility
Absent
direction, every path is byte-identical to before. Verified by running the pre-changecolumn-layout.tsand this one side by side over 46,080 comparisons of geometry and hit testing — 960 LTR configurations (1–4 columns; gaps 0/24/48/720; valid, invalid and surplus explicit widths; all threeequalWidthstates; separators; non-uniform per-column gaps) across 6 content widths from 0 to 5000. Zero differences.Tests
contracts539 ·layout-engine(bun) 968 ·layout-bridge1792 ·painters/dom1551.tsc,vp fmt --checkandvp lintclean.One failure in
packages/layout-engine/tests,architecture-boundariesGuard E, flags paint-time DOM measurement inpainters/dom/src/ruler/ruler-renderer.ts— an unmodified file that fails identically on a clean checkout of this base. Three scale tests (incrementalLayout.affected-frontier,incrementalLayout.checkpointMapEquality,persistent-page-surface) and one timing budget (incrementalLayout.semanticFlow) exceed their limits under worker contention on this machine and pass in isolation.New coverage includes underfilling and overfilling explicit strips, separator mirroring onto the same physical gutter, hit testing at both edges and in the gutter, three columns with non-uniform per-column gaps,
determineColumnunder RTL (which had no coverage at all), anddirectionsurviving clone/normalize/resolve and both equality checks.For the separator gate specifically: a wide right-aligned RTL table whose x is outside its own
column, the same table belonging to the first column in both directions, a paragraph nudged into
the gutter by a negative indent, and a fragment on a zero-gap column boundary. Each was verified by
mutation — reverting the ownership lookup fails three of them, and restoring the inclusive span
bound fails the zero-gap one.
One open question is now settled. Whether Word treats the first
<w:col>as the column youstart in (the rightmost, under RTL) or as the leftmost one visually decides whether unequal
authored widths need reversing in an RTL section. Tested against Word with a two-section file — an
RTL section and an LTR control, each
w:num="2" w:equalWidth="0"with a narrow first column — thenarrow column renders on the right in the RTL section and on the left in the LTR control. The
first
<w:col>is fill order, which is what this PR assumes, so no width reversal is needed.On the "nothing sets
direction" review findingCorrect, and worth stating precisely.
tests/src/test-helpers/to-flow-blocks.tsnow readsw:sectPr/w:bidi(ST_OnOff, so a bare element means on, applied after the element loop so sibling order withw:colsdoes not matter) and setscolumns.direction. That makessection-breaks-rtl-columns.test.tsa real end-to-end check: paragraph 1 moves from x=72 to x=330 on a Letter page, and neutering the mirror fails it.That is a test adapter, and it does not fix real documents. The production PM/OOXML→FlowBlock adapter lives in
@superdoc/docx-engine, which is a plain npm dependency here — there is nosectPrparsing anywhere underpackages/superdoc/src.SectionDirectionContext.pageDirectionstill has zero readers and zero writers.So this branch is the complete engine half, proven end to end through an adapter that doubles as an exact reference for what the external one must do. Reaching users needs one more assignment wherever
sectPris projected ontoSectionBreakBlock.columns, alongside the existingw:colshandling. Happy to write it if you point me at the seam, or to take a patch.Word behaviour was checked against the OOXML spec and against Word's rendering of the same document; there is no pixel-diff gate to attach.