Deleting the last node view can throw errors

[tilgovi]

As reported in #40 (comment), deleting the last character in the only remaining node view can throw errors. This is reproducible in the provided demo.

react-dom.development.js:12056 Uncaught DOMException: Failed to execute 'removeChild' on 'Node': The node to be removed is not a child of this node.
    at removeChildFromContainer (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:8473:23)
    at commitDeletionEffectsOnFiber (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17504:21)
    at recursivelyTraverseDeletionEffects (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17482:13)
    at commitDeletionEffectsOnFiber (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17573:15)
    at recursivelyTraverseDeletionEffects (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17482:13)
    at commitDeletionEffectsOnFiber (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17573:15)
    at recursivelyTraverseDeletionEffects (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17482:13)
    at commitDeletionEffectsOnFiber (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17530:17)
    at commitDeletionEffects (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17473:13)
    at recursivelyTraverseMutationEffects (http://localhost:5173/node_modules/.vite/deps/chunk-OB2TTU35.js?v=886cae67:17670:17)
flushSyncCallbacks @ react-dom.development.js:12056
(anonymous) @ react-dom.development.js:25651

[tilgovi]

In trying to diagnose this, I've determined that the basic keymap from prosemirror-commands ends up applying joinBackward when deleting the last remaining character in a paragraph as long as there's a paragraph before it. However, when deleting the last character in the only remaining paragraph, this issue occurs. I suspect this could be caused by the wrapper div we put around the paragraph node view, but I'm unsure. It could be that when ProseMirror's keymap doesn't handle the backspace/delete, the browser is deleting the node view component's DOM.

[tilgovi]

This seems to vary depending on which elements are p tags and which are div or span, which makes me think this is a matter of the browser's default contenteditable behavior and whether or not it lets you delete the React component elements. When it does, that causes problems, because ProseMirror doesn't realize that it needs to destroy the node view until it reads the mutation from the DOM and at that point React tries to unmount a node that's already been removed.

This is really unfortunate and is one of the real problems with using the mutation model from prosemirror-view as opposed to the more modern beforeinput event.

[tilgovi]

Interestingly, adding display: contents to the wrapper div seems to help, at least in Chrome.

[tilgovi]

Firefox seems to have no trouble, with or without display: contents.

[tilgovi]

Okay, I can confirm that when we have

<div><p><span style="display: contents"><span>a</span></span></p></div>

and we delete the a character, ProseMirror sees 5 mutations that result in lifting the <p> out of the <div>, destroying the <div>, and appending a <br> to the <p> (note that Chrome adds this <br>, not ProseMirror, but for the same reason ProseMirror will add a trailing <br> to keep a blank line from collapsing to zero height).

If we add display: contents to the <div>, there are two mutations that result in removing both <span> elements and replacing them with a <br>. React doesn't notice because it doesn't think it needs to change its content DOM wrapper element, but this is also removing something out from under React.

This is going to require more thinking. It probably isn't a problem with more complex node views where the non-content parts aren't editable, but for simple node views this is a real problem. We haven't run into this internally since we're still using a different, hacky, implementation for these kinds of node views that we were maybe hoping to move away from.

[smoores-dev]

If we add display: contents to the <div>, there are two mutations that result in removing both <span> elements and replacing them with a <br>.

Just making sure I'm getting this right; the result of these mutations is the following?

<div><p><br /></p></div>

[tilgovi]

I think that's right, yes.

[jcayabyab]

I'm also experiencing this issue. I have a simple schema with a NodeView that reads attributes from the node to determine styles for a wrapping div element. Can confirm that adding display: contents; to the wrapping div fixes it for me. However, I do get some weird behavior when deleting the last character out of any nodeview in the editor. When I have:

<div style="display: contents;">
  <div style="...">
    <div>
      <div>
        <span>1</span>
      </div>
    </div>
  </div>
</div>

and I delete the 1, I get:

<div style="display: contents;">
  <div style="...">
    <div>
      <br>
      <div></div>
    </div>
  </div>
</div>

This is causing the Node to still exist in the view, but it is not editable or selectable. I would expect to see the following DOM tree get created as a result:

<div style="display: contents;">
  <div style="...">
    <div>
      <div>
        <br class="ProseMirror-trailingBreak">
      </div>
    </div>
  </div>
</div>

The difference is that the regular backspacing inserts this non-styled br instead of the ProseMirror-generated br with the ProseMirror-trailingBreak class.

I'm running this on the latest version of Chrome. Even more interesting, I don't seem to be able to reproduce this consistently - either it will always produce the first output, or it will always produce the second output. If I could get some direction on what may be causing this, please let me know.

[tilgovi]

I don't think this is a problem we can solve. I would suggest avoiding single-element node views for the moment, until we can ship something much more comprehensive like #47. We'll try to document the limitations in the next weeks and make the roadmap more clear.

[saranrapjs]

I'm finding that running the code on #47 locally, I still see the DOM errors we've encountered here. I'm not yet sure what to make of that, but figured I'd share here, by way of maybe hypothesizing that it's possible that this issue isn't 100% solved by moving over to React-managed views.

[saranrapjs]

However, I do get some weird behavior when deleting the last character out of any nodeview in the editor.

@jcayabyab I've also seen something similar, and what I found is that it looks like the DOM mutations produced by the browser (later picked up by ProseMirrors' MutationObserver, and forwarded thru react-prosemirror) are sometimes unexpected or incorrect, in a way that seems to vary depending on the specific HTML structure of the node view and child toDOM. To wit, with a node view configuration for a node whose schema is { content: "inline*" }...

....this configuration produces the error in this issue (note that the contentDOM and React component both end up wrapping content with divs):

