Skip to content

Commit b014618

Browse files
fix(comments): keep floating comment bubbles aligned with the selected thread (SD-2210 and SD-2223) (#2390)
* fix(comments): align floating sidebar comments to active text Replace viewport-based bubble scrolling with sidebar layer translation so the active floating comment aligns to the clicked document text. Also constrain the floating comments container to act as a clipped viewport for the translated sidebar content. * fix(comments): align document text to clicked floating thread When a floating comment bubble is clicked, scroll the editor anchor to the bubble's current viewport Y and apply the sidebar translation after the post-activation remeasure so the thread and document text stay aligned. Also reset the sidebar offset when no thread is active. * fix(comments): preserve clicked thread on overlapping anchors Pass a preferred active thread when navigating from a sidebar bubble so overlapping comment/tracked-change ranges keep the clicked thread selected instead of falling back to the regular comment. Also adds a regression test for the overlapping tracked-change case. * fix(comments): align pending comment bubble to new selection Use the current selection's viewport Y when creating a pending comment so the new bubble is brought into view and aligned with the text being commented on. Also normalize pending-thread key resolution in FloatingComments and let the pending bubble render immediately from selection bounds before the live editor position arrives. * fix(comments): clear stale instant sidebar alignment on no-op focus Only queue instant sidebar alignment when clicking a thread that will actually become active. Clear any pending instant target on no-op or resolved-card focus paths so later editor-driven selections do not reuse a stale bubble Y position. * test(comments): add behavior tests * test: adjust existing tests * fix(comments): avoid reactivating resolved threads on bubble click Do not pass preferredActiveThreadId when focusing a resolved comment bubble. This preserves scroll-to-anchor behavior without letting the editor-side preferred-thread override re-select resolved threads. Also adds a regression test covering the resolved bubble path. * fix(comments): ignore repeat clicks on the active editor highlight
1 parent ca8ea8f commit b014618

17 files changed

Lines changed: 890 additions & 53 deletions

packages/super-editor/src/core/presentation-editor/PresentationEditor.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2191,6 +2191,52 @@ export class PresentationEditor extends EventEmitter {
21912191
}
21922192
}
21932193

2194+
/**
2195+
* Scroll a comment or tracked-change anchor so its top edge lands at the
2196+
* requested viewport Y coordinate.
2197+
*
2198+
* @param threadId - Comment or tracked-change identifier
2199+
* @param targetClientY - Desired top position in client/viewport coordinates
2200+
* @param options - Scrolling options
2201+
* @param options.behavior - Scroll behavior ('auto' | 'smooth')
2202+
* @returns True when the thread could be resolved and scrolling was applied
2203+
*/
2204+
scrollThreadAnchorToClientY(
2205+
threadId: string,
2206+
targetClientY: number,
2207+
options: { behavior?: ScrollBehavior } = {},
2208+
): boolean {
2209+
if (!threadId || !Number.isFinite(targetClientY)) return false;
2210+
2211+
const threadPosition = this.#collectCommentPositions()[threadId];
2212+
if (!threadPosition) return false;
2213+
2214+
const selectionBounds = this.getSelectionBounds(threadPosition.start, threadPosition.end);
2215+
const currentTop = selectionBounds?.bounds?.top;
2216+
if (!Number.isFinite(currentTop)) return false;
2217+
2218+
const deltaY = currentTop - targetClientY;
2219+
if (Math.abs(deltaY) < 1) return true;
2220+
2221+
const behavior = options.behavior ?? 'auto';
2222+
const scrollTarget = this.#scrollContainer ?? this.#visibleHost;
2223+
2224+
if (scrollTarget instanceof Window) {
2225+
const currentScrollY = scrollTarget.scrollY ?? scrollTarget.pageYOffset ?? 0;
2226+
scrollTarget.scrollTo({ top: currentScrollY + deltaY, behavior });
2227+
return true;
2228+
}
2229+
2230+
if (scrollTarget instanceof HTMLElement) {
2231+
const maxScrollTop = Math.max(0, scrollTarget.scrollHeight - scrollTarget.clientHeight);
2232+
const nextScrollTop = Math.max(0, Math.min(maxScrollTop, scrollTarget.scrollTop + deltaY));
2233+
scrollTarget.scrollTo({ top: nextScrollTop, behavior });
2234+
return true;
2235+
}
2236+
2237+
return false;
2238+
}
2239+
21942240
/**
21952241
* Find the DOM element containing a specific document position.
21962242
* Returns the most specific (smallest range) matching element.

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

Lines changed: 68 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ import type { Node as ProseMirrorNode } from 'prosemirror-model';
1717
import { CellSelection } from 'prosemirror-tables';
1818
import type { Editor } from '../../Editor.js';
1919
import type { Layout, FlowBlock, Measure } from '@superdoc/contracts';
20+
import { comments_module_events } from '@superdoc/common';
2021
import type { CellAnchorState, PendingMarginClick, HeaderFooterRegion } from '../types.js';
2122
import type { PositionHit, PageGeometryHelper, TableHitResult } from '@superdoc/layout-bridge';
2223
import type { SelectionDebugHudState } from '../selection/SelectionDebug.js';
@@ -38,6 +39,7 @@ import {
3839
import { debugLog } from '../selection/SelectionDebug.js';
3940
import { DOM_CLASS_NAMES, buildInlineImagePmSelector } from '@superdoc/painter-dom';
4041
import { isSemanticFootnoteBlockId } from '../semantic-flow-constants.js';
42+
import { CommentsPluginKey } from '@extensions/comment/comments-plugin.js';
4143

4244
// =============================================================================
4345
// Constants
@@ -49,6 +51,7 @@ const AUTO_SCROLL_EDGE_PX = 32;
4951
const AUTO_SCROLL_MAX_SPEED_PX = 24;
5052
/** Tolerance for detecting scrollability to handle sub-pixel rounding in browsers */
5153
const SCROLL_DETECTION_TOLERANCE_PX = 1;
54+
const COMMENT_HIGHLIGHT_SELECTOR = '.superdoc-comment-highlight';
5255

