-
Notifications
You must be signed in to change notification settings - Fork 7
fix(tiptap-editor): stop inserting unrenderable content as literal markup #1657
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
Changes from 1 commit
e21da72
cf1f5dd
e8be3b4
62d6a56
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| export * from "./parse-all-extensions-to-doc"; | ||
| export * from "./markdown-to-html"; | ||
| export * from "./normalize-link-href"; | ||
| export * from "./resolve-insert-content"; |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,7 +57,7 @@ const RENDERS_AS_NODE = [ | |
| */ | ||
| function keptOnUnwrap(el: Element) { | ||
| return Array.from(el.childNodes).filter( | ||
| (node) => node.nodeType === Node.ELEMENT_NODE || node.textContent?.trim() | ||
| (node) => node.nodeType === Node.ELEMENT_NODE || hasVisibleText(node.textContent) | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -80,24 +80,37 @@ const BLOCK_LEVEL = [LEADING_BLOCK, "p", "div"].join(", "); | |
| function unwrapTableStructure(el: Element): Node[] { | ||
| return Array.from(el.childNodes).flatMap((node) => { | ||
| if (node.nodeType !== Node.ELEMENT_NODE) { | ||
| return node.textContent?.trim() ? [node] : []; | ||
| return hasVisibleText(node.textContent) ? [node] : []; | ||
| } | ||
|
|
||
| const child = node as Element; | ||
| return child.matches(TABLE_STRUCTURE) ? unwrapTableStructure(child) : [child]; | ||
| }); | ||
| } | ||
|
|
||
| /** | ||
| * Characters that take up no space, so text made only of them reads as empty. | ||
| * `trim()` already drops U+00A0, but not these: they are format characters | ||
| * rather than whitespace, and a list item holding only one of them would | ||
| * otherwise count as content and render as a blank line. | ||
| */ | ||
| const ZERO_WIDTH = /[\u200B-\u200D\u2060\uFEFF]/g; | ||
|
|
||
| /** True when the text holds something a reader would actually see. */ | ||
| function hasVisibleText(value?: string | null) { | ||
| return !!value?.replace(ZERO_WIDTH, "").trim(); | ||
| } | ||
|
|
||
| /** True when the element holds nothing the schema would render. */ | ||
| function holdsNothingRenderable(el: Element) { | ||
| return !el.textContent?.trim() && !el.querySelector(RENDERS_AS_NODE); | ||
| return !hasVisibleText(el.textContent) && !el.querySelector(RENDERS_AS_NODE); | ||
| } | ||
|
|
||
| /** True when the item holds visible text ahead of the given child. */ | ||
| function hasTextBefore(child: Element) { | ||
| let node: ChildNode | null = child.previousSibling; | ||
| while (node) { | ||
| if (node.textContent?.trim()) { | ||
| if (hasVisibleText(node.textContent)) { | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a list item has only a zero-width text node before a leading block—for example, the tight nested-list markdown Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in Reproduced it first rather than taking the reasoning: Took your first option. Anything text-like ahead of the block is invisible by definition at that point, since 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: |
||
| return true; | ||
| } | ||
| node = node.previousSibling; | ||
|
|
@@ -355,7 +368,7 @@ export function parseAllExtensionsToDoc(value?: string) { | |
| // behind something like an empty <span> still counts as leading the item. | ||
| // Cheap matches() first: textContent walks the whole nested subtree. | ||
| let first = li.firstElementChild; | ||
| while (first && !first.matches(RENDERS_AS_NODE) && !first.textContent?.trim()) { | ||
| while (first && !first.matches(RENDERS_AS_NODE) && !hasVisibleText(first.textContent)) { | ||
| first = first.nextElementSibling; | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| import { createNodeFromContent } from "@tiptap/core"; | ||
| import { Schema } from "@tiptap/pm/model"; | ||
|
|
||
| /** A plain-text node, the one shape insertContent inserts verbatim. */ | ||
| interface TextContent { | ||
| type: "text"; | ||
| text: string; | ||
| } | ||
|
|
||
| /** | ||
| * Decides what to hand `insertContent` so that content the schema renders as | ||
| * nothing does not come back as literal markup. | ||
| * | ||
| * ⛔ Never pass normalised HTML to `insertContent` directly. When the parsed | ||
| * result is EMPTY, tiptap's `insertContentAt` leaves its `isOnlyTextContent` | ||
| * flag true (the forEach over the nodes runs zero times) and falls through to | ||
| * `tr.insertText(value)` with the HTML STRING, so pasting say | ||
| * `<iframe src=...></iframe>` puts `<iframe src=...>` in the document as | ||
| * visible text. The same branch double-escapes when the parse is text alone: | ||
| * `A & B` arrives as those literal characters rather than `A & B`. | ||
| * See @tiptap/core 2.26.2 dist/index.js, insertContentAt. | ||
| * | ||
| * @param schema the editor's schema, which decides what renders at all | ||
| * @param html normalised HTML, straight from parseAllExtensionsToDoc | ||
| * @param fallbackText what the user actually had, used when nothing renders | ||
| * @returns the value for insertContent, or null when there is nothing to insert | ||
| */ | ||
| export function resolveInsertContent( | ||
| schema: Schema, | ||
| html: string, | ||
| fallbackText?: string | ||
| ): string | TextContent | null { | ||
| const content = createNodeFromContent(html, schema, { | ||
| parseOptions: { preserveWhitespace: "full" } | ||
| }); | ||
|
|
||
| let text = ""; | ||
| let isOnlyText = content.childCount > 0; | ||
|
|
||
| content.forEach((node) => { | ||
| isOnlyText = isOnlyText && node.isText && node.marks.length === 0; | ||
| text += node.text ?? ""; | ||
| }); | ||
|
|
||
| // Renders as nothing. Show what the user actually had rather than our | ||
| // intermediate HTML, so nothing is silently dropped and nothing is invented. | ||
| if (content.childCount === 0) { | ||
| return fallbackText?.length ? { type: "text", text: fallbackText } : null; | ||
| } | ||
|
|
||
| // Text alone: insert the parsed text, not the HTML that encoded it. | ||
| if (isOnlyText) { | ||
| return text.length ? { type: "text", text } : null; | ||
| } | ||
|
|
||
| return html; | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,103 @@ | ||
| import { vi } from "vitest"; | ||
|
|
||
| // Only the hive-post pass needs stubbing here: it is the one that reads | ||
| // `el.innerText`, which jsdom does not implement, and it calls `.trim()` on it | ||
| // unguarded, so a hive-post-shaped href throws in a spec instead of exercising | ||
| // anything. Same re-mock pattern CLAUDE.md documents for `@/utils`. | ||
| vi.mock("@/features/tiptap-editor/extensions", async () => ({ | ||
| ...(await vi.importActual("@/features/tiptap-editor/extensions")), | ||
| HIVE_POST_PURE_REGEX: /$a^/ | ||
| })); | ||
|
qodo-code-review[bot] marked this conversation as resolved.
Outdated
|
||
|
|
||
| import { Editor, getSchema } from "@tiptap/core"; | ||
| import { simpleMarkdownToHTML } from "@ecency/render-helper"; | ||
|
|
||
| import { parseAllExtensionsToDoc } from "@/features/tiptap-editor/functions/parse-all-extensions-to-doc"; | ||
| import { resolveInsertContent } from "@/features/tiptap-editor/functions/resolve-insert-content"; | ||
| import { PUBLISH_EDITOR_EXTENSIONS } from "./publish-editor-extensions"; | ||
|
|
||
| const schema = getSchema(PUBLISH_EDITOR_EXTENSIONS); | ||
|
|
||
| /** Pastes exactly as the clipboard text strategy does. */ | ||
| function paste(pastedText: string): { html: string; text: string } { | ||
| const editor = new Editor({ extensions: PUBLISH_EDITOR_EXTENSIONS, content: "<p>draft</p>" }); | ||
| try { | ||
| const parsed = parseAllExtensionsToDoc(simpleMarkdownToHTML(pastedText)); | ||
| const content = resolveInsertContent(editor.schema, parsed, pastedText); | ||
| if (content) { | ||
| editor.chain().insertContent(content).run(); | ||
| } | ||
| return { html: editor.getHTML(), text: editor.getText() }; | ||
| } finally { | ||
| editor.destroy(); | ||
| } | ||
| } | ||
|
|
||
| // Regression: when the parsed result is empty, tiptap's insertContentAt never | ||
| // falsifies its isOnlyTextContent flag (the forEach runs zero times) and falls | ||
| // through to tr.insertText with the HTML STRING, so the markup lands in the | ||
| // document as visible text. Confirmed identical in jsdom and real Chromium, so | ||
| // this is tiptap behaviour rather than a DOM quirk. | ||
| describe("pasting content the editor cannot render", () => { | ||
| // Both before and after the fix the result is literal text, so "is it escaped" | ||
| // cannot tell them apart. What distinguishes them is WHOSE text it is: the | ||
| // editor used to receive our sanitised, attribute-normalised HTML, which the | ||
| // author never typed and which had already lost part of what they pasted. | ||
| it.each([ | ||
| ["an iframe embed", "<iframe src=https://a.test/x></iframe>"], | ||
| ["a video element", "<video src=x controls></video>"] | ||
| ])("keeps the text the user actually copied for %s", (_l: string, pastedText: string) => { | ||
| expect(paste(pastedText).text).toContain(pastedText); | ||
| }); | ||
|
|
||
| it.each([ | ||
| // the sanitiser quotes the attribute, so this form is ours and not the author's | ||
| ["a requoted iframe src", "<iframe src=https://a.test/x></iframe>", 'src="https://a.test/x"'], | ||
| // and it drops src from <video> while inventing controls="", losing the URL | ||
| ["an invented video attribute", "<video src=x controls></video>", 'controls=""'] | ||
| ])("never inserts our converted HTML: %s", (_l: string, pastedText: string, ours: string) => { | ||
| expect(paste(pastedText).text).not.toContain(ours); | ||
| }); | ||
|
|
||
| it("keeps the rest of the document intact", () => { | ||
| expect(paste("<video src=x controls></video>").text).toContain("draft"); | ||
| }); | ||
| }); | ||
|
|
||
| describe("resolveInsertContent", () => { | ||
| it("passes ordinary HTML straight through", () => { | ||
| expect(resolveInsertContent(schema, "<p>ok</p>", "ok")).toBe("<p>ok</p>"); | ||
| }); | ||
|
|
||
| it("returns the fallback text when nothing renders", () => { | ||
| expect(resolveInsertContent(schema, '<iframe src="x"></iframe>', "raw text")).toEqual({ | ||
| type: "text", | ||
| text: "raw text" | ||
| }); | ||
| }); | ||
|
|
||
| it("returns null when nothing renders and there is no fallback", () => { | ||
| expect(resolveInsertContent(schema, '<iframe src="x"></iframe>')).toBeNull(); | ||
| }); | ||
|
|
||
| // The same insertText branch double-escapes text-only content, so `A & B` | ||
| // used to reach the document as those literal characters. | ||
| it("decodes entities when the content is text alone", () => { | ||
| expect(resolveInsertContent(schema, "A & B", "A & B")).toEqual({ | ||
| type: "text", | ||
| text: "A & B" | ||
| }); | ||
| }); | ||
|
|
||
| it("inserts decoded text rather than the HTML that encoded it", () => { | ||
| const editor = new Editor({ extensions: PUBLISH_EDITOR_EXTENSIONS, content: "<p></p>" }); | ||
| try { | ||
| const content = resolveInsertContent(editor.schema, "1 < 2", "1 < 2"); | ||
| editor.chain().insertContent(content!).run(); | ||
|
|
||
| expect(editor.getHTML()).toBe("<p>1 < 2</p>"); | ||
| } finally { | ||
| editor.destroy(); | ||
| } | ||
| }); | ||
| }); | ||
Uh oh!
There was an error while loading. Please reload this page.