From 9fb31fd52fa4df85e29a6c01fd9256d8a7d6e8eb Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 8 Nov 2024 10:55:02 +0100 Subject: [PATCH 01/10] Fix not working drag & drop on images that are in uploading state. --- .../src/imageupload/imageuploadediting.ts | 74 ++++++++- .../tests/imageupload/imageuploadediting.js | 146 ++++++++++++++++++ 2 files changed, 219 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index 7ba5bffffb7..c6d1df40443 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -16,7 +16,8 @@ import { type Writer, type DataTransfer, type ViewElement, - type NodeAttributes + type NodeAttributes, + type DowncastAttributeEvent } from 'ckeditor5/src/engine.js'; import { Notification } from 'ckeditor5/src/ui.js'; @@ -66,6 +67,14 @@ export default class ImageUploadEditing extends Plugin { */ private readonly _uploadImageElements: Map; + /** + * An internal mapping of {@link module:upload/filerepository~FileLoader#id file loader UIDs} and + * upload responses for handling images dragged during their upload process. When such images are later + * dropped, their original upload IDs no longer exist in the registry (as the original upload completed). + * This map preserves the upload responses to properly handle such cases. + */ + private readonly _uploadedImages = new Map(); + /** * @inheritDoc */ @@ -106,6 +115,13 @@ export default class ImageUploadEditing extends Plugin { key: 'uploadId' }, model: 'uploadId' + } ) + .attributeToAttribute( { + view: { + name: 'img', + key: 'data-ck-upload-id' + }, + model: 'uploadId' } ); // Handle pasted images. @@ -217,6 +233,19 @@ export default class ImageUploadEditing extends Plugin { const loader = fileRepository.loaders.get( uploadId ); if ( !loader ) { + // If the loader does not exist, it means that the image was already uploaded + // and the loader promise was removed from the registry. In that scenario we need + // to restore response object from the internal map. + if ( !isInsertedInGraveyard && this._uploadedImages.has( uploadId ) ) { + // Fire `uploadComplete` to set proper attributes on the image element. + this.fire( 'uploadComplete', { + data: this._uploadedImages.get( uploadId )!, + imageElement: imageElement as Element + } ); + + this._uploadedImages.delete( uploadId ); + } + continue; } @@ -274,12 +303,16 @@ export default class ImageUploadEditing extends Plugin { schema.extend( 'imageBlock', { allowAttributes: [ 'uploadId', 'uploadStatus' ] } ); + + this._registerConverters( 'imageBlock' ); } if ( this.editor.plugins.has( 'ImageInlineEditing' ) ) { schema.extend( 'imageInline', { allowAttributes: [ 'uploadId', 'uploadStatus' ] } ); + + this._registerConverters( 'imageInline' ); } } @@ -360,6 +393,8 @@ export default class ImageUploadEditing extends Plugin { } this.fire( 'uploadComplete', { data, imageElement } ); + + this._uploadedImages.set( loader.id, data ); } ); clean(); @@ -451,6 +486,43 @@ export default class ImageUploadEditing extends Plugin { writer.setAttributes( attributes, image ); } } + + /** + * Registers image upload converters. + * + * @param imageType The type of the image. + */ + private _registerConverters( imageType: 'imageBlock' | 'imageInline' ) { + const { conversion, plugins } = this.editor; + + const fileRepository = plugins.get( FileRepository ); + const imageUtils = plugins.get( ImageUtils ); + + // It sets `data-ck-upload-id` attribute on the view image elements that are not fully uploaded. + // It avoids the situation when image disappears when it's being moved and upload is not finished yet. + // See more: https://github.com/ckeditor/ckeditor5/issues/16967 + conversion.for( 'dataDowncast' ).add( dispatcher => { + dispatcher.on( `attribute:uploadId:${ imageType }`, ( evt, data, conversionApi ) => { + if ( !conversionApi.consumable.test( data.item, evt.name ) ) { + return; + } + + const loader = fileRepository.loaders.get( data.attributeNewValue as string ); + + if ( !loader || !loader.data ) { + return null; + } + + const viewElement = conversionApi.mapper.toViewElement( data.item as Element )!; + const img = imageUtils.findViewImgElement( viewElement ); + + if ( img ) { + conversionApi.consumable.consume( data.item, evt.name ); + conversionApi.writer.setAttribute( 'data-ck-upload-id', loader.id, img ); + } + } ); + } ); + } } /** diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index ffcc8adca53..2506f8577ab 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -1580,6 +1580,138 @@ describe( 'ImageUploadEditing', () => { } ); } ); + describe( 'data downcast conversion of images with uploading state', () => { + it( 'should dump the `data-ck-upload-id` into the data', async () => { + const onDispatch = sinon.spy( ( evt, data, conversionApi ) => { + const wasConsumed = conversionApi.consumable.test( data.item, 'attribute:uploadId:imageInline' ); + + expect( wasConsumed ).to.be.true; + } ); + + editor.conversion.for( 'downcast' ).add( dispatcher => + dispatcher.on( 'attribute:uploadId:imageInline', onDispatch, { priority: 'high' } ) + ); + + setModelData( model, '[]foo' ); + + const file = createNativeFileMock(); + editor.execute( 'uploadImage', { file } ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); + + await timeout( 50 ); + + const uploadId = adapterMocks[ 0 ].loader.id; + + expect( getModelData( editor.model ) ).to.be.equal( + `[]foo` + ); + + expect( onDispatch ).to.be.calledOnce; + expect( editor.getData() ).to.be.equal( + `

