Merge pull request #80 from ckeditor/t/79

Fix: Do not register batches which have only non-document operations. Closes #79. Closes ckeditor/ckeditor5#781.

Author: Reinmar

src/undoengine.js

@@ -65,6 +65,16 @@ export default class UndoEngine extends Plugin {
 
 		this.listenTo( this.editor.model, 'applyOperation', ( evt, args ) => {
 			const operation = args[ 0 ];
+
+			// Do not register batch if the operation is not a document operation.
+			// This prevents from creating empty undo steps, where all operations where non-document operations.
+			// Non-document operations creates and alters content in detached tree fragments (for example, document fragments).
+			// Most of time this is preparing data before it is inserted into actual tree (for example during copy & paste).
+			// Such operations should not be reversed.
+			if ( !operation.isDocumentOperation ) {
+				return;
+			}
+
 			const batch = operation.delta.batch;
 
 			// If changes are not a part of a batch or this is not a new batch, omit those changes.

tests/undoengine-integration.js

@@ -918,7 +918,7 @@ describe( 'UndoEngine integration', () => {
 		} );
 	} );
 
-	describe( 'pasting', () => {
+	describe( 'clipboard', () => {
 		function pasteHtml( editor, html ) {
 			editor.editing.view.document.fire( 'paste', {
 				dataTransfer: createDataTransfer( { 'text/html': html } ),
@@ -930,7 +930,8 @@ describe( 'UndoEngine integration', () => {
 			return {
 				getData( type ) {
 					return data[ type ];
-				}
+				},
+				setData() {}
 			};
 		}
 
@@ -958,6 +959,22 @@ describe( 'UndoEngine integration', () => {
 			editor.execute( 'undo' );
 			output( '<paragraph>Foo[]</paragraph>' );
 		} );
+
+		// ckeditor5#781
+		it( 'cutting should not create empty undo step', () => {
+			input( '<paragraph>Fo[oba]r</paragraph>' );
+
+			editor.editing.view.document.fire( 'cut', {
+				dataTransfer: createDataTransfer(),
+				preventDefault() {},
+				method: 'cut'
+			} );
+
+			editor.execute( 'undo' );
+
+			output( '<paragraph>Fo[oba]r</paragraph>' );
+			undoDisabled();
+		} );
 	} );
 
 	describe( 'other edge cases', () => {

tests/undoengine.js

@@ -56,6 +56,31 @@ describe( 'UndoEngine', () => {
 			expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
 		} );
 
+		it( 'should not add a batch that has only non-document operations', () => {
+			sinon.spy( undo._undoCommand, 'addBatch' );
+
+			model.change( writer => {
+				const docFrag = writer.createDocumentFragment();
+				const element = writer.createElement( 'paragraph' );
+				writer.insert( element, docFrag, 0 );
+				writer.insertText( 'foo', null, element, 0 );
+			} );
+
+			expect( undo._undoCommand.addBatch.called ).to.be.false;
+		} );
+
+		it( 'should add a batch that has both document and non-document operations', () => {
+			sinon.spy( undo._undoCommand, 'addBatch' );
+
+			model.change( writer => {
+				const element = writer.createElement( 'paragraph' );
+				writer.insertText( 'foo', null, element, 0 );
+				writer.insert( element, root, 0 );
+			} );
+
+			expect( undo._undoCommand.addBatch.calledOnce ).to.be.true;
+		} );
+
 		it( 'should add a batch to undo command, if it\'s type is undo and it comes from redo command', () => {
 			sinon.spy( undo._undoCommand, 'addBatch' );
 			sinon.spy( undo._redoCommand, 'clearStack' );