const reactNodeViews:  Record<string, ReactNodeViewConstructor> = {
  myNode: () => ({
    component: ({ children }) => <div><p>Some uneditable text</p><div>{children}</div></div>,
    dom: document.createElement("div"),
    contentDOM: document.createElement("div"),
  }),
};
// produces a DOM that looks like: <div><p>Some uneditable text</p><div><div><span>inline content</span></div></div></div>

....a slightly different configuration — with a span as contentDOM — no longer throws the error, but produces something similar to what you saw — when you try to delete the last character in the NodeView's children, you'll see the node view's children visually update on screen — but no ProseMirror transaction is dispatched, so the last letter is "stuck":

const reactNodeViews:  Record<string, ReactNodeViewConstructor> = {
  myNode: () => ({
    component: ({ children }) => <div><p>Some uneditable text</p><div>{children}</div></div>,
    dom: document.createElement("div"),
    contentDOM: document.createElement("span"),
  }),
};
// produces a DOM that looks like: <div><p>Some uneditable text</p><div><span><span>inline content</span></span></div></div>

....I found, at least in my case, that ensuring the contentDOM element type is different from both its child and parent elements, seems to not reproduce the error thrown nor a stuck DOM state:

const reactNodeViews:  Record<string, ReactNodeViewConstructor> = {
  myNode: () => ({
    component: ({ children }) => <div><p>Some uneditable text</p><div>{children}</div></div>,
    dom: document.createElement("div"),
    contentDOM: document.createElement("p"),
  }),
};
// produces a DOM that looks like: <div><p>Some uneditable text</p><div><p><span>inline content</span></p></div></div>

[tilgovi]

I'm not yet sure what to make of that, but figured I'd share here, by way of maybe hypothesizing that it's possible that this issue isn't 100% solved by moving over to React-managed views.

I wouldn't be terribly surprised. It's possible the mutation-based approached might simply not work with React and we absolutely need proper beforeinput support. React is not happy trying to destroy elements that have already been removed from the DOM, if I remember correctly, and the browser implementation of content editable will remove elements sometimes in ways we probably need to prevent.

[tilgovi]

@smoores-dev, if I recall correctly, you started looking into the mutation-based approached to try to minimize diff with prosemirror-view and also support mobile. Does what I'm speculating about mutations make sense to you? Do you think it makes sense just to draw a line and say that we only support browsers with proper editing events implementations?

[smoores-dev]

I have to look into this a bit more; @saranrapjs what are you running locally, exactly, that you're seeing these errors? I can reproduce this if I'm using the ProseMirror View-style node views (which is not surprising, and I don't know that we'll be able to get around that), but I don't see it with the React-based ones.

It would be possible to switch back to beforeinput, yes; on the whole, I suspect that would be a good move, if we're comfortable with the limitations. But I'm not totally sure that I am comfortable with the limitations?? (and yes, to be clear, we moved away from beforeinput because Chrome-on-Android's beforeinput implementation is completely broken). Ugh. Definitely interested in hearing others' thoughts.

[saranrapjs]

what are you running locally, exactly, that you're seeing these errors?

On the react-editor-view branch, if I run yarn run demo, and just press backspace from the end of the document in the demo (towards deleting everything), eventually I get this error. Are the node view in this demo+that branch not rendering using React?

[saranrapjs]

Though I'm finding that this error gets thrown when deleting near a mark boundary, and in the demo I think only paragraphs have a defined nodeView — so maybe your assumption still stands, that this only should happen on react-editor-view for non-React node views (inclusive of mark views)? Removing the marks from the demo, I no longer get the error.

[smoores-dev]

Ah, ok, thanks; I had been deleting entire paragraphs at a time, but deleting only a character at a time I'm also seeing that error when I get to the marks.

Marks are also rendered with React on that branch, though; unless you click the "Switch to ProseMirror node views" button, everything is being rendered with the new React components, regardless of whether there are custom node views defined or not. I can take a look at this this week and see if there's anything obvious, but @tilgovi's right that this behavior definitely comes from the DOMObserver/MutationObserver implementation, and isn't a problem with the beforeinput-based approach. The beforeinput approach also makes IME input trivial (it just works, with no special handling necessary), whereas someone just identified a lingering IME bug in Chrome with the DOMObserver implementation (in #41).

@saranrapjs what's your gut reaction to this library basically not supporting Android Chrome?

[smoores-dev]

Ah, actually what I'm seeing is that the error shows up when I get into the inline decoration, not the marks. I actually have on my list of things to do to look into some DOMObserver related issues with inline decorations, so this might not be as much of a concern/show-stopper as it seems, actually.

[smoores-dev]

I just posted this in Gitter, but I just learned that Slate.js (which also relies on beforeinput) actually does support Chrome on Android, and it does so by observing a combination of composition events and mutations:

Chrome on Android was until recently unsupported except for in older versions of Slate (0.47) but has recently been added. For clarity, due to the differences in Android's support of the beforeInput event, Android input uses compositions and mutations which is different from other browsers. This means that Android support progresses separately from other browsers and due to it being new, may have more bugs.

https://docs.slatejs.org/general/faq#what-browsers-and-devices-does-slate-support

So I might take a look into what they're doing there!

[tilgovi]

I can try to find out whether Chrome on Android is likely to align better soon.

[saranrapjs]

While we're evaluating how the new architecture should handle this, I think I have a fix that should work within the architecture on main. I'd be curious what others think: #42

[smoores-dev]

@saranrapjs thank you for resolving this! I'm going to call this closed, as it's fixed by #62 and released in v0.4.0. @jcayabyab please give that a shot! Note that there is a breaking change in there; React node views no longer get their position as a prop, instead they can use the useNodePos hook to subscribe to changes to their position.