diff --git a/packages-content-model/roosterjs-content-model-core/lib/coreApi/formatContentModel.ts b/packages-content-model/roosterjs-content-model-core/lib/coreApi/formatContentModel.ts index d7d1d2c5f6e..3e01ee49079 100644 --- a/packages-content-model/roosterjs-content-model-core/lib/coreApi/formatContentModel.ts +++ b/packages-content-model/roosterjs-content-model-core/lib/coreApi/formatContentModel.ts @@ -42,9 +42,7 @@ export const formatContentModel: FormatContentModel = (core, formatter, options) if (shouldAddSnapshot) { core.undo.isNested = true; - if (core.undo.snapshotsManager.hasNewContent || entityStates) { - core.api.addUndoSnapshot(core, !!canUndoByBackspace); - } + core.api.addUndoSnapshot(core, !!canUndoByBackspace, entityStates); } try { diff --git a/packages-content-model/roosterjs-content-model-core/lib/editor/SnapshotsManagerImpl.ts b/packages-content-model/roosterjs-content-model-core/lib/editor/SnapshotsManagerImpl.ts index cfe4d25f59e..338d076295a 100644 --- a/packages-content-model/roosterjs-content-model-core/lib/editor/SnapshotsManagerImpl.ts +++ b/packages-content-model/roosterjs-content-model-core/lib/editor/SnapshotsManagerImpl.ts @@ -52,8 +52,9 @@ class SnapshotsManagerImpl implements SnapshotsManager { currentSnapshot.html == snapshot.html && !currentSnapshot.entityStates && !snapshot.entityStates; + const addSnapshot = !currentSnapshot || shouldAddSnapshot(currentSnapshot, snapshot); - if (this.snapshots.currentIndex < 0 || !currentSnapshot || !isSameSnapshot) { + if (this.snapshots.currentIndex < 0 || addSnapshot) { this.clearRedo(); this.snapshots.snapshots.push(snapshot); this.snapshots.currentIndex++; @@ -129,3 +130,13 @@ class SnapshotsManagerImpl implements SnapshotsManager { export function createSnapshotsManager(snapshots?: Snapshots): SnapshotsManager { return new SnapshotsManagerImpl(snapshots); } + +function shouldAddSnapshot(currentSnapshot: Snapshot, snapshot: Snapshot) { + return ( + currentSnapshot.html !== snapshot.html || + (currentSnapshot.entityStates && + snapshot.entityStates && + currentSnapshot.entityStates !== snapshot.entityStates) || + (!currentSnapshot.entityStates && snapshot.entityStates) + ); +} diff --git a/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteBlock.ts b/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteBlock.ts index 0b48a50f51e..6b58dc828e9 100644 --- a/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteBlock.ts +++ b/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteBlock.ts @@ -38,9 +38,6 @@ export function deleteBlock( : undefined; if (operation !== undefined) { - const wrapper = blockToDelete.wrapper; - - wrapper.parentNode?.removeChild(wrapper); replacement ? blocks.splice(index, 1, replacement) : blocks.splice(index, 1); context?.deletedEntities.push({ entity: blockToDelete, diff --git a/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteSegment.ts b/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteSegment.ts index 64bccd128ff..1f35bb9bdcd 100644 --- a/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteSegment.ts +++ b/packages-content-model/roosterjs-content-model-core/lib/publicApi/selection/deleteSegment.ts @@ -48,9 +48,6 @@ export function deleteSegment( ? 'removeFromEnd' : undefined; if (operation !== undefined) { - const wrapper = segmentToDelete.wrapper; - - wrapper.parentNode?.removeChild(wrapper); segments.splice(index, 1); context?.deletedEntities.push({ entity: segmentToDelete, diff --git a/packages-content-model/roosterjs-content-model-core/test/coreApi/formatContentModelTest.ts b/packages-content-model/roosterjs-content-model-core/test/coreApi/formatContentModelTest.ts index 40a382043e4..884709a8055 100644 --- a/packages-content-model/roosterjs-content-model-core/test/coreApi/formatContentModelTest.ts +++ b/packages-content-model/roosterjs-content-model-core/test/coreApi/formatContentModelTest.ts @@ -93,7 +93,7 @@ describe('formatContentModel', () => { newImages: [], }); expect(createContentModel).toHaveBeenCalledTimes(1); - expect(addUndoSnapshot).toHaveBeenCalledTimes(1); + expect(addUndoSnapshot).toHaveBeenCalledTimes(2); expect(addUndoSnapshot).toHaveBeenCalledWith(core, false, undefined); expect(setContentModel).toHaveBeenCalledTimes(1); expect(setContentModel).toHaveBeenCalledWith(core, mockedModel, undefined, undefined); @@ -725,7 +725,7 @@ describe('formatContentModel', () => { expect(callback).toHaveBeenCalledTimes(1); expect(addUndoSnapshot).toHaveBeenCalledTimes(2); - expect(addUndoSnapshot).toHaveBeenCalledWith(core, false); + expect(addUndoSnapshot).toHaveBeenCalledWith(core, false, undefined); expect(addUndoSnapshot).toHaveBeenCalledWith(core, false, undefined); expect(setContentModel).toHaveBeenCalledTimes(1); expect(setContentModel).toHaveBeenCalledWith(core, mockedModel, undefined, undefined); @@ -750,7 +750,7 @@ describe('formatContentModel', () => { expect(callback).toHaveBeenCalledTimes(1); expect(addUndoSnapshot).toHaveBeenCalledTimes(2); - expect(addUndoSnapshot).toHaveBeenCalledWith(core, false); + expect(addUndoSnapshot).toHaveBeenCalledWith(core, false, mockedEntityState); expect(addUndoSnapshot).toHaveBeenCalledWith(core, false, mockedEntityState); expect(setContentModel).toHaveBeenCalledTimes(1); expect(setContentModel).toHaveBeenCalledWith(core, mockedModel, undefined, undefined); @@ -771,7 +771,7 @@ describe('formatContentModel', () => { formatContentModel(core, callback); expect(callback).toHaveBeenCalledTimes(1); - expect(addUndoSnapshot).toHaveBeenCalledTimes(1); + expect(addUndoSnapshot).toHaveBeenCalledTimes(2); expect(addUndoSnapshot).toHaveBeenCalledWith(core, true, undefined); expect(setContentModel).toHaveBeenCalledTimes(1); expect(setContentModel).toHaveBeenCalledWith(core, mockedModel, undefined, undefined); @@ -800,7 +800,7 @@ describe('formatContentModel', () => { formatContentModel(core, callback); expect(callback).toHaveBeenCalledTimes(1); - expect(addUndoSnapshot).toHaveBeenCalledTimes(1); + expect(addUndoSnapshot).toHaveBeenCalledTimes(2); expect(addUndoSnapshot).toHaveBeenCalledWith(core, true, undefined); expect(setContentModel).toHaveBeenCalledTimes(1); expect(setContentModel).toHaveBeenCalledWith(core, mockedModel, undefined, undefined); diff --git a/packages-content-model/roosterjs-content-model-core/test/editor/SnapshotsManagerImplTest.ts b/packages-content-model/roosterjs-content-model-core/test/editor/SnapshotsManagerImplTest.ts index 70d0d6ec426..636f850d0b4 100644 --- a/packages-content-model/roosterjs-content-model-core/test/editor/SnapshotsManagerImplTest.ts +++ b/packages-content-model/roosterjs-content-model-core/test/editor/SnapshotsManagerImplTest.ts @@ -300,6 +300,171 @@ describe('SnapshotsManagerImpl.addSnapshot', () => { ]); }); + it('Add snapshot with entity state with equal entity states', () => { + const mockedEntityStates = 'ENTITYSTATES' as any; + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + ]); + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + ]); + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + ]); + }); + + it('Add snapshot with entity state with different entity states', () => { + const mockedEntityStates = 'ENTITYSTATES' as any; + const mockedEntityStates2 = 'ENTITYSTATES2' as any; + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + ]); + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + ]); + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates2, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + }, + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates2, + }, + ]); + }); + + it('Add snapshot without entity state after a snapshot with empty state', () => { + const mockedEntityStates = 'ENTITYSTATES' as any; + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + ]); + + service.addSnapshot( + { + html: 'test', + isDarkMode: false, + }, + false + ); + + expect(snapshots.snapshots).toEqual([ + { + html: 'test', + isDarkMode: false, + entityStates: mockedEntityStates, + }, + ]); + }); + it('Has onChanged', () => { const onChanged = jasmine.createSpy('onChanged'); snapshots.onChanged = onChanged; diff --git a/packages-content-model/roosterjs-content-model-dom/lib/domUtils/reuseCachedElement.ts b/packages-content-model/roosterjs-content-model-dom/lib/domUtils/reuseCachedElement.ts index 282bad51333..471c94ece4a 100644 --- a/packages-content-model/roosterjs-content-model-dom/lib/domUtils/reuseCachedElement.ts +++ b/packages-content-model/roosterjs-content-model-dom/lib/domUtils/reuseCachedElement.ts @@ -11,10 +11,12 @@ import { isEntityElement } from './entityUtils'; */ export function reuseCachedElement(parent: Node, element: Node, refNode: Node | null): Node | null { if (element.parentNode == parent) { + const isEntity = isEntityElement(element); + // Remove nodes before the one we are hitting since they don't appear in Content Model at this position. // But we don't want to touch entity since it would better to keep entity at its place unless it is removed // In that case we will remove it after we have handled all other nodes - while (refNode && refNode != element && !isEntityElement(refNode)) { + while (refNode && refNode != element && (isEntity || !isEntityElement(refNode))) { const next = refNode.nextSibling; refNode.parentNode?.removeChild(refNode); diff --git a/packages-content-model/roosterjs-content-model-dom/test/domUtils/reuseCachedElementTest.ts b/packages-content-model/roosterjs-content-model-dom/test/domUtils/reuseCachedElementTest.ts index 7a834034f9f..1f612a8c6cb 100644 --- a/packages-content-model/roosterjs-content-model-dom/test/domUtils/reuseCachedElementTest.ts +++ b/packages-content-model/roosterjs-content-model-dom/test/domUtils/reuseCachedElementTest.ts @@ -66,6 +66,7 @@ describe('reuseCachedElement', () => { const refNode = document.createElement('div'); const element = document.createElement('span'); const nextNode = document.createElement('br'); + const removeChildSpy = spyOn(Node.prototype, 'removeChild').and.callThrough(); parent.appendChild(refNode); parent.appendChild(element); @@ -75,6 +76,7 @@ describe('reuseCachedElement', () => { const result = reuseCachedElement(parent, element, refNode); + expect(removeChildSpy).not.toHaveBeenCalled(); expect(parent.outerHTML).toBe( '

' ); @@ -82,4 +84,31 @@ describe('reuseCachedElement', () => { expect(parent.firstChild?.nextSibling).toBe(refNode); expect(result).toBe(refNode); }); + + it('RefNode is entity, current element is entity', () => { + const parent = document.createElement('div'); + const refNode = document.createElement('div'); + const element = document.createElement('span'); + const nextNode = document.createElement('br'); + const removeChildSpy = spyOn(Node.prototype, 'removeChild').and.callThrough(); + + parent.appendChild(refNode); + parent.appendChild(element); + parent.appendChild(nextNode); + + setEntityElementClasses(refNode, 'TestEntity', true); + setEntityElementClasses(element, 'TestEntity2', true); + + const result = reuseCachedElement(parent, element, refNode); + + expect(removeChildSpy).toHaveBeenCalledTimes(1); + expect(removeChildSpy).toHaveBeenCalledWith(refNode); + + expect(parent.outerHTML).toBe( + '

' + ); + expect(parent.firstChild).toBe(element); + expect(parent.firstChild?.nextSibling).toBe(nextNode); + expect(result).toBe(nextNode); + }); }); diff --git a/packages/roosterjs-editor-adapter/lib/editor/EditorAdapter.ts b/packages/roosterjs-editor-adapter/lib/editor/EditorAdapter.ts index f986577a5fd..84a85f712fc 100644 --- a/packages/roosterjs-editor-adapter/lib/editor/EditorAdapter.ts +++ b/packages/roosterjs-editor-adapter/lib/editor/EditorAdapter.ts @@ -152,6 +152,8 @@ export class EditorAdapter extends Editor implements ILegacyEditor { * Dispose this editor, dispose all plugins and custom data */ dispose(): void { + super.dispose(); + const core = this.contentModelEditorCore; if (core) { @@ -167,8 +169,6 @@ export class EditorAdapter extends Editor implements ILegacyEditor { this.contentModelEditorCore = undefined; } - - super.dispose(); } /**