Make sure picture element attributes are preserved during downcast.

ckeditor · Oct 16, 2024 · 96a7aff · 96a7aff
1 parent 07892dd
commit 96a7aff
Show file tree

Hide file tree

Showing 2 changed files with 79 additions and 16 deletions.
diff --git a/packages/ckeditor5-image/src/image/converters.ts b/packages/ckeditor5-image/src/image/converters.ts
@@ -229,13 +229,6 @@ export function downcastSourcesAttribute( imageUtils: ImageUtils ): ( dispatcher
 		const attributeNewValue = data.attributeNewValue as null | Array<ViewElementAttributes>;
 
 		if ( attributeNewValue && attributeNewValue.length ) {
-			// Make sure <picture> does not break attribute elements, for instance <a> in linked images.
-			const pictureElement = viewWriter.createContainerElement( 'picture', null,
-				attributeNewValue.map( sourceAttributes => {
-					return viewWriter.createEmptyElement( 'source', sourceAttributes );
-				} )
-			);
-
 			// Collect all wrapping attribute elements.
 			const attributeElements = [];
 			let viewElement = imgElement.parent;
@@ -249,19 +242,25 @@ export function downcastSourcesAttribute( imageUtils: ImageUtils ): ( dispatcher
 				viewElement = parentElement;
 			}
 
-			const prevPictureElement = imgElement.parent!.is( 'element', 'picture' ) ? imgElement.parent : null;
+			const hasPictureElement = imgElement.parent!.is( 'element', 'picture' );
+
+			// Reuse existing <picture> element (ckeditor5#17192) or create a new one.
+			const pictureElement = hasPictureElement ? imgElement.parent : viewWriter.createContainerElement( 'picture', null );
 
-			if ( prevPictureElement ) {
-				// Insert a new picture, move img into it, remove previous picture element (ckeditor5#17192).
-				viewWriter.insert( viewWriter.createPositionBefore( prevPictureElement ), pictureElement );
-				viewWriter.move( viewWriter.createRangeOn( imgElement ), viewWriter.createPositionAt( pictureElement, 'end' ) );
-				viewWriter.remove( prevPictureElement );
-			} else {
-				// Insert the picture and move img into it.
+			if ( !hasPictureElement ) {
 				viewWriter.insert( viewWriter.createPositionBefore( imgElement ), pictureElement );
-				viewWriter.move( viewWriter.createRangeOn( imgElement ), viewWriter.createPositionAt( pictureElement, 'end' ) );
 			}
 
+			viewWriter.remove( viewWriter.createRangeIn( pictureElement ) );
+
+			viewWriter.insert( viewWriter.createPositionAt( pictureElement, 'end' ),
+				attributeNewValue.map( sourceAttributes => {
+					return viewWriter.createEmptyElement( 'source', sourceAttributes );
+				} )
+			);
+
+			viewWriter.move( viewWriter.createRangeOn( imgElement ), viewWriter.createPositionAt( pictureElement, 'end' ) );
+
 			// Apply collected attribute elements over the new picture element.
 			for ( const attributeElement of attributeElements ) {
 				viewWriter.wrap( viewWriter.createRangeOn( pictureElement ), attributeElement );

diff --git a/packages/ckeditor5-image/tests/pictureediting.js b/packages/ckeditor5-image/tests/pictureediting.js
@@ -1945,6 +1945,70 @@ describe( 'PictureEditing', () => {
 						'</figure>'
 					);
 				} );
+
+				it( 'should keep existing picture element attributes when downcasting "sources" attribute', () => {
+					editor.model.schema.extend( 'imageBlock', {
+						allowAttributes: [ 'pictureClass' ]
+					} );
+
+					editor.conversion.for( 'upcast' ).add( dispatcher => {
+						dispatcher.on( 'element:picture', ( _evt, data, conversionApi ) => {
+							const viewItem = data.viewItem;
+							const modelElement = data.modelCursor.parent;
+
+							conversionApi.writer.setAttribute( 'pictureClass', viewItem.getAttribute( 'class' ), modelElement );
+						} );
+					} );
+
+					editor.conversion.for( 'downcast' ).add( dispatcher => {
+						dispatcher.on( 'attribute:pictureClass:imageBlock', ( evt, data, conversionApi ) => {
+							const element = conversionApi.mapper.toViewElement( data.item );
+							const pictureElement = element.getChild( 0 );
+
+							conversionApi.writer.setAttribute( 'class', data.attributeNewValue, pictureElement );
+						} );
+					} );
+
+					editor.setData(
+						'<figure class="image">' +
+							'<picture class="test-class">' +
+								'<source srcset="">' +
+								'<img src="/assets/sample.png">' +
+							'</picture>' +
+							'<figcaption>Caption</figcaption>' +
+						'</figure>'
+					);
+
+					model.change( writer => {
+						writer.setAttribute(
+							'sources',
+							[
+								{
+									srcset: '/assets/sample2.png'
+								}
+							],
+							modelDocument.getRoot().getChild( 0 )
+						);
+					} );
+
+					expect( getViewData( view, { withoutSelection: true } ) ).to.equal(
+						'<figure class="ck-widget image" contenteditable="false">' +
+							'<picture class="test-class">' +
+								'<source srcset="/assets/sample2.png"></source>' +
+								'<img src="/assets/sample.png"></img>' +
+							'</picture>' +
+							'<figcaption ' +
+								'aria-label="Caption for the image" ' +
+								'class="ck-editor__editable ck-editor__nested-editable" ' +
+								'contenteditable="true" ' +
+								'data-placeholder="Enter image caption" ' +
+								'role="textbox" ' +
+								'tabindex="-1">' +
+									'Caption' +
+							'</figcaption>' +
+						'</figure>'
+					);
+				} );
 			} );
 		} );
 	} );