From 24f5af0b6f4f71a747c448dc8d6ee0235bc4d8a2 Mon Sep 17 00:00:00 2001 From: Jiuqing Song Date: Fri, 22 Nov 2024 11:05:44 -0800 Subject: [PATCH] Fix #2880 optimize() causes segment cache to be wrong when merging (#2889) --- .../formatTextSegmentBeforeSelectionMarker.ts | 5 ++ ...matTextSegmentBeforeSelectionMarkerTest.ts | 7 +- .../lib/corePlugin/cache/domIndexerImpl.ts | 20 +++++- .../corePlugin/cache/domIndexerImplTest.ts | 72 +++++++++++++++++++ .../handlers/handleSegmentDecorator.ts | 5 +- .../test/modelToDom/handlers/handleBrTest.ts | 3 +- .../handlers/handleSegmentDecoratorTest.ts | 49 ++++--------- .../utils/handleSegmentCommonTest.ts | 3 +- 8 files changed, 115 insertions(+), 49 deletions(-) diff --git a/packages/roosterjs-content-model-api/lib/publicApi/utils/formatTextSegmentBeforeSelectionMarker.ts b/packages/roosterjs-content-model-api/lib/publicApi/utils/formatTextSegmentBeforeSelectionMarker.ts index cca5bceb881..b0ca265b4ed 100644 --- a/packages/roosterjs-content-model-api/lib/publicApi/utils/formatTextSegmentBeforeSelectionMarker.ts +++ b/packages/roosterjs-content-model-api/lib/publicApi/utils/formatTextSegmentBeforeSelectionMarker.ts @@ -48,6 +48,11 @@ export function formatTextSegmentBeforeSelectionMarker( if (previousSegment && previousSegment.segmentType === 'Text') { result = true; + + // Preserve pending format if any when format text segment, so if there is pending format (e.g. from paste) + // and some auto action happens after paste, the pending format will still take effect + context.newPendingFormat = 'preserve'; + rewrite = callback( model, previousSegment, diff --git a/packages/roosterjs-content-model-api/test/publicApi/utils/formatTextSegmentBeforeSelectionMarkerTest.ts b/packages/roosterjs-content-model-api/test/publicApi/utils/formatTextSegmentBeforeSelectionMarkerTest.ts index 5c58f674525..7dd4b2f8c2e 100644 --- a/packages/roosterjs-content-model-api/test/publicApi/utils/formatTextSegmentBeforeSelectionMarkerTest.ts +++ b/packages/roosterjs-content-model-api/test/publicApi/utils/formatTextSegmentBeforeSelectionMarkerTest.ts @@ -23,13 +23,16 @@ describe('formatTextSegmentBeforeSelectionMarker', () => { const formatWithContentModelSpy = jasmine .createSpy('formatWithContentModel') .and.callFake((callback, options) => { - const result = callback(input, { + const context: FormatContentModelContext = { newEntities: [], deletedEntities: [], newImages: [], canUndoByBackspace: true, - }); + }; + const result = callback(input, context); + expect(result).toBe(expectedResult); + expect(context.newPendingFormat).toBe(expectedResult ? 'preserve' : undefined); }); formatTextSegmentBeforeSelectionMarker( diff --git a/packages/roosterjs-content-model-core/lib/corePlugin/cache/domIndexerImpl.ts b/packages/roosterjs-content-model-core/lib/corePlugin/cache/domIndexerImpl.ts index e51c7f888c9..2d2b80723ab 100644 --- a/packages/roosterjs-content-model-core/lib/corePlugin/cache/domIndexerImpl.ts +++ b/packages/roosterjs-content-model-core/lib/corePlugin/cache/domIndexerImpl.ts @@ -592,14 +592,14 @@ export class DomIndexerImpl implements DomIndexer { const { previousSibling, nextSibling } = node; if ( - (segmentItem = getIndexedSegmentItem(previousSibling)) && + (segmentItem = getIndexedSegmentItem(getLastLeaf(previousSibling))) && (existingSegment = segmentItem.segments[segmentItem.segments.length - 1]) && (index = segmentItem.paragraph.segments.indexOf(existingSegment)) >= 0 ) { // When we can find indexed segment before current one, use it as the insert index this.indexNode(segmentItem.paragraph, index + 1, node, existingSegment.format); } else if ( - (segmentItem = getIndexedSegmentItem(nextSibling)) && + (segmentItem = getIndexedSegmentItem(getFirstLeaf(nextSibling))) && (existingSegment = segmentItem.segments[0]) && (index = segmentItem.paragraph.segments.indexOf(existingSegment)) >= 0 ) { @@ -691,3 +691,19 @@ export class DomIndexerImpl implements DomIndexer { this.onSegment(textNode, paragraph, [text]); } } + +function getLastLeaf(node: Node | null): Node | null { + while (node?.lastChild) { + node = node.lastChild; + } + + return node; +} + +function getFirstLeaf(node: Node | null): Node | null { + while (node?.firstChild) { + node = node.firstChild; + } + + return node; +} diff --git a/packages/roosterjs-content-model-core/test/corePlugin/cache/domIndexerImplTest.ts b/packages/roosterjs-content-model-core/test/corePlugin/cache/domIndexerImplTest.ts index 478b8db5fde..4937f5205c8 100644 --- a/packages/roosterjs-content-model-core/test/corePlugin/cache/domIndexerImplTest.ts +++ b/packages/roosterjs-content-model-core/test/corePlugin/cache/domIndexerImplTest.ts @@ -17,6 +17,7 @@ import { DOMSelection, } from 'roosterjs-content-model-types'; import { + addLink, createBr, createContentModelDocument, createEntity, @@ -1301,6 +1302,77 @@ describe('domIndexerImpl.reconcileChildList', () => { segments: [segment], }); }); + + it('Added Text after link that contains image and text', () => { + const domIndexer = new DomIndexerImpl(true); + const a = document.createElement('a'); + const img = document.createElement('img'); + const text = document.createTextNode('test'); + const newText = document.createTextNode('a'); + const div = document.createElement('div'); + + a.appendChild(img); + a.appendChild(text); + div.appendChild(a); + div.appendChild(newText); + + const paragraph = createParagraph(); + const segmentImg = createImage('src'); + const segmentText = createText('test'); + + addLink(segmentImg, { + format: { href: 'test' }, + dataset: {}, + }); + addLink(segmentText, { + format: { href: 'test' }, + dataset: {}, + }); + + paragraph.segments.push(segmentImg, segmentText); + + ((img as Node) as IndexedSegmentNode).__roosterjsContentModel = { + paragraph: paragraph, + segments: [segmentImg], + }; + ((text as Node) as IndexedSegmentNode).__roosterjsContentModel = { + paragraph: paragraph, + segments: [segmentText], + }; + + const result = domIndexer.reconcileChildList([newText], []); + + expect(result).toBeTrue(); + expect(paragraph).toEqual({ + blockType: 'Paragraph', + segments: [ + { + segmentType: 'Image', + src: 'src', + format: {}, + dataset: {}, + link: { format: { href: 'test' }, dataset: {} }, + }, + { + segmentType: 'Text', + text: 'test', + format: {}, + link: { format: { href: 'test' }, dataset: {} }, + }, + { segmentType: 'Text', text: 'a', format: {} }, + ], + format: {}, + }); + expect(((newText as Node) as IndexedSegmentNode).__roosterjsContentModel.paragraph).toBe( + paragraph + ); + expect( + ((newText as Node) as IndexedSegmentNode).__roosterjsContentModel.segments.length + ).toBe(1); + expect(((newText as Node) as IndexedSegmentNode).__roosterjsContentModel.segments[0]).toBe( + paragraph.segments[2] + ); + }); }); describe('domIndexerImpl.reconcileElementId', () => { diff --git a/packages/roosterjs-content-model-dom/lib/modelToDom/handlers/handleSegmentDecorator.ts b/packages/roosterjs-content-model-dom/lib/modelToDom/handlers/handleSegmentDecorator.ts index d2dcc7706aa..1a2f2da82da 100644 --- a/packages/roosterjs-content-model-dom/lib/modelToDom/handlers/handleSegmentDecorator.ts +++ b/packages/roosterjs-content-model-dom/lib/modelToDom/handlers/handleSegmentDecorator.ts @@ -14,8 +14,7 @@ export const handleSegmentDecorator: ContentModelSegmentHandler { const { code, link } = segment; @@ -27,7 +26,6 @@ export const handleSegmentDecorator: ContentModelSegmentHandler { handleBr(document, parent, br, context, newSegments); expect(parent.innerHTML).toBe('
'); - expect(newSegments.length).toBe(2); + expect(newSegments.length).toBe(1); expect((newSegments[0] as HTMLElement).outerHTML).toBe('
'); - expect((newSegments[1] as HTMLElement).outerHTML).toBe('
'); }); }); diff --git a/packages/roosterjs-content-model-dom/test/modelToDom/handlers/handleSegmentDecoratorTest.ts b/packages/roosterjs-content-model-dom/test/modelToDom/handlers/handleSegmentDecoratorTest.ts index 022733f2116..748e1d4b39b 100644 --- a/packages/roosterjs-content-model-dom/test/modelToDom/handlers/handleSegmentDecoratorTest.ts +++ b/packages/roosterjs-content-model-dom/test/modelToDom/handlers/handleSegmentDecoratorTest.ts @@ -1,5 +1,4 @@ import { createModelToDomContext } from '../../../lib/modelToDom/context/createModelToDomContext'; -import { expectHtml } from '../../testUtils'; import { handleSegmentDecorator } from '../../../lib/modelToDom/handlers/handleSegmentDecorator'; import { ContentModelCode, @@ -20,8 +19,7 @@ describe('handleSegmentDecorator', () => { function runTest( link: ContentModelLink | undefined, code: ContentModelCode | undefined, - expectedInnerHTML: string, - expectedSegmentNodesHTML: (string | string[])[] + expectedInnerHTML: string ) { parent = document.createElement('span'); parent.textContent = 'test'; @@ -37,10 +35,7 @@ describe('handleSegmentDecorator', () => { handleSegmentDecorator(document, parent, segment, context, segmentNodes); expect(parent.innerHTML).toBe(expectedInnerHTML); - expect(segmentNodes.length).toBe(expectedSegmentNodesHTML.length); - expectedSegmentNodesHTML.forEach((expectedHTML, i) => { - expectHtml((segmentNodes[i] as HTMLElement).outerHTML, expectedHTML); - }); + expect(segmentNodes.length).toBe(0); } it('simple link', () => { @@ -52,9 +47,7 @@ describe('handleSegmentDecorator', () => { dataset: {}, }; - runTest(link, undefined, 'test', [ - 'test', - ]); + runTest(link, undefined, 'test'); }); it('link with color', () => { @@ -67,9 +60,7 @@ describe('handleSegmentDecorator', () => { dataset: {}, }; - runTest(link, undefined, 'test', [ - 'test', - ]); + runTest(link, undefined, 'test'); }); it('link without underline', () => { @@ -84,8 +75,7 @@ describe('handleSegmentDecorator', () => { runTest( link, undefined, - 'test', - ['test'] + 'test' ); }); @@ -101,9 +91,7 @@ describe('handleSegmentDecorator', () => { }, }; - runTest(link, undefined, 'test', [ - 'test', - ]); + runTest(link, undefined, 'test'); }); it('simple code', () => { @@ -113,7 +101,7 @@ describe('handleSegmentDecorator', () => { }, }; - runTest(undefined, code, 'test', ['test']); + runTest(undefined, code, 'test'); }); it('code with font', () => { @@ -123,9 +111,7 @@ describe('handleSegmentDecorator', () => { }, }; - runTest(undefined, code, 'test', [ - 'test', - ]); + runTest(undefined, code, 'test'); }); it('link and code', () => { @@ -142,10 +128,7 @@ describe('handleSegmentDecorator', () => { }, }; - runTest(link, code, 'test', [ - 'test', - 'test', - ]); + runTest(link, code, 'test'); }); it('Link with onNodeCreated', () => { @@ -194,12 +177,7 @@ describe('handleSegmentDecorator', () => { dataset: {}, }; - runTest( - link, - undefined, - 'test', - ['test'] - ); + runTest(link, undefined, 'test'); }); it('code with display: block', () => { @@ -210,9 +188,7 @@ describe('handleSegmentDecorator', () => { }, }; - runTest(undefined, code, 'test', [ - 'test', - ]); + runTest(undefined, code, 'test'); }); it('link with background color', () => { @@ -228,8 +204,7 @@ describe('handleSegmentDecorator', () => { runTest( link, undefined, - 'test', - ['test'] + 'test' ); }); }); diff --git a/packages/roosterjs-content-model-dom/test/modelToDom/utils/handleSegmentCommonTest.ts b/packages/roosterjs-content-model-dom/test/modelToDom/utils/handleSegmentCommonTest.ts index 8a3b3c923a4..8292a138235 100644 --- a/packages/roosterjs-content-model-dom/test/modelToDom/utils/handleSegmentCommonTest.ts +++ b/packages/roosterjs-content-model-dom/test/modelToDom/utils/handleSegmentCommonTest.ts @@ -33,9 +33,8 @@ describe('handleSegmentCommon', () => { 'test' ); expect(onNodeCreated).toHaveBeenCalledWith(segment, txt); - expect(segmentNodes.length).toBe(2); + expect(segmentNodes.length).toBe(1); expect(segmentNodes[0]).toBe(txt); - expect(segmentNodes[1]).toBe(txt.parentNode!); }); it('element with child', () => {