Merge pull request #342.

Fix: The image converters should not assume that <img> is a first child of a <figure>. Closes ckeditor/ckeditor5#6294.

src/image/converters.js

@@ -8,6 +8,7 @@
  */
 
 import first from '@ckeditor/ckeditor5-utils/src/first';
+import { getViewImgFromWidget } from './utils';
 
 /**
  * Returns a function that converts the image view representation:
@@ -35,7 +36,7 @@ export function viewFigureToModel() {
 		}
 
 		// Find an image element inside the figure element.
-		const viewImage = Array.from( data.viewItem.getChildren() ).find( viewChild => viewChild.is( 'img' ) );
+		const viewImage = getViewImgFromWidget( data.viewItem );
 
 		// Do not convert if image element is absent, is missing src attribute or was already converted.
 		if ( !viewImage || !viewImage.hasAttribute( 'src' ) || !conversionApi.consumable.test( viewImage, { name: true } ) ) {
@@ -81,7 +82,7 @@ export function srcsetAttributeConverter() {
 
 		const writer = conversionApi.writer;
 		const figure = conversionApi.mapper.toViewElement( data.item );
-		const img = figure.getChild( 0 );
+		const img = getViewImgFromWidget( figure );
 
 		if ( data.attributeNewValue === null ) {
 			const srcset = data.attributeOldValue;
@@ -122,7 +123,7 @@ export function modelToViewAttributeConverter( attributeKey ) {
 
 		const viewWriter = conversionApi.writer;
 		const figure = conversionApi.mapper.toViewElement( data.item );
-		const img = figure.getChild( 0 );
+		const img = getViewImgFromWidget( figure );
 
 		if ( data.attributeNewValue !== null ) {
 			viewWriter.setAttribute( data.attributeKey, data.attributeNewValue, img );

src/image/utils.js

@@ -25,7 +25,7 @@ export function toImageWidget( viewElement, writer, label ) {
 	return toWidget( viewElement, writer, { label: labelCreator } );
 
 	function labelCreator() {
-		const imgElement = viewElement.getChild( 0 );
+		const imgElement = getViewImgFromWidget( viewElement );
 		const altText = imgElement.getAttribute( 'alt' );
 
 		return altText ? `${ altText } ${ label }` : label;
@@ -107,6 +107,19 @@ export function isImageAllowed( model ) {
 		isInOtherImage( selection );
 }
 
+/**
+ * Get view `<img>` element from the view widget (`<figure>`).
+ *
+ * Assuming that image is always a first child of a widget (ie. `figureView.getChild( 0 )`) is unsafe as other features might
+ * inject their own elements to the widget.
+ *
+ * @param {module:engine/view/element~Element} figureView
+ * @returns {module:engine/view/element~Element}
+ */
+export function getViewImgFromWidget( figureView ) {
+	return Array.from( figureView.getChildren() ).find( viewChild => viewChild.is( 'img' ) );
+}
+
 // Checks if image is allowed by schema in optimal insertion parent.
 //
 // @returns {Boolean}

src/imageupload/imageuploadediting.js

@@ -17,6 +17,7 @@ import env from '@ckeditor/ckeditor5-utils/src/env';
 import ImageUploadCommand from '../../src/imageupload/imageuploadcommand';
 import { fetchLocalImage, isLocalImage } from '../../src/imageupload/utils';
 import { createImageTypeRegExp } from './utils';
+import { getViewImgFromWidget } from '../image/utils';
 
 /**
  * The editing part of the image upload feature. It registers the `'imageUpload'` command.
@@ -217,7 +218,7 @@ export default class ImageUploadEditing extends Plugin {
 				/* istanbul ignore next */
 				if ( env.isSafari ) {
 					const viewFigure = editor.editing.mapper.toViewElement( imageElement );
-					const viewImg = viewFigure.getChild( 0 );
+					const viewImg = getViewImgFromWidget( viewFigure );
 
 					editor.editing.view.once( 'render', () => {
 						// Early returns just to be safe. There might be some code ran

src/imageupload/imageuploadprogress.js

@@ -13,6 +13,7 @@ import Plugin from '@ckeditor/ckeditor5-core/src/plugin';
 import FileRepository from '@ckeditor/ckeditor5-upload/src/filerepository';
 import uploadingPlaceholder from '../../theme/icons/image_placeholder.svg';
 import env from '@ckeditor/ckeditor5-utils/src/env';
+import { getViewImgFromWidget } from '../image/utils';
 
 import '../../theme/imageuploadprogress.css';
 import '../../theme/imageuploadicon.css';
@@ -143,7 +144,7 @@ function _showPlaceholder( placeholder, viewFigure, writer ) {
 		writer.addClass( 'ck-image-upload-placeholder', viewFigure );
 	}
 
-	const viewImg = viewFigure.getChild( 0 );
+	const viewImg = getViewImgFromWidget( viewFigure );
 
 	if ( viewImg.getAttribute( 'src' ) !== placeholder ) {
 		writer.setAttribute( 'src', placeholder, viewImg );
@@ -270,7 +271,7 @@ function _removeUIElement( viewFigure, writer, uniqueProperty ) {
 // @param {module:upload/filerepository~FileLoader} loader
 function _displayLocalImage( viewFigure, writer, loader ) {
 	if ( loader.data ) {
-		const viewImg = viewFigure.getChild( 0 );
+		const viewImg = getViewImgFromWidget( viewFigure );
 
 		writer.setAttribute( 'src', loader.data, viewImg );
 	}

tests/image/converters.js

@@ -259,5 +259,23 @@ describe( 'Image converters', () => {
 				'<figure class="ck-widget image" contenteditable="false"><img src=""></img></figure>'
 			);
 		} );
+
+		it( 'should set attribute on <img> even if other element is present inside figure', () => {
+			editor.model.schema.register( 'foo', {
+				allowIn: 'image'
+			} );
+			editor.conversion.for( 'downcast' ).elementToElement( { model: 'foo', view: 'foo' } );
+
+			setModelData( model, '<image src=""><foo></foo></image>' );
+			const image = document.getRoot().getChild( 0 );
+
+			model.change( writer => {
+				writer.setAttribute( 'alt', 'foo bar', image );
+			} );
+
+			expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+				'<figure class="ck-widget image" contenteditable="false"><foo></foo><img alt="foo bar" src=""></img></figure>'
+			);
+		} );
 	} );
 } );