[SD-1131] Collapse adjacent table-cell borders #1611
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.
ℹ️ 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".
| // For top: use cell's if defined, otherwise use table's top border for first row OR continuation | ||
| top: cellBordersAttr.top ?? borderValueToSpec(treatAsFirstRow ? tableBorders.top : tableBorders.insideH), | ||
| top: treatAsFirstRow ? (cellBordersAttr?.top ?? borderValueToSpec(tableBorders?.top)) : undefined, | ||
| // For bottom: use cell's if defined, otherwise use table's bottom border for last row OR before continuation |
There was a problem hiding this comment.
In renderTableRow, the resolved top border is only populated when treatAsFirstRow is true and otherwise left undefined, so tableBorders.insideH is never used for interior rows. This means tables that rely on table-level insideH (common when cells omit explicit borders) will lose horizontal gridlines between rows; prior logic used insideH for non-first rows. Consider restoring the insideH fallback for interior rows or otherwise mapping it to the row-owned border so those gridlines render.
Useful? React with 👍 / 👎.
packages/layout-engine/painters/dom/src/table/renderTableRow.ts
Outdated
Show resolved
Hide resolved
afn
force-pushed
the
afn/SD-1131-table-border-collapse
branch
from
4184ba8 to
9c1f075
Compare
afn
left a comment
afn
left a comment
There was a problem hiding this comment.
@harbournick Couple of questions/notes for you in the comments above
| let resolvedBorders; | ||
| if (hasBordersAttribute && !hasExplicitBorders) { | ||
| // Cell explicitly has borders={} meaning "no borders" | ||
| // TODO: Is this actually accurate? Shouldn't borders={} mean "don't override any borders"? |
There was a problem hiding this comment.
@harbournick I wanted to check this with you: wouldn't "no borders" be represented as { top: { style: 'none', ... }, ... } rather than {}?
| // For right: use cell's if defined, otherwise use table's right for last col only | ||
| right: cellBordersAttr.right ?? borderValueToSpec(isLastCol ? tableBorders.right : undefined), | ||
| }; | ||
| } else if (hasExplicitBorders) { |
There was a problem hiding this comment.
@harbournick I wanted to call this out to make sure I didn't miss anything subtle in combining the three cases (tableBorders and hasExplicitBorders, only tableBorders, only hasExplicitBorders) into a single one.
| @@ -0,0 +1,9 @@ | |||
| import { BorderSpec } from '@superdoc/contracts'; | |||
|
|
|||
| export function getBorderWidth(border: BorderSpec | null | undefined): number { | |||
There was a problem hiding this comment.
Does measuring/dom seem like the right place for this? It means that painters/dom is now dependent on measuring/dom, which it wasn't before. Open to other suggestions of which package to put it in.
* Fix bug with border size units: eliminate eighthPointsToPixels conversion in processInlineCellBorders (legacy-handle-table-cell-node.js) * Eliminate redunant logic in renderTableRow: have a single case handle any combination of explicit borders & table borders * Collapse borders by only painting right and bottom borders (omitting left and top) unless the cell is the first column or the first row (or treated as the first row, because it is first on the page)
Unclear where the original 0.5 value came from; change it to 1 to reflect the default value used in rendering table borders.
Revert earlier change which disabled rendering of top/left borders, because this would cause the height of a thick left border to be too short for a cell with added top/bottom borders (e.g. see "Thick border all around" in SD-1131-table-borders.docx). Instead, render all four borders, but allow them to overlap, to simulate collapsed borders.
afn
force-pushed
the
afn/SD-1131-table-border-collapse
branch
from
86e6c6a to
f99a9db
Compare
|
Note that the failing visual tests are also failing in main (looks like expected is missing text that's present in actual). To that end, see #1622 which changes the PR validation workflow to run on every commit, rather than only on open PRs. |
TODO: Needs visual tests, which will be added once #1422 is done.