Merge pull request #97 from ckeditor/t/8

Fix: Bare `<img>` (not wrapped with `<figure class="image">`) can now be pasted into the editor. Closes #8.

Author: Reinmar

Diff for: src/image/converters.js

@@ -7,7 +7,6 @@
  * @module image/image/converters
  */
 
-import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position';
 import modelWriter from '@ckeditor/ckeditor5-engine/src/model/writer';
 
@@ -20,52 +19,46 @@ import modelWriter from '@ckeditor/ckeditor5-engine/src/model/writer';
  *
  *		<image src="..." alt="..."></image>
  *
+ * The entire contents of `<figure>` except the first `<img>` is being converted as children
+ * of the `<image>` model element.
+ *
  * @returns {Function}
  */
-export function viewToModelImage() {
+export function viewFigureToModel() {
 	return ( evt, data, consumable, conversionApi ) => {
-		const viewFigureElement = data.input;
-
-		// *** Step 1: Validate conversion.
-		// Check if figure element can be consumed.
-		if ( !consumable.test( viewFigureElement, { name: true, class: 'image' } ) ) {
+		// Do not convert if this is not an "image figure".
+		if ( !consumable.test( data.input, { name: true, class: 'image' } ) ) {
 			return;
 		}
 
-		// Check if image element can be converted in current context.
+		// Do not convert if image cannot be placed in model at this context.
 		if ( !conversionApi.schema.check( { name: 'image', inside: data.context, attributes: 'src' } ) ) {
 			return;
 		}
 
-		// Check if img element is placed inside figure element and can be consumed with `src` attribute.
-		const viewImg = viewFigureElement.getChild( 0 );
+		// Find an image element inside the figure element.
+		const viewImage = Array.from( data.input.getChildren() ).find( viewChild => viewChild.is( 'img' ) );
 
-		if ( !viewImg || viewImg.name != 'img' || !consumable.test( viewImg, { name: true, attribute: 'src' } ) ) {
+		// Do not convert if image element is absent, is missing src attribute or was already converted.
+		if ( !viewImage || !viewImage.hasAttribute( 'src' ) || !consumable.test( viewImage, { name: true } ) ) {
 			return;
 		}
 
-		// *** Step2: Convert to model.
-		consumable.consume( viewFigureElement, { name: true, class: 'image' } );
-		consumable.consume( viewImg, { name: true, attribute: 'src' } );
+		// Convert view image to model image.
+		const modelImage = conversionApi.convertItem( viewImage, consumable, data );
 
-		// Create model element.
-		const modelImage = new ModelElement( 'image', {
-			src: viewImg.getAttribute( 'src' )
-		} );
+		// Convert rest of figure element's children, but in the context of model image, because those converted
+		// children will be added as model image children.
+		data.context.push( modelImage );
 
-		// Convert `alt` attribute if present.
-		if ( consumable.consume( viewImg, { attribute: [ 'alt' ] } ) ) {
-			modelImage.setAttribute( 'alt', viewImg.getAttribute( 'alt' ) );
-		}
+		const modelChildren = conversionApi.convertChildren( data.input, consumable, data );
 
-		// Convert children of converted view element and append them to `modelImage`.
-		// TODO https://github.com/ckeditor/ckeditor5-engine/issues/736.
-		data.context.push( modelImage );
-		const modelChildren = conversionApi.convertChildren( viewFigureElement, consumable, data );
-		const insertPosition = ModelPosition.createAt( modelImage, 'end' );
-		modelWriter.insert( insertPosition, modelChildren );
 		data.context.pop();
 
+		// Add converted children to model image.
+		modelWriter.insert( ModelPosition.createAt( modelImage ), modelChildren );
+
+		// Set model image as conversion result.
 		data.output = modelImage;
 	};
 }

Diff for: src/image/imageengine.js

@@ -9,8 +9,10 @@
 
 import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
-import { viewToModelImage, createImageAttributeConverter } from './converters';
+import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
+import { viewFigureToModel, createImageAttributeConverter } from './converters';
 import { toImageWidget } from './utils';
+import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement';
 import ViewEmptyElement from '@ckeditor/ckeditor5-engine/src/view/emptyelement';
 
@@ -52,8 +54,19 @@ export default class ImageEngine extends Plugin {
 		createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'src' );
 		createImageAttributeConverter( [ editing.modelToView, data.modelToView ], 'alt' );
 
+		// Build converter for view img element to model image element.
+		buildViewConverter().for( data.viewToModel )
+			.from( { name: 'img', attribute: { src: /./ } } )
+			.toElement( ( viewImage ) => new ModelElement( 'image', { src: viewImage.getAttribute( 'src' ) } ) );
+
+		// Build converter for alt attribute.
+		buildViewConverter().for( data.viewToModel )
+			.from( { name: 'img', attribute: { alt: /./ } } )
+			.consuming( { attribute: [ 'alt' ] } )
+			.toAttribute( ( viewImage ) => ( { key: 'alt', value: viewImage.getAttribute( 'alt' ) } ) );
+
 		// Converter for figure element from view to model.
-		data.viewToModel.on( 'element:figure', viewToModelImage() );
+		data.viewToModel.on( 'element:figure', viewFigureToModel() );
 	}
 }
 

Diff for: tests/image/converters.js

@@ -3,11 +3,12 @@
  * For licensing, see LICENSE.md.
  */
 
-import { viewToModelImage, createImageAttributeConverter } from '../../src/image/converters';
+import { viewFigureToModel, createImageAttributeConverter } from '../../src/image/converters';
 import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import { createImageViewElement } from '../../src/image/imageengine';
 import { toImageWidget } from '../../src/image/utils';
 import buildModelConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildmodelconverter';
+import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter';
 import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element';
 import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view';
 import { setData as setModelData, getData as getModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
@@ -28,10 +29,6 @@ describe( 'Image converters', () => {
 				schema.allow( { name: 'image', attributes: [ 'alt', 'src' ], inside: '$root' } );
 				schema.objects.add( 'image' );
 
-				buildModelConverter().for( )
-					.fromElement( 'image' )
-					.toElement( () => toImageWidget( createImageViewElement() ) );
-
 				buildModelConverter().for( editor.editing.modelToView )
 					.fromElement( 'image' )
 					.toElement( () => toImageWidget( createImageViewElement() ) );
@@ -41,46 +38,87 @@ describe( 'Image converters', () => {
 			} );
 	} );
 
-	describe( 'viewToModelImage', () => {
-		let dispatcher, schema;
+	describe( 'viewFigureToModel', () => {
+		function expectModel( model ) {
+			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( model );
+		}
+
+		let dispatcher, schema, imgConverterCalled;
 
 		beforeEach( () => {
+			imgConverterCalled = false;
+
 			schema = document.schema;
+			schema.allow( { name: '$text', inside: 'image' } );
+
 			dispatcher = editor.data.viewToModel;
-			dispatcher.on( 'element:figure', viewToModelImage() );
+			dispatcher.on( 'element:figure', viewFigureToModel() );
+			dispatcher.on( 'element:img', ( evt, data, consumable ) => {
+				if ( consumable.consume( data.input, { name: true, attribute: 'src' } ) ) {
+					data.output = new ModelElement( 'image', { src: data.input.getAttribute( 'src' ) } );
+
+					imgConverterCalled = true;
+				}
+			} );
 		} );
 
-		it( 'should convert view figure element', () => {
-			editor.setData( '<figure class="image"><img src="foo.png" alt="bar baz"></img></figure>' );
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image alt="bar baz" src="foo.png"></image>' );
+		it( 'should find img element among children and convert it using already defined converters', () => {
+			editor.setData( '<figure class="image"><img src="foo.png" /></figure>' );
+
+			expectModel( '<image src="foo.png"></image>' );
+			expect( imgConverterCalled ).to.be.true;
 		} );
 
-		it( 'should convert without alt', () => {
-			editor.setData( '<figure class="image"><img src="foo.png"></img></figure>' );
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image src="foo.png"></image>' );
+		it( 'should convert non-img children in image context and append them to model image element', () => {
+			buildViewConverter().for( editor.data.viewToModel ).fromElement( 'foo' ).toElement( 'foo' );
+			buildViewConverter().for( editor.data.viewToModel ).fromElement( 'bar' ).toElement( 'bar' );
+
+			schema.registerItem( 'foo' );
+			schema.registerItem( 'bar' );
+
+			schema.allow( { name: 'foo', inside: 'image' } );
+
+			editor.setData( '<figure class="image">x<img src="foo.png" />y<foo></foo><bar></bar></figure>' );
+
+			// Element bar not converted because schema does not allow it.
+			expectModel( '<image src="foo.png">xy<foo></foo></image>' );
 		} );
 
-		it( 'should not convert if figure element is already consumed', () => {
+		it( 'should be possible to overwrite', () => {
 			dispatcher.on( 'element:figure', ( evt, data, consumable ) => {
-				consumable.consume( data.input, { name: true, class: 'image' } );
+				consumable.consume( data.input, { name: true } );
+				consumable.consume( data.input.getChild( 0 ), { name: true } );
 
-				data.output = new ModelElement( 'not-image' );
+				data.output = new ModelElement( 'myImage', { data: { src: data.input.getChild( 0 ).getAttribute( 'src' ) } } );
 			}, { priority: 'high' } );
 
-			editor.setData( '<figure class="image"><img src="foo.png" alt="bar baz"></img></figure>' );
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<not-image></not-image>' );
+			editor.setData( '<figure class="image"><img src="foo.png" />xyz</figure>' );
+
+			expectModel( '<myImage data="{"src":"foo.png"}"></myImage>' );
 		} );
 
-		it( 'should not convert image if schema disallows it', () => {
-			schema.disallow( { name: 'image', attributes: [ 'alt', 'src' ], inside: '$root' } );
+		// Test exactly what figure converter does, which is putting it's children element to image element.
+		// If this has not been done, it means that figure converter was not used.
+		it( 'should not convert if figure do not have class="image" attribute', () => {
+			editor.setData( '<figure><img src="foo.png" />xyz</figure>' );
 
-			editor.setData( '<figure class="image"><img src="foo.png"></img></figure>' );
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' );
+			// Default image converter will be fired.
+			expectModel( '<image src="foo.png"></image>' );
 		} );
 
-		it( 'should not convert image if there is no img element', () => {
-			editor.setData( '<figure class="image"></figure>' );
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' );
+		it( 'should not convert if there is no img element among children', () => {
+			editor.setData( '<figure class="image">xyz</figure>' );
+
+			// Figure converter outputs nothing and text is disallowed in root.
+			expectModel( '' );
+		} );
+
+		it( 'should not convert if img element was not converted', () => {
+			// Image element missing src attribute.
+			editor.setData( '<figure class="image"><img alt="abc" />xyz</figure>' );
+
+			// Figure converter outputs nothing and text is disallowed in root.
+			expectModel( '' );
 		} );
 	} );
 

Diff for: tests/image/imageengine.js

@@ -58,7 +58,7 @@ describe( 'ImageEngine', () => {
 			} );
 
 			it( 'should not convert if there is no image class', () => {
-				editor.setData( '<figure><img src="foo.png" alt="alt text" /></figure>' );
+				editor.setData( '<figure class="quote">My quote</figure>' );
 
 				expect( getModelData( document, { withoutSelection: true } ) )
 					.to.equal( '' );
@@ -140,6 +140,36 @@ describe( 'ImageEngine', () => {
 
 				sinon.assert.calledOnce( conversionSpy );
 			} );
+
+			it( 'should convert bare img element', () => {
+				editor.setData( '<img src="foo.png" alt="alt text" />' );
+
+				expect( getModelData( document, { withoutSelection: true } ) )
+					.to.equal( '<image alt="alt text" src="foo.png"></image>' );
+			} );
+
+			it( 'should not convert alt attribute on non-img element', () => {
+				const data = editor.data;
+				const editing = editor.editing;
+
+				document.schema.registerItem( 'div', '$block' );
+				document.schema.allow( { name: 'div', attributes: 'alt', inside: '$root' } );
+
+				buildModelConverter().for( data.modelToView, editing.modelToView ).fromElement( 'div' ).toElement( 'div' );
+				buildViewConverter().for( data.viewToModel ).fromElement( 'div' ).toElement( 'div' );
+
+				editor.setData( '<div alt="foo"></div>' );
+
+				expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<div></div>' );
+			} );
+
+			it( 'should handle figure with two images', () => {
+				document.schema.allow( { name: '$text', inside: 'image' } );
+
+				editor.setData( '<figure class="image"><img src="foo.jpg" /><img src="bar.jpg" />abc</figure>' );
+
+				expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image src="foo.jpg">abc</image>' );
+			} );
 		} );
 	} );