Merge pull request #316 from ckeditor/t/ckeditor5/1985

Fix: Image upload should handle images that are deeply nested in other blocks. Closes ckeditor/ckeditor5#1985.

src/imageupload/imageuploadediting.js

@@ -14,7 +14,7 @@ import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter';
 import env from '@ckeditor/ckeditor5-utils/src/env';
 
 import ImageUploadCommand from '../../src/imageupload/imageuploadcommand';
-import { isImageType, isLocalImage, fetchLocalImage } from '../../src/imageupload/utils';
+import { fetchLocalImage, isImageType, isLocalImage } from '../../src/imageupload/utils';
 
 /**
  * The editing part of the image upload feature. It registers the `'imageUpload'` command.
@@ -134,30 +134,32 @@ export default class ImageUploadEditing extends Plugin {
 			const changes = doc.differ.getChanges( { includeChangesInGraveyard: true } );
 
 			for ( const entry of changes ) {
-				if ( entry.type == 'insert' && entry.name == 'image' ) {
+				if ( entry.type == 'insert' && entry.name != '$text' ) {
 					const item = entry.position.nodeAfter;
 					const isInGraveyard = entry.position.root.rootName == '$graveyard';
 
-					// Check if the image element still has upload id.
-					const uploadId = item.getAttribute( 'uploadId' );
+					for ( const image of getImagesFromChangeItem( editor, item ) ) {
+						// Check if the image element still has upload id.
+						const uploadId = image.getAttribute( 'uploadId' );
 
-					if ( !uploadId ) {
-						continue;
-					}
+						if ( !uploadId ) {
+							continue;
+						}
 
-					// Check if the image is loaded on this client.
-					const loader = fileRepository.loaders.get( uploadId );
+						// Check if the image is loaded on this client.
+						const loader = fileRepository.loaders.get( uploadId );
 
-					if ( !loader ) {
-						continue;
-					}
+						if ( !loader ) {
+							continue;
+						}
 
-					if ( isInGraveyard ) {
-						// If the image was inserted to the graveyard - abort the loading process.
-						loader.abort();
-					} else if ( loader.status == 'idle' ) {
-						// If the image was inserted into content and has not been loaded yet, start loading it.
-						this._readAndUpload( loader, item );
+						if ( isInGraveyard ) {
+							// If the image was inserted to the graveyard - abort the loading process.
+							loader.abort();
+						} else if ( loader.status == 'idle' ) {
+							// If the image was inserted into content and has not been loaded yet, start loading it.
+							this._readAndUpload( loader, image );
+						}
 					}
 				}
 			}
@@ -188,15 +190,16 @@ export default class ImageUploadEditing extends Plugin {
 		} );
 
 		return loader.read()
-			.then( data => {
-				const viewFigure = editor.editing.mapper.toViewElement( imageElement );
-				const viewImg = viewFigure.getChild( 0 );
+			.then( () => {
 				const promise = loader.upload();
 
 				// Force re–paint in Safari. Without it, the image will display with a wrong size.
 				// https://github.com/ckeditor/ckeditor5/issues/1975
 				/* istanbul ignore next */
 				if ( env.isSafari ) {
+					const viewFigure = editor.editing.mapper.toViewElement( imageElement );
+					const viewImg = viewFigure.getChild( 0 );
+
 					editor.editing.view.once( 'render', () => {
 						// Early returns just to be safe. There might be some code ran
 						// in between the outer scope and this callback.
@@ -221,10 +224,6 @@ export default class ImageUploadEditing extends Plugin {
 					} );
 				}
 
-				editor.editing.view.change( writer => {
-					writer.setAttribute( 'src', data, viewImg );
-				} );
-
 				model.enqueueChange( 'transparent', writer => {
 					writer.setAttribute( 'uploadStatus', 'uploading', imageElement );
 				} );
@@ -318,3 +317,9 @@ export default class ImageUploadEditing extends Plugin {
 export function isHtmlIncluded( dataTransfer ) {
 	return Array.from( dataTransfer.types ).includes( 'text/html' ) && dataTransfer.getData( 'text/html' ) !== '';
 }
+
+function getImagesFromChangeItem( editor, item ) {
+	return Array.from( editor.model.createRangeOn( item ) )
+		.filter( value => value.item.is( 'image' ) )
+		.map( value => value.item );
+}

src/imageupload/imageuploadprogress.js

@@ -97,6 +97,7 @@ export default class ImageUploadProgress extends Plugin {
 				// Hide placeholder and initialize progress bar showing upload progress.
 				_hidePlaceholder( viewFigure, viewWriter );
 				_showProgressBar( viewFigure, viewWriter, loader, editor.editing.view );
+				_displayLocalImage( viewFigure, viewWriter, loader );
 			}
 
 			return;
@@ -261,3 +262,16 @@ function _removeUIElement( viewFigure, writer, uniqueProperty ) {
 		writer.remove( writer.createRangeOn( element ) );
 	}
 }
+
+// Displays local data from file loader.
+//
+// @param {module:engine/view/element~Element} imageFigure
+// @param {module:engine/view/downcastwriter~DowncastWriter} writer
+// @param {module:upload/filerepository~FileLoader} loader
+function _displayLocalImage( viewFigure, writer, loader ) {
+	if ( loader.data ) {
+		const viewImg = viewFigure.getChild( 0 );
+
+		writer.setAttribute( 'src', loader.data, viewImg );
+	}
+}

tests/imageupload/imageuploadediting.js

@@ -334,7 +334,7 @@ describe( 'ImageUploadEditing', () => {
 			'</figure>]' );
 	} );
 
