Merge pull request #933 from ckeditor/t/932

Fix: Unified values returned in `data.output` during view-to-model conversion. See breaking changes. Closes #932.

BREAKING CHANGE: `ViewConversionDispatcher#convert()` will always return `model.DocumentFragment` (which may be empty in various cases). `conversionApi#convertItem()` will log a warning if `data.output` contains a different value than `model.Node` or `model.DocumentFragment` or `null`. `conversionApi#convertChildren()` will always return `model.DocumentFragment`.

src/conversion/viewconversiondispatcher.js

@@ -8,16 +8,18 @@
  */
 
 import ViewConsumable from './viewconsumable';
-import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
-import mix from '@ckeditor/ckeditor5-utils/src/mix';
-import extend from '@ckeditor/ckeditor5-utils/src/lib/lodash/extend';
 import ModelRange from '../model/range';
 import ModelPosition from '../model/position';
 import ModelTreeWalker from '../model/treewalker';
 import ModelNode from '../model/node';
 import ModelDocumentFragment from '../model/documentfragment';
 import { remove } from '../model/writer';
 
+import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
+import mix from '@ckeditor/ckeditor5-utils/src/mix';
+import extend from '@ckeditor/ckeditor5-utils/src/lib/lodash/extend';
+import log from '@ckeditor/ckeditor5-utils/src/log';
+
 /**
  * `ViewConversionDispatcher` is a central point of {@link module:engine/view/view view} conversion, which is a process of
  * converting given {@link module:engine/view/documentfragment~DocumentFragment view document fragment} or
@@ -130,36 +132,35 @@ export default class ViewConversionDispatcher {
 	 * @fires element
 	 * @fires text
 	 * @fires documentFragment
-	 * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element}
-	 * viewItem Part of the view to be converted.
+	 * @param {module:engine/view/documentfragment~DocumentFragment|module:engine/view/element~Element} viewItem
+	 * Part of the view to be converted.
 	 * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher`
 	 * events. See also {@link ~ViewConversionDispatcher#event:element element event}.
 	 * @returns {module:engine/model/documentfragment~DocumentFragment} Model data that is a result of the conversion process
-	 * wrapped by DocumentFragment. Converted marker stamps will be set as DocumentFragment
+	 * wrapped in `DocumentFragment`. Converted marker stamps will be set as that document fragment's
 	 * {@link module:engine/view/documentfragment~DocumentFragment#markers static markers map}.
 	 */
 	convert( viewItem, additionalData = {} ) {
 		this.fire( 'viewCleanup', viewItem );
 
 		const consumable = ViewConsumable.createFrom( viewItem );
-		const conversionResult = this._convertItem( viewItem, consumable, additionalData );
+		let conversionResult = this._convertItem( viewItem, consumable, additionalData );
 
-		// In some cases conversion output doesn't have to be a node and in this case we do nothing additional with this data.
-		if ( !( conversionResult instanceof ModelNode || conversionResult instanceof ModelDocumentFragment ) ) {
-			return conversionResult;
+		// We can get a null here if conversion failed (see _convertItem())
+		// or simply if an item could not be converted (e.g. due to the schema).
+		if ( !conversionResult ) {
+			return new ModelDocumentFragment();
 		}
 
-		let documentFragment = conversionResult;
-
-		// When conversion result is not a DocumentFragment we need to wrap it by DocumentFragment.
-		if ( !documentFragment.is( 'documentFragment' ) ) {
-			documentFragment = new ModelDocumentFragment( [ documentFragment ] );
+		// When conversion result is not a document fragment we need to wrap it in document fragment.
+		if ( !conversionResult.is( 'documentFragment' ) ) {
+			conversionResult = new ModelDocumentFragment( [ conversionResult ] );
 		}
 
 		// Extract temporary markers stamp from model and set as static markers collection.
-		documentFragment.markers = extractMarkersFromModelFragment( documentFragment );
+		conversionResult.markers = extractMarkersFromModelFragment( conversionResult );
 
-		return documentFragment;
+		return conversionResult;
 	}
 
 	/**
@@ -180,6 +181,21 @@ export default class ViewConversionDispatcher {
 			this.fire( 'documentFragment', data, consumable, this.conversionApi );
 		}
 
+		// Handle incorrect `data.output`.
+		if ( data.output && !( data.output instanceof ModelNode || data.output instanceof ModelDocumentFragment ) ) {
+			/**
+			 * Dropped incorrect conversion result.
+			 *
+			 * Item may be converted to either {@link module:engine/model/node~Node model node} or
+			 * {@link module:engine/model/documentfragment~DocumentFragment model document fragment}.
+			 *
+			 * @error view-conversion-dispatcher-incorrect-result
+			 */
+			log.warn( 'view-conversion-dispatcher-incorrect-result: Dropped incorrect conversion result.', [ input, data.output ] );
+
+			return null;
+		}
+
 		return data.output;
 	}
 
