-
Notifications
You must be signed in to change notification settings - Fork 62
fix: support cell spacing #1879
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 489b04e27c
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ['tableIndent', ({ value, type }) => ({ width: twipsToPixels(value), type })], | ||
| ['tableCellSpacing', ({ value, type }) => ({ w: String(value), type })], | ||
| ['tableCellSpacing', ({ value }) => ({ value: twipsToPixels(value), type: 'px' })], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Preserve tblCellSpacing units when encoding
The new transform forces tableCellSpacing into pixels via twipsToPixels(value) and hard-codes type: 'px' (tbl-translator.js:90). This drops the original unit, so a DOCX table using w:tblCellSpacing with w:type="pct" (supported by the tblCellSpacing translator tests) will be misinterpreted as twips and rendered at the wrong spacing. Consider preserving the original type or only converting when the type is dxa.
Useful? React with 👍 / 👎.
| const cellSpacingPx = tableMeasure.cellSpacingPx ?? 0; | ||
| const tableBorderWidths = tableMeasure.tableBorderWidths; | ||
| const contentOffsetX = tableBorderWidths?.left ?? 0; | ||
| const contentOffsetY = tableBorderWidths?.top ?? 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gate selection offsets to tables with container borders
selectionToRects now unconditionally offsets table text by tableBorderWidths (layout-bridge/src/index.ts:1527-1530), but the DOM renderer only applies a container border when borderCollapse === 'separate' and cellSpacing is set (renderTableFragment.ts:302-308). For the default collapsed case, borders are drawn on cells and the container origin is not shifted, so adding contentOffsetX/Y will move selection highlights down/right by the border width. This should be gated on the same condition used for rendering the container border.
Useful? React with 👍 / 👎.
|
Based on my review of the code and understanding of OOXML specifications, I can now provide a comprehensive review. Let me analyze the key changes: Status: PASS I've reviewed this PR for OOXML spec compliance, focusing on the SummaryThe changes correctly implement table cell spacing according to ECMA-376 specifications. The implementation properly handles the Key Changes Reviewed1. tbl-translator.js:90 - Cell spacing conversion['tableCellSpacing', ({ value, type }) => ({ value: twipsToPixels(value), type })]✅ Correct: The spec defines 2. tbl-translator.test.js:154 - Test expectation updateexpect(result.attrs.tableCellSpacing).toEqual({ value: 0.5, type: 'dxa' });✅ Correct: With input UPDATE: Looking at the diff, the original test returned 3. legacy-handle-table-cell-node.js:58,308 - Border spacing logichasBorderSpacing: !!tableProperties?.tableCellSpacing✅ Correct: According to ECMA-376, when 4. contracts/index.ts - Type definition updatecellSpacing?: CellSpacing;
// where CellSpacing = { type: string; value: number; }✅ Correct: Properly represents OOXML measurement with both value and type (e.g., OOXML Spec ComplianceThe implementation correctly follows ECMA-376 Part 1 §17.4.19 (
For detailed spec information, see: https://ooxml.dev/spec?q=tblCellSpacing No Issues FoundAll attributes, elements, and default values align with the ECMA-376 specification. The conversion logic properly handles the measurement type pattern used consistently across other table properties. |
Adds cell spacing support