Skip to content

Refactor composition handling to support prosemirror-view behavior#219

Draft
smoores-dev wants to merge 7 commits into
mainfrom
more-composition
Draft

Refactor composition handling to support prosemirror-view behavior#219
smoores-dev wants to merge 7 commits into
mainfrom
more-composition

Conversation

@smoores-dev
Copy link
Copy Markdown
Member

ProseMirror View's composition handling does not match my initial read of the code from years ago. Here's the actual behavior:

  • When a composition is updated, the document is updated correspondingly. This was very surprising to me and does not line up with React ProseMirror right now — we don't update the document until the composition is finished
  • When a change that originates from somewhere other than the composition overlaps with the composition's text node, the composition is cancelled

This doesn't yet handle the second case, but just handling the first case was very challenging with our setup.

Just as a refresher, here's our flow for a normal input:

  1. beforeinput event is fired
  2. We inspect the range and event.data to determine what text to insert
  3. We insert that text into the document
  4. We use flushSync, so we synchronously re-render the editor view tree, updating the vdom with the new text
  5. React updates the DOM with the new text
  6. We run view.commitPendingEffects in a layout effect, to sync the selection and run any view plugins that need to run before the browser paints

I have updated compositions to follow a similar-ish flow, but it is more complicated:

  1. beforeinput event is fired
  2. We inspect the range and event.data to determine what text to insert
  3. We insert that text into the document
  4. We use flushSync, so we synchronously re-render the editor view tree. This does not update the vdom with the new text, because TextNodeView detects that it's in a composition and freezes its rendered content via this.renderRef. This is strictly necessary — updating the rendered content will cause React to overwrite the composition node, and the browser will cancel the composition.
  5. React updates the DOM, but nothing has changed
  6. We run view.commitPendingEffects in a setTimeout. This is because the DOM, at this moment, has not been updated yet, because our TextNodeView froze its content.
  7. The browser updates the composition node with the new text
  8. Then our pending effects run

This means that we now have a path that runs view.commitPendingEffects in a setTimeout, which I have not convinced myself is safe (but is, I think, no less safe than pm-view's approach?). In addition, for the same reason, the code in useNodeViewDescription that finds the newly-created composition node and assigns it to the CompositionNodeView has to run in a setTimeout (it will still run before the commitPendingEffects call, which is what matters).

I still have to update this to fix the second behavior from above, and I'll probably have to update the composition tests as well.

Comment thread src/plugins/beforeInputPlugin.ts Outdated
Comment on lines +219 to +227
// When we insert the text that corresponds to an ongoing composition,
// the relevant TextNodeView will pause re-rendering so that React doesn't
// clobber the composition in the DOM. This means that we have to wait for
// the browser to update the DOM itself before attempting to reconcile
// the selection, so we specifically defer pending effects to the next
// macro task
if (view instanceof ReactEditorView) {
view.deferPendingEffects = true;
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Another option I'm considering, but haven't tested yet, is doing like view.dom.once('input', () => {...}) in the beforeinput handler, and running the view.dispatch from there. That feels like it should allow us to drop the setTimeout in commitPendingEffects. I tried using an input handler yesterday and there was some other timing issue that I now can't remember, so it's possible that won't be an option.

Copy link
Copy Markdown
Member Author

@smoores-dev smoores-dev May 4, 2026

Choose a reason for hiding this comment

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

I was unable to get this working :/ but I do think that the current setTimeout implementation is working quite well. It does seem like maybe the selection management isn't quite right yet (only an issue during the composition, before and after the selection is set correctly and works as expected).

@smoores-dev
Copy link
Copy Markdown
Member Author

Ok, fixed up a few more things. Two known limitations/issues:

  1. I can't get the selection to be correct during a composition involved a cursorWrapper. This isn't outrageously common (it only happens if you have a stored mark when you start a composition), but it's not exactly an edge case. The effect is that the DOM cursor appears to be sitting at the beginning of the composition while you're composing, and then pops to the correct position when you commit. I'm honestly not user that it's possible to fix this with the level of timing access we currently have — all of the timing of this stuff is extremely complex. I think to do this right, we might need to manually insert the cursor wrapper, rather than doing it via a React widget, because we need to insert it, wait for the initial composition mutation to occur in the DOM, and then remove it, all before we attempt to update the selection. I think. Also this isn't a regression, right now this behavior happens in Chrome on all compositions, not just cursorWrapper ones.
  2. The editor crashes if you cancel a cursorWrapper composition. This one feels blocking, but I'm not sure what to do about it. It seems like if you cancel a composition, and that results in an empty inline element (e.g. a strong), Firefox will also remove the inline element, and then React crashes because it thinks it was supposed to remove it. I guess maybe we could specifically try to find it after adding the cursorWrapper, stash it, and then re-add it in compositionend if it's been removed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant