fix: table rendering fidelity (SD-3345, SD-2969)#3617
Open
tupizz wants to merge 7 commits into
Open
Conversation
Rewrite resolveShadingFillColor to the full CT_Shd model (ECMA-376 §17.3.5): nil/none yields no background; a pattern (pctNN/solid) blends the foreground over the fill; the auto sentinel resolves to white (fill) and black (foreground). parseTableCell resolves style-based shading through it, so a table style's pct10/auto band renders as the gray Word paints instead of being dropped. (SD-2969)
In a keepNext chain anchored to a table, reserve only the first row's height (not the full table) so the table starts on the page and splits, matching Word. Mirrors the paragraph anchor's first-line reservation; previously a heading plus a tall splittable table advanced together and left a large gap. (SD-3345)
Add resolveBorderConflict and isPresentBorder to collapse two cells' shared-edge borders to the single displayed border: if either side is nil/none the opposing border wins; otherwise greater weight (lines x style number) wins, with ties broken by style precedence, then color brightness, then reading order. Pure helper with unit tests; wired into the table painter in the following commit.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Render w:tblPrEx row-level borders (merged per edge over the table borders) and resolve collapsed shared edges with single-owner ownership plus the §17.4.66 winner, so adjacent cell borders draw exactly once (no doubling) and asymmetric or borderless edges keep their line (no drop). Handle trailing w:gridAfter columns: the rightmost real cell owns the right border, the resize overlay skips the degenerate spacer, and a cell spanning past the next row's coverage owns its full-width bottom while that row suppresses its top. (SD-3345, SD-2969)
tupizz
force-pushed
the
tadeu/sd-3028-table-fidelity
branch
from
c01a888 to
fd8661b
Compare
![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: fd8661b791
ℹ️ 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".
Two §17.4.66 cell-border conflict cases in the single-owner collapsed model were resolved against the wrong precedence, both visible in sd-1279-tables.docx vs Word: - A style-bordered row above a row whose tblPrEx override suppresses the shared horizontal edge (insideH none) lost its closing border: the lower row owns that edge but draws nothing, and the upper row's border came from the table style (no cell tcBorder for the neighbor path to pick up). The grid no longer closed. The upper row now draws its own insideH to close the grid when the next row's override suppresses the edge (present beats none). - A divider between two cells that BOTH set the shared vertical edge to w:val="nil" was still drawn because the resolver fell back to the table style insideV, treating an explicit nil the same as an unset side. An explicit nil on both adjacent cells now suppresses the divider, while a merely-unset side still inherits insideH/insideV. Adds isExplicitNoneBorder to distinguish authored nil from unset. Scope verified across the layout corpus: only sd-1279-tables changes rendering (the only doc with a present interior border plus authored nil on both sides of a shared edge). Adds 5 regression tests.
In the single-owner collapsed model, two geometry helpers decide who owns a shared interior edge by walking ONE row's measure: `rowRightEdgeCol` (the row's rightmost occupied column, used for right-border ownership and the gridAfter right edge) and `nextRowMaxCol` (used by the SD-3345 `nextRowLeavesRightGap` check). A row's measure only lists cells that START in that row, so on a w:vMerge (rowspan) continuation row the columns held by a cell spanning from above look empty. Both helpers then undercounted, which: - made a leftmost cell on a continuation row look like the rightmost column, so it drew the table/style insideV as its right border (doubling the gray column's left), and - made a rowspan-covered column below a spanning cell look like a gridAfter gap, so the spanning cell drew its own bottom while the cell below still drew its top (doubling the horizontal divider). Visible in sd-1797-autofit-tables.docx (PCI assessment template): the gray "Identify/Describe" column showed 2px interior lines where Word draws 1px. Fix: compute per-row occupied grid columns INCLUDING cells that span into the row via rowspan, and feed that to both helpers. Tables without rowspans are unchanged (occupancy equals the row's own cells), and genuine gridAfter gaps still draw the spanning cell's bottom. Verified zero doubled edges across the fixture (112 cells) and adds 3 regression tests (incl. SD-3345 preserved).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Table rendering fidelity for the SD-3345 Harvey documents and SD-2969, under the SD-3028 epic. Four independent fixes, one per commit:
Style-based cell shading (pct/auto)
feat(super-converter)resolveShadingFillColornow implements the full CT_Shd model (ECMA-376 §17.3.5):nil/noneyields no background, a pattern (pctNN/solid) blends the foreground over the fill, and theautosentinel resolves to white (fill) and black (foreground).parseTableCellresolves style-based shading through it, so a table style'spct10/autoband renders as the gray Word paints instead of being dropped. (SD-2969)keepNext table pagination
fix(layout-engine)In a keepNext chain anchored to a table, reserve only the first row's height, so the table starts on the page and splits, matching Word. Previously a heading plus a tall splittable table advanced together and left a large gap. (SD-3345)
§17.4.66 cell-border conflict resolution
feat(painter)New
resolveBorderConflicthelper that collapses two cells' shared-edge borders to the single displayed border: anil/noneside yields to the opposing border; otherwise greater weight wins, then style precedence, then color brightness, then reading order. Pure helper with unit tests.Collapsed cell-border rendering
fix(table)Renders
w:tblPrExrow-level borders (merged per edge over the table borders) and uses single-owner ownership plus the §17.4.66 winner, so adjacent cell borders draw exactly once (no doubling), asymmetric and borderless edges keep their line (no drop), and trailingw:gridAftercolumns are handled: the rightmost real cell owns the right border, the resize overlay skips the degenerate spacer, and a cell spanning past the next row's coverage owns its full-width bottom while that row suppresses its top. (SD-3345, SD-2969)Test plan
pnpm test:visualcorpus gate (high blast radius: keepNext pagination and the shared border resolver)pnpm check:public(newcontractsTableRowAttrs.bordersfield)Notes
resolveShadingFillColorhas a second caller (legacy-handle-table-cell-node.js); behavior shifts fornil/none/solidtoward spec correctness, full suite green.resolveBorderConflictweighting is style-based, notsz-width-based; this does not affect any fixture here (all are identical-border or present-vs-none cases).Linear: SD-3028 (epic), SD-3345, SD-2969