-	it( 'should use read data once it is present', done => {
+	it( 'should not use read data once it is present', done => {
 		const file = createNativeFileMock();
 		setModelData( model, '<paragraph>{}foo bar</paragraph>' );
 		editor.execute( 'imageUpload', { file } );
@@ -343,7 +343,8 @@ describe( 'ImageUploadEditing', () => {
 			tryExpect( done, () => {
 				expect( getViewData( view ) ).to.equal(
 					'[<figure class="ck-widget image" contenteditable="false">' +
-						`<img src="${ base64Sample }"></img>` +
+						// Rendering the image data is left to a upload progress converter.
+						'<img></img>' +
 						'</figure>]' +
 					'<p>foo bar</p>'
 				);

tests/imageupload/imageuploadprogress.js

@@ -15,7 +15,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
 import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard';
 
-import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks';
+import { createNativeFileMock, NativeFileReaderMock, UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks';
 import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
@@ -107,6 +107,55 @@ describe( 'ImageUploadProgress', () => {
 		loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) );
 	} );
 
+	// See https://github.com/ckeditor/ckeditor5/issues/1985.
+	it( 'should work if image parent is refreshed by the differ', function( done ) {
+		model.schema.register( 'outerBlock', {
+			allowWhere: '$block',
+			isBlock: true
+		} );
+
+		model.schema.register( 'innerBlock', {
+			allowIn: 'outerBlock',
+			isLimit: true
+		} );
+
+		model.schema.extend( '$block', { allowIn: 'innerBlock' } );
+		editor.conversion.elementToElement( { model: 'outerBlock', view: 'outerBlock' } );
+		editor.conversion.elementToElement( { model: 'innerBlock', view: 'innerBlock' } );
+
+		model.document.registerPostFixer( () => {
+			for ( const change of doc.differ.getChanges() ) {
+				// The differ.refreshItem() simulates remove and insert of and image parent thus preventing image from proper work.
+				if ( change.type == 'insert' && change.name == 'image' ) {
+					doc.differ.refreshItem( change.position.parent );
+
+					return true;
+				}
+			}
+		} );
+
+		setModelData( model, '<outerBlock><innerBlock><paragraph>[]</paragraph></innerBlock></outerBlock>' );
+
+		editor.execute( 'imageUpload', { file: createNativeFileMock() } );
+
+		model.document.once( 'change', () => {
+			try {
+				expect( getViewData( view ) ).to.equal(
+					'<outerBlock><innerBlock>[<figure class="ck-appear ck-widget image" contenteditable="false">' +
+					`<img src="${ base64Sample }"></img>` +
+					'<div class="ck-progress-bar"></div>' +
+					'</figure>]</innerBlock></outerBlock>'
+				);
+
+				done();
+			} catch ( err ) {
+				done( err );
+			}
+		}, { priority: 'lowest' } );
+
+		loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) );
+	} );
+
 	it( 'should work correctly when there is no "reading" status and go straight to "uploading"', () => {
 		const fileRepository = editor.plugins.get( FileRepository );
 		const file = createNativeFileMock();