From 0a8bfffb964b0c2c05a3f44a6ea8d2a8091880b8 Mon Sep 17 00:00:00 2001 From: Laurent Cozic Date: Fri, 25 Sep 2026 12:01:53 +0100 Subject: [PATCH] Desktop, Mobile: Fix table editor losing and mis-rendering inline markdown (#16632) --- .../extensions/rendering/renderTables.test.ts | 66 ++++++++ .../extensions/rendering/renderTables.ts | 160 +++++++++++------- 2 files changed, 168 insertions(+), 58 deletions(-) diff --git a/packages/editor/CodeMirror/extensions/rendering/renderTables.test.ts b/packages/editor/CodeMirror/extensions/rendering/renderTables.test.ts index 09730a7350..09e5d15f41 100644 --- a/packages/editor/CodeMirror/extensions/rendering/renderTables.test.ts +++ b/packages/editor/CodeMirror/extensions/rendering/renderTables.test.ts @@ -39,6 +39,13 @@ describe('renderTables', () => { { input: 'a **b** c', expected: 'a b c', inner: 'a b c' }, // Escaped pipes are unescaped for display. { input: 'a \\| b', expected: 'a | b', inner: 'a | b' }, + // Nested markup renders both layers. + { input: '**[label](https://example.com)**', expected: 'label', inner: 'label' }, + { input: '[**label**](https://example.com)', expected: 'label', inner: 'label' }, + { input: '*[label](https://example.com)*', expected: 'label', inner: 'label' }, + { input: '**~~strike~~**', expected: 'strike', inner: 'strike' }, + // Code spans stay literal. + { input: '`**not bold**`', expected: '**not bold**', inner: '**not bold**' }, ])('renderInlineMarkdown should render $input', ({ input, expected, inner }) => { const div = document.createElement('div'); renderInlineMarkdown(div, input); @@ -202,4 +209,63 @@ describe('renderTables', () => { } }); + test.each([ + '**[label](https://example.com)**', + '**bold**', + '*italic*', + '`code`', + ])('focus/blur cycles should not degrade cell markdown: %s', async (cellSource) => { + jest.useFakeTimers(); + let editor: EditorView | null = null; + try { + const markdown = `| a | b |\n|---|---|\n| ${cellSource} | y |`; + editor = await createEditor(markdown); + document.body.appendChild(editor.dom); + + // Each cycle re-renders the cell without editing it. + for (let i = 0; i < 3; i++) { + const cell = findCellTextDivs(editor)[2]; + focusCell(cell); + cell.dispatchEvent(new Event('blur')); + jest.advanceTimersByTime(200); + jest.runOnlyPendingTimers(); + + const rendered = findCellTextDivs(editor)[2]; + expect(rendered.classList.contains('cm-tw-raw')).toBe(false); + } + + expect(editor.state.doc.toString()).toContain(cellSource); + } finally { + editor?.destroy(); + jest.useRealTimers(); + } + }); + + test('a blurred cell should not write its rendered text back to the document', async () => { + jest.useFakeTimers(); + let editor: EditorView | null = null; + try { + editor = await createEditor('| a | b |\n|---|---|\n| **[x](http://e.com)** | y |'); + document.body.appendChild(editor.dom); + + const cell = findCellTextDivs(editor)[2]; + focusCell(cell); + cell.dispatchEvent(new Event('blur')); + jest.advanceTimersByTime(200); + jest.runOnlyPendingTimers(); + + // An input event on the now-rendered cell, as a stray mutation + // would fire, must not harvest its stripped text. + const rendered = findCellTextDivs(editor)[2]; + rendered.dispatchEvent(new Event('input')); + jest.advanceTimersByTime(700); + jest.runOnlyPendingTimers(); + + expect(editor.state.doc.toString()).toContain('**[x](http://e.com)**'); + } finally { + editor?.destroy(); + jest.useRealTimers(); + } + }); + }); diff --git a/packages/editor/CodeMirror/extensions/rendering/renderTables.ts b/packages/editor/CodeMirror/extensions/rendering/renderTables.ts index a668afe123..2339a4ea2a 100644 --- a/packages/editor/CodeMirror/extensions/rendering/renderTables.ts +++ b/packages/editor/CodeMirror/extensions/rendering/renderTables.ts @@ -27,6 +27,8 @@ const W = 'cm-tw'; const CELL = 'cm-tw-c'; const HDR = 'cm-tw-h'; const CTX = 'cm-tw-ctx'; +// Marks a cell holding raw source. Only these may be read back into the model. +const RAW = 'cm-tw-raw'; // Cache for rendered table widget heights so CodeMirror can estimate // heights correctly for scroll position and coordinate mapping. @@ -59,45 +61,54 @@ const escapeHtml = (s: string): string => { // shown as plain |. The assembled HTML is run through DOMPurify before // insertion, so unsafe URL schemes (javascript:, data:, ...) and any tags // or attributes that slipped through the regex are removed. +// Wrapper contents recurse so nested markup works (**[label](url)** is a bold +// link). Code spans do not: their contents are literal in markdown. +const inlineMarkdownToHtml = (segment: string): string => { + // Single regex with alternatives, scanned left-to-right. Each branch + // captures its inner content. Single * and _ emphasis use word- + // boundary guards so identifiers like `foo_bar_baz` or `a*b*c` are + // not rendered as emphasis. + const re = /\*\*([\s\S]+?)\*\*|__([\s\S]+?)__|(? lastIdx) { + parts.push(escapeHtml(segment.slice(lastIdx, m.index))); + } + if (m[1] !== undefined || m[2] !== undefined) { + parts.push(`${inlineMarkdownToHtml((m[1] ?? m[2])!)}`); + } else if (m[3] !== undefined || m[4] !== undefined) { + parts.push(`${inlineMarkdownToHtml((m[3] ?? m[4])!)}`); + } else if (m[5] !== undefined) { + parts.push(`${escapeHtml(m[5])}`); + } else if (m[6] !== undefined) { + parts.push(`${inlineMarkdownToHtml(m[6])}`); + } else { + parts.push(`${inlineMarkdownToHtml(m[7]!)}`); + } + lastIdx = m.index + m[0].length; + } + if (lastIdx < segment.length) { + parts.push(escapeHtml(segment.slice(lastIdx))); + } + return parts.join(''); +}; + export const renderInlineMarkdown = (parent: HTMLElement, text: string) => { // Normalise: escaped pipes → |, and split on literal
for soft breaks. const normalised = text.replace(/\\\|/g, '|'); - const segments = normalised.split(//i); - const parts: string[] = []; - for (let s = 0; s < segments.length; s++) { - if (s > 0) parts.push('
'); - const segment = segments[s]; - // Single regex with alternatives, scanned left-to-right. Each branch - // captures its inner content. Single * and _ emphasis use word- - // boundary guards so identifiers like `foo_bar_baz` or `a*b*c` are - // not rendered as emphasis. - const re = /\*\*([^*]+)\*\*|__([^_]+)__|(? lastIdx) { - parts.push(escapeHtml(segment.slice(lastIdx, m.index))); - } - if (m[1] !== undefined || m[2] !== undefined) { - parts.push(`${escapeHtml((m[1] ?? m[2])!)}`); - } else if (m[3] !== undefined || m[4] !== undefined) { - parts.push(`${escapeHtml((m[3] ?? m[4])!)}`); - } else if (m[5] !== undefined) { - parts.push(`${escapeHtml(m[5])}`); - } else if (m[6] !== undefined) { - parts.push(`${escapeHtml(m[6])}`); - } else { - parts.push(`${escapeHtml(m[7]!)}`); - } - lastIdx = m.index + m[0].length; - } - if (lastIdx < segment.length) { - parts.push(escapeHtml(segment.slice(lastIdx))); - } - } - parent.innerHTML = sanitizeHtml(parts.join('')); + const html = normalised + .split(//i) + .map(inlineMarkdownToHtml) + .join('
'); + parent.innerHTML = sanitizeHtml(html); }; +// Stashed on the container so destroy() can reach toDOM()'s closure state. +const teardownKey = Symbol('tableWidgetTeardown'); +type TableWidgetContainer = HTMLElement & { [teardownKey]?: ()=> void }; + class TableWidget extends WidgetType { public constructor( private tableText: string, @@ -202,6 +213,9 @@ class TableWidget extends WidgetType { let scrollbarDragging = false; let lastFocusedTextDiv: HTMLElement | null = null; + // Disconnected on destroy so a detached DOM cannot still schedule syncs. + const cellObservers: MutationObserver[] = []; + // Debounced dispatch so the document source stays in sync with cell // edits — important so the preview pane reflects in-cell changes // (e.g. deleting an image) without waiting for blur or a structural @@ -214,19 +228,14 @@ class TableWidget extends WidgetType { } }; - // Sync the focused cell's current text into the table model. Other - // cells are kept in sync continuously via their oninput handler, so - // this is just a final read of whichever cell is being edited right - // now. Reading textContent of an unfocused cell would be wrong — - // rendered cells have stripped markdown markers (** etc). + // Sync the raw-mode cell's text into the model. Reading a rendered cell + // would drop one layer of markup per round-trip, since its textContent + // has the markers stripped (#16498). const syncDirtyCells = () => { - const active = doc.activeElement as HTMLElement | null; - if (!active || !active.classList.contains('cm-tw-text')) return; - if (!container.contains(active)) return; for (let ri = 0; ri < allCells.length; ri++) { for (let ci = 0; ci < allCells[ri].length; ci++) { const td = allCells[ri][ci].querySelector('.cm-tw-text') as HTMLElement; - if (td !== active) continue; + if (!td || !td.classList.contains(RAW)) continue; const v = (td.textContent || '').trim().replace(/\n/g, '
').replace(/\|/g, '\\|'); const isH = ri === 0; if (isH) table.header.cells[ci].content = v; @@ -250,6 +259,37 @@ class TableWidget extends WidgetType { // swap to the raw source so the user edits the markdown text. renderInlineMarkdown(textDiv, text); + // Track IME composition so we don't rebuild the cell DOM + // mid-composition — rebuilding would cancel the IME and drop + // any in-progress candidates. + let isComposing = false; + + // Some browsers do not fire `input` reliably when non-text nodes + // (e.g. ) are removed via Backspace inside contentEditable. + // A MutationObserver catches DOM-level changes that `input` misses. + const mo = new win.MutationObserver(() => { + if (isComposing) return; + if (textDiv.classList.contains(RAW)) scheduleLiveSync(); + }); + mo.observe(textDiv, { childList: true, characterData: true, subtree: true }); + cellObservers.push(mo); + + // Observer is suspended while swapping: the render is itself a + // childList mutation, which would otherwise schedule a sync. + const showRendered = (src: string) => { + mo.disconnect(); + textDiv.classList.remove(RAW); + renderInlineMarkdown(textDiv, src); + mo.observe(textDiv, { childList: true, characterData: true, subtree: true }); + }; + + const showRaw = (src: string) => { + mo.disconnect(); + textDiv.textContent = src.replace(/\\\|/g, '|'); + textDiv.classList.add(RAW); + mo.observe(textDiv, { childList: true, characterData: true, subtree: true }); + }; + // Push this cell's current edit-mode text into the table model. // Called on every input so the model stays in sync even if a // rebuild is triggered by an external event (image paste, toolbar @@ -257,6 +297,7 @@ class TableWidget extends WidgetType { // Not trimmed: an edge space the user just typed is real content // and must stay visible while editing (see #15918). const pushToModel = () => { + if (!textDiv.classList.contains(RAW)) return; const v = (textDiv.textContent || '') .replace(/\n/g, '
').replace(/\|/g, '\\|'); if (isHdr) table.header.cells[c].content = v; @@ -277,6 +318,7 @@ class TableWidget extends WidgetType { }; const scheduleLiveSync = () => { + if (!textDiv.classList.contains(RAW)) return; pushToModel(); cancelLiveSync(); const offset = caretOffset(); @@ -330,10 +372,6 @@ class TableWidget extends WidgetType { }, 500); }; - // Track IME composition so we don't rebuild the cell DOM - // mid-composition — rebuilding would cancel the IME and drop - // any in-progress candidates. - let isComposing = false; textDiv.addEventListener('compositionstart', () => { isComposing = true; cancelLiveSync(); @@ -347,14 +385,6 @@ class TableWidget extends WidgetType { if (isComposing) return; scheduleLiveSync(); }; - // Some browsers do not fire `input` reliably when non-text nodes - // (e.g. ) are removed via Backspace inside contentEditable. - // A MutationObserver catches DOM-level changes that `input` misses. - const mo = new win.MutationObserver(() => { - if (isComposing) return; - if (doc.activeElement === textDiv) scheduleLiveSync(); - }); - mo.observe(textDiv, { childList: true, characterData: true, subtree: true }); // Sync CM cursor to this cell so toolbar commands work, and // swap the rendered DOM for the raw markdown source for editing. @@ -367,7 +397,7 @@ class TableWidget extends WidgetType { const src = isHdr ? table.header.cells[c]?.content ?? '' : table.body[r - 1]?.cells[c]?.content ?? ''; - textDiv.textContent = src.replace(/\\\|/g, '|'); + showRaw(src); // Place caret at end so typing appends (matches prior behaviour // where cells started empty of selection). const sel = doc.defaultView!.getSelection(); @@ -401,6 +431,9 @@ class TableWidget extends WidgetType { // context menu action), the old container is detached. // Do nothing — the rebuild already has the latest data. if (!container.isConnected) return; + // Already re-rendered (second blur, or a race with this + // timer) — its textContent is no longer source. + if (!textDiv.classList.contains(RAW)) return; const v = (textDiv.textContent || '').trim(); const orig = isHdr ? table.header.cells[c]?.content @@ -426,8 +459,7 @@ class TableWidget extends WidgetType { const src = isHdr ? table.header.cells[c]?.content ?? '' : table.body[r - 1]?.cells[c]?.content ?? ''; - textDiv.textContent = ''; - renderInlineMarkdown(textDiv, src); + showRendered(src); }, 80); }; @@ -861,9 +893,21 @@ class TableWidget extends WidgetType { container.classList.toggle('cm-tw-mod-link', overLink); }); + // Runs on every rebuild and on teardown. No dispatch here: destroy() + // runs inside CodeMirror's update cycle, where it is not allowed. + (container as TableWidgetContainer)[teardownKey] = () => { + for (const observer of cellObservers) observer.disconnect(); + cellObservers.length = 0; + cancelLiveSync(); + }; + return container; } + public destroy(dom: HTMLElement) { + (dom as TableWidgetContainer)[teardownKey]?.(); + } + public ignoreEvent() { return true; } }