Merge pull request #13371 from ckeditor/ck/13366

Fix (clipboard, engine): Dragging images in the editor should not lag in Firefox. Closes #13366.

packages/ckeditor5-clipboard/src/clipboardobserver.ts

@@ -83,8 +83,11 @@ export default class ClipboardObserver extends DomEventObserver<
 	}
 
 	public onDomEvent( domEvent: ClipboardEvent | DragEvent ): void {
+		const nativeDataTransfer = 'clipboardData' in domEvent ? domEvent.clipboardData! : domEvent.dataTransfer!;
+		const cacheFiles = domEvent.type == 'drop' || domEvent.type == 'paste';
+
 		const evtData: ClipboardEventData = {
-			dataTransfer: new DataTransfer( 'clipboardData' in domEvent ? domEvent.clipboardData! : domEvent.dataTransfer! )
+			dataTransfer: new DataTransfer( nativeDataTransfer, { cacheFiles } )
 		};
 
 		if ( domEvent.type == 'drop' || domEvent.type == 'dragover' ) {

packages/ckeditor5-clipboard/tests/clipboardobserver.js

@@ -11,9 +11,12 @@ import View from '@ckeditor/ckeditor5-engine/src/view/view';
 import DataTransfer from '@ckeditor/ckeditor5-engine/src/view/datatransfer';
 import DowncastWriter from '@ckeditor/ckeditor5-engine/src/view/downcastwriter';
 import createViewRoot from '@ckeditor/ckeditor5-engine/tests/view/_utils/createroot';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 
 describe( 'ClipboardObserver', () => {
-	let view, doc, writer, observer, root, el, range, eventSpy, preventDefaultSpy, stopPropagationSpy;
+	let view, doc, writer, observer, root, el, range, eventSpy, preventDefaultSpy, stopPropagationSpy, mockedDomDataTransferFilesSpy;
+
+	testUtils.createSinonSandbox();
 
 	beforeEach( () => {
 		view = new View();
@@ -68,6 +71,7 @@ describe( 'ClipboardObserver', () => {
 			expect( data.dataTransfer.getData( 'x/y' ) ).to.equal( 'foo:x/y' );
 
 			expect( preventDefaultSpy.calledOnce ).to.be.true;
+			expect( mockedDomDataTransferFilesSpy.calledOnce ).to.be.true;
 		} );
 	} );
 
@@ -97,6 +101,7 @@ describe( 'ClipboardObserver', () => {
 			expect( data.dropRange ).to.be.null;
 
 			expect( preventDefaultSpy.calledOnce ).to.be.true;
+			expect( mockedDomDataTransferFilesSpy.calledOnce ).to.be.true;
 		} );
 
 		it( 'should be fired with the right event data – dropRange (when no info about it in the drop event)', () => {
@@ -309,21 +314,55 @@ describe( 'ClipboardObserver', () => {
 			expect( data.domEvent.type ).to.equal( 'dragover' );
 			expect( data.dataTransfer.files ).to.deep.equal( dataTransfer.files );
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/13366
+		it( 'should not access native DataTransfer files if not needed', () => {
+			const dataTransfer = mockDomDataTransfer();
+			const targetElement = mockDomTargetElement( {} );
+
+			doc.on( 'dragover', eventSpy );
+
+			observer.onDomEvent( {
+				type: 'dragover',
+				target: targetElement,
+				dataTransfer,
+				preventDefault: preventDefaultSpy
+			} );
+
+			expect( eventSpy.calledOnce ).to.be.true;
+
+			const data = eventSpy.args[ 0 ][ 1 ];
+
+			expect( data.domTarget ).to.equal( targetElement );
+
+			expect( data.dataTransfer ).to.be.instanceOf( DataTransfer );
+			expect( data.dataTransfer.getData( 'x/y' ) ).to.equal( 'foo:x/y' );
+
+			expect( data.dropRange ).to.be.null;
+
+			expect( preventDefaultSpy.calledOnce ).to.be.true;
+			expect( mockedDomDataTransferFilesSpy.notCalled ).to.be.true;
+		} );
 	} );
-} );
 
-// Returns a super simple mock of HTMLElement (we use only ownerDocument from it).
-function mockDomTargetElement( documentMock ) {
-	return {
-		ownerDocument: documentMock
-	};
-}
-
-function mockDomDataTransfer() {
-	return {
-		files: [],
-		getData( type ) {
-			return 'foo:' + type;
-		}
-	};
-}
+	// Returns a super simple mock of HTMLElement (we use only ownerDocument from it).
+	function mockDomTargetElement( documentMock ) {
+		return {
+			ownerDocument: documentMock
+		};
+	}
+
+	function mockDomDataTransfer() {
+		mockedDomDataTransferFilesSpy = sinon.spy();
+
+		return {
+			get files() {
+				mockedDomDataTransferFilesSpy();
+				return [];
+			},
+			getData( type ) {
+				return 'foo:' + type;
+			}
+		};
+	}
+} );

packages/ckeditor5-engine/src/view/datatransfer.ts

@@ -16,19 +16,38 @@ export default class DataTransfer {
 	/**
 	 * The array of files created from the native `DataTransfer#files` or `DataTransfer#items`.
 	 */
-	public readonly files: Array<File>;
+	private _files: Array<File> | null;
 
 	/**
 	 * The native DataTransfer object.
 	 */
 	private _native: DomDataTransfer;
 
-	constructor( nativeDataTransfer: DomDataTransfer ) {
-		this.files = getFiles( nativeDataTransfer );
+	/**
+	 * @param nativeDataTransfer The native [`DataTransfer`](https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer) object.
+	 * @param options.cacheFiles Whether `files` list should be initialized in the constructor.
+	 */
+	constructor( nativeDataTransfer: DomDataTransfer, options: { cacheFiles?: boolean } = {} ) {
+		// We should store references to the File instances in case someone would like to process this files
+		// outside the event handler. Files are stored only for `drop` and `paste` events because they are not usable
+		// in other events and are generating a huge delay on Firefox while dragging.
+		// See https://github.com/ckeditor/ckeditor5/issues/13366.
+		this._files = options.cacheFiles ? getFiles( nativeDataTransfer ) : null;
 
 		this._native = nativeDataTransfer;
 	}
 
+	/**
+	 * The array of files created from the native `DataTransfer#files` or `DataTransfer#items`.
+	 */
+	public get files(): Array<File> {
+		if ( !this._files ) {
+			this._files = getFiles( this._native );
+		}
+
+		return this._files;
+	}
+
 	/**
 	 * Returns an array of available native content types.
 	 */

packages/ckeditor5-engine/tests/view/datatransfer.js

@@ -47,6 +47,7 @@ describe( 'DataTransfer', () => {
 
 			const dt = new DataTransfer( nativeDataTransfer );
 
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
 			expect( dt.files ).to.deep.equal( [ 'file1' ] );
 
 			expect( spy.get.calledOnce ).to.be.true;
@@ -66,11 +67,105 @@ describe( 'DataTransfer', () => {
 
 			const dt = new DataTransfer( nativeDataTransfer );
 
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
+
+			expect( spy.get.calledOnce ).to.be.true;
+		} );
+
+		it( 'should cache files if cacheFiles option is set', () => {
+			const nativeDataTransfer = {
+				get files() {
+					return {
+						0: 'file1',
+						length: 1
+					};
+				}
+			};
+
+			const spy = sinon.spy( nativeDataTransfer, 'files', [ 'get' ] );
+
+			const dt = new DataTransfer( nativeDataTransfer, { cacheFiles: true } );
+
+			expect( spy.get.calledOnce ).to.be.true;
+			expect( dt._files ).to.deep.equal( [ 'file1' ] );
+
+			// Access getter.
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
+
+			expect( spy.get.calledOnce ).to.be.true;
+		} );
+
+		it( 'should not cache files if cacheFiles option is not set', () => {
+			const nativeDataTransfer = {
+				get files() {
+					return {
+						0: 'file1',
+						length: 1
+					};
+				}
+			};
+
+			const spy = sinon.spy( nativeDataTransfer, 'files', [ 'get' ] );
+
+			const dt = new DataTransfer( nativeDataTransfer );
+
+			expect( spy.get.calledOnce ).to.be.false;
+			expect( dt._files ).to.be.null;
+
+			// Access getter.
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
+
+			expect( spy.get.calledOnce ).to.be.true;
+		} );
+
+		it( 'should cache files (from items) if cacheFiles option is set', () => {
+			const nativeDataTransfer = {
+				get items() {
+					return {
+						0: { kind: 'file', getAsFile: () => 'file1' },
+						length: 1
+					};
+				}
+			};
+
+			const spy = sinon.spy( nativeDataTransfer, 'items', [ 'get' ] );
+
+			const dt = new DataTransfer( nativeDataTransfer, { cacheFiles: true } );
+
+			expect( spy.get.calledOnce ).to.be.true;
+			expect( dt._files ).to.deep.equal( [ 'file1' ] );
+
+			// Access getter.
+			expect( dt.files ).to.deep.equal( [ 'file1' ] );
+
+			expect( spy.get.calledOnce ).to.be.true;
+		} );
+
+		it( 'should not cache files (from items) if cacheFiles option is not set', () => {
+			const nativeDataTransfer = {
+				get items() {
+					return {
+						0: { kind: 'file', getAsFile: () => 'file1' },
+						length: 1
+					};
+				}
+			};
+
+			const spy = sinon.spy( nativeDataTransfer, 'items', [ 'get' ] );
+
+			const dt = new DataTransfer( nativeDataTransfer );
+
+			expect( spy.get.calledOnce ).to.be.false;
+			expect( dt._files ).to.be.null;
+
+			// Access getter.
 			expect( dt.files ).to.deep.equal( [ 'file1' ] );
 
 			expect( spy.get.calledOnce ).to.be.true;
 		} );
 	} );
+
 	describe( 'getData()', () => {
 		it( 'should return data from the native data transfer', () => {
 			const dt = new DataTransfer( {