fix: table gap clicks selecting wrong paragraph and jumping scroll (SD-2356)#2707
Conversation
superdoc-bot
bot
added
review: thorough
review: careful
and removed
review: thorough
labels
luccas-harbour
requested review from
VladaHarbour,
artem-harbour,
caio-pizzol,
chittolinag,
harbournick and
tupizz
caio-pizzol
left a comment
caio-pizzol
left a comment
There was a problem hiding this comment.
@luccas-harbour good direction — nearest-paragraph selection is a clear improvement over always picking the last one.
I tested both customer docs manually and the scroll jump still happens. two things going on:
-
when a table splits across pages, the container used to find the nearest line includes lines from ALL pages of that table — so the click can land on a line that's on a different page, and the view scrolls there.
-
clicking on page margins doesn't trigger the "no hit" path. the table is so wide that margin clicks get resolved as table clicks, so the cursor moves and the view jumps.
the getFirstTextPosition early-exit fix (the found flag) doesn't have a test that exercises it — existing tests use single-paragraph docs that pass either way. a quick test with two paragraphs would cover it.
one small cleanup suggestion and a couple test gaps. left inline comments.
| return resolveLineAtX(hitChainLine, clientX); | ||
| } | ||
|
|
||
| if (fragmentEl.classList.contains(CLASS.tableFragment)) { |
There was a problem hiding this comment.
when a table splits across pages, the container here can hold lines from all pages of that table. so findLineAtY picks the closest line overall, which might be on a different page — and the view scrolls there.
tested with both customer docs, the jump still happens. could you scope this to only lines within the current page fragment?
|
|
||
| // Handle click outside text content | ||
| // Handle click outside text content — keep cursor and scroll position unchanged. | ||
| if (!rawHit) { |
There was a problem hiding this comment.
this path never fires during manual testing — the table extends into the margin area, so margin clicks get resolved as table hits instead of reaching here.
| return resolveLineAtX(lineEl, viewX); | ||
| } | ||
|
|
||
| function resolveLineInContainer(containerEl: HTMLElement, viewX: number, viewY: number): number | null { |
There was a problem hiding this comment.
this function is the same as resolveFragment above — same query, same two calls. only difference is a debug log that's off by default. could reuse resolveFragment and delete this one?
| const scrollDelta = Math.abs(scrollAfter - scrollBefore); | ||
| expect( | ||
| scrollDelta, | ||
| `Scroll jumped by ${scrollDelta}px after clicking page margin — expected no significant scroll change`, |
There was a problem hiding this comment.
selBefore is captured but never checked after the click. worth adding:
const selAfter = await superdoc.getSelection();
expect(selAfter.from).toBe(selBefore.from);2 participants
Summary
hitTestTableFragmentwith a nearest-distance algorithm that picks the closest paragraph to the click point.DomPointerMappingso gap clicks in nested table DOM resolve to the nearest line within the correct cell.