Move rendering responsibility fully into React #47
Conversation
smoores-dev
requested review from
tilgovi,
gabrielainsf,
halfghaninne,
ilyaGurevich,
annabialas and
saranrapjs
it doesn't look like any of the implementation here required changes to any of the existing React wrappers for ProseMirror view components, yes? is there a model where the React-based EditorView is offered as an additional import, but where we could commit to keeping compatibility with the regular ProseMirror EditorView? i can imagine scenarios where retaining that compatibility is important, presuming it doesn't really burden this library to do so |
I think that I wouldn't be in favor of that; there's almost no overlap between the two approaches, so we'd basically be maintaining two separate libraries. What kind of compatibility requirement are you imagining, though? |
@smoores-dev I'm picturing:
Maybe this is a question we can ask Marijn? e.g. whether there's a "safe" way we should go about maintaining an EditorView implementation that tries to stay in sync with other |
|
@saranrapjs Gotcha. Yeah I suspect that when it comes down to it, it's going to be a tradeoff between the risks you've outlined here, and the risks of the approach we have now (which mostly center around the wrapping elements). We've already run into several frustrating corner cases with the wrapping elements: #42 is one of these, and #26 is an attempt to mitigate a few others. I actually haven't decided which way I personally fall on this; there are obvious downsides to both options. |
|
@tilgovi @saranrapjs (and anyone else interesting): So... what do you think? Should we actually try to do this? I don't see any reason that we literally could not continue implementing the rest of the EditorView API, though we might have to be thoughtful about how to handle, for example, commands that require the view (even I started down this road after #42; there may be a direct kludge (or even more elegant solution) that can solve that literal case, but it made me worried that there are more challenges with the general approach of wrapping elements that we initially expected. I think the challenge with this approach, as @saranrapjs has pointed out, is figuring out how to ensure that whatever we do here actually plays nicely with existing prosemirror libraries, including user-contributed ones. In particular, this... might mean some support for plugins with a view prop...? But that also seems a little too goofy for me; maybe it's acceptable to say that we only support state-only plugins, and the whole reason to use React is so that it can own the entirety of your view. |
That seems reasonable to me. |
|
Unless we want to pursue compiling a secondary ReactDOM renderer that we can flush synchronously, I'm pretty ready to give up on trying to let ProseMirror own the DOM structure. |
And didn't you do some experiments with that from which you concluded even that had problems? |
Indeed; secondary renderers can't share context with primary renderers, so context has to be manually bound across the renderer gap, and each node view is in its own private react element tree, so you lose the legibility in dev tools (and you can't pass context from parent node views to child node views). Some more info here for anyone following along: https://discuss.prosemirror.net/t/announcing-react-prosemirror/5328/13 |
|
(I am going to ask Marijn about this and will try to ferry any notable details from that conversation back here!) |
|
Alright, thanks @saranrapjs! If you think he'd be open to it, could be neat to have Marijn join the conversation here, too! |
|
quoting Marijn:
I think without a plan to account for something like |
I don't totally understand what this would mean, I don't think. Are you suggesting two different APIs exposed by this library, the current one and the one proposed by this PR? |
Is there any code on this PR that breaks |
|
Nothing about the original API (the Basically, I think we have to pick. I don't think this library can meaningfully support the actual EditorView class if we go with an all-React rendering approach. But I'm not at all opposed to attempting to run prosemirror-view's test suite (or at least a version of it) here to try to give us more confidence about compatibility. I also want to be clear that I would much rather not be in a position where this feels like an approach we have to consider, but at this point we've been struggling to make React-based NodeViews interop with prosemirror-view for several years, and while I think react-prosemirror is the closest we've ever gotten, it still has some hurdles that feel increasingly gnarly. Basically, I'm saying that I don't think we're picking between "It works generally and also works with native EditorView" and "It probably works generally and doesn't work with native EditorView"; rather the first option is more like "It sort of works if you're very careful and we don't have a clear path toward it actually working robustly". |
I think I'm not seeing how this is confusing — these seem like non-overlapping but mutually compatible code entrypoints we could maintain in parallel if we want to.
I think I'm still not totally convinced that we do? To put it another way: if we want to make a bet that we can get a compatible implementation of My feeling is that if there's a path to evaluating the stability of a |
|
Ah, I think I see where I was misunderstanding you. I had been interpreting your previous messages as a suggestion that we should indefinitely maintain both interfaces.
This is, in fact, exactly what my plan was, and I should have made that clearer upfront! I have no intention of merging this branch (or at least, exporting/documenting any of the new functionality) until we have a way to measure our confidence. I only opened this draft at this stage so that I could get a sense of whether this felt like a worthwhile endeavor to you all before I sink any more time into making it more robust! |
|
I don't want to rush this branch but I think this library provides very little value as it is now. It has some nice ideas, but without the stuff we've done internally at the Times to make simple node views work without wrappers, I'm skeptical that it's very useful to many people. |
Rather than attempting to map the new node positions backward to determine what the key used to be for that node, start with the previous positions and map them forward, taking care to skip any positions that are deleted by the transaction, and adding any new nodes that didn't exist in the old doc. This improves stability and correctness in more complex cases, such as splitting or wrapping nodes, and nested deletions.
tilgovi
force-pushed
the
react-editor-view
branch
from
7514130
to
70dc89b
Compare
smoores-dev
temporarily deployed
to
github-pages
This resolves an issue where LayoutGroup effects were not cleaned up when the LayoutGroup itself was unmounted. React executes layout effect destroy functions from parent to child on unmount, which is opposite how it runs during normal render. The result was that the destroyQueue was never processed on unmount. Now, if the LayoutGroup is being unmounted, we call destroy functions immediately, without queuing.
smoores-dev
temporarily deployed
to
github-pages
This tracks a few PRs from main that update hooks like useEditorState and useEditorEffect to always provide non-null state and view objects. It also updates the useEditorView hook (now useEditor) to look more like that on main, inlining several function calls and moving most hook usage from the ProseMirror component into the useEditor hook.
* Add a component for rendering ProseMirror-native widgets * Support plugin-provided custom node views
When rendering nested ProseMirror editors, e.g. when taking control over an atom node's content, the nested DocNodeView would register itself as the child of the atom node in the view descriptor tree. This could cause subtle issues when, e.g., ProseMirror attempts to determine whether the content is ltr or rtl, because it would treat the nested editor as the contentDOM of the atom, even if it was rendered in a portal into a non- contiguous part of the page. Presumably other issues could also arise from the fact that ProseMirror thought this node had a contentDOM when it actually does not. This change resolves this by simply providing a top- level ChildDescriptorContext, so that nested editors never accidentally cross the boundary up into their parents' view descriptor tree.
Browsers have very particular opinions about where they will allow users to place selections in contenteditable elements. ProseMirror manages this in part by placing <br> elements in locations that browsers otherwise wouldn't allow user selections. This PR brings react-prosemirror closer to matching all of the situations that ProseMirror itself uses these "trailing hacks". Rather than only in empty textblocks, we now also place trailing hacks when a textblock node ends with a non-text node, a widget, or a text node that ends with a newline. Also, Safari and Chrome both have cursor drawing/selection bugs that prevent users from making selections after non-contenteditable inline nodes in some situations. To work around this, in these browsers, we add an empty image element between the trailing non- contenteditable node and the "trailing hack", which allows users to place cursors there.
smoores-dev
had a problem deploying
to
github-pages
There were a few places where we incorrectly used view.someProp to retrieve a prop value, and then did something with the value. This will _always_ result in the first value being returned, even in cases where multiple plugins provide a value for the prop, and only some of them are relevant at the callsite. Instead, we now pass a function to view.someProp that only returns a truthy value when it has found a prop provider that provides the relevant value, so that we can fallback through the entire list of plugins.
Previously, `deleteContentBackward|Forward` inputs would delete exactly one code point. This created invalid strings when the last code point was part of a multi-code-point character. To resolve this, we now determine the length of the unicode character, and delete by that many code points, which matches default contentEditable behavior.
Rather than using Slate.js's more manual approach for determining how many code points to delete when handling a deleteContentBackward|Forward input type, rely on the browser's default determination via getTargetRanges.
|
Is there a |
|
@plaisted yeah! The README on this branch has been updated to reflect the changes to the APIs that this branch makes, and it's published to npm under the |
This is a re-imagining of this project's integration with ProseMirror's EditorView. Instead of attempting to carefully coordinate between React and ProseMirror, this fully moves the responsibility of rendering the editor state to the DOM into React.
How did this work before?
The primary work of this library is to manage the relationship between React and ProseMirror's EditorView. As laid out in The Problem Statement in the README, this is a fairly delicate relationship, and it's not without its tradeoffs. A short review:
Until now, this library has relied on the default behavior of ProseMirror's EditorView for rendering non-node-view parts of the ProseMirror document, and has attempted to mitigate the challenges stated above by:
Why are we interested in alternative solutions?
The current implementation is quite good at what it does, and it's proven to be very valuable. However, there are some downsides:
ulwith childlis, the actualulandlielements have to be managed outside of React, in the constructor, and set as the contentDOM and DOM respectively.dispatchTransactioncalluseLayoutEffect). This is when the node views are created/updated, but the React-based ones are not synchronously rendered.createPortalfrom within a React NodeView behave weirdly #69)What does this approach do differently?
This PR proposes to take over user input tracking (manual text input, at least) and rendering responsibilities from ProseMirror's EditorView, consolidating all rendering responsibilities into React.
This has several advantages:
<p>tag will result in only a<p>tag in the document.The notable disadvantage to this approach is that non-React node views and widgets will have to be wrapped in a single wrapping element. This is less cumbersome than the React-based node view wrappers in the current approach, but is worth calling out.
How does it work?
This PR extends the EditorView class and overrides its NodeView and DOMObserver properties. In their places, we provide custom NodeViews, that act exactly the same as the default implementation, but execute no-ops when their
updatemethods are called, and a custom DOMObserver that only listens for selection changes, and ignores DOM mutations.We then introduce a
beforeinputevent handler that produces ProseMirror transactions based on the events it receives, which replaces the DOMObserver functionality that we eschewed in our selection-only implementation.We also introduce a new React component suite, one for each type of NodeView, to replace the truncated
updatemethods. When state is updated, we render aDocNodeViewcomponent withview.state.docand the document decorations. Each NodeView component is responsible for rendering its children, so this eventually renders the entire ProseMirror document (and all of its decorations) in a single React tree.