Merge pull request #9780 from ckeditor/ck/9779

Fix (engine): Added checks for the upcast attribute-to-marker converter to prevent converter from doing anything if there are no changes to consume for given view element. Part of the #9779.

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

@@ -653,7 +653,7 @@ function upcastDataToMarker( config ) {
 		// This converter handles both element conversion and attribute conversion, which means that if a single
 		// `config.converterPriority` is used, it will lead to problems. For example, if `'high'` priority is used,
 		// then attribute conversion will be performed before a lot of element upcast converters.
-		// On the other hand we want to support `config.converterPriority` and overwriting conveters.
+		// On the other hand we want to support `config.converterPriority` and overwriting converters.
 		//
 		// To have it work, we need to do some extra processing for priority for attribute converter.
 		// Priority `'low'` value should be the base value and then we will change it depending on `config.converterPriority` value.
@@ -682,12 +682,23 @@ function upcastAttributeToMarker( config ) {
 	return ( evt, data, conversionApi ) => {
 		const attrName = `data-${ config.view }`;
 
+		// Check if any attribute for the given view item can be consumed before changing the conversion data
+		// and consuming view items with these attributes.
+		if (
+			!conversionApi.consumable.test( data.viewItem, { attributes: attrName + '-end-after' } ) &&
+			!conversionApi.consumable.test( data.viewItem, { attributes: attrName + '-start-after' } ) &&
+			!conversionApi.consumable.test( data.viewItem, { attributes: attrName + '-end-before' } ) &&
+			!conversionApi.consumable.test( data.viewItem, { attributes: attrName + '-start-before' } )
+		) {
+			return;
+		}
+
 		// This converter wants to add a model element, marking a marker, before/after an element (or maybe even group of elements).
 		// To do that, we can use `data.modelRange` which is set on an element (or a group of elements) that has been upcasted.
 		// But, if the processed view element has not been upcasted yet (it does not have been converted), we need to
 		// fire conversion for its children first, then we will have `data.modelRange` available.
 		if ( !data.modelRange ) {
-			data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) );
+			Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) );
 		}
 
 		if ( conversionApi.consumable.consume( data.viewItem, { attributes: attrName + '-end-after' } ) ) {
@@ -893,7 +904,7 @@ function prepareToAttributeConverter( config, shallow ) {
 		// If the range is not created yet, let's create it by converting children of the current node first.
 		if ( !data.modelRange ) {
 			// Convert children and set conversion result as a current data.
-			data = Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) );
+			Object.assign( data, conversionApi.convertChildren( data.viewItem, data.modelCursor ) );
 		}
 
 		// Set attribute on current `output`. `Schema` is checked inside this helper function.

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

@@ -1012,7 +1012,8 @@ describe( 'UpcastHelpers', () => {
 					expect( conversionApi.writer ).to.instanceof( Writer );
 
 					return 'group:' + name.split( '_' )[ 0 ];
-				} } );
+				}
+			} );
 
 			expectResult(
 				viewParse(
@@ -1044,11 +1045,31 @@ describe( 'UpcastHelpers', () => {
 			expectResult(
 				viewParse( '<div data-group-end-after="foo" data-group-start-before="foo"><p>Foo</p></div>' ),
 				'<paragraph>Foo</paragraph>',
-				[
-					{ name: 'group:foo', start: [ 0 ], end: [ 1 ] }
-				]
+				{ name: 'group:foo', start: [ 0 ], end: [ 1 ] }
 			);
 		} );
+
+		it( 'should not invoke conversion API when the attributes are not consumable', () => {
+			upcastHelpers.dataToMarker( { view: 'fake' } );
+
+			let conversionConsumeSpy = sinon.spy();
+
+			upcastDispatcher.on( 'element:div', ( evt, data, conversionApi ) => {
+				conversionConsumeSpy = sinon.spy( conversionApi.consumable, 'consume' );
+			} );
+
+			expectResult(
+				viewParse( '<div data-group-end-after="foo" data-group-start-before="foo"><p>Foo</p></div>' ),
+				'<paragraph>Foo</paragraph>',
+				[]
+			);
+
+			for ( const consumeCall of conversionConsumeSpy.getCalls() ) {
+				if ( consumeCall.args[ 1 ] ) {
+					expect( consumeCall.args[ 1 ] ).to.not.have.property( 'attributes' );
+				}
+			}
+		} );
 	} );
 
 	function expectResult( viewToConvert, modelString, markers ) {

packages/ckeditor5-table/tests/table-integration.js

@@ -187,13 +187,41 @@ describe( 'Table feature – integration', () => {
 		} );
 
 		it( 'should not make the Model#hasContent() method return "true" when an empty table cell is selected', () => {
-			setModelData( editor.model, '<table>' +
-				'<tableRow>' +
-					'[<tableCell><paragraph></paragraph></tableCell>]' +
-				'</tableRow>' +
-			'</table>' );
+			setModelData( editor.model, (
+				'<table>' +
+					'<tableRow>' +
+						'[<tableCell><paragraph></paragraph></tableCell>]' +
+					'</tableRow>' +
+				'</table>'
+			) );
 
 			expect( editor.model.hasContent( editor.model.document.selection.getFirstRange() ) ).to.be.false;
 		} );
 	} );
 } );
+
+describe( 'Table feature – integration with markers', () => {
+	let editor;
+
+	afterEach( () => {
+		editor.destroy();
+	} );
+
+	// https://github.com/ckeditor/ckeditor5/pull/9780
+	it( 'should work with the upcast marker to data conversion with table containing an empty cell', async () => {
+		function CustomPlugin( editor ) {
+			// Define the conversion in a plugin as this needs to be loaded before the Table plugin.
+			editor.conversion.for( 'upcast' ).dataToMarker( {
+				view: 'foo'
+			} );
+		}
+
+		editor = await ClassicTestEditor
+			.create( '', { plugins: [ CustomPlugin, Paragraph, TableEditing ] } );
+
+		editor.setData( '<table><tbody><tr><td></td></tr></tbody></table>' );
+
+		expect( getModelData( editor.model, { withoutSelection: true } ) )
+			.to.equal( '<table><tableRow><tableCell><paragraph></paragraph></tableCell></tableRow></table>' );
+	} );
+} );