Merge pull request #10814 from ckeditor/ck/10279-ghs-figure-duplicates

Fix (image, media-embed, table): The `<figure>` element should not get replicated while GHS is enabled. Closes #10279. Tests (html-support): Added tests for double converted `<figure>` elements. See #10279.
ckeditor · Nov 12, 2021 · 634d424 · 634d424
2 parents fe11106 + 81e5153
commit 634d424
Show file tree

Hide file tree

Showing 9 changed files with 134 additions and 11 deletions.
diff --git a/packages/ckeditor5-html-support/tests/integrations/image.js b/packages/ckeditor5-html-support/tests/integrations/image.js
@@ -266,6 +266,24 @@ describe( 'ImageElementSupport', () => {
 			expect( editor.getData() ).to.equal( expectedHtml );
 		} );
 
+		it( 'should not double convert figure element', () => {
+			dataFilter.loadAllowedConfig( [ {
+				name: /^.*$/,
+				styles: true,
+				attributes: true,
+				classes: true
+			} ] );
+
+			const expectedHtml =
+				'<figure class="image">' +
+					'<img src="/assets/sample.png">' +
+				'</figure>';
+
+			editor.setData( expectedHtml );
+
+			expect( editor.getData() ).to.equal( expectedHtml );
+		} );
+
 		it( 'should not consume attributes already consumed (downcast)', () => {
 			[
 				'htmlAttributes',

diff --git a/packages/ckeditor5-html-support/tests/integrations/mediaembed.js b/packages/ckeditor5-html-support/tests/integrations/mediaembed.js
@@ -291,6 +291,24 @@ describe( 'MediaEmbedElementSupport', () => {
 			expect( editor.getData() ).to.equal( expectedHtml );
 		} );
 
+		it( 'should not double convert figure element', () => {
+			dataFilter.loadAllowedConfig( [ {
+				name: /^.*$/,
+				styles: true,
+				attributes: true,
+				classes: true
+			} ] );
+
+			const expectedHtml =
+				'<figure class="media">' +
+					'<oembed url="https://www.youtube.com/watch?v=ZVv7UMQPEWk"></oembed>' +
+				'</figure>';
+
+			editor.setData( expectedHtml );
+
+			expect( editor.getData() ).to.equal( expectedHtml );
+		} );
+
 		it( 'should not consume media figure element that is already consumed (upcast)', () => {
 			editor.conversion.for( 'upcast' )
 				.add( dispatcher => {

diff --git a/packages/ckeditor5-html-support/tests/integrations/table.js b/packages/ckeditor5-html-support/tests/integrations/table.js
@@ -790,6 +790,30 @@ describe( 'TableElementSupport', () => {
 		expect( editor.getData() ).to.equal( expectedHtml );
 	} );
 
+	it( 'should not double convert figure element', () => {
+		dataFilter.loadAllowedConfig( [ {
+			name: /^.*$/,
+			styles: true,
+			attributes: true,
+			classes: true
+		} ] );
+
+		const expectedHtml =
+			'<figure class="table">' +
+				'<table>' +
+					'<tbody>' +
+						'<tr>' +
+							'<td>foo</td>' +
+						'</tr>' +
+					'</tbody>' +
+				'</table>' +
+			'</figure>';
+
+		editor.setData( expectedHtml );
+
+		expect( editor.getData() ).to.equal( expectedHtml );
+	} );
+
 	it( 'should not consume attributes already consumed (downcast)', () => {
 		[
 			'htmlAttributes',

diff --git a/packages/ckeditor5-image/src/image/converters.js b/packages/ckeditor5-image/src/image/converters.js
@@ -44,6 +44,9 @@ export function upcastImageFigure( imageUtils ) {
 			return;
 		}
 
+		// Consume the figure to prevent other converters from processing it again.
+		conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'image' } );
+
 		// Convert view image to model image.
 		const conversionResult = conversionApi.convertItem( viewImage, data.modelCursor );
 
@@ -52,12 +55,12 @@ export function upcastImageFigure( imageUtils ) {
 
 		// When image wasn't successfully converted then finish conversion.
 		if ( !modelImage ) {
+			// Revert consumed figure so other features can convert it.
+			conversionApi.consumable.revert( data.viewItem, { name: true, classes: 'image' } );
+
 			return;
 		}
 
-		// Consume the figure to prevent other converters from processing it again.
-		conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'image' } );
-
 		// Convert rest of the figure element's children as an image children.
 		conversionApi.convertChildren( data.viewItem, modelImage );
 

diff --git a/packages/ckeditor5-image/tests/image/converters.js b/packages/ckeditor5-image/tests/image/converters.js
@@ -225,13 +225,34 @@ describe( 'Image converters', () => {
 			expectModel( '' );
 		} );
 
-		it( 'should not left unconverted figure media element', () => {
+		it( 'should not consume if the img element was not converted', () => {
+			editor.data.upcastDispatcher.on( 'element:img', ( evt, data, conversionApi ) => {
+				conversionApi.consumable.consume( data.viewItem, { name: true } );
+				data.modelRange = conversionApi.writer.createRange( data.modelCursor );
+			}, { priority: 'high' } );
+
+			editor.data.upcastDispatcher.on( 'element:figure', ( evt, data, conversionApi ) => {
+				expect( conversionApi.consumable.test( data.viewItem, { name: true, classes: 'image' } ) ).to.be.true;
+			}, { priority: 'low' } );
+
+			editor.setData( '<figure class="image"><img src="/assets/sample.png" /></figure>' );
+		} );
+
+		it( 'should not left unconsumed figure media element', () => {
 			editor.data.upcastDispatcher.on( 'element:figure', ( evt, data, conversionApi ) => {
 				expect( conversionApi.consumable.test( data.viewItem, { name: true, classes: 'image' } ) ).to.be.false;
 			}, { priority: 'low' } );
 
 			editor.setData( '<figure class="image"><img src="/assets/sample.png" /></figure>' );
 		} );
+
+		it( 'should consume the figure element before the img conversion starts', () => {
+			editor.data.upcastDispatcher.on( 'element:img', ( evt, data, conversionApi ) => {
+				expect( conversionApi.consumable.test( data.viewItem.parent, { name: true, classes: 'image' } ) ).to.be.false;
+			}, { priority: 'low' } );
+
+			editor.setData( '<figure class="image"><img src="/assets/sample.png" /></figure>' );
+		} );
 	} );
 
 	describe( 'downcastImageAttribute', () => {

diff --git a/packages/ckeditor5-media-embed/src/mediaembedediting.js b/packages/ckeditor5-media-embed/src/mediaembedediting.js
@@ -260,7 +260,7 @@ export default class MediaEmbedEditing extends Plugin {
 				dispatcher.on( 'element:figure', converter );
 
 				function converter( evt, data, conversionApi ) {
-					if ( !conversionApi.consumable.test( data.viewItem, { name: true, classes: 'media' } ) ) {
+					if ( !conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'media' } ) ) {
 						return;
 					}
 
@@ -272,10 +272,9 @@ export default class MediaEmbedEditing extends Plugin {
 					const modelElement = first( modelRange.getItems() );
 
 					if ( !modelElement ) {
-						return;
+						// Revert consumed figure so other features can convert it.
+						conversionApi.consumable.revert( data.viewItem, { name: true, classes: 'media' } );
 					}
-
-					conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'media' } );
 				}
 			} );
 	}

diff --git a/packages/ckeditor5-media-embed/tests/mediaembedediting.js b/packages/ckeditor5-media-embed/tests/mediaembedediting.js
@@ -601,6 +601,30 @@ describe( 'MediaEmbedEditing', () => {
 							.to.equal( '' );
 					} );
 
+					it( 'should not consume if the media element was not converted', () => {
+						editor.data.upcastDispatcher.on( 'element:o-embed', ( evt, data, conversionApi ) => {
+							conversionApi.consumable.consume( data.viewItem, { name: true } );
+							data.modelRange = conversionApi.writer.createRange( data.modelCursor );
+						}, { priority: 'highest' } );
+
+						editor.data.upcastDispatcher.on( 'element:figure', ( evt, data, conversionApi ) => {
+							expect( conversionApi.consumable.test( data.viewItem, { name: true, classes: 'media' } ) ).to.be.true;
+						}, { priority: 'low' } );
+
+						editor.setData( '<figure class="media"><o-embed url="https://ckeditor.com"></o-embed></figure>' );
+
+						expect( getModelData( model, { withoutSelection: true } ) )
+							.to.equal( '' );
+					} );
+
+					it( 'should consume the figure element before the o-embed conversion starts', () => {
+						editor.data.upcastDispatcher.on( 'element:o-embed', ( evt, data, conversionApi ) => {
+							expect( conversionApi.consumable.test( data.viewItem.parent, { name: true, classes: 'media' } ) ).to.be.false;
+						}, { priority: 'low' } );
+
+						editor.setData( '<figure class="media"><o-embed url="https://ckeditor.com"></o-embed></figure>' );
+					} );
+
 					it( 'should not convert if the figure is already consumed', () => {
 						editor.data.upcastDispatcher.on( 'element:figure', ( evt, data, conversionApi ) => {
 							conversionApi.consumable.consume( data.viewItem, { name: true, class: 'media' } );

diff --git a/packages/ckeditor5-table/src/converters/upcasttable.js b/packages/ckeditor5-table/src/converters/upcasttable.js
@@ -37,6 +37,9 @@ export function upcastTableFigure() {
 				return;
 			}
 
+			// Consume the figure to prevent other converters from processing it again.
+			conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'table' } );
+
 			// Convert view table to model table.
 			const conversionResult = conversionApi.convertItem( viewTable, data.modelCursor );
 
@@ -45,11 +48,12 @@ export function upcastTableFigure() {
 
 			// When table wasn't successfully converted then finish conversion.
 			if ( !modelTable ) {
+				// Revert consumed figure so other features can convert it.
+				conversionApi.consumable.revert( data.viewItem, { name: true, classes: 'table' } );
+
 				return;
 			}
 
-			conversionApi.consumable.consume( data.viewItem, { name: true, classes: 'table' } );
-
 			conversionApi.convertChildren( data.viewItem, conversionApi.writer.createPositionAt( modelTable, 'end' ) );
 			conversionApi.updateConversionResult( modelTable, data );
 		} );

diff --git a/packages/ckeditor5-table/tests/converters/upcasttable.js b/packages/ckeditor5-table/tests/converters/upcasttable.js
@@ -81,15 +81,27 @@ describe( 'upcastTable()', () => {
 		editor.conversion.for( 'upcast' ).add( dispatcher => {
 			dispatcher.on( 'element:table', ( evt, data, conversionApi ) => {
 				conversionApi.consumable.consume( data.viewItem, { name: true } );
-
 				data.modelRange = conversionApi.writer.createRange( data.modelCursor );
 			}, { priority: 'highest' } );
+
+			dispatcher.on( 'element:figure', ( evt, data, conversionApi ) => {
+				expect( conversionApi.consumable.test( data.viewItem, { name: true, classes: 'table' } ) ).to.be.true;
+			}, { priority: 'low' } );
 		} );
+
 		editor.setData( '<figure class="table"><table>xyz</table></figure>' );
 
 		expectModel( '<paragraph>xyz</paragraph>' );
 	} );
 
+	it( 'should consume the figure element before the table conversion starts', () => {
+		editor.data.upcastDispatcher.on( 'element:table', ( evt, data, conversionApi ) => {
+			expect( conversionApi.consumable.test( data.viewItem.parent, { name: true, classes: 'table' } ) ).to.be.false;
+		}, { priority: 'low' } );
+
+		editor.setData( '<figure class="table"><table>xyz</table></figure>' );
+	} );
+
 	it( 'should convert if figure do not have class="table" attribute', () => {
 		editor.setData(
 			'<figure>' +