Merge pull request #39 from ckeditor/t/38

Fix: When image upload is aborted, now the "image placeholder" element is permanently removed so it is not reinserted on undo. Closes #38.

src/imageuploadengine.js

@@ -53,7 +53,7 @@ export default class ImageUploadEngine extends Plugin {
 			}
 		} );
 
-		doc.on( 'change', ( evt, type, data, batch ) => {
+		doc.on( 'change', ( evt, type, data ) => {
 			// Listen on document changes and:
 			// * start upload process when image with `uploadId` attribute is inserted,
 			// * abort upload process when image `uploadId` attribute is removed.
@@ -68,7 +68,7 @@ export default class ImageUploadEngine extends Plugin {
 
 							if ( loader ) {
 								if ( type === 'insert' && loader.status == 'idle' ) {
-									this.load( loader, batch, imageElement );
+									this.load( loader, imageElement );
 								}
 
 								if ( type === 'remove' ) {
@@ -88,18 +88,17 @@ export default class ImageUploadEngine extends Plugin {
 	 *
 	 * @protected
 	 * @param {module:upload/filerepository~FileLoader} loader
-	 * @param {module:engine/model/batch~Batch} batch
 	 * @param {module:engine/model/element~Element} imageElement
 	 */
-	load( loader, batch, imageElement ) {
+	load( loader, imageElement ) {
 		const editor = this.editor;
 		const t = editor.locale.t;
 		const doc = editor.document;
 		const fileRepository = editor.plugins.get( FileRepository );
 		const notification = editor.plugins.get( Notification );
 
 		doc.enqueueChanges( () => {
-			batch.setAttribute( imageElement, 'uploadStatus', 'reading' );
+			doc.batch( 'transparent' ).setAttribute( imageElement, 'uploadStatus', 'reading' );
 		} );
 
 		loader.read()
@@ -112,15 +111,15 @@ export default class ImageUploadEngine extends Plugin {
 				editor.editing.view.render();
 
 				doc.enqueueChanges( () => {
-					batch.setAttribute( imageElement, 'uploadStatus', 'uploading' );
+					doc.batch( 'transparent' ).setAttribute( imageElement, 'uploadStatus', 'uploading' );
 				} );
 
 				return promise;
 			} )
 			.then( data => {
 				doc.enqueueChanges( () => {
-					batch.setAttribute( imageElement, 'uploadStatus', 'complete' );
-					batch.setAttribute( imageElement, 'src', data.original );
+					doc.batch( 'transparent' ).setAttribute( imageElement, 'uploadStatus', 'complete' );
+					doc.batch( 'transparent' ).setAttribute( imageElement, 'src', data.original );
 
 					// Srcset attribute for responsive images support.
 					const srcsetAttribute = Object.keys( data )
@@ -134,7 +133,7 @@ export default class ImageUploadEngine extends Plugin {
 						.join( ', ' );
 
 					if ( srcsetAttribute != '' ) {
-						batch.setAttribute( imageElement, 'srcset', srcsetAttribute );
+						doc.batch( 'transparent' ).setAttribute( imageElement, 'srcset', srcsetAttribute );
 					}
 				} );
 
@@ -151,16 +150,16 @@ export default class ImageUploadEngine extends Plugin {
 
 				clean();
 
-				// Remove image from insertion batch.
+				// Permanently remove image from insertion batch.
 				doc.enqueueChanges( () => {
-					batch.remove( imageElement );
+					doc.batch( 'transparent' ).remove( imageElement );
 				} );
 			} );
 
 		function clean() {
 			doc.enqueueChanges( () => {
-				batch.removeAttribute( imageElement, 'uploadId' );
-				batch.removeAttribute( imageElement, 'uploadStatus' );
+				doc.batch( 'transparent' ).removeAttribute( imageElement, 'uploadId' );
+				doc.batch( 'transparent' ).removeAttribute( imageElement, 'uploadStatus' );
 			} );
 
 			fileRepository.destroyLoader( loader );

tests/imageuploadengine.js

@@ -10,6 +10,7 @@ import ImageEngine from '@ckeditor/ckeditor5-image/src/image/imageengine';
 import ImageUploadEngine from '../src/imageuploadengine';
 import ImageUploadCommand from '../src/imageuploadcommand';
 import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import UndoEngine from '@ckeditor/ckeditor5-undo/src/undoengine';
 import DataTransfer from '@ckeditor/ckeditor5-clipboard/src/datatransfer';
 import FileRepository from '../src/filerepository';
 import { AdapterMock, createNativeFileMock, NativeFileReaderMock } from './_utils/mocks';
@@ -33,7 +34,7 @@ describe( 'ImageUploadEngine', () => {
 		} );
 
 		return ClassicTestEditor.create( {
-			plugins: [ ImageEngine, ImageUploadEngine, Paragraph ]
+			plugins: [ ImageEngine, ImageUploadEngine, Paragraph, UndoEngine ]
 		} )
 		.then( newEditor => {
 			editor = newEditor;
@@ -235,6 +236,50 @@ describe( 'ImageUploadEngine', () => {
 		sinon.assert.calledOnce( abortSpy );
 	} );
 
+	it( 'image should be permanently removed if it is removed by user during upload', done => {
+		const file = createNativeFileMock();
+		const notification = editor.plugins.get( Notification );
+		setModelData( document, '<paragraph>{}foo bar</paragraph>' );
+
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			evt.stop();
+		}, { priority: 'high' } );
+
+		editor.execute( 'imageUpload', { file } );
+
+		document.once( 'changesDone', () => {
+			// This is called after "manual" remove.
+			document.once( 'changesDone', () => {
+				// This is called after attributes are removed.
+				let undone = false;
+
+				document.once( 'changesDone', () => {
+					if ( !undone ) {
+						undone = true;
+
+						// This is called after abort remove.
+						expect( getModelData( document ) ).to.equal( '<paragraph>[]foo bar</paragraph>' );
+
+						editor.execute( 'undo' );
+
+						// Expect that the image has not been brought back.
+						expect( getModelData( document ) ).to.equal( '<paragraph>[]foo bar</paragraph>' );
+
+						done();
+					}
+				} );
+			} );
+		} );
+
+		const image = document.getRoot().getChild( 0 );
+		document.enqueueChanges( () => {
+			const batch = document.batch();
+
+			batch.remove( image );
+		} );
+	} );
+
 	it( 'should create responsive image if server return multiple images', done => {
 		const file = createNativeFileMock();
 		setModelData( document, '<paragraph>{}foo bar</paragraph>' );