Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

The image uploading listener for handling base64/blob images no longer stops inputTransformation event #264

Merged
merged 12 commits into from
Jan 22, 2019
Merged

The image uploading listener for handling base64/blob images no longer stops inputTransformation event #264

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
3aa606d
Tests: Adjusted tests to changes in upload plugin.
f1ames Dec 7, 2018
c6b54b7
Merge branch 'master' into t/ckeditor5-upload/87
f1ames Dec 7, 2018
d7ac45f
Pass promise instead of a file instance to loader while processing pa…
f1ames Dec 17, 2018
9c7c2b2
Merge branch 'master' into t/ckeditor5-upload/87
f1ames Dec 17, 2018
07dac82
Merge branch 'master' into t/258
f1ames Jan 11, 2019
168c5f1
Tests: Prevent alert logs.
f1ames Jan 11, 2019
a964f16
Tests: Fix test for Edge.
f1ames Jan 11, 2019
4be2897
Tests: Fix stubbing for Edge test.
f1ames Jan 14, 2019
876cc3c
Update src/imageupload/utils.js
jodator Jan 15, 2019
c89e211
Refactoring.
f1ames Jan 15, 2019
343afd7
Merge branch 'master' into t/258
f1ames Jan 15, 2019
f5a52f3
Do not show warning on empty error.
f1ames Jan 16, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 10 additions & 21 deletions src/imageupload/imageuploadediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm I'm worried about that construct. I think that we could expose those methods on the view if they're need. Tahen we could write it as:

const view = editor.editing.view; // ps.: also used for crateRangeIn()

/// the code as now

view.setAttribute( 'src', '', fetchableImage.imageElement );

This unfortunately will require small changes in the engine.

Edit: I've just found this construct in the Clipboard docs. I'll report an issue there. So do not change this ATM.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ps.: reporter here: https://github.com/ckeditor/ckeditor5-clipboard/issues/46#issuecomment-454025196

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I'll leave it as is for now. Btw. one may use change block here:

editor.editing.view.change( writer => {
    // code...
} );

to obtain the writer. However, it uses DowncastWriter not UpcastWriter (still their setAttribute() methods uses exactly same API).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking abut it and while it looks safe we probably ought to fix it globally as in referenced ticket.


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 );
} );
}
} );
}

Expand Down
18 changes: 9 additions & 9 deletions src/imageupload/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
f1ames marked this conversation as resolved.
Show resolved Hide resolved
* 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( () => {
f1ames marked this conversation as resolved.
Show resolved Hide resolved
// We always resolve a promise so `Promise.all` will not reject if one of many fetch fails.
resolve( { image, file: null } );
reject();
} );
} );
}
Expand Down
162 changes: 135 additions & 27 deletions tests/imageupload/imageuploadediting.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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>' );

Expand All @@ -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>' );

Expand Down Expand Up @@ -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>' );

Expand All @@ -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>' );

Expand Down Expand Up @@ -822,8 +866,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.
Expand Down