Better spacing for paragraphs with borders/shades#2554
Better spacing for paragraphs with borders/shades#2554johanneswilm wants to merge 1 commit intosuperdoc-dev:mainfrom
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 748be8e4c5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| toLine: slice.toLine, | ||
| x: adjustedX, | ||
| y: state.cursorY, | ||
| y: state.cursorY + borderExpansion.top, |
There was a problem hiding this comment.
layoutParagraphBlock now offsets paragraph fragments by borderExpansion.top, but the slice fit still uses state.contentBottom - state.cursorY (text-only space). When a bordered paragraph starts near the page bottom, sliceLines can accept lines that fit only without the new top offset, so the rendered text is pushed below the page’s content area and gets clipped (overflow: hidden on pages). The same mismatch also allows cursorY to advance past contentBottom after placing a “fitting” slice. Reserve top/bottom border expansion before slicing (or include it in the fit checks) so the fragment only takes lines that truly fit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@johanneswilm real issue — bordered paragraphs near the page bottom can overflow since the border size isn't subtracted from the available space before fitting lines.
There was a problem hiding this comment.
@johanneswilm the approach works — shifting the fragment down makes borders and shading land correctly.
one issue: border space and paragraph spacing are being added together, but Word treats them as overlapping. that's where the extra 13-14px gap comes from. tested and confirmed with a comparison (will add screenshots in a follow-up comment).
minor: sliceLines doesn't account for border space, so bordered paragraphs near the page bottom could overflow slightly. fine as a follow-up.
no code quality issues. tests for the new border logic are missing — a few cases in layout-paragraph.test.ts would be good.
file:
border-test-doc.docx
word:
superdoc:
| state.page.fragments.push(fragment); | ||
| state.cursorY += fragmentHeight; | ||
|
|
||
| state.cursorY += fragmentHeight + borderExpansion.top + borderExpansion.bottom; |
There was a problem hiding this comment.
border space and paragraph spacing are stacking here — but Word treats them as overlapping. the spec says border space is measured from the text, ignoring paragraph spacing (ECMA-376 §17.3.1.42).
tested with a doc that has borders (space=4) + 10pt spacing: SuperDoc adds ~6px extra per side vs Word. that adds up to the ~13px gap you mentioned in the PR description.
fix direction: make them overlap (use the bigger one) instead of adding both.
There was a problem hiding this comment.
Hey @caiopizzol , thanks for the thorough review. I actually read through quite a bit of ECMA-376 as part of this. Really complex issue!
There was a problem hiding this comment.
Thank you, for your work on this!
|
@johanneswilm thanks for this! closing in favor of #2586 which uses your commit as the base and fixes the extra gap between paragraphs you noticed. |
3 participants
Before:
After:
Microsoft Word 365:
Note: Spacing is better, but still not entirely correct. There is still an additional space of 13-14px between paragraphs in SuperDoc that is not present in Microsoft Word 365. Also note how the shading does not expand into the space between border and paragraph on Microsoft Word 365.
shading-border 2.docx