Merge pull request #7787 from ckeditor/i/6698

Fix (engine): Upcast conversion will now try to wrap text and inline elements in paragraph if such content is converted in a place where is not allowed but a paragraph is allowed. Closes #7753. Closes #6698.

Internal (paragraph): Auto-paragraphing content conversion moved to the engine package.

Tests (table): Added tests for upcasting inline elements inside a table cell.

packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -24,6 +24,7 @@ import ViewDocument from '../view/document';
 import ViewDowncastWriter from '../view/downcastwriter';
 
 import ModelRange from '../model/range';
+import { autoParagraphEmptyRoots } from '../model/utils/autoparagraphing';
 
 /**
  * Controller for the data pipeline. The data pipeline controls how data is retrieved from the document
@@ -140,6 +141,12 @@ export default class DataController {
 		this.on( 'init', () => {
 			this.fire( 'ready' );
 		}, { priority: 'lowest' } );
+
+		// Fix empty roots after DataController is 'ready' (note that init method could be decorated and stopped).
+		// We need to handle this event because initial data could be empty and post-fixer would not get triggered.
+		this.on( 'ready', () => {
+			this.model.enqueueChange( 'transparent', autoParagraphEmptyRoots );
+		}, { priority: 'lowest' } );
 	}
 
 	/**

packages/ckeditor5-engine/src/conversion/upcastdispatcher.js

@@ -11,6 +11,7 @@ import ViewConsumable from './viewconsumable';
 import ModelRange from '../model/range';
 import ModelPosition from '../model/position';
 import { SchemaContext } from '../model/schema';
+import { isParagraphable, wrapInParagraph } from '../model/utils/autoparagraphing';
 
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 import EmitterMixin from '@ckeditor/ckeditor5-utils/src/emittermixin';
@@ -349,22 +350,32 @@ export default class UpcastDispatcher {
 	 * @see module:engine/conversion/upcastdispatcher~UpcastConversionApi#splitToAllowedParent
 	 */
 	_splitToAllowedParent( node, modelCursor ) {
+		const { schema, writer } = this.conversionApi;
+
 		// Try to find allowed parent.
-		const allowedParent = this.conversionApi.schema.findAllowedParent( modelCursor, node );
+		let allowedParent = schema.findAllowedParent( modelCursor, node );
 
-		// When there is no parent that allows to insert node then return `null`.
-		if ( !allowedParent ) {
-			return null;
-		}
+		if ( allowedParent ) {
+			// When current position parent allows to insert node then return this position.
+			if ( allowedParent === modelCursor.parent ) {
+				return { position: modelCursor };
+			}
 
-		// When current position parent allows to insert node then return this position.
-		if ( allowedParent === modelCursor.parent ) {
-			return { position: modelCursor };
+			// When allowed parent is in context tree (it's outside the converted tree).
+			if ( this._modelCursor.parent.getAncestors().includes( allowedParent ) ) {
+				allowedParent = null;
+			}
 		}
 
-		// When allowed parent is in context tree.
-		if ( this._modelCursor.parent.getAncestors().includes( allowedParent ) ) {
-			return null;
+		if ( !allowedParent ) {
+			// Check if the node wrapped with a paragraph would be accepted by the schema.
+			if ( !isParagraphable( modelCursor, node, schema ) ) {
+				return null;
+			}
+
+			return {
+				position: wrapInParagraph( modelCursor, writer )
+			};
 		}
 
 		// Split element to allowed parent.

packages/ckeditor5-engine/src/conversion/upcasthelpers.js

@@ -10,6 +10,7 @@ import { cloneDeep } from 'lodash-es';
 import { attachLinkToDocumentation } from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 import priorities from '@ckeditor/ckeditor5-utils/src/priorities';
+import { isParagraphable, wrapInParagraph } from '../model/utils/autoparagraphing';
 
 /* global console */
 
@@ -464,20 +465,33 @@ export function convertToModelFragment() {
  * @returns {Function} {@link module:engine/view/text~Text View text} converter.
  */
 export function convertText() {
-	return ( evt, data, conversionApi ) => {
-		if ( conversionApi.schema.checkChild( data.modelCursor, '$text' ) ) {
-			if ( conversionApi.consumable.consume( data.viewItem ) ) {
-				const text = conversionApi.writer.createText( data.viewItem.data );
+	return ( evt, data, { schema, consumable, writer } ) => {
+		let position = data.modelCursor;
 
-				conversionApi.writer.insert( text, data.modelCursor );
+		// When node is already converted then do nothing.
+		if ( !consumable.test( data.viewItem ) ) {
+			return;
+		}
 
-				data.modelRange = conversionApi.writer.createRange(
-					data.modelCursor,
-					data.modelCursor.getShiftedBy( text.offsetSize )
-				);
-				data.modelCursor = data.modelRange.end;
+		if ( !schema.checkChild( position, '$text' ) ) {
+			if ( !isParagraphable( position, '$text', schema ) ) {
+				return;
 			}
+
+			position = wrapInParagraph( position, writer );
 		}
+
+		consumable.consume( data.viewItem );
+
+		const text = writer.createText( data.viewItem.data );
+
+		writer.insert( text, position );
+
+		data.modelRange = writer.createRange(
+			position,
+			position.getShiftedBy( text.offsetSize )
+		);
+		data.modelCursor = data.modelRange.end;
 	};
 }
 

packages/ckeditor5-engine/src/model/model.js

@@ -25,6 +25,7 @@ import deleteContent from './utils/deletecontent';
 import modifySelection from './utils/modifyselection';
 import getSelectedContent from './utils/getselectedcontent';
 import { injectSelectionPostFixer } from './utils/selection-post-fixer';
+import { autoParagraphEmptyRoots } from './utils/autoparagraphing';
 import CKEditorError from '@ckeditor/ckeditor5-utils/src/ckeditorerror';
 
 // @if CK_DEBUG_ENGINE // const { dumpTrees } = require( '../dev-utils/utils' );
@@ -122,6 +123,9 @@ export default class Model {
 
 		injectSelectionPostFixer( this );
 
+		// Post-fixer which takes care of adding empty paragraph elements to the empty roots.
+		this.document.registerPostFixer( autoParagraphEmptyRoots );
+
 		// @if CK_DEBUG_ENGINE // this.on( 'applyOperation', () => {
 		// @if CK_DEBUG_ENGINE // 	dumpTrees( this.document, this.document.version );
 		// @if CK_DEBUG_ENGINE // }, { priority: 'lowest' } );

packages/ckeditor5-engine/src/model/utils/autoparagraphing.js

@@ -0,0 +1,77 @@
+/**
+ * @license Copyright (c) 2003-2020, CKSource - Frederico Knabben. All rights reserved.
+ * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
+ */
+
+/**
+ * @module engine/model/utils/autoparagraphing
+ */
+
+/**
+ * Fixes all empty roots.
+ *
+ * @protected
+ * @param {module:engine/model/writer~Writer} writer The model writer.
+ * @returns {Boolean} `true` if any change has been applied, `false` otherwise.
+ */
+export function autoParagraphEmptyRoots( writer ) {
+	const { schema, document } = writer.model;
+
+	for ( const rootName of document.getRootNames() ) {
+		const root = document.getRoot( rootName );
+
+		if ( root.isEmpty && !schema.checkChild( root, '$text' ) ) {
+			// If paragraph element is allowed in the root, create paragraph element.
+			if ( schema.checkChild( root, 'paragraph' ) ) {
+				writer.insertElement( 'paragraph', root );
+
+				// Other roots will get fixed in the next post-fixer round. Those will be triggered
+				// in the same batch no matter if this method was triggered by the post-fixing or not
+				// (the above insertElement call will trigger the post-fixers).
+				return true;
+			}
+		}
+	}
+
+	return false;
+}
+
+/**
+ * Checks if the given node wrapped with a paragraph would be accepted by the schema in the given position.
+ *
+ * @protected
+ * @param {module:engine/model/position~Position} position The position at which to check.
+ * @param {module:engine/model/node~Node|String} nodeOrType The child node or child type to check.
+ * @param {module:engine/model/schema~Schema} schema A schema instance used for element validation.
+ */
+export function isParagraphable( position, nodeOrType, schema ) {
+	const context = schema.createContext( position );
+
+	// When paragraph is allowed in this context...
+	if ( !schema.checkChild( context, 'paragraph' ) ) {
+		return false;
+	}
+
+	// And a node would be allowed in this paragraph...
+	if ( !schema.checkChild( context.push( 'paragraph' ), nodeOrType ) ) {
+		return false;
+	}
+
+	return true;
+}
+
+/**
+ * Inserts a new paragraph at the given position and returns a position inside that paragraph.
+ *
+ * @protected
+ * @param {module:engine/model/position~Position} position The position where a paragraph should be inserted.
+ * @param {module:engine/model/writer~Writer} writer The model writer.
+ * @returns {module:engine/model/position~Position} Position inside the created paragraph.
+ */
+export function wrapInParagraph( position, writer ) {
+	const paragraph = writer.createElement( 'paragraph' );
+
+	writer.insert( paragraph, position );
+
+	return writer.createPositionAt( paragraph, 0 );
+}

packages/ckeditor5-engine/tests/controller/datacontroller.js

@@ -155,8 +155,8 @@ describe( 'DataController', () => {
 
 			const viewFragment = new ViewDocumentFragment( viewDocument, [ parseView( 'foo' ) ] );
 
-			// Model fragment in root.
-			expect( stringify( data.toModel( viewFragment ) ) ).to.equal( '' );
+			// Model fragment in root (note that it is auto-paragraphed because $text is not allowed directly in $root).
+			expect( stringify( data.toModel( viewFragment ) ) ).to.equal( '<paragraph>foo</paragraph>' );
 
 			// Model fragment in inline root.
 			expect( stringify( data.toModel( viewFragment, [ 'inlineRoot' ] ) ) ).to.equal( 'foo' );

packages/ckeditor5-engine/tests/conversion/upcastdispatcher.js

@@ -605,6 +605,43 @@ describe( 'UpcastDispatcher', () => {
 				sinon.assert.calledOnce( spy );
 			} );
 
+			it(
+				'should auto-paragraph an element if it is not allowed at the insertion position' +
+				'but would be inserted if auto-paragraphed',
+				() => {
+					const spy = sinon.spy();
+
+					model.schema.register( 'section', {
+						allowIn: '$root'
+					} );
+					model.schema.register( 'span', {
+						allowIn: 'paragraph'
+					} );
+					model.schema.extend( 'paragraph', {
+						allowIn: 'section'
+					} );
+
+					dispatcher.on( 'documentFragment', ( evt, data, conversionApi ) => {
+						const section = conversionApi.writer.createElement( 'section' );
+						const position = ModelPosition._createAt( section, 0 );
+
+						const span = conversionApi.writer.createElement( 'span' );
+
+						const result = conversionApi.splitToAllowedParent( span, position );
+
+						expect( section.getChild( 0 ).name ).to.equal( 'paragraph' );
+						expect( result ).to.deep.equal( {
+							position: ModelPosition._createAt( section.getChild( 0 ), 0 )
+						} );
+
+						spy();
+					} );
+
+					model.change( writer => dispatcher.convert( new ViewDocumentFragment( viewDocument ), writer ) );
+					sinon.assert.calledOnce( spy );
+				}
+			);
+
 			it( 'should return null if element is not allowed in position and any of ancestors', () => {
 				const spy = sinon.spy();
 

packages/ckeditor5-engine/tests/conversion/upcasthelpers.js

@@ -968,7 +968,7 @@ describe( 'upcast-converters', () => {
 
 		it( 'should not convert text if it is wrong with schema', () => {
 			schema.addChildCheck( ( ctx, childDef ) => {
-				if ( childDef.name == '$text' && ctx.endsWith( '$root' ) ) {
+				if ( ( childDef.name == '$text' || childDef.name == 'paragraph' ) && ctx.endsWith( '$root' ) ) {
 					return false;
 				}
 			} );
@@ -988,6 +988,31 @@ describe( 'upcast-converters', () => {
 			expect( conversionResult.getChild( 0 ).data ).to.equal( 'foobar' );
 		} );
 
+		it( 'should auto-paragraph a text if it is not allowed at the insertion position but would be inserted if auto-paragraphed', () => {
+			schema.addChildCheck( ( ctx, childDef ) => {
+				if ( childDef.name == '$text' && ctx.endsWith( '$root' ) ) {
+					return false;
+				}
+			} );
+
+			const viewText = new ViewText( viewDocument, 'foobar' );
+			dispatcher.on( 'text', convertText() );
+			let conversionResult = model.change( writer => dispatcher.convert( viewText, writer, context ) );
+
+			expect( conversionResult ).to.be.instanceof( ModelDocumentFragment );
+			expect( conversionResult.childCount ).to.equal( 1 );
+			expect( conversionResult.getChild( 0 ).name ).to.equal( 'paragraph' );
+			expect( conversionResult.getNodeByPath( [ 0, 0 ] ) ).to.be.instanceof( ModelText );
+			expect( conversionResult.getNodeByPath( [ 0, 0 ] ).data ).to.equal( 'foobar' );
+
+			conversionResult = model.change( writer => dispatcher.convert( viewText, writer, [ '$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' );
+		} );
+
 		it( 'should support unicode', () => {
 			const viewText = new ViewText( viewDocument, 'நிலைக்கு' );
 

packages/ckeditor5-engine/tests/model/utils/deletecontent.js

@@ -941,10 +941,15 @@ describe( 'DataController utils', () => {
 					{ rootName: 'bodyRoot' }
 				);
 
-				deleteContent( model, doc.selection, { doNotAutoparagraph: true } );
+				// This must be tested inside a change block to check results before the post-fixers get triggered.
+				model.change( () => {
+					deleteContent( model, doc.selection, { doNotAutoparagraph: true } );
 
-				expect( getData( model, { rootName: 'bodyRoot' } ) )
-					.to.equal( '[]' );
+					expect( getData( model, { rootName: 'bodyRoot' } ) ).to.equal( '[]' );
+				} );
+
+				// Note that auto-paragraphing post-fixer injected a paragraph into the empty root.
+				expect( getData( model, { rootName: 'bodyRoot' } ) ).to.equal( '<paragraph>[]</paragraph>' );
 			} );
 		} );
 

packages/ckeditor5-engine/tests/model/utils/selection-post-fixer.js

@@ -75,7 +75,8 @@ describe( 'Selection post-fixer', () => {
 		it( 'should not crash if there is no correct position for model selection', () => {
 			setModelData( model, '' );
 
-			expect( getModelData( model ) ).to.equal( '[]' );
+			// Note that auto-paragraphing post-fixer injected a paragraph into the empty root.
+			expect( getModelData( model ) ).to.equal( '<paragraph>[]</paragraph>' );
 		} );
 
 		it( 'should react to structure changes', () => {