foo

` + ); + } ); + + it( 'should not crash if uploadId of down casted image is not found in loaders repository', async () => { + setModelData( model, '[]foo' ); + + const file = createNativeFileMock(); + editor.execute( 'uploadImage', { file } ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); + + await timeout( 50 ); + + const uploadId = adapterMocks[ 0 ].loader.id; + + sinon + .stub( fileRepository.loaders, 'get' ) + .withArgs( uploadId ) + .returns( null ); + + expect( getModelData( editor.model ) ).to.be.equal( + `[]foo` + ); + + expect( editor.getData() ).to.be.equal( '

foo

' ); + } ); + + it( 'should not downcast consumed uploadId image attribute', async () => { + editor.conversion.for( 'downcast' ).add( dispatcher => + dispatcher.on( 'attribute:uploadId:imageInline', ( evt, data, conversionApi ) => { + conversionApi.consumable.consume( data.item, 'attribute:uploadId:imageInline' ); + }, { priority: 'high' } ) + ); + + setModelData( model, '[]foo' ); + + const file = createNativeFileMock(); + editor.execute( 'uploadImage', { file } ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); + + await timeout( 50 ); + + const uploadId = adapterMocks[ 0 ].loader.id; + + expect( getModelData( editor.model ) ).to.be.equal( + `[]foo` + ); + + expect( editor.getData() ).to.be.equal( '

foo

' ); + } ); + + it( 'should restore image from `_uploadedImages` if it was pasted from clipboard', async () => { + setModelData( model, '[]foo' ); + + const file = createNativeFileMock(); + editor.execute( 'uploadImage', { file } ); + loader.file.then( () => nativeReaderMock.mockSuccess( base64Sample ) ); + + await timeout( 50 ); + + // Let's copy image in uploading state. + const uploadId = adapterMocks[ 0 ].loader.id; + expect( getModelData( editor.model ) ).to.be.equal( + `[]foo` + ); + + // Lets check if content of clipboard is correct. + const data = { + dataTransfer: createDataTransfer(), + preventDefault: () => {}, + stopPropagation: () => {} + }; + + viewDocument.fire( 'copy', data ); + expect( data.dataTransfer.getData( 'text/html' ) ).to.equal( `` ); + + // Let's resolve uploading status and ensure that image is loaded. + await new Promise( res => { + model.document.once( 'change', res, { priority: 'lowest' } ); + loader.file.then( () => adapterMocks[ 0 ].mockSuccess( { default: '/assets/sample.png', 800: 'image-800.png' } ) ); + } ); + + expect( editor.getData() ).to.be.equal( + '

foo

' + ); + + // Make sure it's no longer present in registry, so image upload is completed. + expect( fileRepository.loaders.get( uploadId ) ).to.be.null; + + // Let's paste the image from clipboard, it has upload id, which should be stored in plugin cache. + setModelData( model, 'hello[]' ); + + viewDocument.fire( 'paste', { + dataTransfer: mockDataTransfer( `` ), + preventDefault: () => {}, + stopPropagation: () => {} + } ); + + expect( editor.getData() ).to.be.equal( + '

hello

' + ); + } ); + } ); + // 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). @@ -1713,3 +1845,17 @@ function base64ToBlob( base64Data ) { function timeout( ms ) { return new Promise( res => setTimeout( res, ms ) ); } + +function createDataTransfer() { + const store = new Map(); + + return { + setData( type, data ) { + store.set( type, data ); + }, + + getData( type ) { + return store.get( type ); + } + }; +} From 207a2e5c8c84d30385ff20896f1af5a1ab35e4ee Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 8 Nov 2024 13:58:45 +0100 Subject: [PATCH 02/10] Fix upcast --- .../src/imageupload/imageuploadediting.ts | 38 +++++++++++++++---- .../tests/imageupload/imageuploadediting.js | 24 ++++++++++++ 2 files changed, 54 insertions(+), 8 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index c6d1df40443..f4e6db6bd44 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -17,7 +17,8 @@ import { type DataTransfer, type ViewElement, type NodeAttributes, - type DowncastAttributeEvent + type DowncastAttributeEvent, + type UpcastElementEvent } from 'ckeditor5/src/engine.js'; import { Notification } from 'ckeditor5/src/ui.js'; @@ -116,13 +117,34 @@ export default class ImageUploadEditing extends Plugin { }, model: 'uploadId' } ) - .attributeToAttribute( { - view: { - name: 'img', - key: 'data-ck-upload-id' - }, - model: 'uploadId' - } ); + + // Handle the case when the image is not fully uploaded yet but it's being moved. + // See more: https://github.com/ckeditor/ckeditor5/pull/17327 + .add( dispatcher => dispatcher.on( 'element:img', ( evt, data, conversionApi ) => { + const uploadId = data.viewItem.getAttribute( 'data-ck-upload-id' ); + + if ( !uploadId ) { + return; + } + + if ( !data.modelRange ) { + Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); + } + + const [ modelElement ] = Array.from( data.modelRange!.getItems() ); + const loader = fileRepository.loaders.get( uploadId as string ); + + if ( modelElement ) { + // Handle case when `uploadId` is set on the image element but the loader is not present in the registry. + // It may happen when the image was successfully uploaded and the loader was removed from the registry. + // It's still present in the `_uploadedImages` map though. It's why we do not place this line in the condition below. + conversionApi.writer.setAttribute( 'uploadId', uploadId, modelElement ); + + if ( loader && loader.data ) { + conversionApi.writer.setAttribute( 'uploadStatus', loader.status, modelElement ); + } + } + } ) ); // Handle pasted images. // For every image file, a new file loader is created and a placeholder image is diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index 2506f8577ab..2028e71dfa8 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -22,6 +22,7 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils.js'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository.js'; import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks.js'; +import UpcastDispatcher from '@ckeditor/ckeditor5-engine/src/conversion/upcastdispatcher.js'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js'; import { getData as getViewData, stringify as stringifyView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; @@ -1712,6 +1713,29 @@ describe( 'ImageUploadEditing', () => { } ); } ); + describe( 'data upcast of `data-ck-upload-id` attribute', () => { + it( 'should upcast `data-ck-upload-id` attribute', () => { + editor.setData( '

' ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + + it( 'should upcast `uploadStatus` if image is present in registry', () => { + sinon.stub( fileRepository.loaders, 'get' ).withArgs( '123' ).returns( { + status: 'uploading', + data: {} + } ); + + editor.setData( '

' ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + } ); + // 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). From 79e79a4379cb2f4acaf17160afa276abf33cc05c Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 8 Nov 2024 15:12:14 +0100 Subject: [PATCH 03/10] Fix test --- .../src/imageupload/imageuploadediting.ts | 13 ++++++----- .../tests/imageupload/imageuploadediting.js | 22 +++++++++++++++++++ 2 files changed, 29 insertions(+), 6 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index f4e6db6bd44..a17c908f78e 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -121,17 +121,17 @@ export default class ImageUploadEditing extends Plugin { // Handle the case when the image is not fully uploaded yet but it's being moved. // See more: https://github.com/ckeditor/ckeditor5/pull/17327 .add( dispatcher => dispatcher.on( 'element:img', ( evt, data, conversionApi ) => { + if ( !conversionApi.consumable.test( data.viewItem, { attributes: [ 'data-ck-upload-id' ] } ) ) { + return; + } + const uploadId = data.viewItem.getAttribute( 'data-ck-upload-id' ); if ( !uploadId ) { return; } - if ( !data.modelRange ) { - Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) ); - } - - const [ modelElement ] = Array.from( data.modelRange!.getItems() ); + const [ modelElement ] = Array.from( data.modelRange!.getItems( { shallow: true } ) ); const loader = fileRepository.loaders.get( uploadId as string ); if ( modelElement ) { @@ -139,12 +139,13 @@ export default class ImageUploadEditing extends Plugin { // It may happen when the image was successfully uploaded and the loader was removed from the registry. // It's still present in the `_uploadedImages` map though. It's why we do not place this line in the condition below. conversionApi.writer.setAttribute( 'uploadId', uploadId, modelElement ); + conversionApi.consumable.consume( data.viewItem, { attributes: [ 'data-ck-upload-id' ] } ); if ( loader && loader.data ) { conversionApi.writer.setAttribute( 'uploadStatus', loader.status, modelElement ); } } - } ) ); + }, { priority: 'low' } ) ); // Handle pasted images. // For every image file, a new file loader is created and a placeholder image is diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index 2028e71dfa8..199ffcd7553 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -1722,6 +1722,28 @@ describe( 'ImageUploadEditing', () => { ); } ); + it( 'should not upcast empty `data-ck-upload-id` attribute', () => { + editor.setData( '

' ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + + it( 'should not upcast already consumed element', () => { + editor.conversion.for( 'upcast' ).add( dispatcher => + dispatcher.on( 'element:img', ( evt, data, conversionApi ) => { + conversionApi.consumable.consume( data.viewItem, { attributes: [ 'data-ck-upload-id' ] } ); + }, { priority: 'high' } ) + ); + + editor.setData( '

' ); + + expect( getModelData( model, { withoutSelection: true } ) ).to.equal( + '' + ); + } ); + it( 'should upcast `uploadStatus` if image is present in registry', () => { sinon.stub( fileRepository.loaders, 'get' ).withArgs( '123' ).returns( { status: 'uploading', From 25314c198d183be3c472833e8d0d0753818c7835 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Fri, 8 Nov 2024 15:18:39 +0100 Subject: [PATCH 04/10] Fix linter --- packages/ckeditor5-image/tests/imageupload/imageuploadediting.js | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index 199ffcd7553..ab389325036 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -22,7 +22,6 @@ import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils.js'; import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository.js'; import { UploadAdapterMock, createNativeFileMock, NativeFileReaderMock } from '@ckeditor/ckeditor5-upload/tests/_utils/mocks.js'; -import UpcastDispatcher from '@ckeditor/ckeditor5-engine/src/conversion/upcastdispatcher.js'; import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model.js'; import { getData as getViewData, stringify as stringifyView } from '@ckeditor/ckeditor5-engine/src/dev-utils/view.js'; From 7555385407ed6a9d1176b443c8f1c0ef467f6da1 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 18 Nov 2024 13:47:03 +0100 Subject: [PATCH 05/10] Handle case when image in uploading state is being cloned. --- .../src/imageupload/imageuploadediting.ts | 108 ++++++++++-------- .../tests/imageupload/imageuploadediting.js | 2 +- 2 files changed, 59 insertions(+), 51 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index a17c908f78e..2e6b4f461da 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -66,7 +66,7 @@ export default class ImageUploadEditing extends Plugin { * element (reference) and resolve the upload for the correct model element (instead of the one that landed in the `$graveyard` * after image type changed). */ - private readonly _uploadImageElements: Map; + private readonly _uploadImageElements: Map>; /** * An internal mapping of {@link module:upload/filerepository~FileLoader#id file loader UIDs} and @@ -288,7 +288,11 @@ export default class ImageUploadEditing extends Plugin { // can later resolve in the context of the correct model element. The model element could // change for the same upload if one image was replaced by another (e.g. image type was changed), // so this may also replace an existing mapping. - this._uploadImageElements.set( uploadId, imageElement as Element ); + if ( !this._uploadImageElements.has( uploadId ) ) { + this._uploadImageElements.set( uploadId, new Set( [ imageElement as Element ] ) ); + } else { + this._uploadImageElements.get( uploadId )!.add( imageElement as Element ); + } if ( loader.status == 'idle' ) { // If the image was inserted into content and has not been loaded yet, start loading it. @@ -356,67 +360,71 @@ export default class ImageUploadEditing extends Plugin { const imageUploadElements = this._uploadImageElements; model.enqueueChange( { isUndoable: false }, writer => { - writer.setAttribute( 'uploadStatus', 'reading', imageUploadElements.get( loader.id )! ); + const elements = imageUploadElements.get( loader.id )!; + + for ( const element of elements ) { + writer.setAttribute( 'uploadStatus', 'reading', element ); + } } ); return loader.read() .then( () => { const promise = loader.upload(); - const imageElement = imageUploadElements.get( loader.id )!; - - // 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 -- @preserve */ - if ( env.isSafari ) { - const viewFigure = editor.editing.mapper.toViewElement( imageElement )!; - const viewImg = imageUtils.findViewImgElement( viewFigure )!; - - 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. - if ( !viewImg.parent ) { - return; - } - const domFigure = editor.editing.view.domConverter.mapViewToDom( viewImg.parent ) as HTMLElement | undefined; + for ( const imageElement of imageUploadElements.get( loader.id )! ) { + // 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 -- @preserve */ + if ( env.isSafari ) { + const viewFigure = editor.editing.mapper.toViewElement( imageElement )!; + const viewImg = imageUtils.findViewImgElement( viewFigure )!; + + 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. + if ( !viewImg.parent ) { + return; + } - if ( !domFigure ) { - return; - } + const domFigure = editor.editing.view.domConverter.mapViewToDom( viewImg.parent ) as HTMLElement | undefined; - const originalDisplay = domFigure.style.display; + if ( !domFigure ) { + return; + } - domFigure.style.display = 'none'; + const originalDisplay = domFigure.style.display; - // Make sure this line will never be removed during minification for having "no effect". - ( domFigure as any )._ckHack = domFigure.offsetHeight; + domFigure.style.display = 'none'; - domFigure.style.display = originalDisplay; - } ); - } + // Make sure this line will never be removed during minification for having "no effect". + ( domFigure as any )._ckHack = domFigure.offsetHeight; - if ( editor.ui ) { - editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); - } + domFigure.style.display = originalDisplay; + } ); + } - model.enqueueChange( { isUndoable: false }, writer => { - writer.setAttribute( 'uploadStatus', 'uploading', imageElement ); - } ); + if ( editor.ui ) { + editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); + } + + model.enqueueChange( { isUndoable: false }, writer => { + writer.setAttribute( 'uploadStatus', 'uploading', imageElement ); + } ); + } return promise; } ) .then( data => { model.enqueueChange( { isUndoable: false }, writer => { - const imageElement = imageUploadElements.get( loader.id )!; - - writer.setAttribute( 'uploadStatus', 'complete', imageElement ); + for ( const imageElement of imageUploadElements.get( loader.id )! ) { + writer.setAttribute( 'uploadStatus', 'complete', imageElement ); + this.fire( 'uploadComplete', { data, imageElement } ); + } if ( editor.ui ) { editor.ui.ariaLiveAnnouncer.announce( t( 'Image upload complete' ) ); } - this.fire( 'uploadComplete', { data, imageElement } ); - this._uploadedImages.set( loader.id, data ); } ); @@ -443,12 +451,12 @@ export default class ImageUploadEditing extends Plugin { // Permanently remove image from insertion batch. model.enqueueChange( { isUndoable: false }, writer => { - const node = imageUploadElements.get( loader.id ); - - // Handle situation when the image has been removed and then `abort` exception was thrown. - // See: https://github.com/cksource/ckeditor5-commercial/issues/6817 - if ( node && node.root.rootName !== '$graveyard' ) { - writer.remove( node ); + for ( const imageElement of imageUploadElements.get( loader.id )! ) { + // Handle situation when the image has been removed and then `abort` exception was thrown. + // See: https://github.com/cksource/ckeditor5-commercial/issues/6817 + if ( imageElement.root.rootName !== '$graveyard' ) { + writer.remove( imageElement ); + } } } ); @@ -457,10 +465,10 @@ export default class ImageUploadEditing extends Plugin { function clean() { model.enqueueChange( { isUndoable: false }, writer => { - const imageElement = imageUploadElements.get( loader.id )!; - - writer.removeAttribute( 'uploadId', imageElement ); - writer.removeAttribute( 'uploadStatus', imageElement ); + for ( const imageElement of imageUploadElements.get( loader.id )! ) { + writer.removeAttribute( 'uploadId', imageElement ); + writer.removeAttribute( 'uploadStatus', imageElement ); + } imageUploadElements.delete( loader.id ); } ); diff --git a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js index ab389325036..7787be5a5ee 100644 --- a/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js +++ b/packages/ckeditor5-image/tests/imageupload/imageuploadediting.js @@ -820,7 +820,7 @@ describe( 'ImageUploadEditing', () => { ); sinon.assert.notCalled( abortSpy ); - sinon.assert.calledOnce( uploadCompleteSpy ); + sinon.assert.calledTwice( uploadCompleteSpy ); } ); it( 'should abort if an image changed type and then was removed', async () => { From 34658ba93db746788d7af779b6dc02ed65af2ab6 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 18 Nov 2024 14:40:13 +0100 Subject: [PATCH 06/10] Move aria announcer. --- .../ckeditor5-image/src/imageupload/imageuploadediting.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index 2e6b4f461da..f9cbfab79b4 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -403,15 +403,15 @@ export default class ImageUploadEditing extends Plugin { } ); } - if ( editor.ui ) { - editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); - } - model.enqueueChange( { isUndoable: false }, writer => { writer.setAttribute( 'uploadStatus', 'uploading', imageElement ); } ); } + if ( editor.ui ) { + editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); + } + return promise; } ) .then( data => { From 9fa19cb303daf57214e92220266641f11e3f02a2 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 25 Nov 2024 13:41:04 +0100 Subject: [PATCH 07/10] Fix failing aria live test. --- .../ckeditor5-image/src/imageupload/imageuploadediting.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index f9cbfab79b4..ff15def6f7b 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -371,6 +371,10 @@ export default class ImageUploadEditing extends Plugin { .then( () => { const promise = loader.upload(); + if ( editor.ui ) { + editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); + } + for ( const imageElement of imageUploadElements.get( loader.id )! ) { // Force re–paint in Safari. Without it, the image will display with a wrong size. // https://github.com/ckeditor/ckeditor5/issues/1975 @@ -408,10 +412,6 @@ export default class ImageUploadEditing extends Plugin { } ); } - if ( editor.ui ) { - editor.ui.ariaLiveAnnouncer.announce( t( 'Uploading image' ) ); - } - return promise; } ) .then( data => { From 269db0db2b26b54eb3f77e34b95d60248b8f35cf Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 2 Dec 2024 13:02:28 +0100 Subject: [PATCH 08/10] No longer crash when undo pasted image in uploading state. --- .../src/imageupload/imageuploadediting.ts | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index ff15def6f7b..3c568b3350a 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -276,7 +276,16 @@ export default class ImageUploadEditing extends Plugin { // If the image was inserted to the graveyard for good (**not** replaced by another image), // only then abort the loading process. if ( !insertedImagesIds.has( uploadId ) ) { - loader.abort(); + // ... but abort it only if all remain images that share the same loader are in the graveyard too. + // This is to prevent situation when we have two images in uploading state and one of them is being + // placed in the graveyard (e.g. using undo). The other one should not be aborted. + const allImagesThatShareUploaderInGraveyard = Array + .from( this._uploadImageElements.get( uploadId )! ) + .every( element => element.root.rootName == '$graveyard' ); + + if ( allImagesThatShareUploaderInGraveyard ) { + loader.abort(); + } } } else { // Remember the upload id of the inserted image. If it acted as a replacement for another From c28e4801e033c390d8ac17e3cc80e0e3df98a0ac Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 2 Dec 2024 13:12:16 +0100 Subject: [PATCH 09/10] No longer delete the image from `_uploadedImages` after first paste. --- .../ckeditor5-image/src/imageupload/imageuploadediting.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index 3c568b3350a..38ca185b223 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -266,7 +266,10 @@ export default class ImageUploadEditing extends Plugin { imageElement: imageElement as Element } ); - this._uploadedImages.delete( uploadId ); + // While it makes sense to remove the image from the `_uploadedImages` map here, + // it's counterintuitive for the user that pastes image in uploading several times. + // It'll work the first time, but the next time the image will be empty because the + // `_uploadedImages` no longer contain the response. } continue; From 1844484b270dbd2c78ba069f65ab54583b559d26 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Tue, 3 Dec 2024 14:20:27 +0100 Subject: [PATCH 10/10] Add enqueue changes to broadcast info about upload complete. --- .../src/imageupload/imageuploadediting.ts | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts index 38ca185b223..240be8c3d0d 100644 --- a/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts +++ b/packages/ckeditor5-image/src/imageupload/imageuploadediting.ts @@ -261,9 +261,13 @@ export default class ImageUploadEditing extends Plugin { // to restore response object from the internal map. if ( !isInsertedInGraveyard && this._uploadedImages.has( uploadId ) ) { // Fire `uploadComplete` to set proper attributes on the image element. - this.fire( 'uploadComplete', { - data: this._uploadedImages.get( uploadId )!, - imageElement: imageElement as Element + editor.model.enqueueChange( { isUndoable: false }, writer => { + writer.setAttribute( 'uploadStatus', 'complete', imageElement ); + + this.fire( 'uploadComplete', { + data: this._uploadedImages.get( uploadId )!, + imageElement: imageElement as Element + } ); } ); // While it makes sense to remove the image from the `_uploadedImages` map here,