5356
const clamp = (value: number, min: number, max: number): number => Math.max(min, Math.min(max, value));
5457

@@ -61,6 +64,49 @@ function isFootnoteBlockId(blockId: string): boolean {
6164
return typeof blockId === 'string' && (blockId.startsWith('footnote-') || isSemanticFootnoteBlockId(blockId));
6265
}
6366

67+
function getCommentHighlightThreadIds(target: EventTarget | null): string[] {
68+
if (!(target instanceof Element)) {
69+
return [];
70+
}
71+
72+
const highlight = target.closest(COMMENT_HIGHLIGHT_SELECTOR);
73+
const threadIds = highlight?.getAttribute('data-comment-ids');
74+
75+
if (!threadIds) {
76+
return [];
77+
}
78+
79+
return threadIds
80+
.split(',')
81+
.map((threadId) => threadId.trim())
82+
.filter(Boolean);
83+
}
84+
85+
function getActiveCommentThreadId(editor: Editor): string | null {
86+
const pluginState = CommentsPluginKey.getState(editor.state) as { activeThreadId?: unknown } | null;
87+
const activeThreadId = pluginState?.activeThreadId;
88+
89+
if (typeof activeThreadId !== 'string' || activeThreadId.length === 0) {
90+
return null;
91+
}
92+
93+
return activeThreadId;
94+
}
95+
96+
function shouldIgnoreRepeatClickOnActiveComment(target: EventTarget | null, activeThreadId: string | null): boolean {
97+
if (!activeThreadId) {
98+
return false;
99+
}
100+
101+
const clickedThreadIds = getCommentHighlightThreadIds(target);
102+
103+
if (clickedThreadIds.length !== 1) {
104+
return false;
105+
}
106+
107+
return clickedThreadIds[0] === activeThreadId;
108+
}
109+
64110
// =============================================================================
65111
// Types
66112
// =============================================================================
@@ -878,6 +924,11 @@ export class EditorInputManager {
878924
return;
879925
}
880926

