From 89973cba689477585ea2e258996c608e520dc30d Mon Sep 17 00:00:00 2001 From: "sergei.kalugin" Date: Wed, 19 Feb 2020 18:51:15 +0300 Subject: [PATCH 1/4] Encapsulate logic for get 'img' form 'figure'. Fix errors with downcast img attributes, and unsafe behabiour. --- src/image/converters.js | 5 +++-- src/image/utils.js | 16 +++++++++++++++- src/imageupload/imageuploadediting.js | 3 ++- src/imageupload/imageuploadprogress.js | 5 +++-- 4 files changed, 23 insertions(+), 6 deletions(-) diff --git a/src/image/converters.js b/src/image/converters.js index 9fd64066..c2e23f61 100644 --- a/src/image/converters.js +++ b/src/image/converters.js @@ -8,6 +8,7 @@ */ import first from '@ckeditor/ckeditor5-utils/src/first'; +import { getImgViewFromFigure } from './utils'; /** * Returns a function that converts the image view representation: @@ -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 = getImgViewFromFigure( 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 = getImgViewFromFigure( figure ); if ( data.attributeNewValue !== null ) { viewWriter.setAttribute( data.attributeKey, data.attributeNewValue, img ); diff --git a/src/image/utils.js b/src/image/utils.js index ed906b42..18bdc523 100644 --- a/src/image/utils.js +++ b/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 = getImgViewFromFigure( viewElement ); const altText = imgElement.getAttribute( 'alt' ); return altText ? `${ altText } ${ label }` : label; @@ -107,6 +107,20 @@ export function isImageAllowed( model ) { isInOtherImage( selection ); } +/** + * Get img view element from the figure view. + * + * Using figureView.getChild(0) - is unsafe, + * because there are can be "figcaption" or smth else. + * This approach provide ability to extend plugin logic with other custom plugins. + * + * @param {module:engine/view/element~Element} figureView + * @returns {module:engine/view/element~Element} + */ +export function getImgViewFromFigure( figureView ) { + return Array.from( figureView.getChildren() ).find( viewChild => viewChild.is( 'img' ) ); +} + // Checks if image is allowed by schema in optimal insertion parent. // // @returns {Boolean} diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index b3083c06..2795fc48 100644 --- a/src/imageupload/imageuploadediting.js +++ b/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 { getImgViewFromFigure } 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 = getImgViewFromFigure( viewFigure ); editor.editing.view.once( 'render', () => { // Early returns just to be safe. There might be some code ran diff --git a/src/imageupload/imageuploadprogress.js b/src/imageupload/imageuploadprogress.js index 0b18b965..d5080ea5 100644 --- a/src/imageupload/imageuploadprogress.js +++ b/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 { getImgViewFromFigure } 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 = getImgViewFromFigure( 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 = getImgViewFromFigure( viewFigure ); writer.setAttribute( 'src', loader.data, viewImg ); } From 49ffc70015727eff21f377baecccbd0ecd9f9a4d Mon Sep 17 00:00:00 2001 From: "sergei.kalugin" Date: Wed, 19 Feb 2020 18:59:48 +0300 Subject: [PATCH 2/4] refactor converters.js. --- src/image/converters.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/image/converters.js b/src/image/converters.js index c2e23f61..110bd00d 100644 --- a/src/image/converters.js +++ b/src/image/converters.js @@ -36,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 = getImgViewFromFigure( 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 } ) ) { From b94362659e050a6ee946fb164c6a79642edc0862 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 26 Feb 2020 16:24:27 +0100 Subject: [PATCH 3/4] Add test for attributes on image conversion with other elements present. --- tests/image/converters.js | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+) diff --git a/tests/image/converters.js b/tests/image/converters.js index da8abbac..96f7a92d 100644 --- a/tests/image/converters.js +++ b/tests/image/converters.js @@ -259,5 +259,23 @@ describe( 'Image converters', () => { '
' ); } ); + + it( 'should set attribute on 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, '' ); + const image = document.getRoot().getChild( 0 ); + + model.change( writer => { + writer.setAttribute( 'alt', 'foo bar', image ); + } ); + + expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal( + '
foo bar
' + ); + } ); } ); } ); From 82f1f01aa42928a6a9ebcebc2c0f598662e1599b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Maciej=20Go=C5=82aszewski?= Date: Wed, 26 Feb 2020 16:30:21 +0100 Subject: [PATCH 4/4] Rename the proposed method to getViewImgFromWidget() and update the docs. --- src/image/converters.js | 8 ++++---- src/image/utils.js | 11 +++++------ src/imageupload/imageuploadediting.js | 4 ++-- src/imageupload/imageuploadprogress.js | 6 +++--- 4 files changed, 14 insertions(+), 15 deletions(-) diff --git a/src/image/converters.js b/src/image/converters.js index 597f726d..f54e3dab 100644 --- a/src/image/converters.js +++ b/src/image/converters.js @@ -8,7 +8,7 @@ */ import first from '@ckeditor/ckeditor5-utils/src/first'; -import { getImgViewFromFigure } from './utils'; +import { getViewImgFromWidget } from './utils'; /** * Returns a function that converts the image view representation: @@ -36,7 +36,7 @@ export function viewFigureToModel() { } // Find an image element inside the figure element. - const viewImage = getImgViewFromFigure( data.viewItem ); + 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 } ) ) { @@ -82,7 +82,7 @@ export function srcsetAttributeConverter() { const writer = conversionApi.writer; const figure = conversionApi.mapper.toViewElement( data.item ); - const img = getImgViewFromFigure( figure ); + const img = getViewImgFromWidget( figure ); if ( data.attributeNewValue === null ) { const srcset = data.attributeOldValue; @@ -123,7 +123,7 @@ export function modelToViewAttributeConverter( attributeKey ) { const viewWriter = conversionApi.writer; const figure = conversionApi.mapper.toViewElement( data.item ); - const img = getImgViewFromFigure( figure ); + const img = getViewImgFromWidget( figure ); if ( data.attributeNewValue !== null ) { viewWriter.setAttribute( data.attributeKey, data.attributeNewValue, img ); diff --git a/src/image/utils.js b/src/image/utils.js index 5c715d78..ad0c3b72 100644 --- a/src/image/utils.js +++ b/src/image/utils.js @@ -25,7 +25,7 @@ export function toImageWidget( viewElement, writer, label ) { return toWidget( viewElement, writer, { label: labelCreator } ); function labelCreator() { - const imgElement = getImgViewFromFigure( viewElement ); + const imgElement = getViewImgFromWidget( viewElement ); const altText = imgElement.getAttribute( 'alt' ); return altText ? `${ altText } ${ label }` : label; @@ -108,16 +108,15 @@ export function isImageAllowed( model ) { } /** - * Get img view element from the figure view. + * Get view `` element from the view widget (`
`). * - * Using figureView.getChild(0) - is unsafe, - * because there are can be "figcaption" or smth else. - * This approach provide ability to extend plugin logic with other custom plugins. + * 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 getImgViewFromFigure( figureView ) { +export function getViewImgFromWidget( figureView ) { return Array.from( figureView.getChildren() ).find( viewChild => viewChild.is( 'img' ) ); } diff --git a/src/imageupload/imageuploadediting.js b/src/imageupload/imageuploadediting.js index 5f9e2279..146050c2 100644 --- a/src/imageupload/imageuploadediting.js +++ b/src/imageupload/imageuploadediting.js @@ -17,7 +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 { getImgViewFromFigure } from '../image/utils'; +import { getViewImgFromWidget } from '../image/utils'; /** * The editing part of the image upload feature. It registers the `'imageUpload'` command. @@ -218,7 +218,7 @@ export default class ImageUploadEditing extends Plugin { /* istanbul ignore next */ if ( env.isSafari ) { const viewFigure = editor.editing.mapper.toViewElement( imageElement ); - const viewImg = getImgViewFromFigure( viewFigure ); + const viewImg = getViewImgFromWidget( viewFigure ); editor.editing.view.once( 'render', () => { // Early returns just to be safe. There might be some code ran diff --git a/src/imageupload/imageuploadprogress.js b/src/imageupload/imageuploadprogress.js index 90f34a37..1d6976ab 100644 --- a/src/imageupload/imageuploadprogress.js +++ b/src/imageupload/imageuploadprogress.js @@ -13,7 +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 { getImgViewFromFigure } from '../image/utils'; +import { getViewImgFromWidget } from '../image/utils'; import '../../theme/imageuploadprogress.css'; import '../../theme/imageuploadicon.css'; @@ -144,7 +144,7 @@ function _showPlaceholder( placeholder, viewFigure, writer ) { writer.addClass( 'ck-image-upload-placeholder', viewFigure ); } - const viewImg = getImgViewFromFigure( viewFigure ); + const viewImg = getViewImgFromWidget( viewFigure ); if ( viewImg.getAttribute( 'src' ) !== placeholder ) { writer.setAttribute( 'src', placeholder, viewImg ); @@ -271,7 +271,7 @@ function _removeUIElement( viewFigure, writer, uniqueProperty ) { // @param {module:upload/filerepository~FileLoader} loader function _displayLocalImage( viewFigure, writer, loader ) { if ( loader.data ) { - const viewImg = getImgViewFromFigure( viewFigure ); + const viewImg = getViewImgFromWidget( viewFigure ); writer.setAttribute( 'src', loader.data, viewImg ); }