Parcourir la source

Pass promise instead of a file instance to loader while processing pasted base64/blob images.

Krzysztof Krztoń il y a 7 ans
Parent
commit
669f5fdeda

+ 10 - 21
packages/ckeditor5-image/src/imageupload/imageuploadediting.js

@@ -101,36 +101,25 @@ export default class ImageUploadEditing extends Plugin {
 		this.listenTo( editor.plugins.get( 'Clipboard' ), 'inputTransformation', ( evt, data ) => {
 			const fetchableImages = Array.from( editor.editing.view.createRangeIn( data.content ) )
 				.filter( value => isLocalImage( value.item ) && !value.item.getAttribute( 'uploadProcessed' ) )
-				.map( value => fetchLocalImage( value.item ) );
+				.map( value => { return { promise: fetchLocalImage( value.item ), imageElement: value.item }; } );
 
 			if ( !fetchableImages.length ) {
 				return;
 			}
 
-			evt.stop();
+			const writer = new UpcastWriter();
 
-			Promise.all( fetchableImages ).then( items => {
-				const writer = new UpcastWriter();
+			for ( const fetchableImage of fetchableImages ) {
+				// Set attribute marking that the image was processed already.
+				writer.setAttribute( 'uploadProcessed', true, fetchableImage.imageElement );
 
-				for ( const item of items ) {
-					if ( !item.file ) {
-						// Failed to fetch image or create a file instance, remove image element.
-						writer.remove( item.image );
-					} else {
-						// Set attribute marking the image as processed.
-						writer.setAttribute( 'uploadProcessed', true, item.image );
+				const loader = fileRepository.createLoader( fetchableImage.promise );
 
-						const loader = fileRepository.createLoader( item.file );
-
-						if ( loader ) {
-							writer.setAttribute( 'src', '', item.image );
-							writer.setAttribute( 'uploadId', loader.id, item.image );
-						}
-					}
+				if ( loader ) {
+					writer.setAttribute( 'src', '', fetchableImage.imageElement );
+					writer.setAttribute( 'uploadId', loader.id, fetchableImage.imageElement );
 				}
-
-				editor.plugins.get( 'Clipboard' ).fire( 'inputTransformation', data );
-			} );
+			}
 		} );
 
 		// Prevents from the browser redirecting to the dropped image.

+ 9 - 9
packages/ckeditor5-image/src/imageupload/utils.js

@@ -22,29 +22,29 @@ export function isImageType( file ) {
 }
 
 /**
- * Creates a promise which fetches the image local source (base64 or blob) and returns as a `File` object.
+ * Creates a promise which fetches the image local source (base64 or blob) and resolves with a `File` object.
  *
  * @param {module:engine/view/element~Element} image Image which source to fetch.
  * @returns {Promise} A promise which resolves when image source is fetched and converted to `File` instance.
- * It resolves with object holding initial image element (as `image`) and its file source (as `file`). If
- * the `file` attribute is null, it means fetching failed.
+ * It resolves with a `File` object. If there were any errors during file processing the promise will be rejected.
  */
 export function fetchLocalImage( image ) {
-	return new Promise( resolve => {
+	return new Promise( ( resolve, reject ) => {
+		const imageSrc = image.getAttribute( 'src' );
+
 		// Fetch works asynchronously and so does not block browser UI when processing data.
-		fetch( image.getAttribute( 'src' ) )
+		fetch( imageSrc )
 			.then( resource => resource.blob() )
 			.then( blob => {
-				const mimeType = getImageMimeType( blob, image.getAttribute( 'src' ) );
+				const mimeType = getImageMimeType( blob, imageSrc );
 				const ext = mimeType.replace( 'image/', '' );
 				const filename = `image.${ ext }`;
 				const file = createFileFromBlob( blob, filename, mimeType );
 
-				resolve( { image, file } );
+				file ? resolve( file ) : reject();
 			} )
 			.catch( () => {
-				// We always resolve a promise so `Promise.all` will not reject if one of many fetch fails.
-				resolve( { image, file: null } );
+				reject();
 			} );
 	} );
 }

+ 117 - 39
packages/ckeditor5-image/tests/imageupload/imageuploadediting.js

@@ -20,7 +20,7 @@ import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
 import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks';
 
 import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
-import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
+import { getData as getViewData, stringify as stringifyView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 
 import log from '@ckeditor/ckeditor5-utils/src/log';
 import env from '@ckeditor/ckeditor5-utils/src/env';
@@ -656,11 +656,13 @@ describe( 'ImageUploadEditing', () => {
 	} );
 
 	it( 'should not upload and remove image if fetch failed', done => {
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			const expected = '<paragraph>[]foo</paragraph>';
-
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		expectData(
+			'<img src="" uploadId="#loader1_id" uploadProcessed="true"></img>',
+			'[<image src="" uploadId="#loader1_id" uploadStatus="reading"></image>]<paragraph>foo</paragraph>',
+			'<paragraph>[]foo</paragraph>',
+			done,
+			false
+		);
 
 		setModelData( model, '<paragraph>[]foo</paragraph>' );
 
@@ -679,14 +681,22 @@ describe( 'ImageUploadEditing', () => {
 	} );
 
 	it( 'should upload only images which were successfully fetched and remove failed ones', done => {
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			const expected = '<paragraph>bar</paragraph>' +
-				`<image src="" uploadId="${ adapterMocks[ 0 ].loader.id }" uploadStatus="reading"></image>` +
-				`[<image src="" uploadId="${ adapterMocks[ 1 ].loader.id }" uploadStatus="reading"></image>]` +
-				'<paragraph>foo</paragraph>';
-
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		const expectedModel = '<paragraph>bar</paragraph>' +
+			'<image src="" uploadId="#loader1_id" uploadStatus="reading"></image>' +
+			'<image src="" uploadId="#loader2_id" uploadStatus="reading"></image>' +
+			'[<image src="" uploadId="#loader3_id" uploadStatus="reading"></image>]' +
+			'<paragraph>foo</paragraph>';
+		const expectedFinalModel = '<paragraph>bar</paragraph>' +
+			'<image src="" uploadId="#loader1_id" uploadStatus="reading"></image>' +
+			'[<image src="" uploadId="#loader2_id" uploadStatus="reading"></image>]' +
+			'<paragraph>foo</paragraph>';
+
+		expectData(
+			'',
+			expectedModel,
+			expectedFinalModel,
+			done
+		);
 
 		setModelData( model, '<paragraph>[]foo</paragraph>' );
 
@@ -717,13 +727,16 @@ describe( 'ImageUploadEditing', () => {
 
 		window.File = undefined;
 
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			window.File = fileFn;
-
-			const expected = '<paragraph>baz[]foo</paragraph>';
-
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		expectData(
+			'<img src="" uploadId="#loader1_id" uploadProcessed="true"></img><p>baz</p>',
+			'<image src="" uploadId="#loader1_id" uploadStatus="reading"></image><paragraph>baz[]foo</paragraph>',
+			'<paragraph>baz[]foo</paragraph>',
+			err => {
+				window.File = fileFn;
+				done( err );
+			},
+			false
+		);
 
 		setModelData( model, '<paragraph>[]foo</paragraph>' );
 
@@ -737,19 +750,15 @@ describe( 'ImageUploadEditing', () => {
 	} );
 
 	it( 'should not upload and remove image when `File` constructor is not supported', done => {
-		const fileFn = window.File;
-
-		window.File = function() {
-			throw new Error( 'Function expected.' ); // Simulating Edge browser behaviour here.
-		};
-
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			window.File = fileFn;
-
-			const expected = '<paragraph>baz[]foo</paragraph>';
-
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' );
+
+		expectData(
+			'<p>baz</p><img src="" uploadId="#loader1_id" uploadProcessed="true"></img>',
+			'<paragraph>baz</paragraph>[<image src="" uploadId="#loader1_id" uploadStatus="reading"></image>]<paragraph>foo</paragraph>',
+			'<paragraph>baz[]</paragraph><paragraph>foo</paragraph>',
+			done,
+			false
+		);
 
 		setModelData( model, '<paragraph>[]foo</paragraph>' );
 
@@ -765,8 +774,10 @@ describe( 'ImageUploadEditing', () => {
 	// Skip this test on Edge as we mock `File` object there so there is no sense in testing it.
 	( isEdgeEnv ? it.skip : it )( 'should get file extension from base64 string', done => {
 		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			tryExpect( done, () => {
-				loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'png' ) );
+			loader.file.then( file => {
+				tryExpect( done, () => {
+					expect( file.name.split( '.' ).pop() ).to.equal( 'png' );
+				} );
 			} );
 		}, { priority: 'low' } );
 
@@ -793,8 +804,10 @@ describe( 'ImageUploadEditing', () => {
 	// Skip this test on Edge as we mock `File` object there so there is no sense in testing it.
 	( isEdgeEnv ? it.skip : it )( 'should use fallback file extension', done => {
 		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			tryExpect( done, () => {
-				loader.file.then( file => expect( file.name.split( '.' ).pop() ).to.equal( 'jpeg' ) );
+			loader.file.then( file => {
+				tryExpect( done, () => {
+					expect( file.name.split( '.' ).pop() ).to.equal( 'jpeg' );
+				} );
 			} );
 		}, { priority: 'low' } );
 
@@ -817,8 +830,72 @@ describe( 'ImageUploadEditing', () => {
 
 		viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );
 	} );
+
+	// Helper for validating clipboard and model data as a result of a paste operation. This function checks both clipboard
+	// data and model data synchronously (`expectedClipboardData`, `expectedModel`) and then the model data after `loader.file`
+	// promise is resolved (so model state after successful/failed file fetch attempt).
+	//
+	// @param {String} expectedClipboardData Expected clipboard data on `inputTransformation` event.
+	// @param {String} expectedModel Expected model data on `inputTransformation` event.
+	// @param {String} expectedModelOnFile Expected model data after all `file.loader` promises are fetched.
+	// @param {Function} doneFn Callback function to be called when all assertions are done or error occures.
+	// @param {Boolean} [onSuccess=true] If `expectedModelOnFile` data should be validated
+	// on `loader.file` a promise successful resolution or promise rejection.
+	function expectData( expectedClipboardData, expectedModel, expectedModelOnFile, doneFn, onSuccess ) {
+		// Check data after paste.
+		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', ( evt, data ) => {
+			const clipboardData = injectLoaderId( expectedClipboardData || '', adapterMocks );
+			const modelData = injectLoaderId( expectedModel, adapterMocks );
+			const finalModelData = injectLoaderId( expectedModelOnFile, adapterMocks );
+
+			if ( clipboardData.length ) {
+				expect( stringifyView( data.content ) ).to.equal( clipboardData );
+			}
+			expect( getModelData( model ) ).to.equal( modelData );
+
+			if ( onSuccess !== false ) {
+				adapterMocks[ 0 ].loader.file.then( () => {
+					// Deffer so the promise could be resolved.
+					setTimeout( () => {
+						expectModel( doneFn, getModelData( model ), finalModelData );
+					} );
+				} );
+			} else {
+				adapterMocks[ 0 ].loader.file.then( () => {
+					expect.fail( 'The `loader.file` should be rejected.' );
+				} ).catch( () => {
+					// Deffer so the promise could be resolved.
+					setTimeout( () => {
+						expectModel( doneFn, getModelData( model ), finalModelData );
+					} );
+				} );
+			}
+		}, { priority: 'low' } );
+	}
 } );
 
+// Replaces '#loaderX_id' parameter in the given string with a loader id. It is used
+// so data string could be created before loader is initialized.
+//
+// @param {String} data String which have 'loader params' replaced.
+// @param {Array.<UploadAdapterMock>} adapters Adapters list. Each adapter holds a reference to a loader which id is used.
+// @returns {String} Data string with 'loader params' replaced.
+function injectLoaderId( data, adapters ) {
+	let newData = data;
+
+	if ( newData.includes( '#loader1_id' ) ) {
+		newData = newData.replace( '#loader1_id', adapters[ 0 ].loader.id );
+	}
+	if ( newData.includes( '#loader2_id' ) ) {
+		newData = newData.replace( '#loader2_id', adapters[ 1 ].loader.id );
+	}
+	if ( newData.includes( '#loader3_id' ) ) {
+		newData = newData.replace( '#loader3_id', adapters[ 2 ].loader.id );
+	}
+
+	return newData;
+}
+
 // Asserts actual and expected model data.
 //
 // @param {function} done Callback function to be called when assertion is done.
@@ -831,7 +908,8 @@ function expectModel( done, actual, expected ) {
 }
 
 // Runs given expect function in a try-catch. It should be used only when `expect` is called as a result of a `Promise`
-// resolution as all errors may be caught by tested code and needs to be rethrow to be correctly processed by a testing framework.
+// resolution. In such cases all errors may be caught by tested code and needs to be rethrow to be correctly processed
+// by a testing framework.
 //
 // @param {Function} doneFn Function to run when assertion is done.
 // @param {Function} expectFn Function containing all assertions.