Преглед изворни кода

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.
Maciej пре 5 година
родитељ
комит
5e857fd0ec

+ 7 - 0
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' } );
 	}
 
 	/**

+ 22 - 11
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.

+ 24 - 10
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;
 	};
 }
 

+ 4 - 0
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' } );

+ 77 - 0
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 );
+}

+ 2 - 2
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' );

+ 37 - 0
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();
 

+ 26 - 1
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, 'நிலைக்கு' );
 

+ 8 - 3
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>' );
 			} );
 		} );
 

+ 2 - 1
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', () => {

+ 3 - 87
packages/ckeditor5-paragraph/src/paragraph.js

@@ -40,7 +40,6 @@ export default class Paragraph extends Plugin {
 	init() {
 		const editor = this.editor;
 		const model = editor.model;
-		const data = editor.data;
 
 		editor.commands.add( 'paragraph', new ParagraphCommand( editor ) );
 		editor.commands.add( 'insertParagraph', new InsertParagraphCommand( editor ) );
@@ -50,10 +49,7 @@ export default class Paragraph extends Plugin {
 
 		editor.conversion.elementToElement( { model: 'paragraph', view: 'p' } );
 
-		// Content autoparagraphing. --------------------------------------------------
-
-		// Handles element which has not been converted by any plugin and checks if it would be converted if
-		// we wrap it in a paragraph or change it to a paragraph.
+		// Conversion for paragraph-like elements which has not been converted by any plugin.
 		editor.conversion.for( 'upcast' ).elementToElement( {
 			model: ( viewElement, { writer } ) => {
 				if ( !Paragraph.paragraphLikeElements.has( viewElement.name ) ) {
@@ -70,64 +66,6 @@ export default class Paragraph extends Plugin {
 			view: /.+/,
 			converterPriority: 'low'
 		} );
-
-		data.upcastDispatcher.on( 'element', ( evt, data, conversionApi ) => {
-			// Do not try auto-paragraphing if the element was already converted.
-			if ( !conversionApi.consumable.test( data.viewItem, { name: data.viewItem.name } ) ) {
-				return;
-			}
-
-			// If the element is not paragraph-like try wrapping it in a paragraph.
-			if ( isParagraphable( data.viewItem, data.modelCursor, conversionApi.schema ) ) {
-				Object.assign( data, wrapInParagraph( data.viewItem, data.modelCursor, conversionApi ) );
-			}
-		}, { priority: 'low' } );
-
-		// Handles not converted text nodes and checks if would be converted if we wraps then by a paragraph.
-		data.upcastDispatcher.on( 'text', ( evt, data, conversionApi ) => {
-			// When node is already converted then do nothing.
-			if ( data.modelRange ) {
-				return;
-			}
-
-			if ( isParagraphable( data.viewItem, data.modelCursor, conversionApi.schema ) ) {
-				Object.assign( data, wrapInParagraph( data.viewItem, data.modelCursor, conversionApi ) );
-			}
-		}, { priority: 'lowest' } );
-
-		// Empty roots autoparagraphing. -----------------------------------------------
-
-		// Post-fixer which takes care of adding empty paragraph elements to empty roots.
-		// Besides fixing content on #changesDone we also need to handle editor.data#ready event because
-		// if initial data is empty or setData() wasn't even called there will be no #change fired.
-		model.document.registerPostFixer( writer => this._autoparagraphEmptyRoots( writer ) );
-
-		editor.data.on( 'ready', () => {
-			model.enqueueChange( 'transparent', writer => this._autoparagraphEmptyRoots( writer ) );
-		}, { priority: 'lowest' } );
-	}
-
-	/**
-	 * Fixes all empty roots.
-	 *
-	 * @private
-	 * @returns {Boolean} `true` if any change has been applied, `false` otherwise.
-	 */
-	_autoparagraphEmptyRoots( writer ) {
-		const model = this.editor.model;
-
-		for ( const rootName of model.document.getRootNames() ) {
-			const root = model.document.getRoot( rootName );
-
-			if ( root.isEmpty && root.rootName != '$graveyard' ) {
-				// If paragraph element is allowed in the root, create paragraph element.
-				if ( model.schema.checkChild( root, 'paragraph' ) ) {
-					writer.insertElement( 'paragraph', root );
-
-					return true;
-				}
-			}
-		}
 	}
 }
 
@@ -175,28 +113,6 @@ Paragraph.paragraphLikeElements = new Set( [
 	'h6',
 	'li',
 	'p',
-	'td'
+	'td',
+	'th'
 ] );
