diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts index 2c82bf6bef..a9e90d5b1f 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/pointer-events/EditorInputManager.ts @@ -97,6 +97,10 @@ function getCommentHighlightThreadIds(target: EventTarget | null): string[] { .filter(Boolean); } +function isDirectSingleCommentHighlightHit(target: EventTarget | null): boolean { + return getCommentHighlightThreadIds(target).length === 1; +} + function resolveTrackChangeThreadId(target: EventTarget | null): string | null { if (!(target instanceof Element)) { return null; @@ -216,6 +220,14 @@ function shouldIgnoreRepeatClickOnActiveComment( return false; } + // Direct clicks on commented text should place a caret at the clicked + // position and let the comments plugin infer the active thread from the + // resulting selection. Only preserve the pointerdown short-circuit for + // nearby non-text surfaces, such as split-run gaps. + if (isDirectSingleCommentHighlightHit(target)) { + return false; + } + return resolveCommentThreadIdNearPointer(target, clientX, clientY) === activeThreadId; } @@ -2230,6 +2242,10 @@ export class EditorInputManager { } #handleSingleCommentHighlightClick(event: PointerEvent, target: HTMLElement | null, editor: Editor): boolean { + if (isDirectSingleCommentHighlightHit(target)) { + return false; + } + const clickedThreadId = resolveCommentThreadIdNearPointer(target, event.clientX, event.clientY); if (!clickedThreadId) { return false; diff --git a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/EditorInputManager.activeCommentClick.test.ts b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/EditorInputManager.activeCommentClick.test.ts index 3915e29ec6..ca2d06e4db 100644 --- a/packages/super-editor/src/editors/v1/core/presentation-editor/tests/EditorInputManager.activeCommentClick.test.ts +++ b/packages/super-editor/src/editors/v1/core/presentation-editor/tests/EditorInputManager.activeCommentClick.test.ts @@ -1,6 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest'; -import { comments_module_events } from '@superdoc/common'; import { clickToPosition } from '@superdoc/layout-bridge'; import { resolvePointerPositionHit } from '../input/PositionHitResolver.js'; import { TextSelection } from 'prosemirror-state'; @@ -203,7 +202,7 @@ describe('EditorInputManager - single-thread comment highlight clicks', () => { return elementsFromPoint; } - it('treats a click on the already-active single-thread highlight as a no-op', () => { + it('lets direct clicks on the active single-thread highlight fall through to generic caret placement', () => { const highlight = document.createElement('span'); highlight.className = 'superdoc-comment-highlight'; highlight.setAttribute('data-comment-ids', 'comment-1'); @@ -211,20 +210,18 @@ describe('EditorInputManager - single-thread comment highlight clicks', () => { dispatchPointerDown(highlight); - expect(mockEditor.emit).toHaveBeenCalledWith('commentsUpdate', { - type: comments_module_events.SELECTED, - activeCommentId: 'comment-1', - }); - expect(resolvePointerPositionHit).not.toHaveBeenCalled(); - expect(TextSelection.create as unknown as Mock).not.toHaveBeenCalled(); - expect(mockEditor.state.tr.setSelection).not.toHaveBeenCalled(); - expect(mockEditor.view.dispatch).not.toHaveBeenCalled(); - expect(mockEditor.view.focus).not.toHaveBeenCalled(); - expect(editorDom.focus).not.toHaveBeenCalled(); - expect(viewportHost.setPointerCapture).not.toHaveBeenCalled(); + expect(mockEditor.emit).not.toHaveBeenCalledWith( + 'commentsUpdate', + expect.objectContaining({ activeCommentId: 'comment-1' }), + ); + expect(mockEditor.commands.setCursorById).not.toHaveBeenCalled(); + expect(resolvePointerPositionHit).toHaveBeenCalled(); + expect(TextSelection.create as unknown as Mock).toHaveBeenCalled(); + expect(mockEditor.state.tr.setSelection).toHaveBeenCalled(); + expect(viewportHost.setPointerCapture).toHaveBeenCalled(); }); - it('activates an inactive single-thread highlight without falling back to generic selection handling', () => { + it('lets direct clicks on an inactive single-thread highlight fall through to generic caret placement', () => { mockEditor.state.comments$.activeThreadId = 'comment-2'; const highlight = document.createElement('span'); @@ -234,16 +231,11 @@ describe('EditorInputManager - single-thread comment highlight clicks', () => { dispatchPointerDown(highlight); - expect(mockEditor.commands.setCursorById).toHaveBeenCalledWith('comment-1', { - activeCommentId: 'comment-1', - }); - expect(resolvePointerPositionHit).not.toHaveBeenCalled(); - expect(TextSelection.create as unknown as Mock).not.toHaveBeenCalled(); - expect(mockEditor.state.tr.setSelection).not.toHaveBeenCalled(); - expect(mockEditor.view.dispatch).not.toHaveBeenCalled(); - expect(mockEditor.view.focus).not.toHaveBeenCalled(); - expect(editorDom.focus).not.toHaveBeenCalled(); - expect(viewportHost.setPointerCapture).not.toHaveBeenCalled(); + expect(mockEditor.commands.setCursorById).not.toHaveBeenCalled(); + expect(resolvePointerPositionHit).toHaveBeenCalled(); + expect(TextSelection.create as unknown as Mock).toHaveBeenCalled(); + expect(mockEditor.state.tr.setSelection).toHaveBeenCalled(); + expect(viewportHost.setPointerCapture).toHaveBeenCalled(); }); it('activates a tracked-change decoration when it owns the clicked visual surface', () => { diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js index 286cae8552..7094bd6a4b 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.test.js @@ -892,6 +892,73 @@ describe('CommentDialog.vue', () => { expect(wrapper.emitted()).not.toHaveProperty('dialog-exit'); }); + it('does not deselect when e.target is wrong but elementFromPoint finds a comment highlight', async () => { + const { wrapper, baseComment } = await mountDialog(); + commentsStoreStub.activeComment.value = baseComment.commentId; + + // Simulate pointer capture redirecting e.target to the viewport host + const viewportHost = document.createElement('div'); + const commentHighlight = document.createElement('span'); + commentHighlight.className = 'superdoc-comment-highlight'; + document.body.appendChild(commentHighlight); + + const originalElementFromPoint = document.elementFromPoint; + document.elementFromPoint = vi.fn(() => commentHighlight); + + const handler = wrapper.element.__clickOutside; + handler({ target: viewportHost, clientX: 50, clientY: 50 }); + + expect(commentsStoreStub.setActiveComment).not.toHaveBeenCalled(); + expect(wrapper.emitted()).not.toHaveProperty('dialog-exit'); + + document.elementFromPoint = originalElementFromPoint; + document.body.removeChild(commentHighlight); + }); + + it('does not deselect when elementFromPoint finds a tracked-change element', async () => { + const { wrapper, baseComment } = await mountDialog(); + commentsStoreStub.activeComment.value = baseComment.commentId; + + const viewportHost = document.createElement('div'); + const trackedInsert = document.createElement('span'); + trackedInsert.className = 'track-insert'; + document.body.appendChild(trackedInsert); + + const originalElementFromPoint = document.elementFromPoint; + document.elementFromPoint = vi.fn(() => trackedInsert); + + const handler = wrapper.element.__clickOutside; + handler({ target: viewportHost, clientX: 50, clientY: 50 }); + + expect(commentsStoreStub.setActiveComment).not.toHaveBeenCalled(); + expect(wrapper.emitted()).not.toHaveProperty('dialog-exit'); + + document.elementFromPoint = originalElementFromPoint; + document.body.removeChild(trackedInsert); + }); + + it('deselects when elementFromPoint returns a non-ignored element', async () => { + const { wrapper, baseComment } = await mountDialog(); + commentsStoreStub.activeComment.value = baseComment.commentId; + + const viewportHost = document.createElement('div'); + const plainDiv = document.createElement('div'); + plainDiv.className = 'some-normal-content'; + document.body.appendChild(plainDiv); + + const originalElementFromPoint = document.elementFromPoint; + document.elementFromPoint = vi.fn(() => plainDiv); + + const handler = wrapper.element.__clickOutside; + handler({ target: viewportHost, clientX: 50, clientY: 50 }); + + expect(commentsStoreStub.setActiveComment).toHaveBeenCalledWith(expect.any(Object), null); + expect(wrapper.emitted('dialog-exit')).toHaveLength(1); + + document.elementFromPoint = originalElementFromPoint; + document.body.removeChild(plainDiv); + }); + it('sorts tracked change parent first, then child comments by creation time', async () => { // Simulate a tracked change with two comments on it // The comments were created after the tracked change but should appear below it diff --git a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue index 77fe4019b1..5af037ee93 100644 --- a/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue +++ b/packages/superdoc/src/components/CommentsLayer/CommentDialog.vue @@ -438,20 +438,24 @@ const setFocus = () => { const handleClickOutside = (e) => { const targetElement = e.target instanceof Element ? e.target : e.target?.parentElement; - const clickedIgnoredTarget = targetElement?.closest?.( - [ - '.comments-dropdown__option-label', - '.superdoc-comment-highlight', - '.sd-editor-comment-highlight', - '.sd-editor-tracked-change-highlight', - '.track-insert', - '.track-insert-dec', - '.track-delete', - '.track-delete-dec', - '.track-format', - '.track-format-dec', - ].join(','), - ); + // Also check what's under the actual click coordinates. Pointer capture + // (used by the presentation editor) can redirect e.target away from the + // originally clicked element, causing the selector check to miss it. + const elementAtPoint = document.elementFromPoint(e.clientX, e.clientY); + const ignoredSelectors = [ + '.comments-dropdown__option-label', + '.superdoc-comment-highlight', + '.sd-editor-comment-highlight', + '.sd-editor-tracked-change-highlight', + '.track-insert', + '.track-insert-dec', + '.track-delete', + '.track-delete-dec', + '.track-format', + '.track-format-dec', + ].join(','); + const clickedIgnoredTarget = + targetElement?.closest?.(ignoredSelectors) || elementAtPoint?.closest?.(ignoredSelectors); if (clickedIgnoredTarget || isCommentHighlighted.value) return; diff --git a/tests/behavior/helpers/editor-interactions.ts b/tests/behavior/helpers/editor-interactions.ts index db9b586a9e..40619a27af 100644 --- a/tests/behavior/helpers/editor-interactions.ts +++ b/tests/behavior/helpers/editor-interactions.ts @@ -1,18 +1,9 @@ import type { Page } from '@playwright/test'; -/** - * Right-clicks at the screen location corresponding to a document position in the SuperDoc editor. - * - * This helper queries the editor's coordinates for the given logical document position, calculates a suitable - * (x, y) point within the bounding rectangle, and dispatches a mouse right-click at that spot. - * - * Throws if coordinates cannot be resolved for the given position. - * - * @param {Page} page - The Playwright test page instance. - * @param {number} pos - The logical document position (character offset) at which to right-click. - * @returns {Promise} Resolves when the click has been dispatched. - */ -export async function rightClickAtDocPos(page: Page, pos: number): Promise { +async function getDocPosCoords( + page: Page, + pos: number, +): Promise<{ left: number; right: number; top: number; bottom: number }> { const coords = await page.evaluate((targetPos) => { const editor = (window as any).editor; const rect = editor?.coordsAtPos?.(targetPos); @@ -29,7 +20,15 @@ export async function rightClickAtDocPos(page: Page, pos: number): Promise throw new Error(`Could not resolve coordinates for document position ${pos}`); } - const x = Math.min(Math.max(coords.left + 1, coords.left), Math.max(coords.right - 1, coords.left + 1)); - const y = (coords.top + coords.bottom) / 2; - await page.mouse.click(x, y, { button: 'right' }); + return coords; +} + +export async function clickAtDocPos(page: Page, pos: number): Promise { + const coords = await getDocPosCoords(page, pos); + await page.mouse.click(coords.left + 1, (coords.top + coords.bottom) / 2); +} + +export async function rightClickAtDocPos(page: Page, pos: number): Promise { + const coords = await getDocPosCoords(page, pos); + await page.mouse.click(coords.left + 1, (coords.top + coords.bottom) / 2, { button: 'right' }); } diff --git a/tests/behavior/tests/comments/sd-2442-commented-text-caret.spec.ts b/tests/behavior/tests/comments/sd-2442-commented-text-caret.spec.ts new file mode 100644 index 0000000000..14557fa093 --- /dev/null +++ b/tests/behavior/tests/comments/sd-2442-commented-text-caret.spec.ts @@ -0,0 +1,102 @@ +import { test, expect } from '../../fixtures/superdoc.js'; +import { addCommentViaUI } from '../../helpers/comments.js'; +import { clickAtDocPos } from '../../helpers/editor-interactions.js'; + +test.use({ config: { toolbar: 'full', comments: 'on' } }); + +test('SD-2442: clicking inside commented text places a caret and allows typing', async ({ superdoc }) => { + await superdoc.type('alpha beta gamma'); + await superdoc.waitForStable(); + + await addCommentViaUI(superdoc, { + textToSelect: 'beta gamma', + commentText: 'outer comment', + }); + + await superdoc.assertCommentHighlightExists({ text: 'beta gamma' }); + + const betaStart = await superdoc.findTextPos('beta'); + const insertionPos = betaStart + 2; + + await superdoc.clickOnLine(0, 5); + await superdoc.waitForStable(); + await expect((await superdoc.getSelection()).from).not.toBe(insertionPos); + + await clickAtDocPos(superdoc.page, insertionPos); + await superdoc.waitForStable(); + + await superdoc.assertSelection(insertionPos); + + await superdoc.page.keyboard.type('X'); + await superdoc.waitForStable(); + + await expect.poll(() => superdoc.getTextContent()).toContain('alpha beXta gamma'); +}); + +test('SD-2442: clicking inside commented text activates the comment bubble', async ({ superdoc }) => { + await superdoc.type('hello world'); + await superdoc.waitForStable(); + + await addCommentViaUI(superdoc, { + textToSelect: 'world', + commentText: 'bubble test', + }); + + await superdoc.assertCommentHighlightExists({ text: 'world' }); + + // Click outside the comment to deselect it first + await superdoc.clickOnLine(0, 0); + await superdoc.waitForStable(); + await expect(superdoc.page.locator('.comments-dialog.is-active')).toHaveCount(0, { timeout: 3000 }); + + // Click inside the commented text + const worldPos = await superdoc.findTextPos('world'); + await clickAtDocPos(superdoc.page, worldPos + 2); + await superdoc.waitForStable(); + + // The comment bubble should be active and stay active + await expect(superdoc.page.locator('.comments-dialog.is-active')).toBeVisible({ timeout: 5000 }); + await expect(superdoc.page.locator('.comments-dialog.is-active')).toContainText('bubble test'); +}); + +test('SD-2442: double-clicking inside commented text selects a word', async ({ superdoc }) => { + await superdoc.type('select this word'); + await superdoc.waitForStable(); + + await addCommentViaUI(superdoc, { + textToSelect: 'this word', + commentText: 'dblclick test', + }); + + await superdoc.assertCommentHighlightExists({ text: 'this word' }); + + // Click outside first + await superdoc.clickOnLine(0, 0); + await superdoc.waitForStable(); + + // Double-click on "word" inside the comment highlight + const wordPos = await superdoc.findTextPos('word'); + const coords = await superdoc.page.evaluate((pos) => { + const editor = (window as any).editor; + const rect = editor?.coordsAtPos?.(pos); + if (!rect) return null; + return { left: Number(rect.left), right: Number(rect.right), top: Number(rect.top), bottom: Number(rect.bottom) }; + }, wordPos); + + if (coords) { + const x = coords.left + 5; + const y = (coords.top + coords.bottom) / 2; + await superdoc.page.mouse.dblclick(x, y); + await superdoc.waitForStable(); + + const sel = await superdoc.getSelection(); + const selectedText = await superdoc.page.evaluate( + ({ from, to }) => { + const editor = (window as any).editor; + return editor.state.doc.textBetween(from, to); + }, + { from: sel.from, to: sel.to }, + ); + expect(selectedText).toBe('word'); + } +});