Skip to content

[Editor] Restore cursor positioning after paste/widget-insert#3722

Open
jeremywiebe wants to merge 8 commits into
mainfrom
jer/fix-editor-paste-cursor-position
Open

[Editor] Restore cursor positioning after paste/widget-insert#3722
jeremywiebe wants to merge 8 commits into
mainfrom
jer/fix-editor-paste-cursor-position

Conversation

@jeremywiebe
Copy link
Copy Markdown
Collaborator

@jeremywiebe jeremywiebe commented Jun 4, 2026

Summary:

While working on the Preview NG work, I was going to clean up the two extra parameters in the onChange callback prop (silent and cb (aka "callback")). While cleaning up the code I saw that there was some code in the paste path that actually provides a function for the cb.

This led me to discover that we'd broken cursor handling when we handle paste in the editor. A while back, we did some cleanup that broke this cursor handling when we stopped passing silent and cb parameters in higher-level editor components, but Editor was still sending them - effectively they were dropped on the floor. This results in the cursor always jumping to the end of the content when you paste anywhere.

Thsi PR restores this behaviour, but does so entirely within the Editor component instead of depending on the deprecated cb onChange parameter. This allows us to still fully remove the silent and cb parameters, but restores cursor handling (all within the Editor).

paste-cursor-demo

Issue: LEMS-4241

Test plan:

Run storybook
Navigate to the Editor Demo page
Add a widget and configure it
Select the widget in the Markdown editor, copy it
Move the cursor to the beginning of the field and type STARTEND
Place the cursor between START and END (eg. START|END)
Paste
The copied widget should be pasted between START and END and the cursor should be sitting at the end fo the pasted widget (just before END).

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

Size Change: -20 B (0%)

Total Size: 508 kB

📦 View Changed
Filename Size Change
packages/perseus-editor/dist/es/index.js 105 kB -20 B (-0.02%)
ℹ️ View Unchanged
Filename Size
packages/kas/dist/es/index.js 20.6 kB
packages/keypad-context/dist/es/index.js 1 kB
packages/kmath/dist/es/index.js 6.32 kB
packages/math-input/dist/es/index.js 98.5 kB
packages/math-input/dist/es/strings.js 1.61 kB
packages/perseus-core/dist/es/index.item-splitting.js 12.1 kB
packages/perseus-core/dist/es/index.js 26.3 kB
packages/perseus-linter/dist/es/index.js 9.65 kB
packages/perseus-score/dist/es/index.js 10.2 kB
packages/perseus-utils/dist/es/index.js 403 B
packages/perseus/dist/es/index.js 200 kB
packages/perseus/dist/es/strings.js 8.6 kB
packages/pure-markdown/dist/es/index.js 1.39 kB
packages/simple-markdown/dist/es/index.js 6.71 kB

compressed-size-action

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Jun 4, 2026

npm Snapshot: Published

Good news!! We've packaged up the latest commit from this PR (8d6a07e) and published it to npm. You
can install it using the tag PR3722.

Example:

pnpm add @khanacademy/perseus@PR3722

If you are working in Khan Academy's frontend, you can run the below command.

./dev/tools/bump_perseus_version.ts -t PR3722

If you are working in Khan Academy's webapp, you can run the below command.

./dev/tools/bump_perseus_version.js -t PR3722

Fixes the cursor positioning issue in the markdown editor after pasting or inserting widgets. The cursor now correctly lands after the content instead of jumping to the end.
Comment on lines -255 to -257
// This command is not implemented. Fall back to setting `value`
// directly.
textarea.value = this.props.content;
Copy link
Copy Markdown
Collaborator Author

@jeremywiebe jeremywiebe Jun 4, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of our supported browsers support execCommand now, so there's no point in keeping this extra fallback code!

Comment on lines -567 to -580
this.props.onChange(
{
content: newContent,
widgets: {
...safeWidgetData,
...this.getWidgetsReferencedIn(newContent),
},
},
() => {
const expectedCursorPosition =
selectionStart + safeText.length;
Util.textarea.moveCursor(textarea, expectedCursorPosition);
// After React commits the new content, place the cursor at the end
// of what we just pasted in.
this._pendingCursorPos = selectionStart + safeText.length;
this.props.onChange({
content: newContent,
widgets: {
...safeWidgetData,
...this.getWidgetsReferencedIn(newContent),
},
);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are moving to remove the callback prop from onChange. This is currently the only known usage, and we can easily manage it without this legacy prop - plus it doesn't require plumbing the callback value up and down the editor component tree (which we did... and which was broken for a while now!)

@jeremywiebe jeremywiebe marked this pull request as ready for review June 4, 2026 16:39
@jeremywiebe jeremywiebe requested review from a team and benchristel June 4, 2026 16:39
Comment thread packages/perseus-editor/src/__tests__/editor.test.tsx Outdated
Comment thread packages/perseus-editor/src/__tests__/editor.test.tsx Outdated
Comment thread packages/perseus-editor/src/__tests__/editor.test.tsx Outdated
Comment thread packages/perseus-editor/src/editor.tsx Outdated
this.props.onChange({
content: newContent,
widgets: newWidgets,
});
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we called onChange, the next render should have the same value for the textarea that we just set. I think we could set the cursor position here, and _pendingCursorPos could go away.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am pretty sure we have to wait for the next render so that our cursor position refers to the new content string (also, in my testing, even if I set the cursor position no the textarea ref after calling this.props.onChange it jumps to the end of the string after rendering.

Perhaps I'm misunderstanding you.

jeremywiebe and others added 2 commits June 4, 2026 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants