8
0
Просмотр исходного кода

Merge pull request #264 from ckeditor/t/258

Other: The image uploading listener for handling `base64/blob` images no longer stops `inputTransformation` event. Closes #263. Closes ckeditor/ckeditor5-paste-from-office#44.
Maciej 7 лет назад
Родитель
Сommit
9a07a9b2ed

+ 11 - 22
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 );
-				} );
+				}
 			} );
 		}
 
@@ -229,7 +218,7 @@ export default class ImageUploadEditing extends Plugin {
 				}
 
 				// Might be 'aborted'.
-				if ( loader.status == 'error' ) {
+				if ( loader.status == 'error' && error ) {
 					notification.showWarning( error, {
 						title: t( 'Upload failed' ),
 						namespace: 'upload'

+ 10 - 12
packages/ckeditor5-image/src/imageupload/utils.js

@@ -22,30 +22,28 @@ 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.
+ * @returns {Promise.<File>} A promise which resolves when image source is fetched and converted to `File` instance.
+ * 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 } );
-			} );
+			.catch( reject );
 	} );
 }
 

+ 173 - 28
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';
@@ -661,11 +661,20 @@ describe( 'ImageUploadEditing', () => {
 	} );
 
 	it( 'should not upload and remove image if fetch failed', done => {
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			const expected = '<paragraph>[]foo</paragraph>';
+		const notification = editor.plugins.get( Notification );
 
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			evt.stop();
+		}, { priority: 'high' } );
+
+		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>' );
 
@@ -684,14 +693,29 @@ 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>';
+		const notification = editor.plugins.get( Notification );
 
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			evt.stop();
+		}, { priority: 'high' } );
+
+		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>' );
 
@@ -710,7 +734,7 @@ describe( 'ImageUploadEditing', () => {
 			if ( counter < 3 ) {
 				return fetch( src );
 			} else {
-				return new Promise( ( res, rej ) => rej() );
+				return Promise.reject();
 			}
 		} );
 
@@ -722,13 +746,23 @@ describe( 'ImageUploadEditing', () => {
 
 		window.File = undefined;
 
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			window.File = fileFn;
+		const notification = editor.plugins.get( Notification );
 
-			const expected = '<paragraph>baz[]foo</paragraph>';
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			evt.stop();
+		}, { priority: 'high' } );
 
-			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>' );
 
@@ -742,19 +776,29 @@ 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.
-		};
+		if ( isEdgeEnv ) {
+			// Since on Edge `File` is already stubbed, restore it to it native form so that exception will be thrown.
+			testUtils.sinon.restore();
+			// Since all stubs were restored, re-stub `scrollToTheSelection`.
+			testUtils.sinon.stub( editor.editing.view, 'scrollToTheSelection' ).callsFake( () => {} );
+		} else {
+			testUtils.sinon.stub( window, 'File' ).throws( 'Function expected.' );
+		}
 
-		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
-			window.File = fileFn;
+		const notification = editor.plugins.get( Notification );
 
-			const expected = '<paragraph>baz[]foo</paragraph>';
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			evt.stop();
+		}, { priority: 'high' } );
 
-			expectModel( done, getModelData( model ), expected );
-		}, { priority: 'low' } );
+		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>' );
 
@@ -822,8 +866,109 @@ describe( 'ImageUploadEditing', () => {
 
 		viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } );
 	} );
+
+	it( 'should not show notification when file loader failed with no error', done => {
+		const notification = editor.plugins.get( Notification );
+
+		let notificationsCount = 0;
+		// Prevent popping up alert window.
+		notification.on( 'show:warning', evt => {
+			notificationsCount++;
+			evt.stop();
+		}, { priority: 'high' } );
+
+		// Check data after paste.
+		editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => {
+			adapterMocks[ 0 ].loader.file.then( () => {
+				expect.fail( 'Promise should be rejected.' );
+			} ).catch( () => {
+				// Deffer so the promise could be resolved.
+				setTimeout( () => {
+					expect( notificationsCount ).to.equal( 0 );
+					done();
+				} );
+			} );
+		}, { priority: 'low' } );
+
+		setModelData( model, '<paragraph>[]foo</paragraph>' );
+
+		const clipboardHtml = `<img src=${ base64Sample } />`;
+		const dataTransfer = mockDataTransfer( clipboardHtml );
+
+		const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) );
+		const targetViewRange = editor.editing.mapper.toViewRange( targetRange );
+
+		// Stub `fetch` in a way that it always fails.
+		testUtils.sinon.stub( window, 'fetch' ).callsFake( () => Promise.reject() );
+
+		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.