fix(tiptap-editor): stop inserting unrenderable content as literal markup - #1657
Conversation
…rkup When a paste parses to an empty slice, tiptap's insertContentAt never falsifies its isOnlyTextContent flag (the forEach over the nodes runs zero times) and falls through to tr.insertText with the HTML STRING, so pasting an iframe embed put `<iframe src=...>` in the document as visible text. The same branch double-escapes text-only content: `A & B` arrived as those characters. Confirmed identical in jsdom and real Chromium, so this is tiptap behaviour rather than a DOM quirk. resolveInsertContent decides what to hand insertContent: the fallback text when nothing renders, the decoded text when the parse is text alone, the HTML otherwise. All three insertContent callers use it. The fallback is the user's own clipboard text rather than our normalised HTML. Both end up as literal text, so escaping cannot tell them apart, but ours is sanitised and attribute-normalised: a pasted `<video src=x controls>` reached the document as `<video controls="">`, having quietly lost the URL. The spec asserts whose text it is, not whether it is escaped. Also treats zero-width characters as invisible. trim() drops U+00A0 but not U+200B, U+200D, U+2060 or U+FEFF, so an item holding only one of those counted as content and rendered as a blank-looking bullet. And records in the spec helper the two things these specs cannot settle: jsdom has no innerText, and jsdom and Chromium disagree on foster parenting of loose text inside a table. Closes #1656
|
ⓘ Your Qodo trial ends soon. Ask your workspace admin to set up billing to keep reviews running after the trial. Manage billing |
Code Review by Qodo
1.
|
|
Warning Review limit reachedNext included review available in 7 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
PR Summary by QodoFix TipTap paste/insert: avoid literal markup for unrenderable or text-only content
AI Description
Diagram
High-Level Assessment
Files changed (9)
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e21da726fc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let node: ChildNode | null = child.previousSibling; | ||
| while (node) { | ||
| if (node.textContent?.trim()) { | ||
| if (hasVisibleText(node.textContent)) { |
There was a problem hiding this comment.
When a list item has only a zero-width text node before a leading block—for example, the tight nested-list markdown - \u200b\n - nested—this now reports no preceding text and prepends an empty <p>, but leaves the zero-width node in place. ProseMirror still wraps that surviving inline node in its own paragraph, so the item acquires both a zero-width paragraph and the newly inserted empty paragraph, producing an extra blank line. Either remove the ignored nodes before prepending or continue treating them as satisfying the leading paragraph requirement.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in cf1f5dd600.
Reproduced it first rather than taking the reasoning: <ul><li>\u200B<ul><li>x</li></ul></li></ul> rendered as <li><p>\u200B</p><p></p><ul>…, two paragraphs, exactly as you describe. Same for the tight markdown form.
Took your first option. Anything text-like ahead of the block is invisible by definition at that point, since hasTextBefore just said so, and dropping it is consistent with treating zero-width characters as invisible everywhere else in the file. Keeping them as satisfying the requirement would have meant the leading paragraph was a zero-width one, which renders as a blank line just the same.
It also fixes the whitespace and newline forms, which had the same shape before the zero-width change and were producing the extra paragraph already: <li>\n<ul>… now yields one empty paragraph rather than two. Four cases added, 3 of which fail without the fix.
Review: treating zero-width characters as invisible made hasTextBefore report no lead-in, so a paragraph was prepended while the zero-width text node survived. ProseMirror wraps that in a paragraph of its own, so the item ended up with two and rendered a blank line. Anything text-like ahead of the block is invisible by definition at that point, so remove it. 3 of the 4 new cases fail without this.
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
…c mock Two review rule violations. The helpers in parse-all-extensions-to-doc relied on inferred return types; they are annotated now, including the ones that predate this PR so the file is consistent. The new spec mocked the extensions barrel to stub the hive-post regex, which is the jsdom innerText workaround. None of its inputs contains an <a href>, so that filter never runs and the mock was never needed. Removed, so the spec runs the real module.
…d nothing Review: removing only text nodes left a skipped wrapper in place, so <li><span></span><ul>...</ul></li> still got two paragraphs, the wrapper's invisible text in one and the inserted empty one after it. The suggested rule was that every sibling skipped while locating the block is non-renderable, and that is not quite true. The walk also skipped a wrapper that renders nothing ITSELF but contains something: <span><img></span> was stepped over, so removing every skipped sibling would have deleted the image. So the walk now stops at anything holding renderable content, which makes the rule true, and everything ahead of the block is removed. That also drops a spurious paragraph the image case was getting: it satisfies the leading paragraph on its own, so none is added. 3 of the 4 new cases fail without this.
|
Fixed in One correction to the suggested rule, which is why the fix is not a one-liner. "Every sibling skipped while finding The So the walk now stops at anything holding renderable content, which makes the rule true, and then everything ahead of the block is removed. That also fixes a spurious paragraph the image case was already getting on develop: the image satisfies the leading-paragraph requirement on its own, so none is added.
Four cases added, 3 fail without the fix. Full web suite 3551, tsc and lint clean. |
1 participant
Fixes #1656, and picks up the three smaller items from the #1655 review that were left out of it.
The leak (#1656)
When a paste parses to an empty slice,
insertContentAtnever falsifies itsisOnlyTextContentflag (theforEachover the nodes runs zero times) and falls through totr.insertTextwith the HTML string, so an iframe embed lands in the document as visible<iframe src=...>. The same branch double-escapes text-only content:A & Barrives as those literal characters. Verified identical in jsdom and real Chromium via Playwright, so it is tiptap behaviour, not a DOM quirk.resolveInsertContentdecides what to handinsertContent:All three
insertContentcallers use it: the clipboard text strategy, the translate dialog, and the toolbar fragments. The twosetContentcallers are unaffected, sincesetContentgoes throughcreateDocumentand never reaches this branch.Why the fallback is the user's own clipboard text. Both before and after, the result is literal text, so "is it escaped" cannot distinguish them. What differs is whose text it is: ours is sanitised and attribute-normalised, and a pasted
<video src=x controls>reached the document as<video controls="">, having quietly dropped the URL. The spec asserts that, rather than asserting on escaping.The three smaller items
trim()drops U+00A0 but not U+200B, U+200D, U+2060 or U+FEFF, so an item holding only one of those counted as content and rendered as a blank-looking bullet. OnehasVisibleTexthelper now backs every emptiness check in the file.innerText, and jsdom and Chromium genuinely disagree on foster parenting of loose text inside a table, so document order for that shape must be checked in a browser rather than asserted in a spec.<tr>was a false alarm and is not fixed here. It was reported as the same schema class, buttableRowis(tableCell | tableHeader)*, not+, so an empty row is valid and nothing throws. Measured before writing any code.Verification
New spec fails 4 of 10 cases against the old insert path; the zero-width cases fail on develop. Full web suite 3543 passing, tsc clean, lint clean.