From df261c24650c650e0368666e2c3e42fc702bad67 Mon Sep 17 00:00:00 2001 From: Henry Heino <46334387+personalizedrefrigerator@users.noreply.github.com> Date: Wed, 9 Sep 2026 15:55:14 -0700 Subject: [PATCH] Desktop: Fixes #16454: In-editor Markdown rendering: Fix markup syntax is visible while clicking and dragging the scrollbar (#16461) --- .gitignore | 3 + .ignore.eslint | 3 + .../utils/makeInlineReplaceExtension.test.ts | 34 ++++++++++++ .../utils/makeInlineReplaceExtension.ts | 55 +++++++++++++------ .../CodeMirror/testing/backspaceOnce.ts | 8 +++ .../CodeMirror/testing/pressReleaseKey.ts | 6 +- .../editor/CodeMirror/testing/typeText.ts | 17 +++++- .../utils/clampSelectionToDocument.test.ts | 39 +++++++++++++ .../utils/clampSelectionToDocument.ts | 27 +++++++++ 9 files changed, 168 insertions(+), 24 deletions(-) create mode 100644 packages/editor/CodeMirror/testing/backspaceOnce.ts create mode 100644 packages/editor/CodeMirror/utils/clampSelectionToDocument.test.ts create mode 100644 packages/editor/CodeMirror/utils/clampSelectionToDocument.ts diff --git a/.gitignore b/.gitignore index 5634b3cb5e..bc904fbf70 100644 --- a/.gitignore +++ b/.gitignore @@ -1177,6 +1177,7 @@ packages/editor/CodeMirror/pluginApi/PluginLoader.js packages/editor/CodeMirror/pluginApi/codeMirrorRequire.js packages/editor/CodeMirror/pluginApi/customEditorCompletion.test.js packages/editor/CodeMirror/pluginApi/customEditorCompletion.js +packages/editor/CodeMirror/testing/backspaceOnce.js packages/editor/CodeMirror/testing/createEditorControl.js packages/editor/CodeMirror/testing/createTestEditor.js packages/editor/CodeMirror/testing/findNodesWithName.js @@ -1186,6 +1187,8 @@ packages/editor/CodeMirror/testing/pressReleaseKey.js packages/editor/CodeMirror/testing/typeText.js packages/editor/CodeMirror/testing/visibleEditorText.js packages/editor/CodeMirror/theme.js +packages/editor/CodeMirror/utils/clampSelectionToDocument.test.js +packages/editor/CodeMirror/utils/clampSelectionToDocument.js packages/editor/CodeMirror/utils/findPositionMatchingLink.test.js packages/editor/CodeMirror/utils/findPositionMatchingLink.js packages/editor/CodeMirror/utils/formatting/RegionSpec.js diff --git a/.ignore.eslint b/.ignore.eslint index 157065ef47..bb3fbc7115 100644 --- a/.ignore.eslint +++ b/.ignore.eslint @@ -1203,6 +1203,7 @@ packages/editor/CodeMirror/pluginApi/PluginLoader.js packages/editor/CodeMirror/pluginApi/codeMirrorRequire.js packages/editor/CodeMirror/pluginApi/customEditorCompletion.test.js packages/editor/CodeMirror/pluginApi/customEditorCompletion.js +packages/editor/CodeMirror/testing/backspaceOnce.js packages/editor/CodeMirror/testing/createEditorControl.js packages/editor/CodeMirror/testing/createTestEditor.js packages/editor/CodeMirror/testing/findNodesWithName.js @@ -1212,6 +1213,8 @@ packages/editor/CodeMirror/testing/pressReleaseKey.js packages/editor/CodeMirror/testing/typeText.js packages/editor/CodeMirror/testing/visibleEditorText.js packages/editor/CodeMirror/theme.js +packages/editor/CodeMirror/utils/clampSelectionToDocument.test.js +packages/editor/CodeMirror/utils/clampSelectionToDocument.js packages/editor/CodeMirror/utils/findPositionMatchingLink.test.js packages/editor/CodeMirror/utils/findPositionMatchingLink.js packages/editor/CodeMirror/utils/formatting/RegionSpec.js diff --git a/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.test.ts b/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.test.ts index 4406ac6b8c..4e7c2775b5 100644 --- a/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.test.ts +++ b/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.test.ts @@ -6,6 +6,7 @@ import replaceFormatCharacters from '../replaceFormatCharacters'; import replaceInlineHtml from '../replaceInlineHtml'; import replaceLinks from '../replaceLinks'; import visibleEditorText from '../../../testing/visibleEditorText'; +import backspaceOnce from '../../../testing/backspaceOnce'; jest.retryTimes(2); @@ -176,4 +177,37 @@ describe('makeInlineReplaceExtension', () => { editor.destroy(); } }); + + it('should update markdown decorations when the mouse is down and the document changes', async () => { + const markdown = '**test**...'; + const initialSelection = EditorSelection.cursor(markdown.length); + const editor = await createTestEditor( + '**test**...', + initialSelection, + ['StrongEmphasis'], + [replaceFormatCharacters], + ); + + try { + editor.dom.dispatchEvent(new MouseEvent('mousedown', { button: 0 })); + editor.dispatch({ selection: initialSelection, userEvent: 'select.pointer' }); + jest.runAllTimers(); + + expectTextContentToBe(visibleEditorText(editor), 'test...'); + + backspaceOnce(editor); + expectTextContentToBe(visibleEditorText(editor), 'test..'); + backspaceOnce(editor); + expectTextContentToBe(visibleEditorText(editor), 'test.'); + backspaceOnce(editor); + expectTextContentToBe(visibleEditorText(editor), '**test**'); + + editor.dom.ownerDocument.dispatchEvent(new MouseEvent('mouseup', { button: 0 })); + await jest.runAllTimersAsync(); + + expectTextContentToBe(visibleEditorText(editor), '**test**'); + } finally { + editor.destroy(); + } + }); }); diff --git a/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.ts b/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.ts index a4bbce72ca..ef9d2ea3f6 100644 --- a/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.ts +++ b/packages/editor/CodeMirror/extensions/rendering/utils/makeInlineReplaceExtension.ts @@ -4,21 +4,31 @@ import { EditorView, Decoration, DecorationSet, WidgetType } from '@codemirror/view'; import { ViewPlugin, ViewUpdate } from '@codemirror/view'; import { syntaxTree } from '@codemirror/language'; -import { Range, StateEffect } from '@codemirror/state'; +import { EditorSelection, EditorState, Range, StateEffect } from '@codemirror/state'; import { SyntaxNodeRef } from '@lezer/common'; import { ReplacementExtension } from '../types'; import nodeIntersectsSelection from './nodeIntersectsSelection'; +import clampSelectionToDocument from '../../../utils/clampSelectionToDocument'; const updateInlineDecorationsEffect = StateEffect.define(); +interface MouseSelectionState { + initialSelection: EditorSelection; +} + +interface VisibleRange { + from: number; + to: number; +} + export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) => ViewPlugin.fromClass(class { public decorations: DecorationSet = Decoration.set([]); - private mouseSelectionInProgress = false; + private mouseSelectionBefore_: MouseSelectionState|null = null; public constructor(private view: EditorView) { view.dom.addEventListener('mousedown', this.onMouseDown, true); view.dom.ownerDocument.addEventListener('mouseup', this.onMouseUp); - this.updateDecorations(view); + this.updateDecorations(view.state, view.visibleRanges); } public destroy() { @@ -28,16 +38,16 @@ export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) private onMouseDown = (event: MouseEvent) => { if (event.button === 0) { - this.mouseSelectionInProgress = true; + this.mouseSelectionBefore_ = { initialSelection: this.view.state.selection }; } }; private onMouseUp = () => { - if (this.mouseSelectionInProgress) { + if (this.mouseSelectionBefore_) { // To prevent unnecessary scroll on iOS, decoration changes need to // happen *after* the gesture ends. requestAnimationFrame(() => { - this.mouseSelectionInProgress = false; + this.mouseSelectionBefore_ = null; this.view.dispatch({ effects: updateInlineDecorationsEffect.of(null), }); @@ -45,14 +55,20 @@ export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) } }; - private updateDecorations(view: EditorView) { - const doc = view.state.doc; - const cursorLine = doc.lineAt(view.state.selection.main.anchor); - const selection = view.state.selection; + private updateDecorations(state: EditorState, visibleRanges: readonly VisibleRange[]) { + const doc = state.doc; + let selection = state.selection; + if (this.mouseSelectionBefore_?.initialSelection) { + selection = clampSelectionToDocument(this.mouseSelectionBefore_.initialSelection, doc); + } + if (this.mouseSelectionBefore_) { + state = state.update({ selection }).state; + } + const cursorLine = doc.lineAt(selection.main.anchor); const parentTagCounts = new Map(); const decorateNode = (node: SyntaxNodeRef) => { - const widgetOrDecoration = extensionSpec.createDecoration(node, view.state, parentTagCounts); + const widgetOrDecoration = extensionSpec.createDecoration(node, state, parentTagCounts); let decoration; if (widgetOrDecoration instanceof WidgetType) { decoration = Decoration.replace({ @@ -63,7 +79,7 @@ export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) } if (decoration) { - const range = extensionSpec.getDecorationRange?.(node, view.state, parentTagCounts) ?? [node.from, node.to]; + const range = extensionSpec.getDecorationRange?.(node, state, parentTagCounts) ?? [node.from, node.to]; const rangeLineFrom = doc.lineAt(range[0]); const rangeLineTo = range.length === 2 ? doc.lineAt(range[1]) : rangeLineFrom; @@ -79,14 +95,14 @@ export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) }; let widgets: Range[] = []; - for (const { from, to } of view.visibleRanges) { + for (const { from, to } of visibleRanges) { parentTagCounts.clear(); - syntaxTree(view.state).iterate({ + syntaxTree(state).iterate({ from, to, enter: node => { parentTagCounts.set(node.name, (parentTagCounts.get(node.name) ?? 0) + 1); - const strategy = extensionSpec.getRevealStrategy?.(node, view.state, parentTagCounts) ?? 'line'; + const strategy = extensionSpec.getRevealStrategy?.(node, state, parentTagCounts) ?? 'line'; let isSelected = false; if (typeof strategy === 'boolean') { @@ -151,12 +167,15 @@ export const makeInlineReplaceExtension = (extensionSpec: ReplacementExtension) transaction.effects.some(effect => effect.is(updateInlineDecorationsEffect)) || extensionSpec.shouldFullReRender?.(transaction) )); - if (this.mouseSelectionInProgress && !update.docChanged && !forceUpdate) { - return; + + // Document changes move the selection, so the original selection may no longer + // be valid: + if (update.docChanged) { + this.mouseSelectionBefore_ = null; } if (update.docChanged || update.viewportChanged || update.selectionSet || forceUpdate) { - this.updateDecorations(update.view); + this.updateDecorations(update.state, update.view.visibleRanges); } } }, { diff --git a/packages/editor/CodeMirror/testing/backspaceOnce.ts b/packages/editor/CodeMirror/testing/backspaceOnce.ts new file mode 100644 index 0000000000..bd44e461ba --- /dev/null +++ b/packages/editor/CodeMirror/testing/backspaceOnce.ts @@ -0,0 +1,8 @@ +import pressReleaseKey from './pressReleaseKey'; +import { EditorView } from '@codemirror/view'; + +const backspaceOnce = (view: EditorView) => { + pressReleaseKey(view, { key: 'Backspace', code: 'Backspace', typesText: '' }); +}; + +export default backspaceOnce; diff --git a/packages/editor/CodeMirror/testing/pressReleaseKey.ts b/packages/editor/CodeMirror/testing/pressReleaseKey.ts index 49e6bdddee..f169e84422 100644 --- a/packages/editor/CodeMirror/testing/pressReleaseKey.ts +++ b/packages/editor/CodeMirror/testing/pressReleaseKey.ts @@ -1,7 +1,7 @@ import { EditorView } from '@codemirror/view'; import typeText from './typeText'; -interface KeyInfo { +export interface KeyInfo { key: string; code: string; // Text to type if the event was not processed @@ -21,8 +21,8 @@ const pressReleaseKey = (editor: EditorView, key: KeyInfo) => { editor.contentDOM.dispatchEvent(keyDownEvent); - if (key.typesText && !keyDownPrevented) { - typeText(editor, key.typesText); + if (key.typesText !== undefined && !keyDownPrevented) { + typeText(editor, key.typesText, key); } editor.contentDOM.dispatchEvent(new KeyboardEvent('keyup', key)); diff --git a/packages/editor/CodeMirror/testing/typeText.ts b/packages/editor/CodeMirror/testing/typeText.ts index 4e9f666e79..79b27175a4 100644 --- a/packages/editor/CodeMirror/testing/typeText.ts +++ b/packages/editor/CodeMirror/testing/typeText.ts @@ -1,15 +1,26 @@ import { EditorView } from '@codemirror/view'; +import { KeyInfo } from './pressReleaseKey'; -const typeText = (editor: EditorView, text: string) => { +const typeText = (editor: EditorView, text: string, key?: KeyInfo) => { const selection = editor.state.selection; const inputHandlers = editor.state.facet(EditorView.inputHandler); + const isBackspace = !!key && key.key === 'Backspace' && text === ''; + + let backspaceOffset = 0; + if (isBackspace && selection.main.empty && selection.main.from > 0) { + backspaceOffset = -1; + } // See how the upstream CodeMirror tests simulate user input: // https://github.com/codemirror/autocomplete/blob/fb1c899464df4d36528331412cdd316548134cb2/test/webtest-autocomplete.ts#L116 // The important part is the userEvent: input.type. const defaultTransaction = editor.state.update({ - changes: [{ from: selection.main.head, insert: text }], - selection: { anchor: selection.main.head + text.length }, + changes: [{ + from: selection.main.from + backspaceOffset, + to: selection.main.to, + insert: text, + }], + selection: { anchor: selection.main.head + text.length + backspaceOffset }, userEvent: 'input.type', }); diff --git a/packages/editor/CodeMirror/utils/clampSelectionToDocument.test.ts b/packages/editor/CodeMirror/utils/clampSelectionToDocument.test.ts new file mode 100644 index 0000000000..01fe7be29e --- /dev/null +++ b/packages/editor/CodeMirror/utils/clampSelectionToDocument.test.ts @@ -0,0 +1,39 @@ +import { EditorSelection, Text } from '@codemirror/state'; +import clampSelectionToDocument from './clampSelectionToDocument'; + +describe('clampSelectionToDocument', () => { + it('should clamp a selection to the document', () => { + const doc = Text.of(['Testing...']); + const end = 'Testing...'.length; + + expect(clampSelectionToDocument( + EditorSelection.single(end + 1), doc, + )).toMatchObject({ + main: { from: end, to: end }, + }); + + // Should map multiple at once + expect(clampSelectionToDocument( + EditorSelection.create([ + EditorSelection.range(-1, 0), + EditorSelection.cursor(1), + EditorSelection.cursor(100), + ], 2), doc, + )).toMatchObject({ + ranges: [ + { from: 0, to: 0 }, + { from: 1, to: 1 }, + { from: end, to: end }, + ], + mainIndex: 2, + }); + }); + + it('should not change a selection within the document', () => { + const doc = Text.of(['Test']); + + // Should return the exact same selection + const selection = EditorSelection.single('Te'.length); + expect(clampSelectionToDocument(selection, doc)).toBe(selection); + }); +}); diff --git a/packages/editor/CodeMirror/utils/clampSelectionToDocument.ts b/packages/editor/CodeMirror/utils/clampSelectionToDocument.ts new file mode 100644 index 0000000000..7debac0db7 --- /dev/null +++ b/packages/editor/CodeMirror/utils/clampSelectionToDocument.ts @@ -0,0 +1,27 @@ +import { EditorSelection, Text } from '@codemirror/state'; + +const clampSelectionToDocument = (selection: EditorSelection, doc: Text) => { + const newRanges = []; + + const clampDocumentPosition = (pos: number) => Math.max(0, Math.min(pos, doc.length)); + + let changed = false; + for (const range of selection.ranges) { + const newAnchor = clampDocumentPosition(range.anchor); + const newHead = clampDocumentPosition(range.head); + if (newAnchor !== range.anchor || newHead !== range.head) { + newRanges.push(EditorSelection.range(newAnchor, newHead)); + changed = true; + } else { + newRanges.push(range); + } + } + + if (changed) { + return EditorSelection.create(newRanges, selection.mainIndex); + } else { + return selection; + } +}; + +export default clampSelectionToDocument;