diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index ed6783f8..98977b11 100644 --- a/src/imageupload/imageuploadediting.js +++ b/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' diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index fc61910c..5ebb9f07 100644 --- a/src/imageupload/utils.js +++ b/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.} 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 ); } ); } diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 37bf4e5f..ee745c80 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/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 = '[]foo'; + 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( + '', + '[]foo', + '[]foo', + done, + false + ); setModelData( model, '[]foo' ); @@ -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 = 'bar' + - `` + - `[]` + - 'foo'; + 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 = 'bar' + + '' + + '' + + '[]' + + 'foo'; + const expectedFinalModel = 'bar' + + '' + + '[]' + + 'foo'; + + expectData( + '', + expectedModel, + expectedFinalModel, + done + ); setModelData( model, '[]foo' ); @@ -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 = 'baz[]foo'; + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + expectData( + '

baz

', + 'baz[]foo', + 'baz[]foo', + err => { + window.File = fileFn; + done( err ); + }, + false + ); setModelData( model, '[]foo' ); @@ -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 = 'baz[]foo'; + // Prevent popping up alert window. + notification.on( 'show:warning', evt => { + evt.stop(); + }, { priority: 'high' } ); - expectModel( done, getModelData( model ), expected ); - }, { priority: 'low' } ); + expectData( + '

baz

', + 'baz[]foo', + 'baz[]foo', + done, + false + ); setModelData( model, '[]foo' ); @@ -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, '[]foo' ); + + const clipboardHtml = ``; + 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.} 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.