Reimplementing prosemirror-view's DOM management with React

[smoores-dev]

In #47, we're proposing a novel approach to integrating React and ProseMirror. Instead of using Portals to inject React components into ProseMirror-managed node views, we attempt to manage the contenteditable DOM tree entirely with React. This requires disabling the EditorView's own DOM management system, so, as a stopgap to allow experimentation, we've forked prosemirror-view in this PR. However, we'd like to eventually publish this approach without a prosemirror-view fork! So let's discuss how we can minimize the scope of changes that would need to be made to prosemirror-view in order to support this approach (as well as any other concerns or ideas about the approach!)

Tagging @eelzon, @tilgovi, @saranrapjs, and @marijnh to get this conversation started!

[smoores-dev]

Previous discussion on this approach can be found on the PR (#47) and the ProseMirror discussion forum: https://discuss.prosemirror.net/t/the-future-of-nytimes-react-prosemirror/5855

[marijnh]

Some more questions:

• Is having the outer editor node (EditorView.dom) outside React unproblematic for you?
• You seem to have gotten rid of the cursor wrapper functionality. This looks like it'll break stored marks when composing. Is that just something you're okay with, or do you have another solution for this?
• The EditorView constructor doesn't initialize a doc view at all with these modifications. Is that intentional? Seems like you won't have a document in the DOM until you update the view state this way.
• Do I understand correctly that prosemirror-view-style node views don't work with this, and node views have to be React components, passed in through your custom component props, here? That'd be a change that breaks a lot of plugins.
• It looks like you drop support for prosemirror-view style custom node views, and replace them with your own React-component node views. This makes sense, but it seems like it means this will never be compatible with existing plugins that use node views.

Starting from the first update, it looks like you leave the old view desc tree around in the time between the editor update and the React update. The code paths where an out of date view desc tree is going to cause issues are, roughly:

• Queries for dom positions or coordinates made synchronously after an editor state update. This doesn't seem like an unlikely scenario.
• DOM mutations or selection changes delivered right after an update. This is possible, if you mess with the DOM and then synchronously update the editor, but maybe not a reasonable use case.
• Mouse or key events, which I suppose really cannot happen during invalid-view-tree timespans.

These are the internal bindings that are being imported: ViewDesc CompositionViewDesc, MarkViewDesc, WidgetViewDesc, TextViewDesc, NodeViewDesc, TrailingHackViewDesc, viewDecorations, computeDocDeco, resetScrollPos, storeScrollPos, selectionToDOM. So the mere two $$fork markers do skip over a bunch of additional reaching-into-internals that's happening.

[smoores-dev]

Is having the outer editor node (EditorView.dom) outside React unproblematic for you?

EditorView.dom is currently managed by React! It defaults to a div and can be controlled with the as prop on the ProseMirror component.

You seem to have gotten rid of the cursor wrapper functionality. This looks like it'll break stored marks when composing. Is that just something you're okay with, or do you have another solution for this?

Ohh... gotcha. I would frame this more as "I have not yet reimplemented cursor wrapper functionality", mostly because I didn't actually know what it was for, so I wasn't sure how to make sure I'd done it right! I can add this to my list of things to work on. Thank you for calling it out!

The EditorView constructor doesn't initialize a doc view at all with these modifications. Is that intentional? Seems like you won't have a document in the DOM until you update the view state this way.

This is intentional! The ViewDescs have to be produced by the React components. But a change from undefined to not undefined is enough to trigger a rerender of the React tree, so just creating the view and initializing the EditorState is enough to trigger the first render and create the doc view; you don't have to wait for an update to the state. So this has the same considerations as any later update; there will be a time between when the render starts and when the DOM is committed where the DOM and ViewDesc tree don't match the document, and then they will be synced up by the time the render is committed to the DOM.

Do I understand correctly that prosemirror-view-style node views don't work with this, and node views have to be React components, passed in through your custom component props, here? That'd be a change that breaks a lot of plugins.

Oof, I will admit to having forgotten that plugins can provide node views. I don't know of any way around this, and it's not the only limitation of this flavor. Here are all of the standard prosemirror-view-style things that don't currently work with this implementation (there might be solutions for these that I just haven't discovered yet!):

