Skip to content

Commit ab30a36

Browse files
authored
fix(editor): prevent scroll-to-top when clicking toolbar buttons (#2236)
* fix(editor): prevent scroll-to-top when clicking toolbar buttons (SD-1780) Toolbar buttons had tabindex="0" but no mousedown prevention, so clicking them caused the browser to transfer focus away from the hidden ProseMirror editor. The subsequent refocus triggered ProseMirror's selectionToDOM, and the browser asynchronously scrolled to make the DOM selection visible inside the hidden editor at position:fixed top:0. This only manifested when the window was the scroll container (not a div with overflow:auto). Two fixes applied: 1. Toolbar mousedown preventDefault for non-input elements. This is the standard pattern used by ProseMirror's example editor, Tiptap, and most WYSIWYG editors. It keeps the PM editor focused throughout toolbar interactions. 2. requestAnimationFrame safety net in wrapHiddenEditorFocus. After the synchronous scroll restoration, a RAF callback catches any async browser scroll caused by layout reflow post-focus. * fix(editor): cancel focus-scroll RAF on intentional scroll Store the RAF handle from wrapHiddenEditorFocus on the instance and cancel it when scrollToPosition is called. Prevents the safety net from undoing intentional scrolls like search navigation. * test(editor): exercise RAF scroll-restore callback in focus wrapping test The previous test only verified that requestAnimationFrame was scheduled but never invoked the callback. Now we capture the RAF callback, simulate async scroll drift (scrollY changing between focus call and RAF execution), and verify scrollTo is called to restore the original position.
1 parent e000efd commit ab30a36

3 files changed

Lines changed: 102 additions & 1 deletion

File tree

packages/super-editor/src/components/toolbar/Toolbar.vue

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,10 +77,32 @@ const handleCommand = ({ item, argument, option }) => {
7777
const restoreSelection = () => {
7878
proxy.$toolbar.activeEditor?.commands?.restoreSelection();
7979
};
80+
81+
/**
82+
* Prevents the browser's default focus-transfer behavior when clicking toolbar buttons.
83+
*
84+
* Without this, clicking a toolbar button moves focus from the hidden ProseMirror editor
85+
* to the toolbar button element. The subsequent refocus of the PM editor can trigger
86+
* browser-native scroll adjustments that jump the page to the top — especially when
87+
* the window (not a div) is the scroll container.
88+
*
89+
* Input elements are excluded so they still receive native focus and cursor placement.
90+
*/
91+
const handleToolbarMousedown = (e) => {
92+
if (e.target.closest('input, textarea, [contenteditable="true"]')) return;
93+
e.preventDefault();
94+
};
8095
</script>
8196
8297
<template>
83-
<div class="superdoc-toolbar" :key="toolbarKey" role="toolbar" aria-label="Toolbar" data-editor-ui-surface>
98+
<div
99+
class="superdoc-toolbar"
100+
:key="toolbarKey"
101+
role="toolbar"
102+
aria-label="Toolbar"
103+
data-editor-ui-surface
104+
@mousedown="handleToolbarMousedown"
105+
>
84106
<ButtonGroup
85107
tabindex="0"
86108
v-if="showLeftSide"

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

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,7 @@ export class PresentationEditor extends EventEmitter {
286286
#errorBannerMessage: HTMLElement | null = null;
287287
#renderScheduled = false;
288288
#pendingDocChange = false;
289+
#focusScrollRafId: number | null = null;
289290
#pendingMapping: Mapping | null = null;
290291
#isRerendering = false;
291292
#selectionSync = new SelectionSyncCoordinator();
@@ -775,6 +776,21 @@ export class PresentationEditor extends EventEmitter {
775776
if (win.scrollX !== beforeX || win.scrollY !== beforeY) {
776777
win.scrollTo(beforeX, beforeY);
777778
}
779+
780+
// Safety net: the browser may asynchronously scroll after ProseMirror's
781+
// selectionToDOM() modifies the DOM selection inside the hidden editor.
782+
// A single requestAnimationFrame catches this post-layout scroll.
783+
// The RAF ID is stored so scrollToPosition() can cancel it — otherwise
784+
// intentional scrolls (e.g. search navigation) would be undone.
785+
if (this.#focusScrollRafId != null) {
786+
win.cancelAnimationFrame(this.#focusScrollRafId);
787+
}
788+
this.#focusScrollRafId = win.requestAnimationFrame(() => {
789+
this.#focusScrollRafId = null;
790+
if (win.scrollX !== beforeX || win.scrollY !== beforeY) {
791+
win.scrollTo(beforeX, beforeY);
792+
}
793+
});
778794
};
779795
}
780796

@@ -2148,6 +2164,14 @@ export class PresentationEditor extends EventEmitter {
21482164
pos: number,
21492165
options: { block?: 'start' | 'center' | 'end' | 'nearest'; behavior?: ScrollBehavior } = {},
21502166
): boolean {
2167+
// Cancel any pending focus-scroll RAF so this intentional scroll is not undone
2168+
// by the wrapHiddenEditorFocus safety net (e.g. search navigation after focus).
2169+
if (this.#focusScrollRafId != null) {
2170+
const win = this.#visibleHost.ownerDocument?.defaultView;
2171+
if (win) win.cancelAnimationFrame(this.#focusScrollRafId);
2172+
this.#focusScrollRafId = null;
2173+
}
2174+
21512175
const activeEditor = this.getActiveEditor();
21522176
const doc = activeEditor?.state?.doc;
21532177
if (!doc) return false;
@@ -2523,6 +2547,15 @@ export class PresentationEditor extends EventEmitter {
25232547
}, 'Layout RAF');
25242548
}
25252549

2550+
// Cancel pending focus-scroll safety net RAF
2551+
if (this.#focusScrollRafId != null) {
2552+
safeCleanup(() => {
2553+
const win = this.#visibleHost?.ownerDocument?.defaultView ?? window;
2554+
win.cancelAnimationFrame(this.#focusScrollRafId!);
2555+
this.#focusScrollRafId = null;
2556+
}, 'Focus scroll RAF');
2557+
}
2558+
25262559
// Cancel pending decoration sync RAF
25272560
if (this.#decorationSyncRafHandle != null) {
25282561
safeCleanup(() => {

packages/super-editor/src/core/presentation-editor/tests/PresentationEditor.focusWrapping.test.ts

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -355,6 +355,52 @@ describe('PresentationEditor - Focus Wrapping (#wrapHiddenEditorFocus)', () => {
355355
editor.editor.view.focus();
356356
}).not.toThrow();
357357
});
358+
359+
it('schedules requestAnimationFrame that restores scroll on async drift', () => {
360+
editor = new PresentationEditor({
361+
element: container,
362+
documentId: 'test-doc',
363+
pageSize: { w: 612, h: 792 },
364+
});
365+
366+
// Capture the RAF callback so we can invoke it manually
367+
let rafCallback: FrameRequestCallback | null = null;
368+
const rafSpy = vi.spyOn(window, 'requestAnimationFrame').mockImplementation((cb) => {
369+
rafCallback = cb;
370+
return 1;
371+
});
372+
const scrollToSpy = vi.spyOn(window, 'scrollTo').mockImplementation(() => {});
373+
374+
// At focus time, scrollX=0 scrollY=0 → captured as beforeX=0 beforeY=0
375+
editor.editor.view.focus();
376+
377+
expect(rafSpy).toHaveBeenCalledTimes(1);
378+
expect(rafCallback).not.toBeNull();
379+
380+
// Simulate the browser async-scrolling to the hidden editor (drift from 0 to 500)
381+
Object.defineProperty(window, 'scrollY', { value: 500, configurable: true });
382+
383+
// Run the RAF callback — it should detect drift and restore to beforeY=0
384+
rafCallback!(0);
385+
expect(scrollToSpy).toHaveBeenCalledWith(0, 0);
386+
});
387+
388+
it('cancels focus-scroll RAF when scrollToPosition is called', () => {
389+
editor = new PresentationEditor({
390+
element: container,
391+
documentId: 'test-doc',
392+
pageSize: { w: 612, h: 792 },
393+
});
394+
395+
const cancelSpy = vi.spyOn(window, 'cancelAnimationFrame');
396+
397+
editor.editor.view.focus();
398+
399+
// scrollToPosition will fail (no layout) but should still cancel the RAF
400+
editor.scrollToPosition(0);
401+
402+
expect(cancelSpy).toHaveBeenCalled();
403+
});
358404
});
359405

360406
describe('mock detection', () => {

0 commit comments

Comments
 (0)