search

ProseMirror / prosemirror

The ProseMirror WYSIWYM editor
http://prosemirror.net/
MIT License
7.59k stars 335 forks source link

recreateWrapper doesn't destroy nodeview #1407

Closed YousefED closed 1 year ago

YousefED commented 1 year ago

I'm inspecting the code of recreateWrapper in https://github.com/ProseMirror/prosemirror-view/blob/f6d96de9f2714bcf97d6ca9b0906d8750a142d1b/src/viewdesc.ts#L1262

There's a check for if (!wrapper.contentDOM) return null. It's difficult for me to figure out the intention behind the code exactly, but I think it's causing issues when used with nodes without contentDOM.

a) I'm wondering if the logic is correct at all? If the nodeview doesn't use contentDOM, we really can't reuse it even if it support the same content (but doesn't use a contentdom to render it)? b) More importantly, if the logic is correct, it seems like we're creating a nodeview without destroying it. We simply return null, but the created wrapper will stay alive and will never be destroyed. This seems like a bug even if (a) is correct

marijnh commented 1 year ago

Do you have a reproduction where this is causing a node view to not be destroyed?

Returning null here just means 'don't perform the wrapper recreation', which is the right thing when nothing is being wrapped.

YousefED commented 1 year ago

I don't have an isolated reproduction at this moment (yet).

If returning null is intended, shouldn't the temporary wrapper (as created using let wrapper = NodeViewDesc.create(this.top, node, outerDeco, innerDeco, view, pos)) be destroyed so it can clean up possibly allocated resources?

marijnh commented 1 year ago

Oh, I see what you mean now. That line covers the (rather unlikely) case where the new node has the exact same children, but is rendered as a node view that doesn't actually display its children. Attached patch rearranges the code a bit to make sure no destroy() is dropped.

YousefED commented 1 year ago

Nice! This seems to line up with my scenario. A node changes from a paragraph to a code editor. Both take only inline content as children, but my code editor doesn't use contentDOM to display it.

I'll test in a bit! Thanks for the quick replies

YousefED commented 1 year ago

I tried to test it, and it now uses the nodeview nicely. However, I think it still triggers an invalid codepath.

After creating updated I see updated.updateChildren(view, pos + 1); is called. This triggers: addTextblockHacks(); which then triggers renderDescs(this.contentDOM, this.children, view). However, this throws an error as this.contentDOM is undefined

It seems like the textblockhack (trailinghackview) should not be added?