-
-function wrapInParagraph( input, position, conversionApi ) {
-	const paragraph = conversionApi.writer.createElement( 'paragraph' );
-
-	conversionApi.writer.insert( paragraph, position );
-	return conversionApi.convertItem( input, conversionApi.writer.createPositionAt( paragraph, 0 ) );
-}
-
-function isParagraphable( node, position, 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' ), node ) ) {
-		return false;
-	}
-
-	return true;
-}

+ 18 - 5
packages/ckeditor5-paragraph/tests/paragraph.js

@@ -76,18 +76,31 @@ describe( 'Paragraph feature', () => {
 			} );
 
 			it( 'should autoparagraph any inline element', () => {
-				editor.model.schema.register( 'span', { allowWhere: '$text' } );
-				editor.model.schema.extend( '$text', { allowIn: 'span' } );
+				editor.model.schema.register( 'inline', { allowWhere: '$text' } );
+				editor.model.schema.extend( '$text', { allowIn: 'inline' } );
 
-				editor.conversion.for( 'downcast' ).elementToElement( { model: 'span', view: 'span' } );
-				editor.conversion.for( 'upcast' ).elementToElement( { model: 'span', view: 'span' } );
+				editor.conversion.for( 'downcast' ).elementToElement( { model: 'inline', view: 'span' } );
+				editor.conversion.for( 'upcast' ).elementToElement( { model: 'inline', view: 'span' } );
 
 				editor.setData( '<span>foo</span>' );
 
-				expect( getModelData( model, { withoutSelection: true } ) ).to.equal( '<paragraph><span>foo</span></paragraph>' );
+				expect( getModelData( model, { withoutSelection: true } ) ).to.equal( '<paragraph><inline>foo</inline></paragraph>' );
 				expect( editor.getData() ).to.equal( '<p><span>foo</span></p>' );
 			} );
 
+			it( 'should autoparagraph any inline element with children', () => {
+				editor.model.schema.register( 'inline', { allowWhere: '$text' } );
+				editor.model.schema.extend( '$text', { allowIn: 'inline' } );
+
+				editor.conversion.for( 'downcast' ).elementToElement( { model: 'inline', view: 'span' } );
+				editor.conversion.for( 'upcast' ).elementToElement( { model: 'inline', view: 'span' } );
+
+				editor.setData( '<span>f<b>123</b>oo</span>' );
+
+				expect( getModelData( model, { withoutSelection: true } ) ).to.equal( '<paragraph><inline>f123oo</inline></paragraph>' );
+				expect( editor.getData() ).to.equal( '<p><span>f123oo</span></p>' );
+			} );
+
 			it( 'should not autoparagraph text (in clipboard holder)', () => {
 				const modelFragment = editor.data.parse( 'foo', [ '$clipboardHolder' ] );
 

+ 42 - 0
packages/ckeditor5-table/tests/converters/upcasttable.js

@@ -500,6 +500,48 @@ describe( 'upcastTable()', () => {
 		} );
 	} );
 
+	describe( 'inline contents', () => {
+		it( 'should upcast inline element inside a table cell', () => {
+			model.schema.register( 'inline', { allowWhere: '$text', isInline: true } );
+			model.schema.extend( '$text', { allowIn: 'inline' } );
+			editor.conversion.elementToElement( { model: 'inline', view: 'span' } );
+
+			editor.setData(
+				'<table>' +
+					'<tr>' +
+						'<td>' +
+							'<span>foo</span>' +
+						'</td>' +
+					'</tr>' +
+				'</table>'
+			);
+
+			expectModel( modelTable( [
+				[ '<paragraph><inline>foo</inline></paragraph>' ]
+			] ) );
+		} );
+
+		it( 'should upcast inline object inside a table cell', () => {
+			model.schema.register( 'inline', { allowWhere: '$text', isInline: true, isObject: true } );
+			model.schema.extend( '$text', { allowIn: 'inline' } );
+			editor.conversion.elementToElement( { model: 'inline', view: 'span' } );
+
+			editor.setData(
+				'<table>' +
+					'<tr>' +
+						'<td>' +
+							'<span>foo</span>' +
+						'</td>' +
+					'</tr>' +
+				'</table>'
+			);
+
+			expectModel( modelTable( [
+				[ '<paragraph><inline>foo</inline></paragraph>' ]
+			] ) );
+		} );
+	} );
+
 	describe( 'handling redundant whitespacing between table elements', () => {
 		it( 'table without thead/tbody/tfoot', () => {
 			editor.setData(