1. Standard node view constructors, as you pointed out
2. toDOM methods that produce actual DOM elements, rather than the array syntax
3. Standard widget decorations (I've replaced these with React components)

I would have to try it, but it's possible that there's a way to do this that involves wrapping these in divs, which of course might introduce the same issues we're facing with the current implementation (especially with toDOM methods that return DOM elements). Worth a shot, though, because it would be great not to have these limitations.

I am somewhat curious to hear what others think about these limitations; I think it's not impossible that the potential users of this library would be willing to accept the tradeoffs. The biggest downside (which is not trivial by any stretch) I see is that there isn't a good way to know whether third party libraries actually use these features or not, which would make it pretty painful to try using now third party prosemirror libraries that weren't specifically built for React ProseMirror.

Starting from the first update, it looks like you leave the old view desc tree around in the time between the editor update and the React update. The code paths where an out of date view desc tree is going to cause issues are, roughly:

• Queries for dom positions or coordinates made synchronously after an editor state update. This doesn't seem like an unlikely scenario.
• DOM mutations or selection changes delivered right after an update. This is possible, if you mess with the DOM and then synchronously update the editor, but maybe not a reasonable use case.
• Mouse or key events, which I suppose really cannot happen during invalid-view-tree timespans.

I think that this is a correct assessment, and I agree that the first of these is the most likely to actually occur in the real world. @tilgovi had some thoughts earlier about selectively using React's flushSync feature to resolve issues with the first scenario in many cases. Specifically, I think we could do something like:

• In an event handler (whether registered with the EditorView or using useEditorEventCallback in a React component), we can wrap calls to dispatchTransaction with flushSync. This will force React to synchronously flush commits to the DOM after these calls, meaning that by the time view.dispatch returns, the DOM will be guaranteed to be updated. My suspicion is that almost all third-party code that dispatches transactions does so via event handlers, so this could resolve a lot of concerns!
• In an effect using useEditorEffect in a React component, we cannot use flushSync (you can't use flushSync in a React effect, because the effect itself is kicked off during another render cycle), but we might be able to raise an error in development if someone queries for position or coords between when view.dispatch is called and when the DOM is committed.

These are the internal bindings that are being imported...

Oh wow, thank you for going through and documenting those! Some of these feel less necessary, strictly speaking, to export than others. computeDocDeco and viewDecorations, for example, need to be used, but are small and straightforward and I wouldn't be opposed to reproducing them in this library.

The ViewDescs are sort of imported from prosemirror-view but they're also modified a bit and I want to see whether it's necessary to actually use instances of these from prosemirror-view, or whether we can get away with just matching the API (though, as you pointed out the other day, matching the API might be a little tricky since technically the API for these is internal and can change at any time).

resetScrollPos and storeScrollPos shouldn't need to be exported when we're done, if we continue along the train of thought that we want to add a prop to EditorView, rather than inheriting from it as we're currently doing. They're only used in the modified updateStateInner implementation, which is where they're used in the original EditorView.updateStateInner.

selectionToDOM we definitely need, though, or at least some way to trigger it. Which, if we can get it down to only this function, might be alright?

[smoores-dev]

I'm going to start a new thread for each of the high level topics of conversation here, if that's alright @marijnh!

[smoores-dev]

🧵 for cursor wrapper functionality

This looks like it'll break stored marks when composing. Is that just something you're okay with, or do you have another solution for this?

I haven't really taken a crack at cursor wrapper functionality, mostly because I didn't understand what it was doing!

[marijnh]

Since compositions cannot be interfered with as they are in progress, cursor wrappers are a kludge that wrap the cursor in the appropriate mark nodes when the composition starts (for example when there are stored marks), so that the new text has the marks (both visually to the user while the composition is in progress, and for the DOM parser).

[smoores-dev]

Iiii see. Ok, cool. I will look into this; I think it may be as simple as re-enabling the default implementation, now that we're properly freezing the DOM during compositions.

[smoores-dev]

🧵

Here are all of the standard prosemirror-view-style things that don't currently work with this implementation (there might be solutions for these that I just haven't discovered yet!):

1. Standard node view constructors
2. toDOM methods that produce actual DOM elements, rather than the array syntax
3. Standard widget decorations (I've replaced these with React components)

[smoores-dev]

I would have to try it, but it's possible that there's a way to do this that involves wrapping these in divs, which of course might introduce the same issues we're facing with the current implementation (especially with toDOM methods that return DOM elements). Worth a shot, though, because it would be great not to have these limitations.

I am somewhat curious to hear what others think about these limitations; I think it's not impossible that the potential users of this library would be willing to accept the tradeoffs. The biggest downside (which is not trivial by any stretch) I see is that there isn't a good way to know whether third party libraries actually use these features or not, which would make it pretty painful to try using now third party prosemirror libraries that weren't specifically built for React ProseMirror.

[saranrapjs]

1. Standard node view constructors

I like the idea of Implementing support for this, with the existing/known caveats we have with wrapped div's. I can imagine someone wanting to use "not React" in some instances, and if it's not too annoying to add support for this...why not.

2. toDOM methods that produce actual DOM elements, rather than the array syntax

For toDOM, I like how it's currently implemented — where the OutputSpec component will throw if an unsupported flavor of toDOM output is used.

3. Standard widget decorations (I've replaced these with React components)

This also seems like a reasonable constraint, though I think I'm not seeing something similar to what's in toDOM where we throw if you try to use them?

[smoores-dev]

I like the idea of Implementing support for this, with the existing/known caveats we have with wrapped div's. I can imagine someone wanting to use "not React" in some instances, and if it's not too annoying to add support for this...why not.

Agreed. I spent today taking a stab at this, and I think the path that I'm currently on will work, though there's currently a selection bug that I need to work out/am hoping won't be a show stopper.

For toDOM, I like how it's currently implemented — where the OutputSpec component will throw if an unsupported flavor of toDOM output is used.

Ok! That's reassuring. For anything more complex, I would expect it to be easier to author a react component than to handcraft DOM in toDOM anyway.

This also seems like a reasonable constraint, though I think I'm not seeing something similar to what's in toDOM where we throw if you try to use them?

Oh that's a good point, that would be easy enough to add!

[smoores-dev]

🧵 for minimizing risk of users "seeing" stale DOM during a state batch

Starting from the first update, it looks like you leave the old view desc tree around in the time between the editor update and the React update. The code paths where an out of date view desc tree is going to cause issues are, roughly:

• Queries for dom positions or coordinates made synchronously after an editor state update. This doesn't seem like an unlikely scenario.
• DOM mutations or selection changes delivered right after an update. This is possible, if you mess with the DOM and then synchronously update the editor, but maybe not a reasonable use case.
• Mouse or key events, which I suppose really cannot happen during invalid-view-tree timespans.

[smoores-dev]

Using React's flushSync feature may allow us to resolve issues with the first scenario in many cases. Specifically, I think we could do something like:

• In an event handler (whether registered with the EditorView or using useEditorEventCallback in a React component), we can wrap calls to dispatchTransaction with flushSync. This will force React to synchronously flush commits to the DOM after these calls, meaning that by the time view.dispatch returns, the DOM will be guaranteed to be updated. My suspicion is that almost all third-party code that dispatches transactions does so via event handlers, so this could resolve a lot of concerns!
• In an effect using useEditorEffect in a React component, we cannot use flushSync (you can't use flushSync in a React effect, because the effect itself is kicked off during another render cycle), but we might be able to raise an error in development if someone queries for position or coords between when view.dispatch is called and when the DOM is committed.

Tagging @tilgovi in case he has other thoughts!

[smoores-dev]

I had forgotten that we actually did this in #49; actually right now all dispatches use flushSync, which is great. This means that you can't dispatchTransaction from a useEditorEffect, but maybe that's fine? If it's not fine, we'll want to add some context detection to our dispatchTransaction wrapper so that it can switch to batch rather than flushSync in that scenario.

[marijnh]

I have tried to work out some approaches to this, but didn't find any that felt robust and neat enough that I feel comfortable adding them to the public API. So I am sorry to say, but I think forking is going to be your best avenue for this. Possibly, that may also help frame the problem in a way where 100% compatibility with prosemirror-view isn't a goal, which may help a lot to keep the scope and messiness of this under control. You could set this up as a React-ified prosemirror-view fork that largely exposes the same API, but replaces the plain-DOM interfaces with versions that make sense in React. 3rd-party commands and such would largely still work, but some extensions would have to be tailored to it to work with it.

Forking doesn't need to be all that uncomfortable—prosemirror-view moves slowly, at this point, and occasionally merging patches that look like you need them into a fork can be pretty painless.

The alternative, trying to come up with (and maintain) an abstraction that makes this very, very finicky use case fit well in the main library, just feels like something I'd regret before long, due to unforeseen problems it introduces, potential future changes in your approach mooting the decisions we make this point, and just the extra complexity that that kind of thing tends to introduce.

[smoores-dev]

Understood, and I think that's a good way to frame the tradeoffs. I'm definitely not saying we'll go this way, I just wanted to make sure it was considered as an option! I do think/hope that the modifications here are quite safe in that regard; the only actual changes are to essentially comment out the implementation of ViewDesc.update (and its subclasses), but I suspect that you're right that there are potentially subtle edge cases to consider with different versions of the library initializing things in a different order in the EditorView constructor, for example, or something.

[tilgovi]

I have sometimes wondered why docView is special in that it doesn't permit a custom node view. Since CustomNodeViewDesc can delegate to its spec's update function, and that function can return false to signal that it will render its own children, I wonder if it would be possible to achieve what we want by allowing custom doc views.

Would that be a generic change that avoids the need to modify EditorView?

[tilgovi]

In other words, is there a reason why the docViewDesc function calls new NodeViewDesc rather than NodeViewDesc.create?

[marijnh]

The top level node isn't rendered by the editor, only its content is. That's why it can't have a custom node view.

[tilgovi]

Ah. In the case of the "mount" option it isn't, but otherwise it sort of is. It has a hard-coded dom of a new div element, its deco is managed, and it has a node view descriptor.

But I see how it would be very awkward if if used with mount and the custom node view returned a different dom element.

[smoores-dev]

I feel like I'm tracing a very spiraling path toward the end goal here, but I did end up taking a stab at a more complete beforeinput-based approach #65, and it's feeling very solid. @tilgovi helped me confirm that main actually doesn't work on Chrome on Android right now due do the issues it has with IME/composition (Chrome on Android primarily uses IME for all languages), so... actually that doesn't really feel like a step backwards anymore! Especially given that Slate has a working Android input manager, leaving us a real path forward to resolving that in the future, I think... this is probably the best way forward?

[marijnh]

If that works, that'd be really cool. I kept running into poorly-supported corner cases last time I tried, but that's a while ago—browsers may have gotten better.

[smoores-dev]

I will definitely let you know what I find!