forwardUpdate implementation in CodeMirror NodeView example code

1

Summary

  • I used the CodeMirror NodeView provided by the Embedded Code Editor example as the basis for my own CodeMirror-based NodeView.
  • While using the NodeView, I experienced some strange issues where text pasted from the clipboard would be pasted twice into CodeMirror, and cause many mysterious “position not defined” errors in the console.
  • Through debugging, I tracked down the problem to the forwardUpdate method of the NodeView.
  • Carefully reading the code, the forwardUpdate implementation seems obviously incorrect to me. I changed the implementation to match my mental model of how ProseMirror works.
  • However, I’m unable to reproduce the paste bug in the Embedded Code Editor demo. To me, it seems like the demo should have the same issue. This has me questioning whether my mental model of ProseMirror is correct.

Details

I was originally using this implementation for forwardUpdate. It is identical to the example from the docs, except for formatting and naming (and management of a preview pane).

// BEFORE -- causes duplication & errors
forwardUpdate(update: CV.ViewUpdate): void {
	// ignore updates whenever codemirror editor is out of focus
	if (this._updating || !this._codeMirror.hasFocus) return;

	let offset = this._getPos() + 1, {main} = update.state.selection;
	let selection =
		TextSelection.create(
			this._proseView.state.doc,
			offset + main.from,
			offset + main.to
		);

	if (update.docChanged || !this._proseView.state.selection.eq(selection)) {
		let tr = this._proseView.state.tr.setSelection(selection);
		update.changes.iterChanges((fromA, toA, fromB, toB, text) => {
			if (text.length) {
				tr.replaceWith(
					offset + fromA,
					offset + toA,
					this._proseView.state.schema.text(text.toString())
				);
			} else {
				tr.delete(offset + fromA, offset + toA)
			}
			offset += (toB - fromB) - (toA - fromA)
		})
		this._proseView.dispatch(tr)
	}
}

While debugging, I noticed:

  • Each paste event would cause two errors like Uncaught RangeError: Position X out of range
  • If I increased the total document length to be longer than X, the error would no longer occur
  • The error itself originates from TextSelection.create – the given positions are larger than the total length of the current document.

Looking more closely at the forwardUpdate implementation, it seems obviously incorrect to me:

  • forwardUpdate is called by CodeMirror immediately after a ViewUpdate, and before the change is propagated to the ProseMirror document (in fact, forwardUpdate is the function responsible for propagating those changes)
  • when forwardUpdate is called the CodeMirror selection will be at the end of the pasted text
  • the purpose of the TextSelection.create is to convert the current CodeMirror selection into a ProseMirror selection
  • however, the indicies passed to TextSelection.create here will always be invalid, because the ProseMirror document has not yet been updated with the pasted text. Before changing the selection, we should add the pasted text to the ProseMirror node, and map all the selection positions through the transaction to find their new locations.

In fact, the forwardUpdate implementation looks so incorrect that I’m surprised it works at all in the demo. It has me questioning whether I actually understand ProseMirror!

Based on this reasoning, I changed the forwardUpdate implementation to:

  • first, apply all the CodeMirror changes to the ProseMirror document
  • then, compute a new ProseMirror selection by mapping the positions through the document change transaction

The new implementation can be found in this diff. Reproduced below:

// AFTER
forwardUpdate(update: CV.ViewUpdate): void {
	// ignore updates whenever codemirror editor is out of focus
	if (this._updating || !this._codeMirror.hasFocus) return;

	let codePos = this._getPos();
	let tr = this._proseView.state.tr;

	if (update.docChanged) {
		// the +1 skips the code block opening token
		let offset = codePos + 1;
		
		update.changes.iterChanges((fromA, toA, fromB, toB, text) => {
			if (text.length) {
				tr.replaceWith(
					offset + fromA,
					offset + toA,
					this._proseView.state.schema.text(text.toString())
				);
			} else {
				tr.delete(offset + fromA, offset + toA)
			}
			offset += (toB - fromB) - (toA - fromA)
		})
	}

	// update selection
	let { main: codeMirrorSelection} = update.state.selection; 
	let codePosAfterTr = tr.mapping.map(codePos);

	let mappedProseSelection = this._proseView.state.selection.map(tr.doc, tr.mapping);

	let desiredProseSelection =
		TextSelection.create(
			tr.doc,
			codePosAfterTr + 1 + codeMirrorSelection.from, // +1 skips code_block start token
			codePosAfterTr + 1 + codeMirrorSelection.to    // +1 skips code_block start token
		);
	if(!mappedProseSelection.eq(desiredProseSelection)) {
		tr = tr.setSelection(desiredProseSelection);
	}
	
	this._proseView.dispatch(tr)
}

After this change, pasting works as expected.

Questions

So, I’m not really looking for specific debugging help, but rather answers to these questions:

  • is my description of the mistake in the original forwardUpdate implementation accurate? Or is the original implementation correct, and I am misunderstanding something?
  • does my approach to fixing forwardUpdate seem generally correct?
  • if the original implementation is incorrect for the reason I say, can you think of any reason why the error is not reproducible in the demo?
  • If my reasoning is correct, would you accept a PR to the Embedded Code Editor page with the fix?

Possibly Related Issues

Version Info

Note that I am using slightly outdated versions of ProseMirror. I was relying on the now-removed Schema type parameter, and can’t upgrade until I find a workaround.

(was any behavior changed recently that might have caused the issue I’m experiencing?)

$ npm outdated
Package                           Current    Wanted    Latest
prosemirror-commands                1.1.9     1.5.1     1.5.1  
prosemirror-gapcursor               1.1.5     1.3.1     1.3.1  
prosemirror-history                 1.1.3     1.3.0     1.3.0  
prosemirror-inputrules              1.1.3     1.2.0     1.2.0  
prosemirror-keymap                  1.1.4     1.2.1     1.2.1  
prosemirror-model                  1.14.2    1.19.0    1.19.0  
prosemirror-schema-basic            1.1.2     1.2.1     1.2.1 
prosemirror-schema-list             1.1.4     1.2.2     1.2.2
prosemirror-state                   1.3.4     1.4.2     1.4.2  
prosemirror-suggest                 0.7.6     0.7.6     2.0.3  
prosemirror-tables                  1.1.1     1.3.2     1.3.2  
prosemirror-transform               1.3.2     1.7.1     1.7.1  
prosemirror-view                   1.18.7    1.30.2    1.30.2

Thank You!

2

Did you see issue #1319, which looks like it fixed this half a year ago in the website sources?

1 Like
3

Ah, thanks, I didn’t see that. So basically, the demo is running the fixed version, but the code shown on the page hasn’t yet been updated with the fix? That would explain everything.

4

Oh, you are right, seems there was an issue in the way the website was rebuilt and that HTML file was stale. Should be better now.