Change marker-to-data logic to improve conversion inside document fragment

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

@@ -405,9 +405,8 @@ export default class DowncastHelpers extends ConversionHelpers {
 	 *
 	 * This conversion creates a representation for model marker boundaries in the view:
 	 *
-	 * * If the marker boundary is at a position where text nodes are allowed, then a view element with the specified tag name
-	 * and `name` attribute is added at this position.
-	 * * In other cases, a specified attribute is set on a view element that is before or after the marker boundary.
+	 * * If the marker boundary is before or after a model element, a view attribute is set on a corresponding view element.
+	 * * In other cases, a view element with the specified tag name is inserted at corresponding view position.
 	 *
 	 * Typically, marker names use the `group:uniqueId:otherData` convention. For example: `comment:e34zfk9k2n459df53sjl34:zx32c`.
 	 * The default configuration for this conversion is that the first part is the `group` part and the rest of
@@ -945,32 +944,33 @@ function insertMarkerData( viewCreator ) {
 // Helper function for `insertMarkerData()` that marks a marker boundary at the beginning or end of given `range`.
 function handleMarkerBoundary( range, isStart, conversionApi, data, viewMarkerData ) {
 	const modelPosition = isStart ? range.start : range.end;
-	const canInsertElement = conversionApi.schema.checkChild( modelPosition, '$text' );
+	const elementAfter = modelPosition.nodeAfter && modelPosition.nodeAfter.is( 'element' ) ? modelPosition.nodeAfter : null;
+	const elementBefore = modelPosition.nodeBefore && modelPosition.nodeBefore.is( 'element' ) ? modelPosition.nodeBefore : null;
 
-	if ( canInsertElement ) {
-		const viewPosition = conversionApi.mapper.toViewPosition( modelPosition );
-
-		insertMarkerAsElement( viewPosition, isStart, conversionApi, data, viewMarkerData );
-	} else {
+	if ( elementAfter || elementBefore ) {
 		let modelElement;
 		let isBefore;
 
 		// If possible, we want to add `data-group-start-before` and `data-group-end-after` attributes.
-		// Below `if` is constructed in a way that will favor adding these attributes.
-		//
-		// Also, I assume that there will be always an element either after or before the position.
-		// If not, then it is a case when we are not in a position where text is allowed and also there are no elements around...
-		if ( isStart && modelPosition.nodeAfter || !isStart && !modelPosition.nodeBefore ) {
-			modelElement = modelPosition.nodeAfter;
+		if ( isStart && elementAfter || !isStart && !elementBefore ) {
+			// [<elementAfter>...</elementAfter> -> <elementAfter data-group-start-before="...">...</elementAfter>
+			// <parent>]<elementAfter> -> <parent><elementAfter data-group-end-before="...">
+			modelElement = elementAfter;
 			isBefore = true;
 		} else {
-			modelElement = modelPosition.nodeBefore;
+			// <elementBefore>...</elementBefore>] -> <elementBefore data-group-end-after="...">...</elementBefore>
+			// </elementBefore>[</parent> -> </elementBefore data-group-start-after="..."></parent>
+			modelElement = elementBefore;
 			isBefore = false;
 		}
 
 		const viewElement = conversionApi.mapper.toViewElement( modelElement );
 
 		insertMarkerAsAttribute( viewElement, isStart, isBefore, conversionApi, data, viewMarkerData );
+	} else {
+		const viewPosition = conversionApi.mapper.toViewPosition( modelPosition );
+
+		insertMarkerAsElement( viewPosition, isStart, conversionApi, data, viewMarkerData );
 	}
 }
 

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

@@ -382,10 +382,9 @@ function formatReturnValue( type, item, previousPosition, nextPosition, length )
 /**
  * Type of the step made by {@link module:engine/model/treewalker~TreeWalker}.
  * Possible values: `'elementStart'` if walker is at the beginning of a node, `'elementEnd'` if walker is at the end of node,
- * `'character'` if walker traversed over a character, or `'text'` if walker traversed over multiple characters (available in
- * character merging mode, see {@link module:engine/model/treewalker~TreeWalker#constructor}).
+ * or `'text'` if walker traversed over text.
  *
- * @typedef {'elementStart'|'elementEnd'|'character'|'text'} module:engine/model/treewalker~TreeWalkerValueType
+ * @typedef {'elementStart'|'elementEnd'|'text'} module:engine/model/treewalker~TreeWalkerValueType
  */
 
 /**
@@ -404,7 +403,7 @@ function formatReturnValue( type, item, previousPosition, nextPosition, length )
  * the position after the item.
  * * Backward iteration: For `'elementEnd'` it is last position inside element. For all other types it is the position
  * before the item.
- * @property {Number} [length] Length of the item. For `'elementStart'` and `'character'` it is 1. For `'text'` it is
+ * @property {Number} [length] Length of the item. For `'elementStart'` it is 1. For `'text'` it is
  * the length of the text. For `'elementEnd'` it is `undefined`.
  */
 

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

@@ -6,6 +6,7 @@
 /* globals console */
 
 import EditingController from '../../src/controller/editingcontroller';
+import DataController from '../../src/controller/datacontroller';
 
 import Model from '../../src/model/model';
 import ModelElement from '../../src/model/element';
@@ -2468,6 +2469,59 @@ describe( 'DowncastHelpers', () => {
 			expectResult( '<p>Foo</p>' );
 		} );
 
+		it( 'default conversion, document fragment, text', () => {
+			const dataController = new DataController( model, new StylesProcessor() );
+			downcastHelpers = new DowncastHelpers( [ dataController.downcastDispatcher ] );
+
+			downcastHelpers.markerToData( { model: 'group' } );
+
+			let modelDocumentFragment;
+
+			model.change( writer => {
+				modelDocumentFragment = writer.createDocumentFragment();
+
+				writer.insertText( 'foobar', [], modelDocumentFragment, 0 );
+
+				const range = writer.createRange(
+					writer.createPositionFromPath( modelDocumentFragment, [ 2 ] ),
+					writer.createPositionFromPath( modelDocumentFragment, [ 5 ] )
+				);
+
+				modelDocumentFragment.markers.set( 'group:foo:bar', range );
+			} );
+
+			const expectedResult = 'fo<group-start name="foo:bar"></group-start>oba<group-end name="foo:bar"></group-end>r';
+
+			expect( dataController.stringify( modelDocumentFragment ) ).to.equal( expectedResult );
+		} );
+
+		it( 'default conversion, document fragment, element', () => {
+			const dataController = new DataController( model, new StylesProcessor() );
+			downcastHelpers = new DowncastHelpers( [ dataController.downcastDispatcher ] );
+
+			downcastHelpers.elementToElement( { model: 'paragraph', view: 'p' } );
+			downcastHelpers.markerToData( { model: 'group' } );
+
+			let modelDocumentFragment;
+
+			model.change( writer => {
+				modelDocumentFragment = writer.createDocumentFragment();
+
+				writer.insertElement( 'paragraph', [], modelDocumentFragment, 0 );
+
+				const range = writer.createRange(
+					writer.createPositionFromPath( modelDocumentFragment, [ 0 ] ),
+					writer.createPositionFromPath( modelDocumentFragment, [ 1 ] )
+				);
+
+				modelDocumentFragment.markers.set( 'group:foo:bar', range );
+			} );
+
+			const expectedResult = '<p data-group-end-after="foo:bar" data-group-start-before="foo:bar">&nbsp;</p>';
+
+			expect( dataController.stringify( modelDocumentFragment ) ).to.equal( expectedResult );
+		} );
+
 		it( 'conversion callback, mixed, multiple markers, name', () => {
 			const customData = {
 				foo: 'bar',