From dd4261bdf73cc7226c63538f8feb5b87f938e26a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 21 Aug 2019 14:53:51 +0200 Subject: [PATCH 1/5] Convert uploadStatus=uploading as a view image src attribute of image being loaded. --- src/imageupload/imageuploadediting.js | 55 ++++++++++++++------------ src/imageupload/imageuploadprogress.js | 14 +++++++ tests/manual/imageupload.js | 5 ++- 3 files changed, 47 insertions(+), 27 deletions(-) diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 1eed5bbd..b7122ed3 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -14,7 +14,7 @@ import UpcastWriter from '@ckeditor/ckeditor5-engine/src/view/upcastwriter'; import env from '@ckeditor/ckeditor5-utils/src/env'; import ImageUploadCommand from '../../src/imageupload/imageuploadcommand'; -import { isImageType, isLocalImage, fetchLocalImage } from '../../src/imageupload/utils'; +import { fetchLocalImage, isImageType, isLocalImage } from '../../src/imageupload/utils'; /** * The editing part of the image upload feature. It registers the `'imageUpload'` command. @@ -134,30 +134,32 @@ export default class ImageUploadEditing extends Plugin { const changes = doc.differ.getChanges( { includeChangesInGraveyard: true } ); for ( const entry of changes ) { - if ( entry.type == 'insert' && entry.name == 'image' ) { + if ( entry.type == 'insert' && entry.name != '$text' ) { const item = entry.position.nodeAfter; const isInGraveyard = entry.position.root.rootName == '$graveyard'; - // Check if the image element still has upload id. - const uploadId = item.getAttribute( 'uploadId' ); + for ( const image of getImagesFromChangeItem( editor, item ) ) { + // Check if the image element still has upload id. + const uploadId = image.getAttribute( 'uploadId' ); - if ( !uploadId ) { - continue; - } + if ( !uploadId ) { + continue; + } - // Check if the image is loaded on this client. - const loader = fileRepository.loaders.get( uploadId ); + // Check if the image is loaded on this client. + const loader = fileRepository.loaders.get( uploadId ); - if ( !loader ) { - continue; - } + if ( !loader ) { + continue; + } - if ( isInGraveyard ) { - // If the image was inserted to the graveyard - abort the loading process. - loader.abort(); - } else if ( loader.status == 'idle' ) { - // If the image was inserted into content and has not been loaded yet, start loading it. - this._readAndUpload( loader, item ); + if ( isInGraveyard ) { + // If the image was inserted to the graveyard - abort the loading process. + loader.abort(); + } else if ( loader.status == 'idle' ) { + // If the image was inserted into content and has not been loaded yet, start loading it. + this._readAndUpload( loader, image ); + } } } } @@ -188,15 +190,16 @@ export default class ImageUploadEditing extends Plugin { } ); return loader.read() - .then( data => { - const viewFigure = editor.editing.mapper.toViewElement( imageElement ); - const viewImg = viewFigure.getChild( 0 ); + .then( () => { const promise = loader.upload(); // Force re–paint in Safari. Without it, the image will display with a wrong size. // https://github.com/ckeditor/ckeditor5/issues/1975 /* istanbul ignore next */ if ( env.isSafari ) { + const viewFigure = editor.editing.mapper.toViewElement( imageElement ); + const viewImg = viewFigure.getChild( 0 ); + editor.editing.view.once( 'render', () => { // Early returns just to be safe. There might be some code ran // in between the outer scope and this callback. @@ -221,10 +224,6 @@ export default class ImageUploadEditing extends Plugin { } ); } - editor.editing.view.change( writer => { - writer.setAttribute( 'src', data, viewImg ); - } ); - model.enqueueChange( 'transparent', writer => { writer.setAttribute( 'uploadStatus', 'uploading', imageElement ); } ); @@ -318,3 +317,9 @@ export default class ImageUploadEditing extends Plugin { export function isHtmlIncluded( dataTransfer ) { return Array.from( dataTransfer.types ).includes( 'text/html' ) && dataTransfer.getData( 'text/html' ) !== ''; } + +function getImagesFromChangeItem( editor, item ) { + return Array.from( editor.model.createRangeOn( item ) ) + .filter( value => value.item.is( 'image' ) ) + .map( value => value.item ); +} diff --git a/src/imageupload/imageuploadprogress.js b/src/imageupload/imageuploadprogress.js index e34b6f2a..0b18b965 100644 --- a/src/imageupload/imageuploadprogress.js +++ b/src/imageupload/imageuploadprogress.js @@ -97,6 +97,7 @@ export default class ImageUploadProgress extends Plugin { // Hide placeholder and initialize progress bar showing upload progress. _hidePlaceholder( viewFigure, viewWriter ); _showProgressBar( viewFigure, viewWriter, loader, editor.editing.view ); + _displayLocalImage( viewFigure, viewWriter, loader ); } return; @@ -261,3 +262,16 @@ function _removeUIElement( viewFigure, writer, uniqueProperty ) { writer.remove( writer.createRangeOn( element ) ); } } + +// Displays local data from file loader. +// +// @param {module:engine/view/element~Element} imageFigure +// @param {module:engine/view/downcastwriter~DowncastWriter} writer +// @param {module:upload/filerepository~FileLoader} loader +function _displayLocalImage( viewFigure, writer, loader ) { + if ( loader.data ) { + const viewImg = viewFigure.getChild( 0 ); + + writer.setAttribute( 'src', loader.data, viewImg ); + } +} diff --git a/tests/manual/imageupload.js b/tests/manual/imageupload.js index 2951f4ca..d504e565 100644 --- a/tests/manual/imageupload.js +++ b/tests/manual/imageupload.js @@ -16,6 +16,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import Typing from '@ckeditor/ckeditor5-typing/src/typing'; import Undo from '@ckeditor/ckeditor5-undo/src/undo'; import { UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; +import Table from '@ckeditor/ckeditor5-table/src/table'; import ImageStyle from '../../src/imagestyle'; import ImageToolbar from '../../src/imagetoolbar'; import Image from '../../src/image'; @@ -28,9 +29,9 @@ ClassicEditor .create( document.querySelector( '#editor' ), { plugins: [ Enter, Typing, Paragraph, Heading, Undo, Bold, Italic, Heading, List, Image, ImageToolbar, Clipboard, - ImageCaption, ImageStyle, ImageUpload + ImageCaption, ImageStyle, ImageUpload, Table ], - toolbar: [ 'heading', '|', 'undo', 'redo', 'bold', 'italic', 'bulletedList', 'numberedList', 'imageUpload' ], + toolbar: [ 'heading', '|', 'undo', 'redo', 'bold', 'italic', 'bulletedList', 'numberedList', 'insertTable', '|', 'imageUpload' ], image: { toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ] } From bd7dcf7e4fc66fd972d3d928e8986e1c4332b4cd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 21 Aug 2019 15:48:20 +0200 Subject: [PATCH 2/5] Fix tests for a new conversion. --- tests/imageupload/imageuploadediting.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/imageupload/imageuploadediting.js b/tests/imageupload/imageuploadediting.js index db94c726..1662ccce 100644 --- a/tests/imageupload/imageuploadediting.js +++ b/tests/imageupload/imageuploadediting.js @@ -334,7 +334,7 @@ describe( 'ImageUploadEditing', () => { ']' ); } ); - it( 'should use read data once it is present', done => { + it( 'should not use read data once it is present', done => { const file = createNativeFileMock(); setModelData( model, '{}foo bar' ); editor.execute( 'imageUpload', { file } ); @@ -343,7 +343,8 @@ describe( 'ImageUploadEditing', () => { tryExpect( done, () => { expect( getViewData( view ) ).to.equal( '[
' + - `` + + // Rendering the image data is left to a upload progress converter. + '' + '
]' + '

foo bar

' ); From 1a19df7acf01a7d7fe3f818d63acb79c029cc750 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 21 Aug 2019 16:15:12 +0200 Subject: [PATCH 3/5] Add tests for refreshing image parent. --- tests/imageupload/imageuploadprogress.js | 50 +++++++++++++++++++++++- 1 file changed, 49 insertions(+), 1 deletion(-) diff --git a/tests/imageupload/imageuploadprogress.js b/tests/imageupload/imageuploadprogress.js index 04156a9d..75d63d15 100644 --- a/tests/imageupload/imageuploadprogress.js +++ b/tests/imageupload/imageuploadprogress.js @@ -15,7 +15,7 @@ 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 { createNativeFileMock, NativeFileReaderMock, UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; import { setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; @@ -107,6 +107,54 @@ describe( 'ImageUploadProgress', () => { loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); + it( 'should work if image parent is refreshed by the differ', function( done ) { + model.schema.register( 'outerBlock', { + allowWhere: '$block', + isBlock: true + } ); + + model.schema.register( 'innerBlock', { + allowIn: 'outerBlock', + isLimit: true + } ); + + model.schema.extend( '$block', { allowIn: 'innerBlock' } ); + editor.conversion.elementToElement( { model: 'outerBlock', view: 'outerBlock' } ); + editor.conversion.elementToElement( { model: 'innerBlock', view: 'innerBlock' } ); + + model.document.registerPostFixer( () => { + for ( const change of doc.differ.getChanges() ) { + // The differ.refreshItem() simulates remove and insert of and image parent thus preventing image from proper work. + if ( change.type == 'insert' && change.name == 'image' ) { + doc.differ.refreshItem( change.position.parent ); + + return true; + } + } + } ); + + setModelData( model, '[]' ); + + editor.execute( 'imageUpload', { file: createNativeFileMock() } ); + + model.document.once( 'change', () => { + try { + expect( getViewData( view ) ).to.equal( + '[
' + + `` + + '
' + + '
]' + ); + + done(); + } catch ( err ) { + done( err ); + } + }, { priority: 'lowest' } ); + + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); + } ); + it( 'should work correctly when there is no "reading" status and go straight to "uploading"', () => { const fileRepository = editor.plugins.get( FileRepository ); const file = createNativeFileMock(); From c7789b8cfab72d9b1b984b7d5ac26c0667d35d93 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 22 Aug 2019 09:28:06 +0200 Subject: [PATCH 4/5] Added a comment. --- tests/imageupload/imageuploadprogress.js | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/imageupload/imageuploadprogress.js b/tests/imageupload/imageuploadprogress.js index 75d63d15..5b2dbad9 100644 --- a/tests/imageupload/imageuploadprogress.js +++ b/tests/imageupload/imageuploadprogress.js @@ -107,6 +107,7 @@ describe( 'ImageUploadProgress', () => { loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); } ); + // See https://github.com/ckeditor/ckeditor5/issues/1985. it( 'should work if image parent is refreshed by the differ', function( done ) { model.schema.register( 'outerBlock', { allowWhere: '$block', From a1ecc0ee521a201b9ee61c29eb12f1ba32e7ed55 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Thu, 22 Aug 2019 09:53:26 +0200 Subject: [PATCH 5/5] Improved manual tests. --- tests/manual/imagestyle.js | 30 ++++++++++++++++++++++++++++-- tests/manual/imageupload.js | 37 ++++++++++++++++++------------------- 2 files changed, 46 insertions(+), 21 deletions(-) diff --git a/tests/manual/imagestyle.js b/tests/manual/imagestyle.js index 8f59891a..0a9182a4 100644 --- a/tests/manual/imagestyle.js +++ b/tests/manual/imagestyle.js @@ -13,7 +13,20 @@ ClassicEditor plugins: [ ArticlePluginSet ], - toolbar: [ 'heading', '|', 'undo', 'redo' ], + toolbar: [ + 'heading', + '|', + 'bold', + 'italic', + 'link', + 'bulletedList', + 'numberedList', + 'blockQuote', + 'insertTable', + 'mediaEmbed', + 'undo', + 'redo' + ], image: { toolbar: [ 'imageStyle:full', 'imageStyle:side' ] } @@ -30,7 +43,20 @@ ClassicEditor plugins: [ ArticlePluginSet ], - toolbar: [ 'heading', '|', 'undo', 'redo' ], + toolbar: [ + 'heading', + '|', + 'bold', + 'italic', + 'link', + 'bulletedList', + 'numberedList', + 'blockQuote', + 'insertTable', + 'mediaEmbed', + 'undo', + 'redo' + ], image: { styles: [ 'alignLeft', 'alignCenter', 'alignRight' ], toolbar: [ 'imageStyle:alignLeft', 'imageStyle:alignCenter', 'imageStyle:alignRight' ] diff --git a/tests/manual/imageupload.js b/tests/manual/imageupload.js index d504e565..8ddf6b34 100644 --- a/tests/manual/imageupload.js +++ b/tests/manual/imageupload.js @@ -6,32 +6,31 @@ /* globals window, document, console */ import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor'; -import Bold from '@ckeditor/ckeditor5-basic-styles/src/bold'; -import Clipboard from '@ckeditor/ckeditor5-clipboard/src/clipboard'; -import Enter from '@ckeditor/ckeditor5-enter/src/enter'; -import Heading from '@ckeditor/ckeditor5-heading/src/heading'; -import Italic from '@ckeditor/ckeditor5-basic-styles/src/italic'; -import List from '@ckeditor/ckeditor5-list/src/list'; -import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; -import Typing from '@ckeditor/ckeditor5-typing/src/typing'; -import Undo from '@ckeditor/ckeditor5-undo/src/undo'; -import { UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; -import Table from '@ckeditor/ckeditor5-table/src/table'; -import ImageStyle from '../../src/imagestyle'; -import ImageToolbar from '../../src/imagetoolbar'; -import Image from '../../src/image'; -import ImageCaption from '../../src/imagecaption'; +import ArticlePluginSet from '@ckeditor/ckeditor5-core/tests/_utils/articlepluginset'; import ImageUpload from '../../src/imageupload'; +import { UploadAdapterMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks'; + const buttonContainer = document.getElementById( 'button-container' ); ClassicEditor .create( document.querySelector( '#editor' ), { - plugins: [ - Enter, Typing, Paragraph, Heading, Undo, Bold, Italic, Heading, List, Image, ImageToolbar, Clipboard, - ImageCaption, ImageStyle, ImageUpload, Table + plugins: [ ArticlePluginSet, ImageUpload ], + toolbar: [ + 'heading', + '|', + 'bold', + 'italic', + 'link', + 'bulletedList', + 'numberedList', + 'blockQuote', + 'imageUpload', + 'insertTable', + 'mediaEmbed', + 'undo', + 'redo' ], - toolbar: [ 'heading', '|', 'undo', 'redo', 'bold', 'italic', 'bulletedList', 'numberedList', 'insertTable', '|', 'imageUpload' ], image: { toolbar: [ 'imageStyle:full', 'imageStyle:side', '|', 'imageTextAlternative' ] }