@@ -188,11 +204,23 @@ export default class ViewConversionDispatcher {
 	 * @see module:engine/conversion/viewconversiondispatcher~ViewConversionApi#convertChildren
 	 */
 	_convertChildren( input, consumable, additionalData = {} ) {
+		// Get all children of view input item.
 		const viewChildren = Array.from( input.getChildren() );
-		const convertedChildren = viewChildren.map( ( viewChild ) => this._convertItem( viewChild, consumable, additionalData ) );
 
-		// Flatten and remove nulls.
-		return convertedChildren.reduce( ( a, b ) => b ? a.concat( b ) : a, [] );
+		// 1. Map those children to model.
+		// 2. Filter out items that has not been converted or for which conversion returned wrong result (for those warning is logged).
+		// 3. Extract children from document fragments to flatten results.
+		const convertedChildren = viewChildren
+			.map( ( viewChild ) => this._convertItem( viewChild, consumable, additionalData ) )
+			.filter( ( converted ) => converted instanceof ModelNode || converted instanceof ModelDocumentFragment )
+			.reduce( ( result, filtered ) => {
+				return result.concat(
+					filtered.is( 'documentFragment' ) ? Array.from( filtered.getChildren() ) : filtered
+				);
+			}, [] );
+
+		// Normalize array to model document fragment.
+		return new ModelDocumentFragment( convertedChildren );
 	}
 
 	/**
@@ -304,6 +332,8 @@ function extractMarkersFromModelFragment( modelItem ) {
  *
  * Every fired event is passed (as first parameter) an object with `output` property. Every event may set and/or
  * modify that property. When all callbacks are done, the final value of `output` property is returned by this method.
+ * The `output` must be either {@link module:engine/model/node~Node model node} or
+ * {@link module:engine/model/documentfragment~DocumentFragment model document fragment} or `null` (as set by default).
  *
  * @method #convertItem
  * @fires module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element
@@ -314,7 +344,8 @@ function extractMarkersFromModelFragment( modelItem ) {
  * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume.
  * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher`
  * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}.
- * @returns {*} The result of item conversion, created and modified by callbacks attached to fired event.
+ * @returns {module:engine/model/node~Node|module:engine/model/documentfragment~DocumentFragment|null} The result of item conversion,
+ * created and modified by callbacks attached to fired event, or `null` if the conversion result was incorrect.
  */
 
 /**
@@ -329,5 +360,6 @@ function extractMarkersFromModelFragment( modelItem ) {
  * @param {module:engine/conversion/viewconsumable~ViewConsumable} consumable Values to consume.
  * @param {Object} [additionalData] Additional data to be passed in `data` argument when firing `ViewConversionDispatcher`
  * events. See also {@link module:engine/conversion/viewconversiondispatcher~ViewConversionDispatcher#event:element element event}.
- * @returns {Array.<*>} Array containing results of conversion of all children of given item.
+ * @returns {module:engine/model/documentfragment~DocumentFragment} Model document fragment containing results of conversion
+ * of all children of given item.
  */

tests/conversion/advanced-converters.js

@@ -519,7 +519,7 @@ describe( 'advanced-converters', () => {
 		it( 'should convert a view element to model', () => {
 			let viewElement = new ViewAttributeElement( 'a', { href: 'foo.html', title: 'Foo title' }, new ViewText( 'foo' ) );
 
-			let modelText = viewDispatcher.convert( viewElement )[ 0 ];
+			let modelText = viewDispatcher.convert( viewElement ).getChild( 0 );
 
 			expect( modelText ).to.be.instanceof( ModelText );
 			expect( modelText.data ).to.equal( 'foo' );
@@ -603,11 +603,14 @@ describe( 'advanced-converters', () => {
 		viewDispatcher.on( 'element:tr', ( evt, data, consumable, conversionApi ) => {
 			if ( consumable.consume( data.input, { name: true } ) ) {
 				data.output = new ModelElement( 'paragraph' );
+
 				const children = conversionApi.convertChildren( data.input, consumable );
 
-				for ( let i = 1; i < children.length; i++ ) {
-					if ( children[ i ] instanceof ModelText && children[ i - 1 ] instanceof ModelText ) {
-						children.splice( i, 0, new ModelText( ' ' ) );
+				for ( let i = 1; i < children.childCount; i++ ) {
+					const child = children.getChild( i );
+
+					if ( child instanceof ModelText && child.previousSibling instanceof ModelText ) {
+						children.insertChildren( i, new ModelText( ' ' ) );
 						i++;
 					}
 				}

tests/conversion/buildviewconverter.js

@@ -6,6 +6,7 @@
 import buildViewConverter from '../../src/conversion/buildviewconverter';
 
 import ModelSchema from '../../src/model/schema';
+import ModelDocumentFragment from '../../src/model/documentfragment';
 import ModelDocument from '../../src/model/document';
 import ModelElement from '../../src/model/element';
 import ModelTextProxy from '../../src/model/textproxy';
@@ -281,7 +282,10 @@ describe( 'View converter builder', () => {
 
 		const element = new ViewAttributeElement( 'span' );
 
-		expect( dispatcher.convert( element, objWithContext ) ).to.null;
+		const result = dispatcher.convert( element, objWithContext );
+
+		expect( result ).to.be.instanceof( ModelDocumentFragment );
+		expect( result.childCount ).to.equal( 0 );
 	} );
 
 	it( 'should throw an error when view element in not valid to convert to marker', () => {
@@ -392,6 +396,16 @@ describe( 'View converter builder', () => {
 		expect( modelToString( conversionResult ) ).to.equal( '<span transformer="megatron">foo</span>' );
 	} );
 
+	it( 'should return model document fragment when converting attributes on text', () => {
+		buildViewConverter().for( dispatcher ).fromElement( 'strong' ).toAttribute( 'bold', true );
+
+		let viewElement = new ViewAttributeElement( 'strong', null, new ViewText( 'foo' ) );
+
+		let conversionResult = dispatcher.convert( viewElement, objWithContext );
+
+		expect( conversionResult.is( 'documentFragment' ) ).to.be.true;
+	} );
+
 	it( 'should set different priorities for `toElement` and `toAttribute` conversion', () => {
 		buildViewConverter().for( dispatcher )
 			.fromAttribute( 'class' )
@@ -527,7 +541,9 @@ describe( 'View converter builder', () => {
 
 		viewElement.setAttribute( 'stop', true );
 		conversionResult = dispatcher.convert( viewElement, objWithContext );
-		expect( conversionResult ).to.be.null;
+
+		expect( conversionResult ).to.be.instanceof( ModelDocumentFragment );
+		expect( conversionResult.childCount ).to.equal( 0 );
 	} );
 
 	it( 'should stop to attribute conversion if creating function returned null', () => {

tests/conversion/view-to-model-converters.js

@@ -66,11 +66,13 @@ describe( 'view-to-model-converters', () => {
 
 			let conversionResult = dispatcher.convert( viewText, objWithContext );
 
-			expect( conversionResult ).to.be.null;
+			expect( conversionResult ).to.be.instanceof( ModelDocumentFragment );
+			expect( conversionResult.childCount ).to.equal( 0 );
 
 			conversionResult = dispatcher.convert( viewText, { context: [ '$block' ] } );
 
 			expect( conversionResult ).to.be.instanceof( ModelDocumentFragment );
+			expect( conversionResult.childCount ).to.equal( 1 );
 			expect( conversionResult.getChild( 0 ) ).to.be.instanceof( ModelText );
 			expect( conversionResult.getChild( 0 ).data ).to.equal( 'foobar' );
 		} );