Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix sending of emptystring class for Prosemirror decoration #1004

Merged
merged 1 commit into from
Apr 14, 2021

Conversation

ascott18
Copy link
Contributor

Prosemirror does not handle emptystring classes well for decorations when the decoration applies to a range that has other decorations. For example, both a spellchecking plugin and the tiptap highlighting plugin.

See this debugger screenshot in prosemirror showing the code that merges decorations together before they're applied. There are two decorations - one from a spellchecker, and one from the Highlight plugin that has no class because the specific range did not receive any specific highlight:
image

The above results in a string like "spellcheck-error ", which is then re-split by prosemirror into an array ["spellcheck-error", ""]. This is in turn fed into HTMLElement.classList.add(...), which throws an error when it receives emptystring:
image

However, there's no reason to send an empty decoration to prosemirror at all, so this change elects to omit such decorations entirely. This both fixes the issue at hand, and also slightly improve performance.

Prosemirror does not handle emptystring classes well for decorations when the decoration applies to a range that has other decorations. For example, both a spellchecking plugin and the tiptap highlighting plugin.

Furthermore, there is no reason to emit empty decorations, so this change resolves that issue as well.
@ascott18
Copy link
Contributor Author

Looks like Prosemirror also fixed this on their end 3 days ago: ProseMirror/prosemirror-view#86. However, not emitting empty decorations is still a worthwhile change.

@hanspagel
Copy link
Contributor

Thanks for the PR!

If I correctly understand your code change that would only help with [], but not with [""] or ["spellcheck-error", ""]. The code only checks the length of the array (= number of items).

Great to see it’s fixed in prosemirror-view already! I’m closing this here and update the depencies.

@hanspagel hanspagel closed this Mar 29, 2021
@ascott18
Copy link
Contributor Author

If I correctly understand your code change that would only help with [], but not with [""] or ["spellcheck-error", ""]. The code only checks the length of the array (= number of items).

@hanspagel Unfortunately there does seem to be a misunderstanding here. The issue is that the tiptap highlight plugin is the origin of the empty string that triggers the issue. Tiptap's highlight plugin would never be the one sending "spellcheck-error" (in your example), because highlight.js and/or lowlight would never provide such a class.

The three arrays you mentioned - [], [""] or ["spellcheck-error", ""], are the arrays that prosemirror ultimately receives. What this PR does is filter out the origin of the empty strings, which is the tiptap highlight plugin, by not emitting a decoration with a class name of "". Such a class name is created when tiptap calls node.classes.join(' ') where node.classes is an empty array - [].join(' ') === "".

Regardless of whether this was fixed in prosemirror, this PR very much still has value as a performance improvement - there's no reason for tiptap to be emitting empty decorations when highlight.js has no specific highlight classes to apply to a particular range of text. Such empty decorations still have to be chewed though by prosemirror, only to just be dropped on the floor because they're not actually applying any meaningful decoration.

@hanspagel hanspagel reopened this Mar 29, 2021
@hanspagel
Copy link
Contributor

Got it! Thanks for the explanation! I’ll merge it and release a new version soonish.

@hanspagel hanspagel merged commit 2466d33 into ueberdosis:main Apr 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants