feat: support rendering of column line separators#2088
feat: support rendering of column line separators#2088JoaaoVerona wants to merge 1 commit intosuperdoc-dev:mainfrom
Conversation
- 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
|
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.
|
|
||
| separatorEl.style.position = 'absolute'; | ||
| separatorEl.style.left = `${separatorX}px`; | ||
| separatorEl.style.top = `${topMargin}px`; |
There was a problem hiding this comment.
the separator currently runs the full height of the content area (top margin to bottom margin). in Word, the line only extends as tall as the actual text in the columns -- so on pages that aren't fully filled (like the last page of a multi-column section), Word's line stops where the text stops.
here, it'll keep going all the way down. totally fine for a first pass -- just flagging it as something we might refine later.
| page.orientation = activeOrientation; | ||
| } | ||
|
|
||
| if (activeColumns.count > 1) { |
There was a problem hiding this comment.
heads up on an edge case here: page.columns is set from activeColumns, which is the raw config from the section properties. but further down in the pipeline, there's a normalization step (normalizeColumns) that checks whether columns actually fit on the page -- if the gap is so large that columns would have zero width, it silently falls back to single-column layout. since page.columns was already set before that check, the renderer would still draw separators even though the page is actually rendering as one column.
unlikely to hit in real documents, but the fix is straightforward -- either set page.columns after normalization, or check the normalized count in the renderer.
| separatorEl.style.top = `${topMargin}px`; | ||
| separatorEl.style.height = `${pageHeight - topMargin - bottomMargin}px`; | ||
| separatorEl.style.width = '1px'; | ||
| separatorEl.style.backgroundColor = '#b3b3b3'; |
There was a problem hiding this comment.
Word renders this separator as a dark line (close to black). #b3b3b3 is a light gray, which will look noticeably different when comparing side-by-side. the rest of our renderer also defaults to #000 for line elements like borders and underlines, so swapping to match would keep things consistent both with Word and with our own codebase.
| } | ||
| } | ||
|
|
||
| private renderColumnSeparators(pageEl: HTMLElement, page: Page, pageWidth: number, pageHeight: number): void { |
There was a problem hiding this comment.
one thing to be aware of: page.columns is a single value per page, set when the page is created. but in OOXML, a "continuous" section break can change the column count mid-page (e.g., a newsletter with a single-column header, then 2-column body, then single-column footer -- all on one page). since page.columns doesn't capture that, the separator would draw across the entire page even if only part of it is multi-column.
this is more of an architectural limitation than something to fix here -- just flagging it so we can track it.
| export type ConstraintBoundary = { | ||
| y: number; | ||
| columns: { count: number; gap: number }; | ||
| columns: { count: number; gap: number; withSeparator?: boolean }; |
There was a problem hiding this comment.
you'll notice the type { count: number; gap: number; withSeparator?: boolean } is written out inline about 14 times across these files. we actually have a ColumnLayout type in @superdoc/contracts that's the exact same shape -- and it's already imported in this file.
using it in the files you touched would mean the next person adding a field (like you did with withSeparator) only has to update it in one place.
| const resolveSectionColumnsByIndex = (options: LayoutOptions, blocks?: FlowBlock[]): Map<number, ColumnLayout> => { | ||
| const result = new Map<number, ColumnLayout>(); | ||
| let activeColumns: ColumnLayout = options.columns ?? { count: 1, gap: 0 }; | ||
| let activeColumns: ColumnLayout = options.columns ?? { count: 1, gap: 0, withSeparator: false }; |
There was a problem hiding this comment.
this fallback { count: 1, gap: 0, withSeparator: false } shows up 6 times across incrementalLayout.ts and index.ts. there's already a SINGLE_COLUMN_DEFAULT constant in section-breaks.ts that represents the same thing, but it's not exported.
if you export it and reuse it here, the next field added won't require hunting down 6 scattered literals -- which is exactly the situation you ran into when adding withSeparator.
|
@JoaaoVerona btw we've created our own OOXML MCP server (https://ooxml.dev/mcp) :) |
2 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