927+
const editor = this.#deps.getEditor();
928+
if (this.#handleRepeatClickOnActiveComment(event, target, editor)) {
929+
return;
930+
}
931+
881932
const layoutState = this.#deps.getLayoutState();
882933
if (!layoutState.layout) {
883934
this.#handleClickWithoutLayout(event, isDraggableAnnotation);
@@ -932,7 +983,6 @@ export class EditorInputManager {
932983
pageGeometryHelper ?? undefined,
933984
);
934985

935-
const editor = this.#deps.getEditor();
936986
const doc = editor.state?.doc;
937987
const epochMapper = this.#deps.getEpochMapper();
938988
const mapped =
@@ -2036,4 +2086,21 @@ export class EditorInputManager {
20362086
editorDom.focus();
20372087
view?.focus();
20382088
}
2089+
2090+
#handleRepeatClickOnActiveComment(event: PointerEvent, target: HTMLElement | null, editor: Editor): boolean {
2091+
const activeThreadId = getActiveCommentThreadId(editor);
2092+
2093+
if (!shouldIgnoreRepeatClickOnActiveComment(target, activeThreadId)) {
2094+
return false;
2095+
}
2096+
2097+
event.preventDefault();
2098+
this.#focusEditor();
2099+
editor.emit?.('commentsUpdate', {
2100+
type: comments_module_events.SELECTED,
2101+
activeCommentId: activeThreadId,
2102+
});
2103+
2104+
return true;
2105+
}
20392106
}
Lines changed: 196 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,196 @@
1+
import { afterEach, beforeEach, describe, expect, it, vi, type Mock } from 'vitest';
2+
3+
import { comments_module_events } from '@superdoc/common';
4+
import { clickToPosition } from '@superdoc/layout-bridge';
5+
import { TextSelection } from 'prosemirror-state';
6+
7+
import {
8+
EditorInputManager,
9+
type EditorInputDependencies,
10+
type EditorInputCallbacks,
11+
} from '../pointer-events/EditorInputManager.js';
12+
13+
vi.mock('@superdoc/layout-bridge', () => ({
14+
clickToPosition: vi.fn(() => ({ pos: 24, layoutEpoch: 1, pageIndex: 0, blockId: 'body-1' })),
15+
getFragmentAtPosition: vi.fn(() => null),
16+
}));
17+
18+
vi.mock('prosemirror-state', async (importOriginal) => {
19+
const original = await importOriginal<typeof import('prosemirror-state')>();
20+
return {
21+
...original,
22+
TextSelection: {
23+
...original.TextSelection,
24+
create: vi.fn(() => ({
25+
empty: true,
26+
$from: { parent: { inlineContent: true } },
27+
})),
28+
},
29+
};
30+
});
31+
32+
describe('EditorInputManager - repeated active comment clicks', () => {
33+
let manager: EditorInputManager;
34+
let viewportHost: HTMLElement;
35+
let visibleHost: HTMLElement;
36+
let mockEditor: {
37+
isEditable: boolean;
38+
state: {
39+
doc: { content: { size: number }; nodesBetween: Mock };
40+
tr: { setSelection: Mock; setStoredMarks: Mock };
41+
selection: { $anchor: null };
42+
storedMarks: null;
43+
comments$: { activeThreadId: string | null };
44+
};
45+
view: {
46+
dispatch: Mock;
47+
dom: HTMLElement;
48+
focus: Mock;
49+
hasFocus: Mock;
50+
};
51+
on: Mock;
52+
off: Mock;
53+
emit: Mock;
54+
};
55+
let mockDeps: EditorInputDependencies;
56+
let mockCallbacks: EditorInputCallbacks;
57+
58+
beforeEach(() => {
59+
viewportHost = document.createElement('div');
60+
viewportHost.className = 'presentation-editor__viewport';
61+
viewportHost.setPointerCapture = vi.fn();
62+
viewportHost.releasePointerCapture = vi.fn();
63+
viewportHost.hasPointerCapture = vi.fn(() => true);
64+
65+
visibleHost = document.createElement('div');
66+
visibleHost.className = 'presentation-editor__visible';
67+
visibleHost.appendChild(viewportHost);
68+
69+
const container = document.createElement('div');
70+
container.className = 'presentation-editor';
71+
container.appendChild(visibleHost);
72+
document.body.appendChild(container);
73+
74+
mockEditor = {
75+
isEditable: true,
76+
state: {
77+
doc: {
78+
content: { size: 100 },
79+
resolve: vi.fn(() => ({ depth: 0 })),
80+
nodesBetween: vi.fn((from, to, callback) => {
81+
callback({ isTextblock: true }, 0);
82+
}),
83+
},
84+
tr: {
85+
setSelection: vi.fn().mockReturnThis(),
86+
setStoredMarks: vi.fn().mockReturnThis(),
87+
},
88+
selection: { $anchor: null },
89+
storedMarks: null,
90+
comments$: { activeThreadId: 'comment-1' },
91+
},
92+
view: {
93+
dispatch: vi.fn(),
94+
dom: document.createElement('div'),
95+
focus: vi.fn(),
96+
hasFocus: vi.fn(() => false),
97+
},
98+
on: vi.fn(),
99+
off: vi.fn(),
100+
emit: vi.fn(),
101+
};
102+
103+
mockDeps = {
104+
getActiveEditor: vi.fn(() => mockEditor as unknown as ReturnType<EditorInputDependencies['getActiveEditor']>),
105+
getEditor: vi.fn(() => mockEditor as unknown as ReturnType<EditorInputDependencies['getEditor']>),
106+
getLayoutState: vi.fn(() => ({ layout: {} as any, blocks: [], measures: [] })),
107+
getEpochMapper: vi.fn(() => ({
108+
mapPosFromLayoutToCurrentDetailed: vi.fn(() => ({ ok: true, pos: 24, toEpoch: 1 })),
109+
})) as unknown as EditorInputDependencies['getEpochMapper'],
110+
getViewportHost: vi.fn(() => viewportHost),
111+
getVisibleHost: vi.fn(() => visibleHost),
112+
getLayoutMode: vi.fn(() => 'vertical'),
113+
getHeaderFooterSession: vi.fn(() => null),
114+
getPageGeometryHelper: vi.fn(() => null),
115+
getZoom: vi.fn(() => 1),
116+
isViewLocked: vi.fn(() => false),
117+
getDocumentMode: vi.fn(() => 'editing'),
118+
getPageElement: vi.fn(() => null),
119+
isSelectionAwareVirtualizationEnabled: vi.fn(() => false),
120+
};
121+
122+
mockCallbacks = {
123+
normalizeClientPoint: vi.fn((clientX: number, clientY: number) => ({ x: clientX, y: clientY })),
124+
scheduleSelectionUpdate: vi.fn(),
125+
updateSelectionDebugHud: vi.fn(),
126+
};
127+
128+
manager = new EditorInputManager();
129+
manager.setDependencies(mockDeps);
130+
manager.setCallbacks(mockCallbacks);
131+
manager.bind();
132+
});
133+
134+
afterEach(() => {
135+
manager.destroy();
136+
document.body.innerHTML = '';
137+
vi.clearAllMocks();
138+
});
139+
140+
function getPointerEventImpl(): typeof PointerEvent | typeof MouseEvent {
141+
return (
142+
(globalThis as unknown as { PointerEvent?: typeof PointerEvent; MouseEvent: typeof MouseEvent }).PointerEvent ??
143+
globalThis.MouseEvent
144+
);
145+
}
146+
147+
function dispatchPointerDown(target: HTMLElement): void {
148+
const PointerEventImpl = getPointerEventImpl();
149+
target.dispatchEvent(
150+
new PointerEventImpl('pointerdown', {
151+
bubbles: true,
152+
cancelable: true,
153+
button: 0,
154+
buttons: 1,
155+
clientX: 10,
156+
clientY: 10,
157+
} as PointerEventInit),
158+
);
159+
}
160+
161+
it('treats a click on the already-active single-thread highlight as a no-op', () => {
162+
const highlight = document.createElement('span');
163+
highlight.className = 'superdoc-comment-highlight';
164+
highlight.setAttribute('data-comment-ids', 'comment-1');
165+
viewportHost.appendChild(highlight);
166+
167+
dispatchPointerDown(highlight);
168+
169+
expect(mockEditor.emit).toHaveBeenCalledWith('commentsUpdate', {
170+
type: comments_module_events.SELECTED,
171+
activeCommentId: 'comment-1',
172+
});
173+
expect(clickToPosition).not.toHaveBeenCalled();
174+
expect(TextSelection.create as unknown as Mock).not.toHaveBeenCalled();
175+
expect(mockEditor.state.tr.setSelection).not.toHaveBeenCalled();
176+
expect(mockEditor.view.dispatch).not.toHaveBeenCalled();
177+
expect(viewportHost.setPointerCapture).not.toHaveBeenCalled();
178+
});
179+
180+
it('does not suppress clicks on overlapping highlights that contain multiple thread ids', () => {
181+
const highlight = document.createElement('span');
182+
highlight.className = 'superdoc-comment-highlight';
183+
highlight.setAttribute('data-comment-ids', 'comment-1,comment-2');
184+
viewportHost.appendChild(highlight);
185+
186+
dispatchPointerDown(highlight);
187+
188+
expect(mockEditor.emit).not.toHaveBeenCalledWith(
189+
'commentsUpdate',
190+
expect.objectContaining({ activeCommentId: 'comment-1' }),
191+
);
192+
expect(clickToPosition).toHaveBeenCalled();
193+
expect(mockEditor.state.tr.setSelection).toHaveBeenCalled();
194+
expect(viewportHost.setPointerCapture).toHaveBeenCalled();
195+
});
196+
});

0 commit comments

Comments
 (0)