Merge pull request #47 from ckeditor/t/42

Fix: Fixed two issues related to dropping images. First, when dropping a file into an empty paragraph, that paragraph should be replaced with that image. Second, drop position should be read correctly when the editor is focused upon drop. Closes #42. Closes #29.

BREAKING CHANGE: `UploadImageCommand` doesn't optimize the drop position itself anymore. Instead, a separate `findOptimalInsertionPosition()` function was introduced.

BREAKING CHANGE: `UploadImageCommand` doesn't verify the type of file anymore. This needs to be done by the caller.

Author: Reinmar

src/imageuploadbutton.js

@@ -11,6 +11,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import ImageUploadEngine from './imageuploadengine';
 import FileDialogButtonView from './ui/filedialogbuttonview';
 import imageIcon from '@ckeditor/ckeditor5-core/theme/icons/image.svg';
+import { isImageType, findOptimalInsertionPosition } from './utils';
 
 /**
  * Image upload button plugin.
@@ -50,7 +51,11 @@ export default class ImageUploadButton extends Plugin {
 
 			view.on( 'done', ( evt, files ) => {
 				for ( const file of files ) {
-					editor.execute( 'imageUpload', { file } );
+					const insertAt = findOptimalInsertionPosition( editor.document.selection );
+
+					if ( isImageType( file ) ) {
+						editor.execute( 'imageUpload', { file, insertAt } );
+					}
 				}
 			} );
 

src/imageuploadcommand.js

@@ -3,13 +3,10 @@
  * For licensing, see LICENSE.md.
  */
 
-import ModelDocumentFragment from '@ckeditor/ckeditor5-engine/src/model/documentfragment';
 import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range';
-import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
 import ModelSelection from '@ckeditor/ckeditor5-engine/src/model/selection';
 import FileRepository from './filerepository';
-import { isImageType } from './utils';
 import Command from '@ckeditor/ckeditor5-core/src/command';
 
 /**
@@ -29,7 +26,9 @@ export default class ImageUploadCommand extends Command {
 	 * @param {Object} options Options for executed command.
 	 * @param {File} options.file Image file to upload.
 	 * @param {module:engine/model/position~Position} [options.insertAt] Position at which the image should be inserted.
-	 * If the position won't be specified the image will be inserted next to the selection.
+	 * If the position is not specified the image will be inserted into the current selection.
+	 * Note: You can use the {@link module:upload/utils~findOptimalInsertionPosition} function to calculate
+	 * (e.g. based on the current selection) a position which is more optimal from UX perspective.
 	 * @param {module:engine/model/batch~Batch} [options.batch] Batch to collect all the change steps.
 	 * New batch will be created if this option is not set.
 	 */
@@ -41,28 +40,25 @@ export default class ImageUploadCommand extends Command {
 		const selection = doc.selection;
 		const fileRepository = editor.plugins.get( FileRepository );
 
-		if ( !isImageType( file ) ) {
-			return;
-		}
-
 		doc.enqueueChanges( () => {
-			const insertAt = options.insertAt || getInsertionPosition( doc );
-
-			// No position to insert.
-			if ( !insertAt ) {
-				return;
-			}
-
 			const imageElement = new ModelElement( 'image', {
 				uploadId: fileRepository.createLoader( file ).id
 			} );
-			const documentFragment = new ModelDocumentFragment( [ imageElement ] );
-			const range = new ModelRange( insertAt );
-			const insertSelection = new ModelSelection();
 
-			insertSelection.setRanges( [ range ] );
-			editor.data.insertContent( documentFragment, insertSelection, batch );
-			selection.setRanges( [ ModelRange.createOn( imageElement ) ] );
+			let insertAtSelection;
+
+			if ( options.insertAt ) {
+				insertAtSelection = new ModelSelection( [ new ModelRange( options.insertAt ) ] );
+			} else {
+				insertAtSelection = doc.selection;
+			}
+
+			editor.data.insertContent( imageElement, insertAtSelection, batch );
+
+			// Inserting an image might've failed due to schema regulations.
+			if ( imageElement.parent ) {
+				selection.setRanges( [ ModelRange.createOn( imageElement ) ] );
+			}
 		} );
 	}
 }
