feat: support rendering of column line separators#2088
feat: support rendering of column line separators#2088caio-pizzol merged 11 commits intosuperdoc-dev:mainfrom
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
@caio-pizzol I could not find, in ECMA-376, a definition for |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
Hey @JoaaoVerona, this is a great start! The withSeparator flag is threaded cleanly through the whole pipeline - extraction, layout, rendering — nice work.
Tests: would be great to add a visual rendering test with a sample .docx that has w:sep="1" on <w:cols>. The test infra auto-discovers .docx files so it's mostly just dropping one in tests/visual/. This is the best way to catch regressions in separator positioning and styling down the road.
On your lineBetween question: lineBetween isn't part of the ECMA-376 spec - the actual XML attribute for column separators is w:sep on <w:cols> (§17.6.4), which is exactly what this PR implements. The name lineBetween probably comes from the Open XML SDK's property naming, not from the XML itself. Word and LibreOffice both use w:sep, so no need to support lineBetween.
left some inline comments on a few things worth tweaking before merging.
|
@JoaaoVerona btw we've created our own OOXML MCP server (https://ooxml.dev/mcp) :) |
|
@JoaaoVerona, let us know if you have any questions on this one |
JoaaoVerona
force-pushed
the
feat/support-column-line-separator
branch
from
fefdd96 to
9a732ac
Compare
|
hey @caio-pizzol, thanks for the info! also, great work with the mcp -- been using it already with CC! additionally to the inline comments:
by looking at document.xml, I can see there are 3 |
correct — the visual test infra isn't set up for external contributors to run locally yet. we'll upload the baseline on our end. we're looking into better ways to support this for the community going forward.
i looked at your document.xml — OOXML sections are end-tagged, meaning the
the confusing part is that the first so the extraction is correct — the values just read counterintuitively because of end-tagged semantics. nothing to fix here. |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@JoaaoVerona the round 1 feedback is all addressed — the separator color, the columnWidth <= 1 guard, the ColumnLayout type usage, and the SINGLE_COLUMN_DEFAULT constant are all looking good. left a few inline comments on some remaining rough edges before this is ready to merge.
- Extracts 'w:sep' tag according to OOXML spec
- Renders one separator for each page column ('w:col' / 'w:num') in DOM painter
Closes superdoc-dev#2067
JoaaoVerona
force-pushed
the
feat/support-column-line-separator
branch
from
9a732ac to
32753c9
Compare
|
hey @caio-pizzol, thanks for the context! inline comments should be solved now. rebased it with main too. |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@JoaaoVerona all the feedback from previous rounds is addressed — looking good.
two small things:
-
the column snapshot in
section-props.tscopies fields by hand and missesequalWidthandwidths. there's already a helper (getColumnConfig) that copies everything — reusing it would avoid this. -
normalizeColumnsForFootnotesinincrementalLayout.tsis a copy ofnormalizeColumnsinindex.ts. worth consolidating so the next person doesn't update one and forget the other.
on tests: renderColumnSeparators doesn't have a unit test for its positioning math and guard clauses. also, tests/visual/columns-with-line-separator.docx won't get picked up by the layout regression suite — it needs to go through pnpm corpus:upload to land in the R2 corpus where rendering.spec.ts auto-discovers test files.
left a couple inline comments.
| if (block.columns) { | ||
| hasProps = true; | ||
| props.columns = { count: block.columns.count, gap: block.columns.gap }; | ||
| props.columns = { count: block.columns.count, gap: block.columns.gap, withSeparator: block.columns.withSeparator }; |
There was a problem hiding this comment.
the manual { count, gap, withSeparator } copy here and at line 138 drops equalWidth and widths — so documents with different-sized columns lose that info in the snapshot. getColumnConfig() in section-breaks.ts already copies everything correctly. worth reusing it here?
| const gap = Math.max(0, input?.gap ?? 0); | ||
| const totalGap = gap * (count - 1); | ||
| const width = (contentWidth - totalGap) / count; | ||
| const withSeparator = input?.withSeparator ?? false; |
There was a problem hiding this comment.
this does the same thing as normalizeColumns in layout-engine/src/index.ts:2633 — both are one-liners wrapping normalizeColumnLayout. not blocking, but if one changes and the other doesn't, things will break quietly.
|
Hey @JoaaoVerona, this PR has had no activity for 7 days. |
Two related fixes so the new column-line-separator feature works correctly on pages where a continuous section break changes column layout mid-page. 1. isColumnConfigChanging now compares withSeparator. Before, a sep-only toggle (count+gap unchanged) returned false, so no mid-page region was created and the toggle was silently dropped. Applied in both section-breaks.ts and the inline fallback in index.ts. 2. constraintBoundaries captured during layout are serialized onto a new page.columnRegions contract field. renderColumnSeparators now iterates regions and draws each separator bounded by its yStart/yEnd instead of painting a single full-page overlay. When no mid-page change occurs, columnRegions is omitted and the renderer falls back to page.columns (unchanged behavior). Verified by loading a fixture with 7 scenarios (2-col, 3-col, unequal widths, separator on/off, continuous breaks toggling the separator). Pages now show per-region separators tiled correctly; a 3-col region followed by a 2-col region no longer paints a shared full-page line. Out of scope here, tracked for follow-up: widths/equalWidth are still dropped at pm-adapter extractColumns, so unequal-width separators render at the equal-width midpoint; body-level w:sep is dropped at v2 docxImporter; there is no w:sep export.
13 unit tests over the separator renderer. Splits coverage into the fallback path (page.columns only) and the region-aware path (page.columnRegions). Fallback path: pins the 2-col and 3-col geometry, and each early-return guard (withSeparator false/undefined, single column, missing margins, no columns at all, pathologically-small columnWidth). Region path: verifies per-region yStart/yEnd bounding, mixed regions (some draw, some skip for withSeparator=false or count<=1), zero-height regions, and that columnRegions wins when both it and page.columns are present. Previously renderColumnSeparators had zero DOM-level coverage — the region-aware refactor in the prior commit relied entirely on layout-engine tests that never exercised the DOM output.
Main adopted the refactor we deferred to SD-2627 while this PR was open:
`cloneColumnLayout` and `normalizeColumnLayout` are now the single source of
truth for column copies, and `ColumnLayout` carries `widths` and `equalWidth`
for unequal-width columns. The merge threads this PR's `withSeparator`
through the new helpers:
- `cloneColumnLayout` and `normalizeColumnLayout` (contracts/column-layout.ts)
now preserve `withSeparator`. Without this, every site using the helpers
would silently drop the separator on clone/normalize.
- `isColumnConfigChanging` combines both intents: detects a change in
`count`, `gap`, `withSeparator`, `equalWidth`, or `widths`. A sep-only
toggle still triggers a new column region.
- All manual `{count, gap, withSeparator}` copies replaced by
`cloneColumnLayout`/`ooXmlSectionColumns` calls picked up from main.
- Two pm-adapter test expectations updated to include `withSeparator: false`
in the extracted columns shape.
Full test suite passes: contracts 177, layout-engine 602, painter-dom 997,
layout-bridge 1166, pm-adapter 1737.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🎉 This PR is included in vscode-ext v2.3.0-next.25 |
|
🎉 This PR is included in @superdoc-dev/react v1.2.0-next.22 The release is available on GitHub release |
|
🎉 This PR is included in template-builder v1.5.0-next.25 The release is available on GitHub release |
|
🎉 This PR is included in esign v2.3.0-next.25 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-cli v0.7.0-next.26 The release is available on GitHub release |
|
🎉 This PR is included in superdoc v1.26.0-next.25 The release is available on GitHub release |
|
🎉 This PR is included in superdoc-sdk v1.6.0-next.23 |
3 participants
Summary
Adds support for rendering "line between" separators between page columns in DOM Painter, extracting their presence by looking up the
w:septag in the layout engine and ProseMirror adapter. Closes #2067.Changes
w:septag according to OOXML spec [1] [2]w:num)withSeparatorpropertyQuestions & Additional details
columnsdata (i.e.y.columns = { count: x.count, gap: x.gap }) that needed to be updated also to copywithSeparator; I could have used spread operator to shallow clone the data, but I went with a more conservative approach to avoid, potentially, copying unnecessary/unintended properties that could be declared in these objects at runtime;withSeparatorwill be true whenw:sep="true",w:sep="1"orw:sep="on", according to the spec;#b3b3b3, as I'm not sure if we could infer this somehow;withSeparatoras optional to make sure we don't break userland, but a second look from someone more experienced with the packaging/release of superdoc would be great.Showcase