fix(layout-bridge): defer table fragment click mapping to geometry fallback #1968
Conversation
…llback When clicking on table cell padding or borders, elementsFromPoint returns the table fragment but not a specific line element. Previously, processFragment would search all lines across all cells using only Y coordinate matching, causing clicks to land in the wrong column. Now when a table fragment is found without a line in the hit chain, we return null to let the geometry fallback (hitTestTableFragment) handle cell resolution correctly using accumulated row/column dimensions.
There was a problem hiding this comment.
Pull request overview
This PR fixes incorrect cursor placement in table cells by ensuring DOM-based click mapping defers to geometry-based table hit-testing when a click lands on a table fragment but doesn’t directly hit a .superdoc-line element (e.g., padding/borders where elementsFromPoint omits the line).
Changes:
- Update
clickToPositionDomto returnnullfor table fragments when no line is present in the hit chain, allowingclickToPositionto fall back tohitTestTableFragment. - Add unit tests covering the “table fragment without line hit” deferral behavior.
- Add a table-related test intended to cover the “line is hit” path (but it currently doesn’t exercise that branch in JSDOM—see comments).
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| packages/layout-engine/layout-bridge/src/dom-mapping.ts | Defers table fragment clicks without direct line hits to geometry fallback by returning null. |
| packages/layout-engine/layout-bridge/test/dom-mapping.test.ts | Adds tests for table fragment fallback behavior when elementsFromPoint omits line elements. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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: b48cd5609b
ℹ️ 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".
When a table splits across pages, each fragment's data-table-boundaries attribute contained segment heights for the entire table rather than just the rows rendered in that fragment. This caused resize handles to overflow beyond the fragment into the page gap and neighboring content. Scope the segment computation to iterate only over the rows actually rendered in each fragment (repeated headers + body rows from fromRow to toRow), matching the existing row rendering logic. Handle partial row heights for mid-row splits.
…d page detection normalizeClientPoint adjusts Y by the page's DOM offset, making containerPoint page-relative. On page 1 this is ~0, but on page 2+ the geometry fallback double-subtracted pageTopY, causing hitTestPage to find the wrong page and table cell hit testing to fail. Fix: when DOM info is available, determine the page from elementsFromPoint and treat containerPoint as already page-relative. Also clean up debug logging from prior iteration.
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
Hey @tupizz! This is great.
I added a few comments, mainly regarding maintainability and cleanness of the codebase.
And also in this case, I would advise creating interaction stories here as well - since this happens specific on custom table through cursor positioning
- Add TABLE_FRAGMENT to DOM_CLASS_NAMES constant, use across packages - Export and reuse findPageElement in clickToPosition instead of duplicating - Extract snapToNearestFragment helper from clickToPosition - Add buildTableFixtures shared test builder in mock-data - Polyfill elementsFromPoint in table hit chain test for proper coverage - Add cross-reference comments between metadata and rendering iterations
|
@caio-pizzol I'll be adding the visual testing |
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
feel free to merge after interaction stories are added
great job!
Verifies that clicking in different table cells correctly places the cursor in the targeted cell, not in nearby paragraphs or wrong columns.
harbournick
deleted the
tadeu/sd-1788-bug-cursor-cannot-be-reliably-placed-in-table-cell
branch
|
🎉 This PR is included in superdoc v1.12.0-next.14 The release is available on GitHub release |
4 participants
Summary
This PR contains three fixes for table interaction in the layout engine:
1. Fix cursor placement in table cells (SD-1788)
Problem: Clicking on table cell padding, borders, or areas where
elementsFromPointdoesn't return a line element caused the cursor to land in the wrong column.Root cause:
clickToPositionDomindom-mapping.tsusedprocessFragmentfor table fragments even when no line was in the hit chain.processFragmentsearches all lines across all cells using only Y-coordinate matching, so it picks the first cell with a matching Y — which is often the wrong column.Fix: Added a guard that returns
nullfor table fragments without a line in the hit chain, deferring to the geometry fallback (hitTestTableFragment) which correctly uses both X and Y coordinates to resolve the target cell.File:
packages/layout-engine/layout-bridge/src/dom-mapping.ts2. Fix table resize handles overflowing on split tables
Problem: When a table splits across page boundaries, the column resize handles (blue vertical lines) extended far beyond the table fragment — overflowing into the page gap and neighboring content below.
Root cause: In
renderTableFragment.ts, thedata-table-boundariessegment computation iterated over all rows in the table block (0 to rowCount), but each fragment only renders a subset of rows (fromRowtotoRow+ optional repeated headers). This caused every fragment to get segments with the full table's height.Fix: Changed the segment computation to build a
renderedRowslist that mirrors the exact row rendering logic.File:
packages/layout-engine/painters/dom/src/table/renderTableFragment.ts3. Fix table cell click on page 2+ (geometry fallback coordinate fix)
Problem: Clicking in empty space below text in a table cell worked on page 1 but failed on page 2+. The cursor either didn't appear or landed in the wrong position.
Root cause: Two issues compounding:
Snap-to-nearest picks wrong target: When clicking empty table cell space (below text lines), the DOM-based mapping returns
null. The geometry fallback's snap-to-nearest logic then picks a nearby paragraph instead of lettinghitTestTableFragmenthandle the click.Coordinate double-subtraction on page 2+:
normalizeClientPointadjusts Y by the page's DOM offset (pageOffsetY), makingcontainerPointpage-relative. On page 1 this offset is ~0, but on page 2+ it's significant. ThenclickToPositionsubtractspageTopYagain, producing incorrect negative coordinates that causehitTestPageto find the wrong page andhitTestTableFragmentto fail entirely.Fix:
hitTestTableFragmenthandle it)elementsFromPoint(same techniquenormalizeClientPointuses) and treatcontainerPointas already page-relative, avoiding the double subtractionFile:
packages/layout-engine/layout-bridge/src/index.tsTest plan