diff --git a/packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.browser.test.ts b/packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.browser.test.ts index c63bc3851..0f2a54f60 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.browser.test.ts +++ b/packages/app/src/features/NoteEditor/RichEditor/__tests__/spec/list.browser.test.ts @@ -1,5 +1,5 @@ import { act } from 'react'; -import { page, userEvent } from 'vitest/browser'; +import { page, userEvent, utils } from 'vitest/browser'; import { renderRichEditorInDOM } from '../utils/renderEditorInDOM'; import { selectContent } from '../utils/utils'; @@ -110,3 +110,174 @@ test('item nesting can be decreased and empty list will be removed', async () => .poll(() => onChanged) .toHaveBeenLastCalledWith('- [ ] 1\n- [ ] 2\n- [ ] 3\n- [ ] 4\n- [ ] 5\n'); }); + +describe('Regressions', () => { + describe('Different lists separated by blank lines remain separate', () => { + [ + { + name: 'regular list and checklist', + expectedLists: 2, + content: [ + '- foo', + '- bar', + '- baz', + '', + '- [ ] 1', + '- [ ] 2', + '- [ ] 3', + ].join('\n'), + }, + { + name: 'regular list and ordered list', + expectedLists: 2, + content: ['- foo', '- bar', '- baz', '', '1. 1', '2. 2', '3. 3'].join( + '\n', + ), + }, + { + name: 'regular list, ordered list, checkbox list', + expectedLists: 3, + content: [ + '- foo', + '- bar', + '- baz', + '', + '1. 1', + '2. 2', + '3. 3', + '', + '- [ ] 1', + '- [ ] 2', + '- [ ] 3', + ].join('\n'), + }, + ].forEach(({ name, content, expectedLists }) => + test(name, async () => { + const { render, getMarkdown, getEditor } = await renderRichEditorInDOM({ + value: content, + }); + + expect( + page.getByRole('list').all(), + 'There are expected number of lists', + ).toHaveLength(expectedLists); + + const rootNode = getEditor().getRootElement(); + const getHTML = () => utils.prettyDOM(rootNode); + + const htmlV1 = getHTML(); + const markdownV1 = getMarkdown(); + + // Cleanup and re-render + await render({ value: '' }); + await expect.poll(() => rootNode).toHaveTextContent(''); + + // Make sure the roundtrip are consistent + await render({ value: markdownV1 }); + expect(getHTML()).toBe(htmlV1); + expect(getMarkdown()).toBe(markdownV1); + }), + ); + }); + + test('3 empty lines splits the lists', async () => { + const editor1 = await renderRichEditorInDOM({ + value: '', + }); + + const editor1Locator = page + .elementLocator(editor1.getEditor().getRootElement()!) + .first(); + + // Create 2 lists + await act(async () => { + await editor1Locator.click(); + }); + await editor1.insert({ type: 'list', data: { type: 'unordered' } }); + await act(async () => { + await userEvent.keyboard('foo'); + await userEvent.keyboard('{Enter}'.repeat(3)); + }); + + await editor1.insert({ type: 'list', data: { type: 'unordered' } }); + await act(async () => { + await userEvent.keyboard('bar'); + }); + + expect(editor1Locator.getByRole('list').all()).toHaveLength(2); + expect(editor1Locator.getByRole('listitem').all()).toHaveLength(2); + + // Create another editor with serialized content + const editor2 = await renderRichEditorInDOM({ + value: editor1.getMarkdown(), + }); + + const editor2Locator = page.elementLocator(editor2.getEditor().getRootElement()!); + expect(editor2Locator.getByRole('list').all()).toHaveLength(2); + expect(editor2Locator.getByRole('listitem').all()).toHaveLength(2); + + // Content must be equal + expect(editor1Locator.element()).not.toBe(editor2Locator.element()); + expect(utils.prettyDOM(editor2Locator)).toBe(utils.prettyDOM(editor1Locator)); + expect(editor2.getMarkdown()).toBe(editor1.getMarkdown()); + }); + + test('nested items stay nested when a regular list is followed by a checklist', async () => { + const value = ` +- foo + - bar +- baz + +- [ ] 1 + - [ ] 2 + - [ ] 3 +- [ ] 4 +- [ ] 5 +`.trim(); + + await renderRichEditorInDOM({ value }); + + const editor = page.getByRole('textbox'); + expect(editor.element().querySelectorAll(':scope > ul')).toHaveLength(2); + expect(editor.getByRole('list').all()).toHaveLength(5); + expect(editor.getByRole('checkbox')).toHaveLength(5); + }); + + test('blank-separated sibling items of the same kind remain one list', async () => { + await renderRichEditorInDOM({ value: '- foo\n\n- bar' }); + + expect(page.getByRole('list').all()).toHaveLength(1); + }); + + test('list kind changes split items even without a blank line', async () => { + await renderRichEditorInDOM({ + value: '- regular\n- [ ] task\n- regular again', + }); + + expect(page.getByRole('list').all()).toHaveLength(3); + expect(page.getByRole('checkbox')).toHaveLength(1); + }); + + test('checked and unchecked task items remain in the same checklist', async () => { + await renderRichEditorInDOM({ + value: '- [ ] first\n\n- [x] second\n - [ ] third', + }); + + expect(page.getByRole('list').all()).toHaveLength(1); + expect(page.getByRole('checkbox')).toHaveLength(3); + }); + + test('ordered list numbering is retained after a task item splits the list', async () => { + await renderRichEditorInDOM({ + value: '3. regular\n4. [ ] task\n5. regular again', + }); + + const editor = page.getByRole('textbox'); + const orderedLists = editor.element().querySelectorAll(':scope > ol'); + + expect(orderedLists).toHaveLength(2); + expect(orderedLists[0].getAttribute('start')).toBe('3'); + expect(orderedLists[1].getAttribute('start')).toBe('5'); + expect(editor.getByRole('checkbox')).toHaveLength(1); + }); +}); diff --git a/packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderEditorInDOM.tsx b/packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderEditorInDOM.tsx index 36c2f7842..9619aea3b 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderEditorInDOM.tsx +++ b/packages/app/src/features/NoteEditor/RichEditor/__tests__/utils/renderEditorInDOM.tsx @@ -22,6 +22,8 @@ import { RichEditorContentProps } from '@features/NoteEditor/RichEditor/RichEdit import { ThemeProvider } from '@features/ThemeProvider'; import { createTestStore } from '@tests/utils/redux'; +import { $convertToMarkdownString } from '../../plugins/Markdown/markdownParser'; + // Mock useUrlOpener to avoid importing monaco-editor-core in tests, // which causes Vite module resolution errors during test setup vi.mock('@hooks/useUrlOpener', () => ({ @@ -73,6 +75,8 @@ export type RichEditorTestAPI = { format: (format: TextFormat) => Promise; command: (format: CommandsPayload) => Promise; getEditor(): LexicalEditor; + getMarkdown(): string; + render: (props: RichEditorContentProps) => Promise; }; export const renderRichEditorInDOM = async ({ @@ -114,7 +118,14 @@ export const renderRichEditorInDOM = async ({ document.body.appendChild(container); const root = createRoot(container); - act(() => root.render(renderEditor({ ...props, editorRef }))); + + const render = (props: RichEditorContentProps) => { + return act(() => + root.render(renderEditor({ ...props, editorRef })), + ) as unknown as Promise; + }; + + render(props); const destroy = () => { act(() => { @@ -163,5 +174,14 @@ export const renderRichEditorInDOM = async ({ if (!editor) throw new Error('Error instance is not set'); return editor; }, + + getMarkdown() { + const editor = editorRef.current; + if (!editor) throw new Error('Error instance is not set'); + + return editor.read(() => $convertToMarkdownString()); + }, + + render, }; }; diff --git a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/list-boundaries.browser.test.ts b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/list-boundaries.browser.test.ts new file mode 100644 index 000000000..c0b660408 --- /dev/null +++ b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/__tests__/list-boundaries.browser.test.ts @@ -0,0 +1,81 @@ +import { page } from 'vitest/browser'; +import { renderRichEditorInDOM } from '@features/NoteEditor/RichEditor/__tests__/utils/renderEditorInDOM'; + +test('same-kind lists separated by a large gap retain their boundaries and empty items', async () => { + const source = [ + '- first', + ...Array(10).fill('-'), + ...Array(100).fill(''), + '* second', + ...Array(10).fill('*'), + ].join('\n'); + const { getEditor, getMarkdown, render } = await renderRichEditorInDOM({ + value: source, + }); + + const editor = page.getByRole('textbox'); + const getListItemCounts = () => + Array.from( + getEditor().getRootElement()?.querySelectorAll(':scope > ul') ?? [], + ).map((list) => list.querySelectorAll(':scope > li').length); + const getGapParagraphCount = () => + getEditor().getRootElement()?.querySelectorAll(':scope > p').length ?? 0; + + expect(getListItemCounts()).toEqual([11, 11]); + expect(editor.getByRole('listitem')).toHaveLength(22); + expect(getGapParagraphCount()).toBe(98); + expect(editor.getByRole('listitem').nth(1)).toHaveTextContent(''); + expect(editor.getByRole('listitem').nth(12)).toHaveTextContent(''); + + const serialized = getMarkdown(); + expect(serialized).toContain('- first'); + expect(serialized).toContain('* second'); + + await render({ value: serialized }); + + expect(getListItemCounts()).toEqual([11, 11]); + expect(page.getByRole('listitem')).toHaveLength(22); + expect(getGapParagraphCount()).toBe(98); + expect(getMarkdown()).toBe(serialized); +}); + +test('a single empty editor paragraph keeps same-kind lists separate after round-trip', async () => { + const { getEditor, getMarkdown, render } = await renderRichEditorInDOM({ + value: '- first\n\n\n\n* second', + }); + const root = () => getEditor().getRootElement()!; + const getTopLevelLists = () => root().querySelectorAll(':scope > ul'); + const getTopLevelParagraphs = () => root().querySelectorAll(':scope > p'); + + expect(getTopLevelLists()).toHaveLength(2); + expect(getTopLevelParagraphs()).toHaveLength(1); + + const serialized = getMarkdown(); + expect(serialized).toContain('- first'); + expect(serialized).toContain('* second'); + + await render({ value: serialized }); + + expect(getTopLevelLists()).toHaveLength(2); + expect(getTopLevelParagraphs()).toHaveLength(1); + expect(getMarkdown()).toBe(serialized); +}); + +test('a list with many empty items remains one list', async () => { + const source = ['- first', ...Array(100).fill('-'), '- last'].join('\n'); + const { getEditor, getMarkdown, render } = await renderRichEditorInDOM({ + value: source, + }); + + const getListItemCount = () => + getEditor().getRootElement()?.querySelectorAll(':scope > ul > li').length; + expect(getEditor().getRootElement()?.querySelectorAll(':scope > ul')).toHaveLength(1); + expect(getListItemCount()).toBe(102); + + const serialized = getMarkdown(); + await render({ value: serialized }); + + expect(getEditor().getRootElement()?.querySelectorAll(':scope > ul')).toHaveLength(1); + expect(getListItemCount()).toBe(102); + expect(getMarkdown()).toBe(serialized); +}); diff --git a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts index 81568997a..e42d9ac7b 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts +++ b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/markdownParser.ts @@ -36,13 +36,52 @@ import { $createRawNode } from './nodes/RawNode'; import { liftFormattingNodes } from './remark/remarkLiftFormatting'; import { fillGapsWithParagraphs } from './remark/remarkPreserveBlankLines'; +const emittedListMarkers = new WeakMap>(); + export const markdownProcessor = unified() .use(remarkParse) .use(remarkGfm) .use(remarkStringify, { bullet: '-', + bulletOther: '*', + bulletOrdered: '.', listItemIndent: 'one', handlers: { + list(node, parent, state, info) { + let listMarkers = emittedListMarkers.get(state); + if (!listMarkers) { + listMarkers = new WeakMap(); + emittedListMarkers.set(state, listMarkers); + } + + if (parent) { + const currentIndex = state.indexStack[state.indexStack.length - 1]; + + for (let i = (currentIndex ?? -1) - 1; i >= 0; i--) { + const sibling = parent.children[i]; + + if ( + sibling.type === 'paragraph' && + sibling.children.length === 0 + ) { + continue; + } + + if (sibling.type === 'list' && sibling.ordered === node.ordered) { + const previousMarker = listMarkers.get(sibling); + if (previousMarker) state.bulletLastUsed = previousMarker; + } + + break; + } + } + + const markdown = defaultHandlers.list(node, parent, state, info); + const marker = state.bulletLastUsed; + if (marker) listMarkers.set(node, marker); + + return markdown; + }, paragraph(node, parent, state, info) { // Empty line will be added after that node anyway // So empty paragraph converts to 1 empty line @@ -153,21 +192,32 @@ export const $convertFromMarkdownString = (rawMarkdown: string) => { return [heading]; } case 'list': { - let listType: ListType = 'bullet'; - if ( - node.children.some( - (item) => item.checked !== undefined && item.checked !== null, - ) - ) { - listType = 'check'; - } else if (node.ordered || typeof node.start === 'number') { - listType = 'number'; + const defaultListType: ListType = + node.ordered || typeof node.start === 'number' ? 'number' : 'bullet'; + const convertedLists: LexicalNode[] = []; + + let currentList: ReturnType | undefined; + let currentListType: ListType | undefined; + for (let i = 0; i < node.children.length; i++) { + const item = node.children[i]; + const itemListType: ListType = + item.checked !== undefined && item.checked !== null + ? 'check' + : defaultListType; + + if (!currentList || currentListType !== itemListType) { + currentListType = itemListType; + currentList = $createListNode( + itemListType, + itemListType === 'number' ? (node.start ?? 1) + i : 1, + ); + convertedLists.push(currentList); + } + + currentList.append(...convertToMarkdownNode(item)); } - const list = $createListNode(listType); - list.append(...convertToMarkdownNodes(node.children)); - - return [list]; + return convertedLists; } case 'listItem': { const listItem = $createListItemNode(node.checked ?? undefined); diff --git a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.test.ts b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.test.ts index 2465ab56c..441c686a2 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.test.ts +++ b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.test.ts @@ -1,3 +1,4 @@ +import { List } from 'mdast'; import remarkParse from 'remark-parse'; import { unified } from 'unified'; @@ -29,6 +30,18 @@ test('Must add 8 paragraphs for gap in 10 lines', () => { expect(fillGapsWithParagraphs(tree).children).toHaveLength(2 + 8); }); +test('Must not insert paragraphs between list items for large source gaps', () => { + const tree = markdownProcessor.parse('- First\n' + '\n'.repeat(10) + '- Second'); + + const list = fillGapsWithParagraphs(tree).children[0] as List; + expect(list.type).toBe('list'); + + expect(list.children).toHaveLength(2); + expect(list.children).toStrictEqual( + Array(2).fill(expect.objectContaining({ type: 'listItem' })), + ); +}); + // TODO: implement lines preservation test.fails('Preserve lines for document with no text', () => { const tree = markdownProcessor.parse('\n'.repeat(10)); diff --git a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts index c6ddd8c0f..4b0be8e0a 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts +++ b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/remark/remarkPreserveBlankLines.ts @@ -21,6 +21,10 @@ export const fillGapsWithParagraphs = (tree: Root) => { // Skip already handled nodes if (skipNodes.has(node)) return SKIP; + // A list may only contain list items. Preserve item-to-item source gaps + // as list looseness, rather than inserting paragraphs that become items. + if (node.type === 'list') return CONTINUE; + const newChildren: RootContent[] = []; for (let i = 0; i < node.children.length; i++) { const current = node.children[i]; diff --git a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/serializeMarkdownTree.test.ts b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/serializeMarkdownTree.test.ts index 387483c03..d09da488f 100644 --- a/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/serializeMarkdownTree.test.ts +++ b/packages/app/src/features/NoteEditor/RichEditor/plugins/Markdown/serializeMarkdownTree.test.ts @@ -1,7 +1,25 @@ import { Paragraph } from 'mdast'; import { u } from 'unist-builder'; -import { serializeMarkdownTree } from './markdownParser'; +import { parseMarkdownToAST, serializeMarkdownTree } from './markdownParser'; + +const createBulletList = (value: string) => + u('list', { + ordered: false, + start: null, + spread: false, + children: [ + u('listItem', { + checked: null, + spread: false, + children: [ + u('paragraph', { + children: [u('text', { value })], + }), + ], + }), + ], + }); describe('AST serialization', () => { test('2 paragraphs have 1 empty line', () => { @@ -47,6 +65,69 @@ describe('AST serialization', () => { ).toBe(`Hello${'\n'.repeat(5)}\nWorld\n`); }); + test('Same-kind lists separated by empty paragraphs use distinct markers', () => { + const output = serializeMarkdownTree({ + type: 'root', + children: [ + createBulletList('First'), + u('paragraph', { children: [] }), + createBulletList('Second'), + u('paragraph', { children: [] }), + u('paragraph', { children: [] }), + createBulletList('Third'), + ], + }); + + const listItemMarkers = output + .split('\n') + .filter( + (line) => + line.endsWith('First') || + line.endsWith('Second') || + line.endsWith('Third'), + ) + .map((line) => line[0]); + + expect(listItemMarkers).toEqual(['-', '*', '-']); + expect( + parseMarkdownToAST(output).children.filter((node) => node.type === 'list'), + ).toHaveLength(3); + }); + + test('Ordered lists separated by empty paragraphs use distinct delimiters', () => { + const createOrderedList = (start: number, value: string) => + u('list', { + ordered: true, + start, + spread: false, + children: [ + u('listItem', { + checked: null, + spread: false, + children: [ + u('paragraph', { + children: [u('text', { value })], + }), + ], + }), + ], + }); + const output = serializeMarkdownTree({ + type: 'root', + children: [ + createOrderedList(3, 'First'), + u('paragraph', { children: [] }), + createOrderedList(4, 'Second'), + ], + }); + + expect(output).toContain('3. First'); + expect(output).toContain('4) Second'); + expect( + parseMarkdownToAST(output).children.filter((node) => node.type === 'list'), + ).toHaveLength(2); + }); + test('Quote with 3 empty paragraphs must contain 5 empty lines', () => { expect( serializeMarkdownTree({