Skip to content

AceNodeView: programmatic setSelection() marks the cursor dirty, scrolling the document to the front matter #1099

Description

@kevinushey

Bug description

AceNodeView treats a programmatic selection restore as a user cursor movement, which makes the visual editor scroll to whichever embedded Ace editor the ProseMirror selection happens to live in.

The changeCursor handler sets cursorDirty without consulting this.updating:

// If the cursor moves and we're in focus, ensure that the cursor is
// visible. Ace's own cursor visiblity mechanisms don't work in embedded
// editors.
this.aceEditor.getSelection().on('changeCursor', () => {
if (this.dom.contains(document.activeElement) && !this.mouseDown) {
this.cursorDirty = true;
}
});
this.aceEditor.renderer.on('afterRender', () => {
// If the cursor position is dirty and the mouse is not down, scroll the
// cursor into view. Don't scroll while the mouse is down, as it will be
// treated as a click-and-drag by Ace.
if (this.cursorDirty && !this.mouseDown) {
this.scrollCursorIntoView();
this.cursorDirty = false;
}
});

this.aceEditor.getSelection().on('changeCursor', () => {
  if (this.dom.contains(document.activeElement) && !this.mouseDown) {
    this.cursorDirty = true;
  }
});

this.aceEditor.renderer.on('afterRender', () => {
  if (this.cursorDirty && !this.mouseDown) {
    this.scrollCursorIntoView();
    this.cursorDirty = false;
  }
});

But setSelection() — which ProseMirror calls from selectionToDOM() whenever EditorView.focus() runs — focuses the Ace editor and applies a selection range, and that fires changeCursor:

public setSelection(anchor: number, head: number) {
// We haven't drawn the editor yet, so queue the selection until we can
// apply it.
if (!this.aceEditor || !this.editSession) {
this.queuedSelection = new QueuedSelection(anchor, head);
return;
}
if (!this.escaping && !this.gapCursorPending) {
this.aceEditor.focus();
}
this.updating = true;
const doc = this.editSession.getDocument();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const AceRange = (window as any).require('ace/range').Range;
const range = AceRange.fromPoints(doc.indexToPosition(anchor, 0), doc.indexToPosition(head, 0));
this.editSession.getSelection().setSelectionRange(range);
this.updating = false;
}

public setSelection(anchor: number, head: number) {
  ...
  if (!this.escaping && !this.gapCursorPending) {
    this.aceEditor.focus();
  }
  this.updating = true;
  ...
  this.editSession.getSelection().setSelectionRange(range);
  this.updating = false;
}

this.updating is set here for exactly this reason, and the value-change handlers a few lines above do check it (L508, L513) — the changeCursor handler is the one that doesn't. So the next Ace render calls scrollCursorIntoView(), which does container.scrollTop -= up on the editing root and yanks the whole document to that node view, even though nothing moved the cursor.

The visible damage depends on where the selection sits. It is worst for a freshly opened document: with no saved editing location the selection is at position 0, which is the YAML front matter block — itself an AceNodeView — so any EditorView.focus() scrolls the document to the very top.

We hit this in RStudio as rstudio/rstudio#18490: running a code chunk in a just-opened .qmd snaps the view to the top of the document and then scrolls back. RStudio calls EditorView.focus() as part of chunk execution (to make the editor the active view), which is enough to trigger it.

Steps to reproduce

In RStudio (any recent build, including current main):

  1. Create a .qmd with YAML front matter, a chunk, enough prose to make the document scroll, and a second chunk near the bottom:
---
title: "Untitled"
format: html
editor: visual
---

```{r setup, include=FALSE}
library(stats)
```

## Section one

<a dozen paragraphs of prose>

```{r}
x <- seq(-4, 4, length.out = 100)
head(x)
```
  1. Open it in Visual mode and do not click anywhere in the document.
  2. Scroll down and run the last chunk (toolbar button or Ctrl+Shift+Enter).
  3. Now click once anywhere in the prose and run the chunk again.

Generic form, independent of RStudio: put the ProseMirror selection inside an embedded code editor, scroll that editor out of view, and call EditorView.focus().

Actual behavior

Step 3 snaps the document to the top (to the front matter block's cursor) in a single frame, then smooth-scrolls back to the chunk. Measuring .pm-scroll-container scrollTop across a run:

run 1   before=413   min=74   -> back to 294
run 2   before=294   min=6    -> back to 294

Step 4 stops it: the selection is now an ordinary ProseMirror text selection rather than one inside an AceNodeView, so setSelection() is no longer routed to an Ace editor. It stays fixed until the document is closed and reopened (which discards the saved editing location and puts the selection back at position 0).

Captured stack at the scroll:

AceNodeView.scrollCursorIntoView
  <- renderer 'afterRender'  (cursorDirty)
  <- Ace editor.focus() / setSelectionRange -> 'changeCursor'
  <- AceNodeView.setSelection
  <- NodeViewDesc.setSelection <- selectionToDOM <- EditorView.focus

Expected behavior

Restoring focus should not move the document. scrollCursorIntoView() should only run for cursor movement the user actually caused.

Suggested fix

Adding the same !this.updating guard the neighbouring handlers use is enough:

 this.aceEditor.getSelection().on('changeCursor', () => {
-  if (this.dom.contains(document.activeElement) && !this.mouseDown) {
+  if (!this.updating && this.dom.contains(document.activeElement) && !this.mouseDown) {
     this.cursorDirty = true;
   }
 });

I A/B tested this by serving a patched panmirror.js to the browser while leaving everything else identical:

unpatched   run 1 jump=215px   run 2 jump=163px
patched     run 1 no jump      run 2 no jump

Legitimate scrollback is unaffected — scrolling a focused chunk out of view and pressing an arrow key still brings the cursor back into view in both builds.

Happy to send this as a PR if you'd like.

Your environment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions