Merge pull request #14697 from ckeditor/ck/12831-link-on-image-lost-w…

…hen-ghs-enabled Fix (html-support): Link on image should not be lost when loading content with `LinkImage` and full GHS enabled. Closes #12831.
ckeditor · Aug 8, 2023 · 3d15516 · 3d15516
1 parent 453c868
commit 3d15516
Show file tree

Hide file tree

Showing 2 changed files with 116 additions and 24 deletions.
diff --git a/packages/ckeditor5-html-support/src/integrations/image.ts b/packages/ckeditor5-html-support/src/integrations/image.ts
@@ -7,14 +7,15 @@
  * @module html-support/integrations/image
  */
 
-import { Plugin } from 'ckeditor5/src/core';
+import { type Editor, Plugin } from 'ckeditor5/src/core';
 import type {
 	DowncastAttributeEvent,
 	DowncastDispatcher,
 	Element,
-	UpcastDispatcher,
-	ViewElement
+	Node,
+	UpcastDispatcher
 } from 'ckeditor5/src/engine';
+import type { ImageUtils } from '@ckeditor/ckeditor5-image';
 
 import DataFilter, { type DataFilterRegisterEvent } from '../datafilter';
 import { type GHSViewAttributes, setViewAttributes, updateViewAttributes } from '../utils';
@@ -87,6 +88,10 @@ export default class ImageElementSupport extends Plugin {
 			conversion.for( 'upcast' ).add( viewToModelImageAttributeConverter( dataFilter ) );
 			conversion.for( 'downcast' ).add( modelToViewImageAttributeConverter() );
 
+			if ( editor.plugins.has( 'LinkImage' ) ) {
+				conversion.for( 'upcast' ).add( viewToModelLinkImageAttributeConverter( dataFilter, editor ) );
+			}
+
 			evt.stop();
 		} );
 	}
@@ -106,26 +111,44 @@ function viewToModelImageAttributeConverter( dataFilter: DataFilter ) {
 			}
 
 			const viewImageElement = data.viewItem;
-			const viewContainerElement = viewImageElement.parent;
 
