diff --git a/packages/super-editor/src/extensions/paragraph/paragraph.js b/packages/super-editor/src/extensions/paragraph/paragraph.js index a910cc2b2..d47099fa5 100644 --- a/packages/super-editor/src/extensions/paragraph/paragraph.js +++ b/packages/super-editor/src/extensions/paragraph/paragraph.js @@ -15,6 +15,48 @@ import { createDropcapPlugin } from './dropcapPlugin.js'; import { shouldSkipNodeView } from '../../utils/headless-helpers.js'; import { parseAttrs } from './helpers/parseAttrs.js'; +/** + * Whether a paragraph's only inline leaf content is break placeholders + * (lineBreak / hardBreak), with no visible text or other embedded objects. + * + * Distinct from `isVisuallyEmptyParagraph`, which returns false when any + * break node is present. This predicate catches the complementary case: + * paragraphs that *look* empty to the user but technically contain a break. + * + * Context: after splitting a list item that ends with a trailing `w:br`, + * the new paragraph inherits that break. In WebKit the resulting DOM shape + * causes native text insertion to land in the list-marker element + * (`contenteditable="false"`) instead of the content area — and + * `ParagraphNodeView.ignoreMutation` silently drops it. Detecting + * this shape lets the `beforeinput` handler insert via ProseMirror + * transaction instead of relying on native DOM insertion. + * + * @param {import('prosemirror-model').Node} node + * @returns {boolean} + */ +export function hasOnlyBreakContent(node) { + if (!node || node.type.name !== 'paragraph') return false; + + const text = (node.textContent || '').replace(/\u200b/g, '').trim(); + if (text.length > 0) return false; + + let hasBreak = false; + let hasOtherContent = false; + + node.descendants((child) => { + if (!child.isInline || !child.isLeaf) return true; + + if (child.type.name === 'lineBreak' || child.type.name === 'hardBreak') { + hasBreak = true; + } else { + hasOtherContent = true; + } + return !hasOtherContent; + }); + + return hasBreak && !hasOtherContent; +} + /** * Input rule regex that matches a bullet list marker (-, +, or *) * @private @@ -294,7 +336,7 @@ export const Paragraph = OxmlNode.create({ addPmPlugins() { const dropcapPlugin = createDropcapPlugin(this.editor); const numberingPlugin = createNumberingPlugin(this.editor); - const listEmptyInputPlugin = new Plugin({ + const listInputFallbackPlugin = new Plugin({ props: { handleDOMEvents: { beforeinput: (view, event) => { @@ -307,11 +349,27 @@ export const Paragraph = OxmlNode.create({ const { selection } = state; if (!selection.empty) return false; - const $from = selection.$from; - const paragraph = $from.parent; - if (!paragraph || paragraph.type.name !== 'paragraph') return false; - if (!isList(paragraph)) return false; - if (!isVisuallyEmptyParagraph(paragraph)) return false; + // Find the enclosing paragraph directly from the resolved position. + // We avoid `findParentNode(isList)` here because `isList` depends on + // `getResolvedParagraphProperties`, a WeakMap cache keyed by node + // identity. After the numbering plugin's `appendTransaction` sets + // `listRendering`, the paragraph node object is replaced, leaving + // the new node uncached — causing `isList` to return false. + const { $from } = selection; + let paragraph = null; + for (let d = $from.depth; d >= 0; d--) { + const node = $from.node(d); + if (node.type.name === 'paragraph') { + paragraph = node; + break; + } + } + if (!paragraph) return false; + + const isListParagraph = + paragraph.attrs?.paragraphProperties?.numberingProperties && paragraph.attrs?.listRendering; + if (!isListParagraph) return false; + if (!isVisuallyEmptyParagraph(paragraph) && !hasOnlyBreakContent(paragraph)) return false; const tr = state.tr.insertText(event.data); view.dispatch(tr); @@ -321,6 +379,6 @@ export const Paragraph = OxmlNode.create({ }, }, }); - return [dropcapPlugin, numberingPlugin, listEmptyInputPlugin, createLeadingCaretPlugin()]; + return [dropcapPlugin, numberingPlugin, listInputFallbackPlugin, createLeadingCaretPlugin()]; }, }); diff --git a/packages/super-editor/src/extensions/paragraph/paragraph.test.js b/packages/super-editor/src/extensions/paragraph/paragraph.test.js index 3e27a216c..ad93af4fd 100644 --- a/packages/super-editor/src/extensions/paragraph/paragraph.test.js +++ b/packages/super-editor/src/extensions/paragraph/paragraph.test.js @@ -2,6 +2,7 @@ import { beforeEach, describe, expect, it, vi } from 'vitest'; import { TextSelection } from 'prosemirror-state'; import { initTestEditor, loadTestDataForEditorTests } from '../../tests/helpers/helpers.js'; import { calculateResolvedParagraphProperties } from './resolvedPropertiesCache.js'; +import { hasOnlyBreakContent } from './paragraph.js'; describe('Paragraph Node', () => { let docx, media, mediaFiles, fonts, editor; @@ -124,4 +125,165 @@ describe('Paragraph Node', () => { expect(editor.state.doc.textContent).toBe('t'); }); + + describe('hasOnlyBreakContent', () => { + it('returns true for a list paragraph containing only a lineBreak', () => { + let paragraphPos = null; + editor.state.doc.descendants((node, pos) => { + if (node.type.name === 'paragraph' && paragraphPos == null) { + paragraphPos = pos; + return false; + } + return true; + }); + + const lineBreakNode = editor.schema.nodes.lineBreak.create(); + const tr = editor.state.tr.insert(paragraphPos + 1, lineBreakNode); + editor.view.dispatch(tr); + + const paragraph = editor.state.doc.nodeAt(paragraphPos); + expect(hasOnlyBreakContent(paragraph)).toBe(true); + }); + + it('returns false for a paragraph with visible text', () => { + editor.commands.insertContent('visible text'); + const paragraph = editor.state.doc.content.content[0]; + expect(hasOnlyBreakContent(paragraph)).toBe(false); + }); + + it('returns false for an empty paragraph with no content at all', () => { + const paragraph = editor.state.doc.content.content[0]; + expect(hasOnlyBreakContent(paragraph)).toBe(false); + }); + + it('returns false for null or non-paragraph nodes', () => { + expect(hasOnlyBreakContent(null)).toBe(false); + expect(hasOnlyBreakContent(undefined)).toBe(false); + + const runNode = editor.schema.nodes.run.create(); + expect(hasOnlyBreakContent(runNode)).toBe(false); + }); + }); + + it('handles beforeinput in a list paragraph with only a lineBreak (SD-1707)', () => { + let paragraphPos = null; + let paragraphNode = null; + editor.state.doc.descendants((node, pos) => { + if (node.type.name === 'paragraph' && paragraphPos == null) { + paragraphPos = pos; + paragraphNode = node; + return false; + } + return true; + }); + + const numberingProperties = { numId: 1, ilvl: 0 }; + const listRendering = { + markerText: '1.', + suffix: 'tab', + justification: 'left', + path: [1], + numberingType: 'decimal', + }; + + // Make the paragraph a list item + let tr = editor.state.tr.setNodeMarkup(paragraphPos, null, { + ...paragraphNode.attrs, + paragraphProperties: { + ...(paragraphNode.attrs.paragraphProperties || {}), + numberingProperties, + }, + numberingProperties, + listRendering, + }); + editor.view.dispatch(tr); + + // Insert a lineBreak so the paragraph has only break content + const lineBreakNode = editor.schema.nodes.lineBreak.create(); + tr = editor.state.tr.insert(paragraphPos + 1, lineBreakNode); + editor.view.dispatch(tr); + + const updatedParagraph = editor.state.doc.nodeAt(paragraphPos); + calculateResolvedParagraphProperties(editor, updatedParagraph, editor.state.doc.resolve(paragraphPos)); + + // Place cursor inside the paragraph + tr = editor.state.tr.setSelection(TextSelection.create(editor.state.doc, paragraphPos + 1)); + editor.view.dispatch(tr); + + const beforeInputEvent = new InputEvent('beforeinput', { + data: 'a', + inputType: 'insertText', + bubbles: true, + cancelable: true, + }); + editor.view.dom.dispatchEvent(beforeInputEvent); + + expect(editor.state.doc.textContent).toBe('a'); + }); + + it('does NOT intercept beforeinput for a list paragraph with visible text', () => { + let paragraphPos = null; + let paragraphNode = null; + editor.state.doc.descendants((node, pos) => { + if (node.type.name === 'paragraph' && paragraphPos == null) { + paragraphPos = pos; + paragraphNode = node; + return false; + } + return true; + }); + + const numberingProperties = { numId: 1, ilvl: 0 }; + const listRendering = { + markerText: '1.', + suffix: 'tab', + justification: 'left', + path: [1], + numberingType: 'decimal', + }; + + // Insert text first, then make it a list item + editor.commands.insertContent('hello'); + + paragraphPos = null; + paragraphNode = null; + editor.state.doc.descendants((node, pos) => { + if (node.type.name === 'paragraph' && paragraphPos == null) { + paragraphPos = pos; + paragraphNode = node; + return false; + } + return true; + }); + + let tr = editor.state.tr.setNodeMarkup(paragraphPos, null, { + ...paragraphNode.attrs, + paragraphProperties: { + ...(paragraphNode.attrs.paragraphProperties || {}), + numberingProperties, + }, + numberingProperties, + listRendering, + }); + editor.view.dispatch(tr); + + const updatedParagraph = editor.state.doc.nodeAt(paragraphPos); + calculateResolvedParagraphProperties(editor, updatedParagraph, editor.state.doc.resolve(paragraphPos)); + + // Place cursor at the end of the text + const endPos = paragraphPos + updatedParagraph.nodeSize - 1; + tr = editor.state.tr.setSelection(TextSelection.create(editor.state.doc, endPos)); + editor.view.dispatch(tr); + + const beforeInputEvent = new InputEvent('beforeinput', { + data: 'x', + inputType: 'insertText', + bubbles: true, + cancelable: true, + }); + + // The handler should NOT intercept because the paragraph has visible text + const prevented = !editor.view.dom.dispatchEvent(beforeInputEvent); + expect(prevented).toBe(false); + }); }); diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.js index 11e13274b..a7a2a9e41 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.js @@ -47,7 +47,17 @@ export const findTrackedMarkBetween = ({ const resolved = doc.resolve(pos); const before = resolved.nodeBefore; - if (before?.type?.name === 'run') { + const after = resolved.nodeAfter; + + // Check if nodeBefore is a text node directly (not wrapped in a run). + // This handles cases where text is inserted outside of run nodes, + // such as in Google Docs exports with paragraph > lineBreak structure. + // Firefox inserts text directly as paragraph children, while Chrome + // tends to use run wrappers, so we need to handle both cases. + if (before?.type?.name === 'text') { + const beforeStart = Math.max(pos - before.nodeSize, 0); + tryMatch(before, beforeStart); + } else if (before?.type?.name === 'run') { const beforeStart = Math.max(pos - before.nodeSize, 0); const node = before.content?.content?.[0]; if (node?.type?.name === 'text') { @@ -55,8 +65,10 @@ export const findTrackedMarkBetween = ({ } } - const after = resolved.nodeAfter; - if (after?.type?.name === 'run') { + // Check if nodeAfter is a text node directly (not wrapped in a run) + if (after?.type?.name === 'text') { + tryMatch(after, pos); + } else if (after?.type?.name === 'run') { const node = after.content?.content?.[0]; if (node?.type?.name === 'text') { tryMatch(node, pos); diff --git a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.test.js b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.test.js index 60069e985..c321ea669 100644 --- a/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.test.js +++ b/packages/super-editor/src/extensions/track-changes/trackChangesHelpers/findTrackedMarkBetween.test.js @@ -1,6 +1,6 @@ import { beforeEach, afterEach, describe, expect, it } from 'vitest'; import { EditorState } from 'prosemirror-state'; -import { TrackDeleteMarkName } from '../constants.js'; +import { TrackDeleteMarkName, TrackInsertMarkName } from '../constants.js'; import { findTrackedMarkBetween } from './findTrackedMarkBetween.js'; import { initTestEditor } from '@tests/helpers/helpers.js'; @@ -114,4 +114,99 @@ describe('findTrackedMarkBetween', () => { }), ); }); + + it('finds trackInsert mark on text node directly (not wrapped in run) at start position', () => { + // This tests the fix for SD-1707: Google Docs exports can have text nodes + // directly as children of paragraph, not wrapped in run nodes. + const insertMark = schema.marks[TrackInsertMarkName].create({ + id: 'abc12345-1234-1234-1234-123456789abc', + author: user.name, + authorEmail: user.email, + date, + }); + // Create: paragraph > text("1" with trackInsert) + run > lineBreak + // This mimics the structure after typing in a Google Docs exported empty list item + const textNode = schema.text('1', [insertMark]); + const lineBreak = schema.nodes.lineBreak.create(); + const run = schema.nodes.run.create({}, lineBreak); + const paragraph = schema.nodes.paragraph.create({}, [textNode, run]); + const doc = schema.nodes.doc.create({}, paragraph); + + const state = createState(doc); + const tr = state.tr; + + // Search from position after the text node (where the run starts) + // This simulates what happens when inserting the 2nd character + const found = findTrackedMarkBetween({ + tr, + from: 3, // Position after text node "1" + to: 4, + markName: TrackInsertMarkName, + attrs: { authorEmail: user.email }, + }); + + expect(found).not.toBeNull(); + expect(found.mark.attrs.id).toBe('abc12345-1234-1234-1234-123456789abc'); + }); + + it('finds trackInsert mark on text node directly when nodeBefore is a text node', () => { + const insertMark = schema.marks[TrackInsertMarkName].create({ + id: 'def67890-5678-5678-5678-567890123def', + author: user.name, + authorEmail: user.email, + date, + }); + // Create: paragraph > text("ab" with trackInsert) + const textNode = schema.text('ab', [insertMark]); + const paragraph = schema.nodes.paragraph.create({}, [textNode]); + const doc = schema.nodes.doc.create({}, paragraph); + + const state = createState(doc); + const tr = state.tr; + + // Search at position right after the text - this is where new text would be inserted + // nodeBefore at pos 3 should be the text node "ab" + const found = findTrackedMarkBetween({ + tr, + from: 3, + to: 4, + markName: TrackInsertMarkName, + attrs: { authorEmail: user.email }, + }); + + expect(found).not.toBeNull(); + expect(found.mark.attrs.id).toBe('def67890-5678-5678-5678-567890123def'); + }); + + it('finds trackInsert mark on text node directly when nodeAfter is a text node', () => { + // This tests the nodeAfter branch of the fix - when the text node comes + // after the search position (e.g., inserting at paragraph start) + const insertMark = schema.marks[TrackInsertMarkName].create({ + id: 'ghi01234-9012-9012-9012-901234567ghi', + author: user.name, + authorEmail: user.email, + date, + }); + // Create: paragraph > text("xy" with trackInsert) + // Search at position 2 (start of paragraph content) where text node is nodeAfter + const textNode = schema.text('xy', [insertMark]); + const paragraph = schema.nodes.paragraph.create({}, [textNode]); + const doc = schema.nodes.doc.create({}, paragraph); + + const state = createState(doc); + const tr = state.tr; + + // Position 2 is at the start of paragraph content (after doc open + paragraph open) + // At this position, nodeAfter should be the text node "xy" + const found = findTrackedMarkBetween({ + tr, + from: 2, + to: 3, + markName: TrackInsertMarkName, + attrs: { authorEmail: user.email }, + }); + + expect(found).not.toBeNull(); + expect(found.mark.attrs.id).toBe('ghi01234-9012-9012-9012-901234567ghi'); + }); }); diff --git a/packages/super-editor/src/tests/data/sd-1707-list-enter-track-changes-with-br.docx b/packages/super-editor/src/tests/data/sd-1707-list-enter-track-changes-with-br.docx new file mode 100644 index 000000000..aa8a6d15b Binary files /dev/null and b/packages/super-editor/src/tests/data/sd-1707-list-enter-track-changes-with-br.docx differ diff --git a/tests/behavior/tests/comments/list-enter-with-br-track-change-grouping.spec.ts b/tests/behavior/tests/comments/list-enter-with-br-track-change-grouping.spec.ts new file mode 100644 index 000000000..dd675f752 --- /dev/null +++ b/tests/behavior/tests/comments/list-enter-with-br-track-change-grouping.spec.ts @@ -0,0 +1,38 @@ +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +import { test, expect } from '../../fixtures/superdoc.js'; +import { assertDocumentApiReady, getDocumentText, listTrackChanges } from '../../helpers/document-api.js'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const DOC_PATH = path.resolve( + __dirname, + '../../../../packages/super-editor/src/tests/data/sd-1707-list-enter-track-changes-with-br.docx', +); + +test.use({ config: { toolbar: 'full', comments: 'on', trackChanges: true } }); + +test('SD-1707 list item with trailing break keeps typed text in one tracked change', async ({ superdoc }) => { + await superdoc.loadDocument(DOC_PATH); + await superdoc.waitForStable(); + await assertDocumentApiReady(superdoc.page); + + await superdoc.setDocumentMode('suggesting'); + await superdoc.waitForStable(); + + const listText = 'Body copy for repro'; + const listTextPos = await superdoc.findTextPos(listText); + await superdoc.setTextSelection(listTextPos + listText.length); + await superdoc.waitForStable(); + + await superdoc.newLine(); + await superdoc.waitForStable(); + + await superdoc.type('abcdef'); + await superdoc.waitForStable(); + + await expect.poll(() => getDocumentText(superdoc.page)).toContain('abcdef'); + await expect.poll(async () => (await listTrackChanges(superdoc.page, { type: 'insert' })).total).toBe(1); + + const inserts = await listTrackChanges(superdoc.page, { type: 'insert' }); + expect(inserts.changes?.[0]?.excerpt).toBe('abcdef'); +});