Fix (image): Handle existing picture element correctly on 'sources' d…

…owncast.
ckeditor · Oct 17, 2024 · 6257c78 · 6257c78
1 parent 45523b7
commit 6257c78
Show file tree

Hide file tree

Showing 2 changed files with 169 additions and 9 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,8 +242,23 @@ export function downcastSourcesAttribute( imageUtils: ImageUtils ): ( dispatcher
 				viewElement = parentElement;
 			}
 
-			// Insert the picture and move img into it.
-			viewWriter.insert( viewWriter.createPositionBefore( imgElement ), pictureElement );
+			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 ( !hasPictureElement ) {
+				viewWriter.insert( viewWriter.createPositionBefore( imgElement ), pictureElement );
+			}
+
+			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.

diff --git a/packages/ckeditor5-image/tests/pictureediting.js b/packages/ckeditor5-image/tests/pictureediting.js
@@ -1865,6 +1865,158 @@ describe( 'PictureEditing', () => {
 
 					expect( editor.getData() ).to.equal( '<p>foo<img src="/assets/sample.png">bar</p>' );
 				} );
+
+				it( 'should downcast changed "sources" attribute on an existing picture element', () => {
+					editor.setData(
+						'<figure class="image">' +
+							'<picture>' +
+								'<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>' +
+								'<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>'
+					);
+				} );
+
+				it( 'should downcast changed "sources" attribute on an existing linked picture element', () => {
+					editor.setData(
+						'<figure class="image">' +
+							'<a href="https://ckeditor.com">' +
+								'<picture>' +
+									'<source srcset="/assets/sample.png">' +
+									'<img src="/assets/sample.png">' +
+								'</picture>' +
+							'</a>' +
+							'<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">' +
+							'<a href="https://ckeditor.com">' +
+								'<picture>' +
+									'<source srcset="/assets/sample2.png"></source>' +
+									'<img src="/assets/sample.png"></img>' +
+								'</picture>' +
+							'</a>' +
+							'<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>'
+					);
+				} );
+
+				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>'
+					);
+				} );
 			} );
 		} );
 	} );