@@ -71,26 +67,4 @@ export default class ImageUploadCommand extends Command {
 //
 // @param {module:engine/model/document~Document} doc
 // @returns {module:engine/model/position~Position|undefined}
-function getInsertionPosition( doc ) {
-	const selection = doc.selection;
-	const selectedElement = selection.getSelectedElement();
-
-	// If selected element is placed directly in root - return position after that element.
-	if ( selectedElement && selectedElement.parent.is( 'rootElement' ) ) {
-		return ModelPosition.createAfter( selectedElement );
-	}
-
-	const firstBlock = doc.selection.getSelectedBlocks().next().value;
 
-	if ( firstBlock ) {
-		const positionAfter = ModelPosition.createAfter( firstBlock );
-
-		// If selection is at the end of the block - return position after the block.
-		if ( selection.focus.isTouching( positionAfter ) ) {
-			return positionAfter;
-		}
-
-		// Otherwise return position before the block.
-		return ModelPosition.createBefore( firstBlock );
-	}
-}

src/imageuploadengine.js

@@ -11,7 +11,8 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import FileRepository from './filerepository';
 import ImageUploadCommand from './imageuploadcommand';
 import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification';
-import { isImageType } from './utils';
+import ModelSelection from '@ckeditor/ckeditor5-engine/src/model/selection';
+import { isImageType, findOptimalInsertionPosition } from './utils';
 
 /**
  * Image upload engine plugin.
@@ -45,11 +46,21 @@ export default class ImageUploadEngine extends Plugin {
 
 		// Execute imageUpload command when image is dropped or pasted.
 		editor.editing.view.on( 'clipboardInput', ( evt, data ) => {
+			let targetModelSelection = new ModelSelection(
+				data.targetRanges.map( viewRange => editor.editing.mapper.toModelRange( viewRange ) )
+			);
+
 			for ( const file of data.dataTransfer.files ) {
+				const insertAt = findOptimalInsertionPosition( targetModelSelection );
+
 				if ( isImageType( file ) ) {
-					editor.execute( 'imageUpload', { file } );
+					editor.execute( 'imageUpload', { file, insertAt } );
 					evt.stop();
 				}
+
+				// Use target ranges only for the first image. Then, use that image position
+				// so we keep adding the next ones after the previous one.
+				targetModelSelection = doc.selection;
 			}
 		} );
 

src/utils.js

@@ -7,6 +7,8 @@
  * @module upload/utils
  */
 
+import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
+
 /**
  * Checks if given file is an image.
  *
@@ -19,3 +21,47 @@ export function isImageType( file ) {
 	return types.test( file.type );
 }
 
+/**
+ * Returns a model position which is optimal (in terms of UX) for inserting an image.
+ *
+ * For instance, if a selection is in a middle of a paragraph, position before this paragraph
+ * will be returned, so that it's not split. If the selection is at the end of a paragraph,
+ * position after this paragraph will be returned.
+ *
+ * Note: If selection is placed in an empty block, that block will be returned. If that position
+ * is then passed to {@link module:engine/controller/datacontroller~DataController#insertContent}
+ * that block will be fully replaced by the image.
+ *
+ * @param {module:engine/model/selection~Selection} selection Selection based on which the
+ * insertion position should be calculated.
+ * @returns {module:engine/model/position~Position} The optimal position.
+ */
+export function findOptimalInsertionPosition( selection ) {
+	const selectedElement = selection.getSelectedElement();
+
+	if ( selectedElement ) {
+		return ModelPosition.createAfter( selectedElement );
+	}
+
+	const firstBlock = selection.getSelectedBlocks().next().value;
+
+	if ( firstBlock ) {
+		// If inserting into an empty block – return position in that block. It will get
+		// replaced with the image by insertContent(). #42.
+		if ( firstBlock.isEmpty ) {
+			return ModelPosition.createAt( firstBlock );
+		}
+
+		const positionAfter = ModelPosition.createAfter( firstBlock );
+
+		// If selection is at the end of the block - return position after the block.
+		if ( selection.focus.isTouching( positionAfter ) ) {
+			return positionAfter;
+		}
+
+		// Otherwise return position before the block.
+		return ModelPosition.createBefore( firstBlock );
+	}
+
+	return selection.focus;
+}

tests/imageuploadbutton.js

@@ -6,27 +6,50 @@
 /* globals document */
 
 import ClassicEditor from '@ckeditor/ckeditor5-editor-classic/src/classiceditor';
+
 import Image from '@ckeditor/ckeditor5-image/src/image';
 import FileDialogButtonView from '../src/ui/filedialogbuttonview';
+import FileRepository from '../src/filerepository';
 import ImageUploadButton from '../src/imageuploadbutton';
 import ImageUploadEngine from '../src/imageuploadengine';
-import { createNativeFileMock } from './_utils/mocks';
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
+import Notification from '@ckeditor/ckeditor5-ui/src/notification/notification';
+
+import { createNativeFileMock, AdapterMock } from './_utils/mocks';
+import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 
 describe( 'ImageUploadButton', () => {
-	let editor;
+	let editor, doc, editorElement, fileRepository;
 
 	beforeEach( () => {
-		const editorElement = document.createElement( 'div' );
+		editorElement = document.createElement( 'div' );
 		document.body.appendChild( editorElement );
 
-		return ClassicEditor.create( editorElement, {
-			plugins: [ Image, ImageUploadButton ]
-		} )
+		return ClassicEditor
+			.create( editorElement, {
+				plugins: [ Paragraph, Image, ImageUploadButton, FileRepository ]
+			} )
 			.then( newEditor => {
 				editor = newEditor;
+				doc = editor.document;
+
+				fileRepository = editor.plugins.get( FileRepository );
+				fileRepository.createAdapter = loader => {
+					return new AdapterMock( loader );
+				};
+
+				// Hide all notifications (prevent alert() calls).
+				const notification = editor.plugins.get( Notification );
+				notification.on( 'show', evt => evt.stop() );
 			} );
 	} );
 
+	afterEach( () => {
+		editorElement.remove();
+
+		return editor.destroy();
+	} );
+
 	it( 'should include ImageUploadEngine', () => {
 		expect( editor.plugins.get( ImageUploadEngine ) ).to.be.instanceOf( ImageUploadEngine );
 	} );
@@ -60,5 +83,52 @@ describe( 'ImageUploadButton', () => {
 		expect( executeStub.firstCall.args[ 0 ] ).to.equal( 'imageUpload' );
 		expect( executeStub.firstCall.args[ 1 ].file ).to.equal( files[ 0 ] );
 	} );
+
+	it( 'should optimize the insertion position', () => {
+		const button = editor.ui.componentFactory.create( 'insertImage' );
+		const files = [ createNativeFileMock() ];
+
+		setModelData( doc, '<paragraph>f[]oo</paragraph>' );
+
+		button.fire( 'done', files );
+
+		const id = fileRepository.getLoader( files[ 0 ] ).id;
+
+		expect( getModelData( doc ) ).to.equal(
+			`[<image uploadId="${ id }" uploadStatus="reading"></image>]` +
+			'<paragraph>foo</paragraph>'
+		);
+	} );
+
+	it( 'should correctly insert multiple files', () => {
+		const button = editor.ui.componentFactory.create( 'insertImage' );
+		const files = [ createNativeFileMock(), createNativeFileMock() ];
+
+		setModelData( doc, '<paragraph>foo[]</paragraph><paragraph>bar</paragraph>' );
+
+		button.fire( 'done', files );
+
+		const id1 = fileRepository.getLoader( files[ 0 ] ).id;
+		const id2 = fileRepository.getLoader( files[ 1 ] ).id;
+
+		expect( getModelData( doc ) ).to.equal(
+			'<paragraph>foo</paragraph>' +
+			`<image uploadId="${ id1 }" uploadStatus="reading"></image>` +
+			`[<image uploadId="${ id2 }" uploadStatus="reading"></image>]` +
+			'<paragraph>bar</paragraph>'
+		);
+	} );
+
+	it( 'should not execute imageUpload if the file is not an image', () => {
+		const executeStub = sinon.stub( editor, 'execute' );
+		const button = editor.ui.componentFactory.create( 'insertImage' );
+		const file = {
+			type: 'media/mp3',
+			size: 1024
+		};
+
+		button.fire( 'done', [ file ] );
+		sinon.assert.notCalled( executeStub );
+	} );
 } );