Skip to content

Commit 8955a80

Browse files
fix: allow caret placement inside commented text on click (SD-2442) (#2708)
* test: add regression coverage for caret placement inside commented text * fix: allow caret placement on direct comment highlight clicks * refactor: simplify click coordinate math in test helpers * fix: use elementFromPoint fallback in comment click-outside handler Pointer capture (used by PresentationEditor during pointerdown) redirects the click event target to the viewport host. The v-click-outside directive fires on the click event, so e.target no longer matches the original comment highlight span. This caused handleClickOutside to clear the active comment immediately after selection-based activation set it. Use document.elementFromPoint as a fallback to check what's actually under the cursor coordinates, so the ignored-selectors check works regardless of pointer capture. * test: add coverage for comment activation and click-outside fallback Unit tests: - elementFromPoint fallback when e.target is wrong (pointer capture) - elementFromPoint fallback for tracked-change selectors - inverse: elementFromPoint returns non-ignored element, bubble dismissed Behavior tests: - clicking inside commented text activates the comment bubble - double-clicking inside commented text selects a word --------- Co-authored-by: Caio Pizzol <caio@harbourshare.com>
1 parent fc86b55 commit 8955a80

6 files changed

Lines changed: 234 additions & 54 deletions

File tree

packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts

Lines changed: 16 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,10 @@ function getCommentHighlightThreadIds(target: EventTarget | null): string[] {
9797
.filter(Boolean);
9898
}
9999

100+
function isDirectSingleCommentHighlightHit(target: EventTarget | null): boolean {
101+
return getCommentHighlightThreadIds(target).length === 1;
102+
}
103+
100104
function resolveTrackChangeThreadId(target: EventTarget | null): string | null {
101105
if (!(target instanceof Element)) {
102106
return null;
@@ -216,6 +220,14 @@ function shouldIgnoreRepeatClickOnActiveComment(
216220
return false;
217221
}
218222

223+
// Direct clicks on commented text should place a caret at the clicked
224+
// position and let the comments plugin infer the active thread from the
225+
// resulting selection. Only preserve the pointerdown short-circuit for
226+
// nearby non-text surfaces, such as split-run gaps.
227+
if (isDirectSingleCommentHighlightHit(target)) {
228+
return false;
229+
}
230+
219231
return resolveCommentThreadIdNearPointer(target, clientX, clientY) === activeThreadId;
220232
}
221233

@@ -2230,6 +2242,10 @@ export class EditorInputManager {
22302242
}
22312243

22322244
#handleSingleCommentHighlightClick(event: PointerEvent, target: HTMLElement | null, editor: Editor): boolean {
2245+
if (isDirectSingleCommentHighlightHit(target)) {
2246+
return false;
2247+
}
2248+
22332249
const clickedThreadId = resolveCommentThreadIdNearPointer(target, event.clientX, event.clientY);
22342250
if (!clickedThreadId) {
22352251
return false;

packages/super-editor/src/editors/v1/core/presentation-editor/tests/EditorInputManager.activeCommentClick.test.ts

Lines changed: 16 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest';
22

3-
import { comments_module_events } from '@superdoc/common';
43
import { clickToPosition } from '@superdoc/layout-bridge';
54
import { resolvePointerPositionHit } from '../input/PositionHitResolver.js';
65
import { TextSelection } from 'prosemirror-state';
@@ -203,28 +202,26 @@ describe('EditorInputManager - single-thread comment highlight clicks', () => {
203202
return elementsFromPoint;
204203
}
205204

206-
it('treats a click on the already-active single-thread highlight as a no-op', () => {
205+
it('lets direct clicks on the active single-thread highlight fall through to generic caret placement', () => {
207206
const highlight = document.createElement('span');
208207
highlight.className = 'superdoc-comment-highlight';
209208
highlight.setAttribute('data-comment-ids', 'comment-1');
210209
viewportHost.appendChild(highlight);
211210

212211
dispatchPointerDown(highlight);
213212

214-
expect(mockEditor.emit).toHaveBeenCalledWith('commentsUpdate', {
215-
type: comments_module_events.SELECTED,
216-
activeCommentId: 'comment-1',
217-
});
218-
expect(resolvePointerPositionHit).not.toHaveBeenCalled();
219-
expect(TextSelection.create as unknown as Mock).not.toHaveBeenCalled();
220-
expect(mockEditor.state.tr.setSelection).not.toHaveBeenCalled();
221-
expect(mockEditor.view.dispatch).not.toHaveBeenCalled();
222-
expect(mockEditor.view.focus).not.toHaveBeenCalled();
223-
expect(editorDom.focus).not.toHaveBeenCalled();
224-
expect(viewportHost.setPointerCapture).not.toHaveBeenCalled();
213+
expect(mockEditor.emit).not.toHaveBeenCalledWith(
214+
'commentsUpdate',
215+
expect.objectContaining({ activeCommentId: 'comment-1' }),
216+
);
217+
expect(mockEditor.commands.setCursorById).not.toHaveBeenCalled();
218+
expect(resolvePointerPositionHit).toHaveBeenCalled();
219+
expect(TextSelection.create as unknown as Mock).toHaveBeenCalled();
220+
expect(mockEditor.state.tr.setSelection).toHaveBeenCalled();
221+
expect(viewportHost.setPointerCapture).toHaveBeenCalled();
225222
});
226223

227-
it('activates an inactive single-thread highlight without falling back to generic selection handling', () => {
224+
it('lets direct clicks on an inactive single-thread highlight fall through to generic caret placement', () => {
228225
mockEditor.state.comments$.activeThreadId = 'comment-2';
229226

230227
const highlight = document.createElement('span');
@@ -234,16 +231,11 @@ describe('EditorInputManager - single-thread comment highlight clicks', () => {
234231

235232
dispatchPointerDown(highlight);
236233

237-
expect(mockEditor.commands.setCursorById).toHaveBeenCalledWith('comment-1', {
238-
activeCommentId: 'comment-1',
239-
});
240-
expect(resolvePointerPositionHit).not.toHaveBeenCalled();
241-
expect(TextSelection.create as unknown as Mock).not.toHaveBeenCalled();
242-
expect(mockEditor.state.tr.setSelection).not.toHaveBeenCalled();
243-
expect(mockEditor.view.dispatch).not.toHaveBeenCalled();
244-
expect(mockEditor.view.focus).not.toHaveBeenCalled();
245-
expect(editorDom.focus).not.toHaveBeenCalled();
246-
expect(viewportHost.setPointerCapture).not.toHaveBeenCalled();
234+
expect(mockEditor.commands.setCursorById).not.toHaveBeenCalled();
235+
expect(resolvePointerPositionHit).toHaveBeenCalled();
236+
expect(TextSelection.create as unknown as Mock).toHaveBeenCalled();
237+
expect(mockEditor.state.tr.setSelection).toHaveBeenCalled();
238+
expect(viewportHost.setPointerCapture).toHaveBeenCalled();
247239
});
248240

249241
it('activates a tracked-change decoration when it owns the clicked visual surface', () => {

packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -892,6 +892,73 @@ describe('CommentDialog.vue', () => {
892892
expect(wrapper.emitted()).not.toHaveProperty('dialog-exit');
893893
});
894894

895+
it('does not deselect when e.target is wrong but elementFromPoint finds a comment highlight', async () => {
896+
const { wrapper, baseComment } = await mountDialog();
897+
commentsStoreStub.activeComment.value = baseComment.commentId;
898+
899+
// Simulate pointer capture redirecting e.target to the viewport host
900+
const viewportHost = document.createElement('div');
901+
const commentHighlight = document.createElement('span');
902+
commentHighlight.className = 'superdoc-comment-highlight';
903+
document.body.appendChild(commentHighlight);
904+
905+
const originalElementFromPoint = document.elementFromPoint;
906+
document.elementFromPoint = vi.fn(() => commentHighlight);
907+
908+
const handler = wrapper.element.__clickOutside;
909+
handler({ target: viewportHost, clientX: 50, clientY: 50 });
910+
911+
expect(commentsStoreStub.setActiveComment).not.toHaveBeenCalled();
912+
expect(wrapper.emitted()).not.toHaveProperty('dialog-exit');
913+
914+
document.elementFromPoint = originalElementFromPoint;
915+
document.body.removeChild(commentHighlight);
916+
});
917+
918+
it('does not deselect when elementFromPoint finds a tracked-change element', async () => {
919+
const { wrapper, baseComment } = await mountDialog();
920+
commentsStoreStub.activeComment.value = baseComment.commentId;
921+
922+
const viewportHost = document.createElement('div');
923+
const trackedInsert = document.createElement('span');
924+
trackedInsert.className = 'track-insert';
925+
document.body.appendChild(trackedInsert);
926+
927+
const originalElementFromPoint = document.elementFromPoint;
928+
document.elementFromPoint = vi.fn(() => trackedInsert);
929+
930+
const handler = wrapper.element.__clickOutside;
931+
handler({ target: viewportHost, clientX: 50, clientY: 50 });
932+
933+
expect(commentsStoreStub.setActiveComment).not.toHaveBeenCalled();
934+
expect(wrapper.emitted()).not.toHaveProperty('dialog-exit');
935+
936+
document.elementFromPoint = originalElementFromPoint;
937+
document.body.removeChild(trackedInsert);
938+
});
939+
940+
it('deselects when elementFromPoint returns a non-ignored element', async () => {
941+
const { wrapper, baseComment } = await mountDialog();
942+
commentsStoreStub.activeComment.value = baseComment.commentId;
943+
944+
const viewportHost = document.createElement('div');
945+
const plainDiv = document.createElement('div');
946+
plainDiv.className = 'some-normal-content';
947+
document.body.appendChild(plainDiv);
948+
949+
const originalElementFromPoint = document.elementFromPoint;
950+
document.elementFromPoint = vi.fn(() => plainDiv);
951+
952+
const handler = wrapper.element.__clickOutside;
953+
handler({ target: viewportHost, clientX: 50, clientY: 50 });
954+
955+
expect(commentsStoreStub.setActiveComment).toHaveBeenCalledWith(expect.any(Object), null);
956+
expect(wrapper.emitted('dialog-exit')).toHaveLength(1);
957+
958+
document.elementFromPoint = originalElementFromPoint;
959+
document.body.removeChild(plainDiv);
960+
});
961+
895962
it('sorts tracked change parent first, then child comments by creation time', async () => {
896963
// Simulate a tracked change with two comments on it
897964
// The comments were created after the tracked change but should appear below it

packages/superdoc/src/components/CommentsLayer/CommentDialog.vue

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -438,20 +438,24 @@ const setFocus = () => {
438438
439439
const handleClickOutside = (e) => {
440440
const targetElement = e.target instanceof Element ? e.target : e.target?.parentElement;
441-
const clickedIgnoredTarget = targetElement?.closest?.(
442-
[
443-
'.comments-dropdown__option-label',
444-
'.superdoc-comment-highlight',
445-
'.sd-editor-comment-highlight',
446-
'.sd-editor-tracked-change-highlight',
447-
'.track-insert',
448-
'.track-insert-dec',
449-
'.track-delete',
450-
'.track-delete-dec',
451-
'.track-format',
452-
'.track-format-dec',
453-
].join(','),
454-
);
441+
// Also check what's under the actual click coordinates. Pointer capture
442+
// (used by the presentation editor) can redirect e.target away from the
443+
// originally clicked element, causing the selector check to miss it.
444+
const elementAtPoint = document.elementFromPoint(e.clientX, e.clientY);
445+
const ignoredSelectors = [
446+
'.comments-dropdown__option-label',
447+
'.superdoc-comment-highlight',
448+
'.sd-editor-comment-highlight',
449+
'.sd-editor-tracked-change-highlight',
450+
'.track-insert',
451+
'.track-insert-dec',
452+
'.track-delete',
453+
'.track-delete-dec',
454+
'.track-format',
455+
'.track-format-dec',
456+
].join(',');
457+
const clickedIgnoredTarget =
458+
targetElement?.closest?.(ignoredSelectors) || elementAtPoint?.closest?.(ignoredSelectors);
455459
456460
if (clickedIgnoredTarget || isCommentHighlighted.value) return;
457461
Lines changed: 15 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,9 @@
11
import type { Page } from '@playwright/test';
22

3-
/**
4-
* Right-clicks at the screen location corresponding to a document position in the SuperDoc editor.
5-
*
6-
* This helper queries the editor's coordinates for the given logical document position, calculates a suitable
7-
* (x, y) point within the bounding rectangle, and dispatches a mouse right-click at that spot.
8-
*
9-
* Throws if coordinates cannot be resolved for the given position.
10-
*
11-
* @param {Page} page - The Playwright test page instance.
12-
* @param {number} pos - The logical document position (character offset) at which to right-click.
13-
* @returns {Promise<void>} Resolves when the click has been dispatched.
14-
*/
15-
export async function rightClickAtDocPos(page: Page, pos: number): Promise<void> {
3+
async function getDocPosCoords(
4+
page: Page,
5+
pos: number,
6+
): Promise<{ left: number; right: number; top: number; bottom: number }> {
167
const coords = await page.evaluate((targetPos) => {
178
const editor = (window as any).editor;
189
const rect = editor?.coordsAtPos?.(targetPos);
@@ -29,7 +20,15 @@ export async function rightClickAtDocPos(page: Page, pos: number): Promise<void>
2920
throw new Error(`Could not resolve coordinates for document position ${pos}`);
3021
}
3122

32-
const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1));
33-
const y = (coords.top + coords.bottom) / 2;
34-
await page.mouse.click(x, y, { button: 'right' });
23+
return coords;
24+
}
25+
26+
export async function clickAtDocPos(page: Page, pos: number): Promise<void> {
27+
const coords = await getDocPosCoords(page, pos);
28+
await page.mouse.click(coords.left + 1, (coords.top + coords.bottom) / 2);
29+
}
30+
31+
export async function rightClickAtDocPos(page: Page, pos: number): Promise<void> {
32+
const coords = await getDocPosCoords(page, pos);
33+
await page.mouse.click(coords.left + 1, (coords.top + coords.bottom) / 2, { button: 'right' });
3534
}
Lines changed: 102 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,102 @@
1+
import { test, expect } from '../../fixtures/superdoc.js';
2+
import { addCommentViaUI } from '../../helpers/comments.js';
3+
import { clickAtDocPos } from '../../helpers/editor-interactions.js';
4+
5+
test.use({ config: { toolbar: 'full', comments: 'on' } });
6+
7+
test('SD-2442: clicking inside commented text places a caret and allows typing', async ({ superdoc }) => {
8+
await superdoc.type('alpha beta gamma');
9+
await superdoc.waitForStable();
10+
11+
await addCommentViaUI(superdoc, {
12+
textToSelect: 'beta gamma',
13+
commentText: 'outer comment',
14+
});
15+
16+
await superdoc.assertCommentHighlightExists({ text: 'beta gamma' });
17+
18+
const betaStart = await superdoc.findTextPos('beta');
19+
const insertionPos = betaStart + 2;
20+
21+
await superdoc.clickOnLine(0, 5);
22+
await superdoc.waitForStable();
23+
await expect((await superdoc.getSelection()).from).not.toBe(insertionPos);
24+
25+
await clickAtDocPos(superdoc.page, insertionPos);
26+
await superdoc.waitForStable();
27+
28+
await superdoc.assertSelection(insertionPos);
29+
30+
await superdoc.page.keyboard.type('X');
31+
await superdoc.waitForStable();
32+
33+
await expect.poll(() => superdoc.getTextContent()).toContain('alpha beXta gamma');
34+
});
35+
36+
test('SD-2442: clicking inside commented text activates the comment bubble', async ({ superdoc }) => {
37+
await superdoc.type('hello world');
38+
await superdoc.waitForStable();
39+
40+
await addCommentViaUI(superdoc, {
41+
textToSelect: 'world',
42+
commentText: 'bubble test',
43+
});
44+
45+
await superdoc.assertCommentHighlightExists({ text: 'world' });
46+
47+
// Click outside the comment to deselect it first
48+
await superdoc.clickOnLine(0, 0);
49+
await superdoc.waitForStable();
50+
await expect(superdoc.page.locator('.comments-dialog.is-active')).toHaveCount(0, { timeout: 3000 });
51+
52+
// Click inside the commented text
53+
const worldPos = await superdoc.findTextPos('world');
54+
await clickAtDocPos(superdoc.page, worldPos + 2);
55+
await superdoc.waitForStable();
56+
57+
// The comment bubble should be active and stay active
58+
await expect(superdoc.page.locator('.comments-dialog.is-active')).toBeVisible({ timeout: 5000 });
59+
await expect(superdoc.page.locator('.comments-dialog.is-active')).toContainText('bubble test');
60+
});
61+
62+
test('SD-2442: double-clicking inside commented text selects a word', async ({ superdoc }) => {
63+
await superdoc.type('select this word');
64+
await superdoc.waitForStable();
65+
66+
await addCommentViaUI(superdoc, {
67+
textToSelect: 'this word',
68+
commentText: 'dblclick test',
69+
});
70+
71+
await superdoc.assertCommentHighlightExists({ text: 'this word' });
72+
73+
// Click outside first
74+
await superdoc.clickOnLine(0, 0);
75+
await superdoc.waitForStable();
76+
77+
// Double-click on "word" inside the comment highlight
78+
const wordPos = await superdoc.findTextPos('word');
79+
const coords = await superdoc.page.evaluate((pos) => {
80+
const editor = (window as any).editor;
81+
const rect = editor?.coordsAtPos?.(pos);
82+
if (!rect) return null;
83+
return { left: Number(rect.left), right: Number(rect.right), top: Number(rect.top), bottom: Number(rect.bottom) };
84+
}, wordPos);
85+
86+
if (coords) {
87+
const x = coords.left + 5;
88+
const y = (coords.top + coords.bottom) / 2;
89+
await superdoc.page.mouse.dblclick(x, y);
90+
await superdoc.waitForStable();
91+
92+
const sel = await superdoc.getSelection();
93+
const selectedText = await superdoc.page.evaluate(
94+
({ from, to }) => {
95+
const editor = (window as any).editor;
96+
return editor.state.doc.textBetween(from, to);
97+
},
98+
{ from: sel.from, to: sel.to },
99+
);
100+
expect(selectedText).toBe('word');
101+
}
102+
});

0 commit comments

Comments
 (0)