-			preserveElementAttributes( viewImageElement, 'htmlImgAttributes' );
+			const viewAttributes = dataFilter.processViewAttributes( viewImageElement, conversionApi );
+
+			if ( viewAttributes ) {
+				conversionApi.writer.setAttribute( 'htmlImgAttributes', viewAttributes, data.modelRange );
+			}
+		}, { priority: 'low' } );
+	};
+}
+
+/**
+ * View-to-model conversion helper preserving allowed attributes on {@link module:image/image~Image Image}
+ * feature model element from link view element.
+ *
+ * @returns Returns a conversion callback.
+ */
+function viewToModelLinkImageAttributeConverter( dataFilter: DataFilter, editor: Editor ) {
+	const imageUtils: ImageUtils = editor.plugins.get( 'ImageUtils' );
+
+	return ( dispatcher: UpcastDispatcher ) => {
+		dispatcher.on( 'element:a', ( evt, data, conversionApi ) => {
+			const viewLink = data.viewItem;
+			const viewImage = imageUtils.findViewImgElement( viewLink );
 
-			if ( viewContainerElement.is( 'element', 'a' ) ) {
-				preserveLinkAttributes( viewContainerElement );
+			if ( !viewImage ) {
+				return;
 			}
 
-			function preserveElementAttributes( viewElement: ViewElement, attributeName: string ) {
-				const viewAttributes = dataFilter.processViewAttributes( viewElement, conversionApi );
+			const modelImage: Node | null = data.modelCursor.parent as Node;
 
-				if ( viewAttributes ) {
-					conversionApi.writer.setAttribute( attributeName, viewAttributes, data.modelRange );
-				}
+			if ( !modelImage.is( 'element', 'imageBlock' ) ) {
+				return;
 			}
 
-			function preserveLinkAttributes( viewContainerElement: ViewElement ) {
-				if ( data.modelRange && data.modelRange.getContainedElement().is( 'element', 'imageBlock' ) ) {
-					preserveElementAttributes( viewContainerElement, 'htmlLinkAttributes' );
-				}
+			const viewAttributes = dataFilter.processViewAttributes( viewLink, conversionApi );
+
+			if ( viewAttributes ) {
+				conversionApi.writer.setAttribute( 'htmlLinkAttributes', viewAttributes, modelImage );
 			}
 		}, { priority: 'low' } );
 	};

diff --git a/packages/ckeditor5-html-support/tests/integrations/image.js b/packages/ckeditor5-html-support/tests/integrations/image.js
@@ -347,6 +347,58 @@ describe( 'ImageElementSupport', () => {
 			expect( marker.getEnd().path ).to.deep.equal( [ 1 ] );
 		} );
 
+		describe( 'BlockImage without LinkImage', () => {
+			let editor, model, editorElement, dataFilter;
+
+			beforeEach( () => {
+				editorElement = document.createElement( 'div' );
+				document.body.appendChild( editorElement );
+
+				return ClassicTestEditor
+					.create( editorElement, {
+						plugins: [ Image, ImageCaption, Paragraph, GeneralHtmlSupport ]
+					} )
+					.then( newEditor => {
+						editor = newEditor;
+						model = editor.model;
+
+						dataFilter = editor.plugins.get( 'DataFilter' );
+					} );
+			} );
+
+			afterEach( () => {
+				editorElement.remove();
+
+				return editor.destroy();
+			} );
+
+			it( 'should not upcast `href` attribute if LinkImage plugin is not available', () => {
+				dataFilter.loadAllowedConfig( [ {
+					name: /.*/,
+					attributes: true
+				} ] );
+
+				editor.setData(
+					'<figure class="image">' +
+						'<a href="www.example.com">' +
+							'<img src="/assets/sample.png">' +
+						'</a>' +
+					'</figure>'
+				);
+
+				expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( {
+					data: '<imageBlock src="/assets/sample.png"></imageBlock>',
+					attributes: {}
+				} );
+
+				expect( editor.getData() ).to.equal(
+					'<figure class="image">' +
+						'<img src="/assets/sample.png">' +
+					'</figure>'
+				);
+			} );
+		} );
+
 		// it( 'should allow modifying styles, classes and attributes', () => {
 		// 	// This should also work when we set `attributes: true` but currently there are some
 		// 	// problems related to GHS picking up non-GHS attributes (like src) due to some attributes not
@@ -882,14 +934,8 @@ describe( 'ImageElementSupport', () => {
 			);
 
 			expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( {
-				data: '<imageBlock htmlLinkAttributes="(1)" src="/assets/sample.png"></imageBlock>',
-				attributes: {
-					1: {
-						attributes: {
-							href: 'www.example.com'
-						}
-					}
-				}
+				data: '<imageBlock linkHref="www.example.com" src="/assets/sample.png"></imageBlock>',
+				attributes: {}
 			} );
 
 			const marker = model.markers.get( 'commented:foo:id' );
@@ -898,6 +944,29 @@ describe( 'ImageElementSupport', () => {
 			expect( marker.getEnd().path ).to.deep.equal( [ 1 ] );
 		} );
 
+		it( 'should upcast `href` attribute if LinkImage plugin is available', () => {
+			dataFilter.loadAllowedConfig( [ {
+				name: /.*/,
+				attributes: true
+			} ] );
+
+			const expectedHtml =
+				'<figure class="image">' +
+					'<a href="www.example.com">' +
+						'<img src="/assets/sample.png">' +
+					'</a>' +
+				'</figure>';
+
+			editor.setData( expectedHtml );
+
+			expect( getModelDataWithAttributes( model, { withoutSelection: true } ) ).to.deep.equal( {
+				data: '<imageBlock linkHref="www.example.com" src="/assets/sample.png"></imageBlock>',
+				attributes: {}
+			} );
+
+			expect( editor.getData() ).to.equal( expectedHtml );
+		} );
+
 		// it( 'should allow modifying styles, classes and attributes', () => {
 		// 	// This should also work when we set `attributes: true` but currently there are some
 		// 	// problems related to GHS picking up non-GHS attributes (like src) due to some attributes not