From 609accf9b6465c06bb6b55486e9c4a97f517a660 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 14 Nov 2018 11:39:18 +0100 Subject: [PATCH 01/11] Support for uploading base64 images. --- src/imageupload/imageuploadediting.js | 161 +++++++++++++++++++++----- 1 file changed, 134 insertions(+), 27 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index f09c488d..03cd350f 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -7,6 +7,8 @@ * @module image/imageupload/imageuploadediting */ +/* global fetch, File */ + import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository'; import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; @@ -74,6 +76,41 @@ export default class ImageUploadEditing extends Plugin { } ); } ); + // Handle images inserted or modified with base64 source. + doc.on( 'change', () => { + const changes = doc.differ.getChanges( { includeChangesInGraveyard: false } ); + const imagesToUpload = []; + + for ( const entry of changes ) { + let item = null; + + if ( entry.type == 'insert' && entry.name == 'image' ) { + // Process entry item if it was an image insertion. + item = entry.position.nodeAfter; + } else if ( entry.type == 'attribute' && entry.attributeKey == 'src' ) { + // Process entry item if it was modification of `src` attribute of an image element. + // Such cases may happen when image with `blob` source is inserted and then have it + // converted to base64 data by clipboard pipeline. + const el = entry.range.start.nodeAfter; + + // Check if modified element is an image element. + if ( el && el.is( 'image' ) ) { + item = el; + } + } + + if ( item && !item.getAttribute( 'uploadId' ) && item.getAttribute( 'src' ) && + item.getAttribute( 'src' ).match( /data:image\/\w+;base64/ ) ) { + imagesToUpload.push( item ); + } + } + + // Upload images with base64 sources. + if ( imagesToUpload.length ) { + this._uploadBase64Images( imagesToUpload, editor ); + } + } ); + // Prevents from the browser redirecting to the dropped image. editor.editing.view.document.on( 'dragover', ( evt, data ) => { data.preventDefault(); @@ -156,33 +193,7 @@ export default class ImageUploadEditing extends Plugin { .then( data => { model.enqueueChange( 'transparent', writer => { writer.setAttributes( { uploadStatus: 'complete', src: data.default }, imageElement ); - - // Srcset attribute for responsive images support. - let maxWidth = 0; - const srcsetAttribute = Object.keys( data ) - // Filter out keys that are not integers. - .filter( key => { - const width = parseInt( key, 10 ); - - if ( !isNaN( width ) ) { - maxWidth = Math.max( maxWidth, width ); - - return true; - } - } ) - - // Convert each key to srcset entry. - .map( key => `${ data[ key ] } ${ key }w` ) - - // Join all entries. - .join( ', ' ); - - if ( srcsetAttribute != '' ) { - writer.setAttribute( 'srcset', { - data: srcsetAttribute, - width: maxWidth - }, imageElement ); - } + this._parseAndSetSrcsetAttributeOnImage( data, imageElement, writer ); } ); clean(); @@ -219,6 +230,85 @@ export default class ImageUploadEditing extends Plugin { fileRepository.destroyLoader( loader ); } } + + /** + * Converts and uploads base64 `src` data of all given images. On successful upload + * the image `src` attribute is replaced with the URL of the remote file. + * + * @protected + * @param {Array.} images Array of image elements to upload. + * @param {module:core/editor/editor~Editor} editor The editor instance. + */ + _uploadBase64Images( images, editor ) { + const fileRepository = editor.plugins.get( FileRepository ); + + for ( const image of images ) { + const src = image.getAttribute( 'src' ); + const ext = src.match( /data:image\/(\w+);base64/ )[ 1 ]; + + // Fetch works asynchronously and so does not block browser UI when processing data. + fetch( src ) + .then( resource => resource.blob() ) + .then( blob => { + const filename = `${ Number( new Date() ) }-image.${ ext }`; + const file = createFileFromBlob( blob, filename ); + + if ( !file ) { + throw new Error( 'File API not supported. Cannot create `File` from `Blob`.' ); + } + + return fileRepository.createLoader( file ).upload(); + } ) + .then( data => { + editor.model.enqueueChange( 'transparent', writer => { + writer.setAttribute( 'src', data.default, image ); + this._parseAndSetSrcsetAttributeOnImage( data, image, writer ); + } ); + } ) + .catch( () => { + // As upload happens in the background without direct user interaction, + // no errors notifications should be shown. + } ); + } + } + + /** + * Creates `srcset` attribute based on a given file upload response and sets it as an attribute to a specific image element. + * + * @protected + * @param {Object} data Data object from which `srcset` will be created. + * @param {module:engine/model/element~Element} image The image element on which `srcset` attribute will be set. + * @param {module:engine/model/writer~Writer} writer + */ + _parseAndSetSrcsetAttributeOnImage( data, image, writer ) { + // Srcset attribute for responsive images support. + let maxWidth = 0; + + const srcsetAttribute = Object.keys( data ) + // Filter out keys that are not integers. + .filter( key => { + const width = parseInt( key, 10 ); + + if ( !isNaN( width ) ) { + maxWidth = Math.max( maxWidth, width ); + + return true; + } + } ) + + // Convert each key to srcset entry. + .map( key => `${ data[ key ] } ${ key }w` ) + + // Join all entries. + .join( ', ' ); + + if ( srcsetAttribute != '' ) { + writer.setAttribute( 'srcset', { + data: srcsetAttribute, + width: maxWidth + }, image ); + } + } } // Returns `true` if non-empty `text/html` is included in the data transfer. @@ -228,3 +318,20 @@ export default class ImageUploadEditing extends Plugin { export function isHtmlIncluded( dataTransfer ) { return Array.from( dataTransfer.types ).includes( 'text/html' ) && dataTransfer.getData( 'text/html' ) !== ''; } + +// Creates `File` instance from the given `Blob` instance using specified filename. +// +// @param {Blob} blob The `Blob` instance from which file will be created. +// @param {String} filename Filename used during file creation. +// @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. +function createFileFromBlob( blob, filename ) { + if ( typeof File === 'function' ) { + return new File( [ blob ], filename ); + } else { + // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. + // The `Blob` object could be used, however it causes the issue with upload itself where filename is read directly + // from a `File` instance. Since `Blob` instance does not provide one, the default "blob" filename is used which + // doesn't work well with most upload adapters (same name for every file + checking file type by extension fails). + return null; + } +} From 07bb3adda612042d9d0c2032ce9448d0f8eac3b4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 14 Nov 2018 11:39:35 +0100 Subject: [PATCH 02/11] Tests: Support for uploading base64 images. --- tests/imageupload/imageuploadediting.js | 220 +++++++++++++++++++++++- 1 file changed, 219 insertions(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index afe81bd2..e8e34ab1 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -30,7 +30,13 @@ import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; describe( 'ImageUploadEditing', () => { // eslint-disable-next-line max-len const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; - let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader, adapterMock; + let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader, adapterMock, uploadStartedCallback; + + const windowFilePolyfill = function( parts, filename, properties ) { + this.name = filename; + this.parts = parts; + this.properties = properties; + }; testUtils.createSinonSandbox(); @@ -41,6 +47,10 @@ describe( 'ImageUploadEditing', () => { loader = newLoader; adapterMock = new UploadAdapterMock( loader ); + if ( uploadStartedCallback ) { + adapterMock.uploadStartedCallback = uploadStartedCallback; + } + return adapterMock; }; } @@ -56,6 +66,11 @@ describe( 'ImageUploadEditing', () => { return nativeReaderMock; } ); + // Use `File` polyfill so test can be run on Edge too (which lacks `File` constructor). + if ( !window.File ) { + window.File = windowFilePolyfill; + } + return VirtualTestEditor .create( { plugins: [ ImageEditing, ImageUploadEditing, Paragraph, UndoEditing, UploadAdapterPluginMock ] @@ -70,6 +85,13 @@ describe( 'ImageUploadEditing', () => { } ); afterEach( () => { + adapterMock = null; + uploadStartedCallback = null; + + if ( window.File === windowFilePolyfill ) { + window.File = null; + } + return editor.destroy(); } ); @@ -548,4 +570,200 @@ describe( 'ImageUploadEditing', () => { expect( spy.calledOnce ).to.equal( true ); } ); + + it( 'should upload image inserted with base64 data', done => { + setModelData( model, '[]foo' ); + + // Since upload starts asynchronously, success needs to be triggered after upload is started. + uploadStartedCallback = function() { + adapterMock.mockSuccess( { default: 'image.png' } ); + }; + + model.document.once( 'change', () => { + model.document.once( 'change', () => { + expectModel( done, model, '[]foo' ); + } ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + + it( 'should upload if image src changed to base64 data', done => { + setModelData( model, 'bar[]' ); + + // Since upload starts asynchronously, success needs to be triggered after upload is started. + uploadStartedCallback = function() { + adapterMock.mockSuccess( { default: 'image2.jpeg' } ); + }; + + model.document.once( 'change', () => { + model.document.once( 'change', () => { + expectModel( done, model, 'bar[]' ); + } ); + } ); + + model.change( writer => { + const range = writer.createRangeIn( model.document.getRoot() ); + + for ( const value of range ) { + if ( value.item.is( 'element', 'image' ) ) { + writer.setAttribute( 'src', base64Sample, value.item ); + } + } + } ); + } ); + + it( 'should create responsive image if server return multiple images from base64 image', done => { + setModelData( model, '[]foo' ); + + // Since upload starts asynchronously, success needs to be triggered after upload is started. + uploadStartedCallback = function() { + adapterMock.mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ); + }; + + model.document.once( 'change', () => { + model.document.once( 'change', () => { + expectModel( done, model, '[]foo' ); + } ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + + it( 'should not upload nor change image data when `File` constructor is not supported', done => { + const fileFn = window.File; + + window.File = undefined; + + setModelData( model, '[]foo' ); + + model.document.once( 'change', () => { + setTimeout( () => { + window.File = fileFn; + + expect( adapterMock ).to.null; + expectModel( done, model, `[]foo` ); + + done(); + }, 50 ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + + it( 'should not initialize upload if inserted image have uploadId', () => { + setModelData( model, '[]foo' ); + + const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); + const uploadSpy = sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + + model.document.once( 'change', () => { + expect( uploadSpy.callCount ).to.equal( 0 ); + expect( getModelData( model ) ).to.equal( + `[]foo` ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.setAttribute( 'uploadId', 42, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + + it( 'should not initialize upload if src of different element than image changed', () => { + setModelData( model, '[]bar' ); + + const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); + const uploadSpy = sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + + model.document.once( 'change', () => { + expect( uploadSpy.callCount ).to.equal( 0 ); + expect( getModelData( model ) ).to.equal( '[]bar' ); + } ); + + model.change( writer => { + const range = writer.createRangeIn( model.document.getRoot() ); + + for ( const value of range ) { + if ( value.item.is( 'element', 'paragraph' ) ) { + writer.setAttribute( 'src', 'bar', value.item ); + } + } + } ); + } ); + + it( 'should not change image src if upload aborted', done => { + setModelData( model, '[]foo' ); + + // Since upload starts asynchronously, success needs to be triggered after upload is started. + uploadStartedCallback = function() { + adapterMock.abort(); + }; + + model.document.once( 'change', () => { + setTimeout( () => { + expect( adapterMock.loader.status ).to.equal( 'error' ); + expectModel( done, model, `[]foo` ); + }, 50 ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + + it( 'should not throw error and not change image src if upload errored', done => { + setModelData( model, '[]foo' ); + + // Since upload starts asynchronously, success needs to be triggered after upload is started. + uploadStartedCallback = function() { + adapterMock.mockError( 'Upload failed.' ); + }; + + model.document.once( 'change', () => { + setTimeout( () => { + expect( adapterMock.loader.status ).to.equal( 'error' ); + expectModel( done, model, `[]foo` ); + }, 50 ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); } ); + +// Since this function is run inside Promise, all errors needs to be caught +// and rethrow to be correctly processed by testing framework. +function expectModel( done, model, expected ) { + try { + expect( getModelData( model ) ).to.equal( expected ); + done(); + } catch ( err ) { + done( err ); + } +} From 648f03cd28c56bc8876406447c04e23e87d2e5f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 14 Nov 2018 17:52:56 +0100 Subject: [PATCH 03/11] Small adjustments for Edge browser. --- src/imageupload/imageuploadediting.js | 10 ++-- tests/imageupload/imageuploadediting.js | 61 +++++++++++++++++-------- 2 files changed, 48 insertions(+), 23 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 03cd350f..97f6e3f7 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -325,13 +325,13 @@ export function isHtmlIncluded( dataTransfer ) { // @param {String} filename Filename used during file creation. // @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. function createFileFromBlob( blob, filename ) { - if ( typeof File === 'function' ) { + try { return new File( [ blob ], filename ); - } else { + } catch ( err ) { // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. - // The `Blob` object could be used, however it causes the issue with upload itself where filename is read directly - // from a `File` instance. Since `Blob` instance does not provide one, the default "blob" filename is used which - // doesn't work well with most upload adapters (same name for every file + checking file type by extension fails). + // However, the `File` function is present (so cannot be checked with `!window.File` or `typeof File === 'function'`), but + // calling it with `new File( ... )` throws an error. This try-catch prevents that. Also when the function will + // be implemented correctly in Edge the code will start working without any changes (see #247). return null; } } diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index e8e34ab1..b01d8b37 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -32,12 +32,6 @@ describe( 'ImageUploadEditing', () => { const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader, adapterMock, uploadStartedCallback; - const windowFilePolyfill = function( parts, filename, properties ) { - this.name = filename; - this.parts = parts; - this.properties = properties; - }; - testUtils.createSinonSandbox(); class UploadAdapterPluginMock extends Plugin { @@ -57,6 +51,12 @@ describe( 'ImageUploadEditing', () => { } beforeEach( () => { + if ( env.isEdge ) { + testUtils.sinon.stub( window, 'File' ).callsFake( () => { + return { name: 'file.jpg' }; + } ); + } + // Most tests assume non-edge environment but we do not set `contenteditable=false` on Edge so stub `env.isEdge`. testUtils.sinon.stub( env, 'isEdge' ).get( () => false ); @@ -66,11 +66,6 @@ describe( 'ImageUploadEditing', () => { return nativeReaderMock; } ); - // Use `File` polyfill so test can be run on Edge too (which lacks `File` constructor). - if ( !window.File ) { - window.File = windowFilePolyfill; - } - return VirtualTestEditor .create( { plugins: [ ImageEditing, ImageUploadEditing, Paragraph, UndoEditing, UploadAdapterPluginMock ] @@ -88,10 +83,6 @@ describe( 'ImageUploadEditing', () => { adapterMock = null; uploadStartedCallback = null; - if ( window.File === windowFilePolyfill ) { - window.File = null; - } - return editor.destroy(); } ); @@ -641,7 +632,10 @@ describe( 'ImageUploadEditing', () => { } ); } ); - it( 'should not upload nor change image data when `File` constructor is not supported', done => { + it( 'should not upload nor change image data when `File` constructor is not present', done => { + // Restore `File` stub. + testUtils.sinon.restore(); + const fileFn = window.File; window.File = undefined; @@ -667,11 +661,42 @@ describe( 'ImageUploadEditing', () => { } ); } ); + it( 'should not upload nor change image data when `File` constructor is not supported', done => { + // Restore `File` stub. + testUtils.sinon.restore(); + + const fileFn = window.File; + + window.File = function() { + throw new Error( 'Function expected.' ); // Simulating Edge browser behaviour here. + }; + + setModelData( model, '[]foo' ); + + model.document.once( 'change', () => { + setTimeout( () => { + window.File = fileFn; + + expect( adapterMock ).to.null; + expectModel( done, model, `[]foo` ); + + done(); + }, 50 ); + } ); + + model.change( writer => { + const image = writer.createElement( 'image' ); + + writer.setAttribute( 'src', base64Sample, image ); + writer.insert( image, model.document.selection.getFirstPosition() ); + } ); + } ); + it( 'should not initialize upload if inserted image have uploadId', () => { setModelData( model, '[]foo' ); const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); - const uploadSpy = sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + const uploadSpy = testUtils.sinon.spy( imageUploadEditing, '_uploadBase64Images' ); model.document.once( 'change', () => { expect( uploadSpy.callCount ).to.equal( 0 ); @@ -692,7 +717,7 @@ describe( 'ImageUploadEditing', () => { setModelData( model, '[]bar' ); const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); - const uploadSpy = sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + const uploadSpy = testUtils.sinon.spy( imageUploadEditing, '_uploadBase64Images' ); model.document.once( 'change', () => { expect( uploadSpy.callCount ).to.equal( 0 ); From 292a7965491fefde17b33ec5fc6617d03c3827de Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 20 Nov 2018 20:38:52 +0100 Subject: [PATCH 04/11] Process local images inside clipboard pipeline and integrate with image upload flow. --- package.json | 2 +- src/imageupload/imageuploadediting.js | 143 ++++++++++---------------- src/imageupload/utils.js | 75 ++++++++++++++ 3 files changed, 130 insertions(+), 90 deletions(-) diff --git a/package.json b/package.json index 38055919..27f10c63 100644 --- a/package.json +++ b/package.json @@ -10,6 +10,7 @@ "ckeditor5-plugin" ], "dependencies": { + "@ckeditor/ckeditor5-clipboard": "^10.0.3", "@ckeditor/ckeditor5-core": "^11.0.1", "@ckeditor/ckeditor5-engine": "^11.0.0", "@ckeditor/ckeditor5-theme-lark": "^11.1.0", @@ -20,7 +21,6 @@ }, "devDependencies": { "@ckeditor/ckeditor5-basic-styles": "^10.0.3", - "@ckeditor/ckeditor5-clipboard": "^10.0.3", "@ckeditor/ckeditor5-editor-classic": "^11.0.1", "@ckeditor/ckeditor5-enter": "^10.1.2", "@ckeditor/ckeditor5-essentials": "^10.1.2", diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 97f6e3f7..e119ab33 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -7,14 +7,13 @@ * @module image/imageupload/imageuploadediting */ -/* global fetch, File */ - import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository'; import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; +import { upcastAttributeToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters'; import ImageUploadCommand from '../../src/imageupload/imageuploadcommand'; -import { isImageType } from '../../src/imageupload/utils'; +import { isImageType, isLocalImage, wrapImageToFetch } from '../../src/imageupload/utils'; /** * The editing part of the image upload feature. @@ -36,6 +35,7 @@ export default class ImageUploadEditing extends Plugin { const editor = this.editor; const doc = editor.model.document; const schema = editor.model.schema; + const conversion = editor.conversion; const fileRepository = editor.plugins.get( FileRepository ); // Setup schema to allow uploadId and uploadStatus for images. @@ -46,6 +46,16 @@ export default class ImageUploadEditing extends Plugin { // Register imageUpload command. editor.commands.add( 'imageUpload', new ImageUploadCommand( editor ) ); + // Register upcast converter for uploadId. + conversion.for( 'upcast' ) + .add( upcastAttributeToAttribute( { + view: { + name: 'img', + key: 'uploadId' + }, + model: 'uploadId' + } ) ); + // Handle pasted images. // For every image file, a new file loader is created and a placeholder image is // inserted into the content. Then, those images are uploaded once they appear in the model @@ -76,39 +86,52 @@ export default class ImageUploadEditing extends Plugin { } ); } ); - // Handle images inserted or modified with base64 source. - doc.on( 'change', () => { - const changes = doc.differ.getChanges( { includeChangesInGraveyard: false } ); - const imagesToUpload = []; + // Handle HTML pasted with images with base64 or blob sources. + // For every image file, a new file loader is created and a placeholder image is + // inserted into the content. Then, those images are uploaded once they appear in the model + // (see Document#change listener below). + this.listenTo( editor.plugins.get( 'Clipboard' ), 'inputTransformation', ( evt, data ) => { + const view = editor.editing.view; - for ( const entry of changes ) { - let item = null; + const fetchableImages = Array.from( view.createRangeIn( data.content ) ) + .filter( value => isLocalImage( value.item ) && !value.item.getAttribute( 'uploadProcessed' ) ) + .map( ( value, index ) => wrapImageToFetch( value.item, index ) ); - if ( entry.type == 'insert' && entry.name == 'image' ) { - // Process entry item if it was an image insertion. - item = entry.position.nodeAfter; - } else if ( entry.type == 'attribute' && entry.attributeKey == 'src' ) { - // Process entry item if it was modification of `src` attribute of an image element. - // Such cases may happen when image with `blob` source is inserted and then have it - // converted to base64 data by clipboard pipeline. - const el = entry.range.start.nodeAfter; - - // Check if modified element is an image element. - if ( el && el.is( 'image' ) ) { - item = el; - } - } + if ( !fetchableImages.length ) { + return; + } - if ( item && !item.getAttribute( 'uploadId' ) && item.getAttribute( 'src' ) && - item.getAttribute( 'src' ).match( /data:image\/\w+;base64/ ) ) { - imagesToUpload.push( item ); + evt.stop(); + + Promise.all( fetchableImages ).then( items => { + for ( const item of items ) { + if ( !item.file ) { + // Failed to fetch image or create a file instance, remove image element. + view.change( writer => { + writer.remove( item.image ); + } ); + } else { + const loader = fileRepository.createLoader( item.file ); + + if ( loader ) { + view.change( writer => { + writer.setAttribute( 'src', '', item.image ); + writer.setAttribute( 'uploadId', loader.id, item.image ); + } ); + } else { + view.change( writer => { + // Set attribute so the image will not be processed 2nd time. + writer.setAttribute( 'uploadProcessed', true, item.image ); + } ); + } + } } - } - // Upload images with base64 sources. - if ( imagesToUpload.length ) { - this._uploadBase64Images( imagesToUpload, editor ); - } + editor.plugins.get( 'Clipboard' ).fire( 'inputTransformation', { + content: data.content, + dataTransfer: data.dataTransfer + } ); + } ); } ); // Prevents from the browser redirecting to the dropped image. @@ -231,47 +254,6 @@ export default class ImageUploadEditing extends Plugin { } } - /** - * Converts and uploads base64 `src` data of all given images. On successful upload - * the image `src` attribute is replaced with the URL of the remote file. - * - * @protected - * @param {Array.} images Array of image elements to upload. - * @param {module:core/editor/editor~Editor} editor The editor instance. - */ - _uploadBase64Images( images, editor ) { - const fileRepository = editor.plugins.get( FileRepository ); - - for ( const image of images ) { - const src = image.getAttribute( 'src' ); - const ext = src.match( /data:image\/(\w+);base64/ )[ 1 ]; - - // Fetch works asynchronously and so does not block browser UI when processing data. - fetch( src ) - .then( resource => resource.blob() ) - .then( blob => { - const filename = `${ Number( new Date() ) }-image.${ ext }`; - const file = createFileFromBlob( blob, filename ); - - if ( !file ) { - throw new Error( 'File API not supported. Cannot create `File` from `Blob`.' ); - } - - return fileRepository.createLoader( file ).upload(); - } ) - .then( data => { - editor.model.enqueueChange( 'transparent', writer => { - writer.setAttribute( 'src', data.default, image ); - this._parseAndSetSrcsetAttributeOnImage( data, image, writer ); - } ); - } ) - .catch( () => { - // As upload happens in the background without direct user interaction, - // no errors notifications should be shown. - } ); - } - } - /** * Creates `srcset` attribute based on a given file upload response and sets it as an attribute to a specific image element. * @@ -318,20 +300,3 @@ export default class ImageUploadEditing extends Plugin { export function isHtmlIncluded( dataTransfer ) { return Array.from( dataTransfer.types ).includes( 'text/html' ) && dataTransfer.getData( 'text/html' ) !== ''; } - -// Creates `File` instance from the given `Blob` instance using specified filename. -// -// @param {Blob} blob The `Blob` instance from which file will be created. -// @param {String} filename Filename used during file creation. -// @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. -function createFileFromBlob( blob, filename ) { - try { - return new File( [ blob ], filename ); - } catch ( err ) { - // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. - // However, the `File` function is present (so cannot be checked with `!window.File` or `typeof File === 'function'`), but - // calling it with `new File( ... )` throws an error. This try-catch prevents that. Also when the function will - // be implemented correctly in Edge the code will start working without any changes (see #247). - return null; - } -} diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index 89943aa4..5f1f49d2 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -7,6 +7,8 @@ * @module image/imageupload/utils */ +/* global fetch, File */ + /** * Checks if a given file is an image. * @@ -18,3 +20,76 @@ export function isImageType( file ) { return types.test( file.type ); } + +/** + * Creates a promise which fetches the image local source (base64 or blob) and returns as a `File` object. + * + * @param {module:engine/view/element~Element} image Image which source to fetch. + * @param {Number} index Image index used as image name suffix. + * @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. + */ +export function wrapImageToFetch( image, index ) { + return new Promise( resolve => { + // Fetch works asynchronously and so does not block browser UI when processing data. + fetch( image.getAttribute( 'src' ) ) + .then( resource => resource.blob() ) + .then( blob => { + const ext = getImageFileType( blob, image.getAttribute( 'src' ) ); + const filename = `${ Number( new Date() ) }-image${ index }.${ ext }`; + const file = createFileFromBlob( blob, filename ); + + resolve( { image, file } ); + } ) + .catch( () => { + // We always resolve a promise so `Promise.all` will not reject if one of many fetch fails. + resolve( { image, file: null } ); + } ); + } ); +} + +/** + * Checks whether given node is an image element with local source (base64 or blob). + * + * @param {module:engine/view/node~Node} node Node to check. + * @returns {Boolean} + */ +export function isLocalImage( node ) { + return node.is( 'element', 'img' ) && node.getAttribute( 'src' ) && + ( node.getAttribute( 'src' ).match( /data:image\/\w+;base64,/g ) || + node.getAttribute( 'src' ).match( /blob:/g ) ); +} + +// Extracts image type based on its blob representation or its source. +// +// @param {String} src Image src attribute value. +// @param {Blob} blob Image blob representation. +// @returns {String} +function getImageFileType( blob, src ) { + if ( blob.type ) { + return blob.type.replace( 'image/', '' ); + } else if ( src.match( /data:image\/(\w+);base64/ ) ) { + return src.match( /data:image\/(\w+);base64/ )[ 1 ].toLowerCase(); + } else { + // Fallback to 'jpeg' as common extension. + return 'jpeg'; + } +} + +// Creates `File` instance from the given `Blob` instance using specified filename. +// +// @param {Blob} blob The `Blob` instance from which file will be created. +// @param {String} filename Filename used during file creation. +// @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. +function createFileFromBlob( blob, filename ) { + try { + return new File( [ blob ], filename ); + } catch ( err ) { + // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. + // However, the `File` function is present (so cannot be checked with `!window.File` or `typeof File === 'function'`), but + // calling it with `new File( ... )` throws an error. This try-catch prevents that. Also when the function will + // be implemented correctly in Edge the code will start working without any changes (see #247). + return null; + } +} From 42a992a26c90c4be6c733c891ed10294b454427b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 20 Nov 2018 20:40:07 +0100 Subject: [PATCH 05/11] Tests: Images with base64 and blob sources upload tests. --- tests/imageupload.js | 3 +- tests/imageupload/imageuploadediting.js | 406 +++++++++++++---------- tests/imageupload/imageuploadprogress.js | 3 +- tests/imageupload/imageuploadui.js | 3 +- 4 files changed, 240 insertions(+), 175 deletions(-) diff --git a/tests/imageupload.js b/tests/imageupload.js index 79ef97a5..a63f6e98 100644 --- a/tests/imageupload.js +++ b/tests/imageupload.js @@ -6,6 +6,7 @@ /* globals document */ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; +import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard'; import Image from '../src/image'; import ImageUpload from '../src/imageupload'; import ImageUploadEditing from '../src/imageupload/imageuploadediting'; @@ -23,7 +24,7 @@ describe( 'ImageUpload', () => { return ClassicEditor .create( editorElement, { - plugins: [ Image, ImageUpload, UploadAdapterPluginMock ] + plugins: [ Image, ImageUpload, UploadAdapterPluginMock, Clipboard ] } ) .then( newEditor => { editor = newEditor; diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index b01d8b37..c55f7939 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -3,7 +3,7 @@ * For licensing, see LICENSE.md. */ -/* globals window, setTimeout */ +/* globals window, setTimeout, atob, URL, Blob */ import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor'; @@ -30,7 +30,9 @@ import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; describe( 'ImageUploadEditing', () => { // eslint-disable-next-line max-len const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; - let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader, adapterMock, uploadStartedCallback; + + let adapterMocks = []; + let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader; testUtils.createSinonSandbox(); @@ -39,11 +41,9 @@ describe( 'ImageUploadEditing', () => { fileRepository = this.editor.plugins.get( FileRepository ); fileRepository.createUploadAdapter = newLoader => { loader = newLoader; - adapterMock = new UploadAdapterMock( loader ); + const adapterMock = new UploadAdapterMock( loader ); - if ( uploadStartedCallback ) { - adapterMock.uploadStartedCallback = uploadStartedCallback; - } + adapterMocks.push( adapterMock ); return adapterMock; }; @@ -68,7 +68,7 @@ describe( 'ImageUploadEditing', () => { return VirtualTestEditor .create( { - plugins: [ ImageEditing, ImageUploadEditing, Paragraph, UndoEditing, UploadAdapterPluginMock ] + plugins: [ ImageEditing, ImageUploadEditing, Paragraph, UndoEditing, UploadAdapterPluginMock, Clipboard ] } ) .then( newEditor => { editor = newEditor; @@ -76,12 +76,14 @@ describe( 'ImageUploadEditing', () => { doc = model.document; view = editor.editing.view; viewDocument = view.document; + + // Stub `view.scrollToTheSelection` as it will fail on VirtualTestEditor without DOM. + testUtils.sinon.stub( view, 'scrollToTheSelection' ).callsFake( () => {} ); } ); } ); afterEach( () => { - adapterMock = null; - uploadStartedCallback = null; + adapterMocks = []; return editor.destroy(); } ); @@ -198,7 +200,11 @@ describe( 'ImageUploadEditing', () => { type: 'media/mp3', size: 1024 }; - const dataTransfer = new DataTransfer( { files: [ fileMock ], types: [ 'Files' ] } ); + const dataTransfer = new DataTransfer( { + files: [ fileMock ], + types: [ 'Files' ], + getData: () => '' + } ); setModelData( model, 'foo[]' ); @@ -224,7 +230,7 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); - expect( getModelData( model ) ).to.equal( '[]foo' ); + expect( getModelData( model ) ).to.equal( 'SomeData[]foo' ); } ); it( 'should not insert image nor crash when pasted image could not be inserted', () => { @@ -288,7 +294,7 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); // Well, there's no clipboard plugin, so nothing happens. - expect( getModelData( model ) ).to.equal( '[]foo' ); + expect( getModelData( model ) ).to.equal( 'SomeData[]foo' ); } ); it( 'should not convert image\'s uploadId attribute if is consumed already', () => { @@ -339,7 +345,7 @@ describe( 'ImageUploadEditing', () => { done(); }, { priority: 'lowest' } ); - adapterMock.mockSuccess( { default: 'image.png' } ); + adapterMocks[ 0 ].mockSuccess( { default: 'image.png' } ); } ); nativeReaderMock.mockSuccess( base64Sample ); @@ -546,7 +552,7 @@ describe( 'ImageUploadEditing', () => { done(); }, { priority: 'lowest' } ); - adapterMock.mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ); + adapterMocks[ 0 ].mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ); } ); nativeReaderMock.mockSuccess( base64Sample ); @@ -562,233 +568,289 @@ describe( 'ImageUploadEditing', () => { expect( spy.calledOnce ).to.equal( true ); } ); - it( 'should upload image inserted with base64 data', done => { - setModelData( model, '[]foo' ); + it( 'should upload image with base64 src', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + const id = adapterMocks[ 0 ].loader.id; + const expected = 'bar' + + `[]` + + 'foo'; - // Since upload starts asynchronously, success needs to be triggered after upload is started. - uploadStartedCallback = function() { - adapterMock.mockSuccess( { default: 'image.png' } ); - }; + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); - model.document.once( 'change', () => { - model.document.once( 'change', () => { - expectModel( done, model, '[]foo' ); - } ); - } ); + setModelData( model, '[]foo' ); - model.change( writer => { - const image = writer.createElement( 'image' ); + const clipboardHtml = `

bar

`; + const dataTransfer = mockDataTransfer( clipboardHtml ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); - } ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should upload if image src changed to base64 data', done => { - setModelData( model, 'bar[]' ); + it( 'should upload image with blob src', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + const id = adapterMocks[ 0 ].loader.id; + const expected = `[]` + + 'foo'; - // Since upload starts asynchronously, success needs to be triggered after upload is started. - uploadStartedCallback = function() { - adapterMock.mockSuccess( { default: 'image2.jpeg' } ); - }; + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); - model.document.once( 'change', () => { - model.document.once( 'change', () => { - expectModel( done, model, 'bar[]' ); - } ); - } ); + setModelData( model, '[]foo' ); - model.change( writer => { - const range = writer.createRangeIn( model.document.getRoot() ); + const clipboardHtml = ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); - for ( const value of range ) { - if ( value.item.is( 'element', 'image' ) ) { - writer.setAttribute( 'src', base64Sample, value.item ); - } - } - } ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should create responsive image if server return multiple images from base64 image', done => { - setModelData( model, '[]foo' ); + it( 'should not upload image if no loader available', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + const expected = `[]foo`; - // Since upload starts asynchronously, success needs to be triggered after upload is started. - uploadStartedCallback = function() { - adapterMock.mockSuccess( { default: 'image.png', 500: 'image-500.png', 800: 'image-800.png' } ); - }; + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); - model.document.once( 'change', () => { - model.document.once( 'change', () => { - expectModel( done, model, '[]foo' ); - } ); - } ); + testUtils.sinon.stub( fileRepository, 'createLoader' ).callsFake( () => null ); - model.change( writer => { - const image = writer.createElement( 'image' ); + setModelData( model, '[]foo' ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); - } ); - } ); + 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 ); - it( 'should not upload nor change image data when `File` constructor is not present', done => { - // Restore `File` stub. - testUtils.sinon.restore(); + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); + } ); - const fileFn = window.File; + it( 'should not upload and remove image if fetch failed', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + const expected = '[]foo'; - window.File = undefined; + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); setModelData( model, '[]foo' ); - model.document.once( 'change', () => { - setTimeout( () => { - window.File = fileFn; + const clipboardHtml = ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); - expect( adapterMock ).to.null; - expectModel( done, model, `[]foo` ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); - done(); - }, 50 ); + // Stub `fetch` so it can be rejected. + testUtils.sinon.stub( window, 'fetch' ).callsFake( () => { + return new Promise( ( res, rej ) => rej() ); } ); - model.change( writer => { - const image = writer.createElement( 'image' ); + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); + } ); + + it( 'should upload only images which were successfully fetched and remove failed ones', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + const expected = 'bar' + + `` + + `[]` + + 'foo'; + + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); + + setModelData( model, '[]foo' ); + + const clipboardHtml = `

bar

` + + ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); + 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 2 first calls are successful and 3rd fails. + let counter = 0; + const fetch = window.fetch; + testUtils.sinon.stub( window, 'fetch' ).callsFake( src => { + counter++; + if ( counter < 3 ) { + return fetch( src ); + } else { + return new Promise( ( res, rej ) => rej() ); + } } ); - } ); - it( 'should not upload nor change image data when `File` constructor is not supported', done => { - // Restore `File` stub. - testUtils.sinon.restore(); + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); + } ); + it( 'should not upload and remove image when `File` constructor is not present', done => { const fileFn = window.File; - window.File = function() { - throw new Error( 'Function expected.' ); // Simulating Edge browser behaviour here. - }; + window.File = undefined; - setModelData( model, '[]foo' ); + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + window.File = fileFn; - model.document.once( 'change', () => { - setTimeout( () => { - window.File = fileFn; + const expected = 'baz[]foo'; - expect( adapterMock ).to.null; - expectModel( done, model, `[]foo` ); + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); - done(); - }, 50 ); - } ); + setModelData( model, '[]foo' ); - model.change( writer => { - const image = writer.createElement( 'image' ); + const clipboardHtml = `

baz

`; + const dataTransfer = mockDataTransfer( clipboardHtml ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); - } ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should not initialize upload if inserted image have uploadId', () => { - setModelData( model, '[]foo' ); + it( 'should not upload and remove image when `File` constructor is not supported', done => { + const fileFn = window.File; - const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); - const uploadSpy = testUtils.sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + window.File = function() { + throw new Error( 'Function expected.' ); // Simulating Edge browser behaviour here. + }; - model.document.once( 'change', () => { - expect( uploadSpy.callCount ).to.equal( 0 ); - expect( getModelData( model ) ).to.equal( - `[]foo` ); - } ); + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + window.File = fileFn; - model.change( writer => { - const image = writer.createElement( 'image' ); + const expected = 'baz[]foo'; - writer.setAttribute( 'src', base64Sample, image ); - writer.setAttribute( 'uploadId', 42, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); - } ); - } ); + expectModel( done, getModelData( model ), expected ); + }, { priority: 'low' } ); - it( 'should not initialize upload if src of different element than image changed', () => { - setModelData( model, '[]bar' ); + setModelData( model, '[]foo' ); - const imageUploadEditing = editor.plugins.get( ImageUploadEditing ); - const uploadSpy = testUtils.sinon.spy( imageUploadEditing, '_uploadBase64Images' ); + const clipboardHtml = `

baz

`; + const dataTransfer = mockDataTransfer( clipboardHtml ); - model.document.once( 'change', () => { - expect( uploadSpy.callCount ).to.equal( 0 ); - expect( getModelData( model ) ).to.equal( '[]bar' ); - } ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); - model.change( writer => { - const range = writer.createRangeIn( model.document.getRoot() ); + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); + } ); - for ( const value of range ) { - if ( value.item.is( 'element', 'paragraph' ) ) { - writer.setAttribute( 'src', 'bar', value.item ); - } + it( 'should get file extension from base64 string', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + try { + expect( loader.file.name.split( '.' ).pop() ).to.equal( 'png' ); + done(); + } catch ( err ) { + done( err ); } - } ); - } ); + }, { priority: 'low' } ); - it( 'should not change image src if upload aborted', done => { setModelData( model, '[]foo' ); - // Since upload starts asynchronously, success needs to be triggered after upload is started. - uploadStartedCallback = function() { - adapterMock.abort(); - }; - - model.document.once( 'change', () => { - setTimeout( () => { - expect( adapterMock.loader.status ).to.equal( 'error' ); - expectModel( done, model, `[]foo` ); - }, 50 ); - } ); + const clipboardHtml = ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); - model.change( writer => { - const image = writer.createElement( 'image' ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); + // Stub `fetch` to return custom blob without type. + testUtils.sinon.stub( window, 'fetch' ).callsFake( () => { + return new Promise( res => res( { + blob() { + return new Promise( res => res( new Blob( [ 'foo', 'bar' ] ) ) ); + } + } ) ); } ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should not throw error and not change image src if upload errored', done => { - setModelData( model, '[]foo' ); + it( 'should use fallback file extension', done => { + editor.plugins.get( 'Clipboard' ).on( 'inputTransformation', () => { + try { + expect( loader.file.name.split( '.' ).pop() ).to.equal( 'jpeg' ); + done(); + } catch ( err ) { + done( err ); + } + }, { priority: 'low' } ); - // Since upload starts asynchronously, success needs to be triggered after upload is started. - uploadStartedCallback = function() { - adapterMock.mockError( 'Upload failed.' ); - }; + setModelData( model, '[]foo' ); - model.document.once( 'change', () => { - setTimeout( () => { - expect( adapterMock.loader.status ).to.equal( 'error' ); - expectModel( done, model, `[]foo` ); - }, 50 ); - } ); + const clipboardHtml = ``; + const dataTransfer = mockDataTransfer( clipboardHtml ); - model.change( writer => { - const image = writer.createElement( 'image' ); + const targetRange = model.createRange( model.createPositionAt( doc.getRoot(), 1 ), model.createPositionAt( doc.getRoot(), 1 ) ); + const targetViewRange = editor.editing.mapper.toViewRange( targetRange ); - writer.setAttribute( 'src', base64Sample, image ); - writer.insert( image, model.document.selection.getFirstPosition() ); + // Stub `fetch` to return custom blob without type. + testUtils.sinon.stub( window, 'fetch' ).callsFake( () => { + return new Promise( res => res( { + blob() { + return new Promise( res => res( new Blob( [ 'foo', 'bar' ] ) ) ); + } + } ) ); } ); + + viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); } ); -// Since this function is run inside Promise, all errors needs to be caught -// and rethrow to be correctly processed by testing framework. -function expectModel( done, model, expected ) { +// Asserts actual and expected model data. +// Note: Since this function is run inside a promise, all errors needs to be caught +// and rethrow to be correctly processed by a testing framework. +// +// @param {function} done Callback function to be called when assertion is done. +// @param {String} actual Actual model data. +// @param {String} expected Expected model data. +function expectModel( done, actual, expected ) { try { - expect( getModelData( model ) ).to.equal( expected ); + expect( actual ).to.equal( expected ); done(); } catch ( err ) { done( err ); } } + +// Creates data transfer object with predefined data. +// +// @param {String} content The content returned as `text/html` when queried. +// @returns {module:clipboard/datatransfer~DataTransfer} DataTransfer object. +function mockDataTransfer( content ) { + return new DataTransfer( { + types: [ 'text/html' ], + getData: type => type === 'text/html' ? content : '' + } ); +} + +// Creates blob url from the given base64 data. +// +// @param {String} base64 The base64 string from which blob url will be generated. +// @returns {String} Blob url. +function base64ToBlobUrl( base64 ) { + return URL.createObjectURL( base64ToBlob( base64.trim() ) ); +} + +// Transforms base64 data into a blob object. +// +// @param {String} The base64 data to be transformed. +// @returns {Blob} Blob object representing given base64 data. +function base64ToBlob( base64Data ) { + const [ type, data ] = base64Data.split( ',' ); + const byteCharacters = atob( data ); + const byteArrays = []; + + for ( let offset = 0; offset < byteCharacters.length; offset += 512 ) { + const slice = byteCharacters.slice( offset, offset + 512 ); + const byteNumbers = new Array( slice.length ); + + for ( let i = 0; i < slice.length; i++ ) { + byteNumbers[ i ] = slice.charCodeAt( i ); + } + + byteArrays.push( new Uint8Array( byteNumbers ) ); + } + + return new Blob( byteArrays, { type } ); +} diff --git a/tests/imageupload/imageuploadprogress.js b/tests/imageupload/imageuploadprogress.js index ffef57b9..1b19b549 100644 --- a/tests/imageupload/imageuploadprogress.js +++ b/tests/imageupload/imageuploadprogress.js @@ -13,6 +13,7 @@ import ImageUploadEditing from '../../src/imageupload/imageuploadediting'; import ImageUploadProgress from '../../src/imageupload/imageuploadprogress'; 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 { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; @@ -54,7 +55,7 @@ describe( 'ImageUploadProgress', () => { return VirtualTestEditor .create( { - plugins: [ ImageEditing, Paragraph, ImageUploadEditing, ImageUploadProgress, UploadAdapterPluginMock ] + plugins: [ ImageEditing, Paragraph, ImageUploadEditing, ImageUploadProgress, UploadAdapterPluginMock, Clipboard ] } ) .then( newEditor => { editor = newEditor; diff --git a/tests/imageupload/imageuploadui.js b/tests/imageupload/imageuploadui.js index 68326d4a..971a537c 100644 --- a/tests/imageupload/imageuploadui.js +++ b/tests/imageupload/imageuploadui.js @@ -15,6 +15,7 @@ import ImageUploadUI from '../../src/imageupload/imageuploadui'; import ImageUploadEditing from '../../src/imageupload/imageuploadediting'; import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; +import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard'; import { createNativeFileMock, UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; @@ -37,7 +38,7 @@ describe( 'ImageUploadUI', () => { return ClassicEditor .create( editorElement, { - plugins: [ Paragraph, Image, ImageUploadEditing, ImageUploadUI, FileRepository, UploadAdapterPluginMock ] + plugins: [ Paragraph, Image, ImageUploadEditing, ImageUploadUI, FileRepository, UploadAdapterPluginMock, Clipboard ] } ) .then( newEditor => { editor = newEditor; From 9d8ee3eaddd966fd1883baae6802f17f5065bde3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 20 Nov 2018 20:52:56 +0100 Subject: [PATCH 06/11] Tests: Skip test depending on 'File' mock on Edge. --- tests/imageupload/imageuploadediting.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index c55f7939..39ce87eb 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -30,6 +30,7 @@ import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; describe( 'ImageUploadEditing', () => { // eslint-disable-next-line max-len const base64Sample = 'data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAQAAAC1HAwCAAAAC0lEQVR42mNk+A8AAQUBAScY42YAAAAASUVORK5CYII='; + const isEdgeEnv = env.isEdge; let adapterMocks = []; let editor, model, view, doc, fileRepository, viewDocument, nativeReaderMock, loader; @@ -51,7 +52,7 @@ describe( 'ImageUploadEditing', () => { } beforeEach( () => { - if ( env.isEdge ) { + if ( isEdgeEnv ) { testUtils.sinon.stub( window, 'File' ).callsFake( () => { return { name: 'file.jpg' }; } ); @@ -736,7 +737,8 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should get file extension from base64 string', done => { + // 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', () => { try { expect( loader.file.name.split( '.' ).pop() ).to.equal( 'png' ); @@ -766,7 +768,8 @@ describe( 'ImageUploadEditing', () => { viewDocument.fire( 'clipboardInput', { dataTransfer, targetRanges: [ targetViewRange ] } ); } ); - it( 'should use fallback file extension', done => { + // 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', () => { try { expect( loader.file.name.split( '.' ).pop() ).to.equal( 'jpeg' ); From 129b5d64da11eeeb5abf44670775cf0cc8e4b353 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Wed, 21 Nov 2018 17:03:38 +0100 Subject: [PATCH 07/11] Pass mime type explicitly when creating File instance. --- src/imageupload/utils.js | 20 +++++++++++--------- 1 file changed, 11 insertions(+), 9 deletions(-) diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index 5f1f49d2..6a7f14cb 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -36,9 +36,10 @@ export function wrapImageToFetch( image, index ) { fetch( image.getAttribute( 'src' ) ) .then( resource => resource.blob() ) .then( blob => { - const ext = getImageFileType( blob, image.getAttribute( 'src' ) ); + const mimeType = getImageMimeType( blob, image.getAttribute( 'src' ) ); + const ext = mimeType.replace( 'image/', '' ); const filename = `${ Number( new Date() ) }-image${ index }.${ ext }`; - const file = createFileFromBlob( blob, filename ); + const file = createFileFromBlob( blob, filename, mimeType ); resolve( { image, file } ); } ) @@ -66,14 +67,14 @@ export function isLocalImage( node ) { // @param {String} src Image src attribute value. // @param {Blob} blob Image blob representation. // @returns {String} -function getImageFileType( blob, src ) { +function getImageMimeType( blob, src ) { if ( blob.type ) { - return blob.type.replace( 'image/', '' ); - } else if ( src.match( /data:image\/(\w+);base64/ ) ) { - return src.match( /data:image\/(\w+);base64/ )[ 1 ].toLowerCase(); + return blob.type; + } else if ( src.match( /data:(image\/\w+);base64/ ) ) { + return src.match( /data:(image\/\w+);base64/ )[ 1 ].toLowerCase(); } else { // Fallback to 'jpeg' as common extension. - return 'jpeg'; + return 'image/jpeg'; } } @@ -81,10 +82,11 @@ function getImageFileType( blob, src ) { // // @param {Blob} blob The `Blob` instance from which file will be created. // @param {String} filename Filename used during file creation. +// @param {String} mimeType File mime type. // @returns {File|null} The `File` instance created from the given blob or `null` if `File API` is not available. -function createFileFromBlob( blob, filename ) { +function createFileFromBlob( blob, filename, mimeType ) { try { - return new File( [ blob ], filename ); + return new File( [ blob ], filename, { type: mimeType } ); } catch ( err ) { // Edge does not support `File` constructor ATM, see https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/9551546/. // However, the `File` function is present (so cannot be checked with `!window.File` or `typeof File === 'function'`), but From 5bdc25a90fe4970dd486c4d529eac254433530e9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Mon, 26 Nov 2018 11:13:05 +0100 Subject: [PATCH 08/11] Tests: Corrected data transfer mock in failing test. --- tests/imageupload/imageuploadediting.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index 14e01a66..afaafe35 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -219,7 +219,7 @@ describe( 'ImageUploadEditing', () => { it( 'should not insert image when file is null', () => { const viewDocument = editor.editing.view.document; - const dataTransfer = new DataTransfer( { files: [ null ], types: [ 'Files' ] } ); + const dataTransfer = new DataTransfer( { files: [ null ], types: [ 'Files' ], getData: () => null } ); setModelData( model, 'foo[]' ); From dfdbd73e122425c48e0fd60229acb6583e278d0f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 27 Nov 2018 11:18:14 +0100 Subject: [PATCH 09/11] Reverted unnecessary change. --- package.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package.json b/package.json index 27f10c63..38055919 100644 --- a/package.json +++ b/package.json @@ -10,7 +10,6 @@ "ckeditor5-plugin" ], "dependencies": { - "@ckeditor/ckeditor5-clipboard": "^10.0.3", "@ckeditor/ckeditor5-core": "^11.0.1", "@ckeditor/ckeditor5-engine": "^11.0.0", "@ckeditor/ckeditor5-theme-lark": "^11.1.0", @@ -21,6 +20,7 @@ }, "devDependencies": { "@ckeditor/ckeditor5-basic-styles": "^10.0.3", + "@ckeditor/ckeditor5-clipboard": "^10.0.3", "@ckeditor/ckeditor5-editor-classic": "^11.0.1", "@ckeditor/ckeditor5-enter": "^10.1.2", "@ckeditor/ckeditor5-essentials": "^10.1.2", From 996461b8965e957fee1075732e332078bf161222 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Tue, 27 Nov 2018 13:06:33 +0100 Subject: [PATCH 10/11] Update src/imageupload/utils.js Co-Authored-By: f1ames --- src/imageupload/utils.js | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index 6a7f14cb..fe5d0332 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -59,6 +59,11 @@ export function wrapImageToFetch( image, index ) { export function isLocalImage( node ) { return node.is( 'element', 'img' ) && node.getAttribute( 'src' ) && ( node.getAttribute( 'src' ).match( /data:image\/\w+;base64,/g ) || + if ( !node.is( 'element', 'img' ) || node.getAttribute( 'src' ) ) { + return false; + } + + return ( node.getAttribute( 'src' ).match( /data:image\/\w+;base64,/g ) || node.getAttribute( 'src' ).match( /blob:/g ) ); } From cd2a8fd335708922efd400ad541bf09cd3d28d50 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 27 Nov 2018 13:05:47 +0100 Subject: [PATCH 11/11] Refactoring. --- src/imageupload/imageuploadediting.js | 29 +++++++++++---------------- src/imageupload/utils.js | 19 ++++++++---------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 73949e0e..20fa317f 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -10,10 +10,11 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository'; import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification'; +import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter'; import { upcastAttributeToAttribute } from '@ckeditor/ckeditor5-engine/src/conversion/upcast-converters'; import ImageUploadCommand from '../../src/imageupload/imageuploadcommand'; -import { isImageType, isLocalImage, wrapImageToFetch } from '../../src/imageupload/utils'; +import { isImageType, isLocalImage, fetchLocalImage } from '../../src/imageupload/utils'; /** * The editing part of the image upload feature. It registers the `'imageUpload'` command. @@ -98,11 +99,9 @@ export default class ImageUploadEditing extends Plugin { // inserted into the content. Then, those images are uploaded once they appear in the model // (see Document#change listener below). this.listenTo( editor.plugins.get( 'Clipboard' ), 'inputTransformation', ( evt, data ) => { - const view = editor.editing.view; - - const fetchableImages = Array.from( view.createRangeIn( data.content ) ) + const fetchableImages = Array.from( editor.editing.view.createRangeIn( data.content ) ) .filter( value => isLocalImage( value.item ) && !value.item.getAttribute( 'uploadProcessed' ) ) - .map( ( value, index ) => wrapImageToFetch( value.item, index ) ); + .map( value => fetchLocalImage( value.item ) ); if ( !fetchableImages.length ) { return; @@ -111,25 +110,21 @@ export default class ImageUploadEditing extends Plugin { evt.stop(); Promise.all( fetchableImages ).then( items => { + const writer = new UpcastWriter(); + for ( const item of items ) { if ( !item.file ) { // Failed to fetch image or create a file instance, remove image element. - view.change( writer => { - writer.remove( item.image ); - } ); + writer.remove( item.image ); } else { + // Set attribute marking the image as processed. + writer.setAttribute( 'uploadProcessed', true, item.image ); + const loader = fileRepository.createLoader( item.file ); if ( loader ) { - view.change( writer => { - writer.setAttribute( 'src', '', item.image ); - writer.setAttribute( 'uploadId', loader.id, item.image ); - } ); - } else { - view.change( writer => { - // Set attribute so the image will not be processed 2nd time. - writer.setAttribute( 'uploadProcessed', true, item.image ); - } ); + writer.setAttribute( 'src', '', item.image ); + writer.setAttribute( 'uploadId', loader.id, item.image ); } } } diff --git a/src/imageupload/utils.js b/src/imageupload/utils.js index fe5d0332..fc61910c 100644 --- a/src/imageupload/utils.js +++ b/src/imageupload/utils.js @@ -25,12 +25,11 @@ export function isImageType( file ) { * Creates a promise which fetches the image local source (base64 or blob) and returns as a `File` object. * * @param {module:engine/view/element~Element} image Image which source to fetch. - * @param {Number} index Image index used as image name suffix. * @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. */ -export function wrapImageToFetch( image, index ) { +export function fetchLocalImage( image ) { return new Promise( resolve => { // Fetch works asynchronously and so does not block browser UI when processing data. fetch( image.getAttribute( 'src' ) ) @@ -38,7 +37,7 @@ export function wrapImageToFetch( image, index ) { .then( blob => { const mimeType = getImageMimeType( blob, image.getAttribute( 'src' ) ); const ext = mimeType.replace( 'image/', '' ); - const filename = `${ Number( new Date() ) }-image${ index }.${ ext }`; + const filename = `image.${ ext }`; const file = createFileFromBlob( blob, filename, mimeType ); resolve( { image, file } ); @@ -57,14 +56,12 @@ export function wrapImageToFetch( image, index ) { * @returns {Boolean} */ export function isLocalImage( node ) { - return node.is( 'element', 'img' ) && node.getAttribute( 'src' ) && - ( node.getAttribute( 'src' ).match( /data:image\/\w+;base64,/g ) || - if ( !node.is( 'element', 'img' ) || node.getAttribute( 'src' ) ) { - return false; - } - - return ( node.getAttribute( 'src' ).match( /data:image\/\w+;base64,/g ) || - node.getAttribute( 'src' ).match( /blob:/g ) ); + if ( !node.is( 'element', 'img' ) || !node.getAttribute( 'src' ) ) { + return false; + } + + return node.getAttribute( 'src' ).match( /^data:image\/\w+;base64,/g ) || + node.getAttribute( 'src' ).match( /^blob:/g ); } // Extracts image type based on its blob representation or its source.