From 370ff8d680b7e0b9ccdc0f87bdadcd67d75eba0b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Wed, 1 Mar 2017 17:23:37 +0100 Subject: [PATCH 1/9] Adding missing image caption in the view during model to view conversion. --- src/imagecaption/imagecaptionengine.js | 75 ++++++++++++++++++-------- src/imagecaption/utils.js | 4 ++ 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index 199c2dbe..e8e2c8aa 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -19,7 +19,7 @@ import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; import ViewMatcher from '@ckeditor/ckeditor5-engine/src/view/matcher'; import { isImage, isImageWidget } from '../image/utils'; -import { captionElementCreator, isCaption, getCaptionFromImage } from './utils'; +import { captionElementCreator, isCaption, getCaptionFromImage, isInsideCaption } from './utils'; /** * The image caption engine plugin. @@ -39,6 +39,7 @@ export default class ImageCaptionEngine extends Plugin { const schema = document.schema; const data = editor.data; const editing = editor.editing; + const mapper = editing.mapper; /** * Last selected caption editable. @@ -47,6 +48,14 @@ export default class ImageCaptionEngine extends Plugin { * @member {module:image/imagecaption/imagecaptionengine~ImageCaptionEngine} #_lastSelectedEditable */ + /** + * Function used to create caption element in editing view. + * + * @private + * @member {Function} + */ + this._captionCreator = captionElementCreator( viewDocument ); + // Schema configuration. schema.registerItem( 'caption' ); schema.allow( { name: '$inline', inside: 'caption' } ); @@ -57,33 +66,37 @@ export default class ImageCaptionEngine extends Plugin { document.on( 'change', insertMissingCaptionElement ); // View to model converter for data pipeline. - const matcher = new ViewMatcher( ( element ) => { - const parent = element.parent; - - // Convert only captions for images. - if ( element.name == 'figcaption' && parent && parent.name == 'figure' && parent.hasClass( 'image' ) ) { - return { name: true }; - } - - return null; - } ); - buildViewConverter() .for( data.viewToModel ) - .from( matcher ) + .from( new ViewMatcher( matchImageCaption ) ) .toElement( 'caption' ); // Model to view converter for data pipeline. - data.modelToView.on( - 'insert:caption', - captionModelToView( new ViewContainerElement( 'figcaption' ) ) - ); + data.modelToView.on( 'insert:caption', captionModelToView( new ViewContainerElement( 'figcaption' ) ) ); // Model to view converter for editing pipeline. + editing.modelToView.on( 'insert:caption', captionModelToView( this._captionCreator ) ); + editing.modelToView.on( - 'insert:caption', - captionModelToView( captionElementCreator( viewDocument ) ) - ); + 'insert', + ( evt, data ) => { + if ( isInsideCaption( data.item ) ) { + const caption = data.item.parent; + const image = caption.parent; + + const viewImage = mapper.toViewElement( image ); + let viewCaption = mapper.toViewElement( caption ); + + // Image should be already converted to the view. + if ( viewImage && !viewCaption ) { + viewCaption = this._captionCreator(); + const viewPosition = ViewPosition.createAt( viewImage, 'end' ); + + mapper.bindElements( caption, viewCaption ); + viewWriter.insert( viewPosition, viewCaption ); + } + } + }, { priority: 'high' } ); // Adding / removing caption element when there is no text in the model. const selection = viewDocument.selection; @@ -153,7 +166,6 @@ export default class ImageCaptionEngine extends Plugin { const selection = editing.view.selection; const imageFigure = selection.getSelectedElement(); const mapper = editing.mapper; - const editableCreator = captionElementCreator( editing.view ); if ( imageFigure && isImageWidget( imageFigure ) ) { const modelImage = mapper.toModelElement( imageFigure ); @@ -161,7 +173,7 @@ export default class ImageCaptionEngine extends Plugin { let viewCaption = mapper.toViewElement( modelCaption ); if ( !viewCaption ) { - viewCaption = editableCreator(); + viewCaption = this._captionCreator(); const viewPosition = ViewPosition.createAt( imageFigure, 'end' ); mapper.bindElements( modelCaption, viewCaption ); @@ -215,7 +227,7 @@ function captionModelToView( elementCreator ) { const imageFigure = conversionApi.mapper.toViewElement( data.range.start.parent ); const viewElement = ( elementCreator instanceof ViewElement ) ? elementCreator.clone( true ) : - elementCreator( data, consumable, conversionApi ); + elementCreator(); const viewPosition = ViewPosition.createAt( imageFigure, 'end' ); conversionApi.mapper.bindElements( data.item, viewElement ); @@ -223,3 +235,20 @@ function captionModelToView( elementCreator ) { } }; } + +// Checks if given element is `figcaption` element and is placed inside image `figure` element. +// +// @private +// @param {module:engine/view/element~Element} element +// @returns {Object|null} Returns object accepted by {@link module:engine/view/matcher~Matcher} or `null` if element +// cannot be matched. +function matchImageCaption( element ) { + const parent = element.parent; + + // Convert only captions for images. + if ( element.name == 'figcaption' && parent && parent.name == 'figure' && parent.hasClass( 'image' ) ) { + return { name: true }; + } + + return null; +} diff --git a/src/imagecaption/utils.js b/src/imagecaption/utils.js index 544ff76c..20629fb0 100644 --- a/src/imagecaption/utils.js +++ b/src/imagecaption/utils.js @@ -61,3 +61,7 @@ export function getCaptionFromImage( imageModelElement ) { return null; } + +export function isInsideCaption( node ) { + return node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image'; +} From 2b0da4ee00b13fe0b36840f1132be4b4117a0868 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 2 Mar 2017 10:47:42 +0100 Subject: [PATCH 2/9] Refactoring in ImageCaptionEngine. --- src/imagecaption/imagecaptionengine.js | 115 +++++++++++++------------ 1 file changed, 62 insertions(+), 53 deletions(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index e8e2c8aa..aab7c8ee 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -17,7 +17,6 @@ import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range'; import viewWriter from '@ckeditor/ckeditor5-engine/src/view/writer'; import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; -import ViewMatcher from '@ckeditor/ckeditor5-engine/src/view/matcher'; import { isImage, isImageWidget } from '../image/utils'; import { captionElementCreator, isCaption, getCaptionFromImage, isInsideCaption } from './utils'; @@ -63,12 +62,12 @@ export default class ImageCaptionEngine extends Plugin { schema.limits.add( 'caption' ); // Add caption element to each image inserted without it. - document.on( 'change', insertMissingCaptionElement ); + document.on( 'change', insertMissingModelCaptionElement ); // View to model converter for data pipeline. buildViewConverter() .for( data.viewToModel ) - .from( new ViewMatcher( matchImageCaption ) ) + .from( matchImageCaption ) .toElement( 'caption' ); // Model to view converter for data pipeline. @@ -77,45 +76,11 @@ export default class ImageCaptionEngine extends Plugin { // Model to view converter for editing pipeline. editing.modelToView.on( 'insert:caption', captionModelToView( this._captionCreator ) ); - editing.modelToView.on( - 'insert', - ( evt, data ) => { - if ( isInsideCaption( data.item ) ) { - const caption = data.item.parent; - const image = caption.parent; - - const viewImage = mapper.toViewElement( image ); - let viewCaption = mapper.toViewElement( caption ); - - // Image should be already converted to the view. - if ( viewImage && !viewCaption ) { - viewCaption = this._captionCreator(); - const viewPosition = ViewPosition.createAt( viewImage, 'end' ); - - mapper.bindElements( caption, viewCaption ); - viewWriter.insert( viewPosition, viewCaption ); - } - } - }, { priority: 'high' } ); - - // Adding / removing caption element when there is no text in the model. - const selection = viewDocument.selection; + // When inserting something to caption in the model - make sure that caption in the view is also present. + editing.modelToView.on( 'insert', insertMissingViewCaptionElement( this._captionCreator, mapper ), { priority: 'high' } ); // Update view before each rendering. - this.listenTo( viewDocument, 'render', () => { - // Check if there is an empty caption view element to remove. - this._removeEmptyCaption(); - - // Check if image widget is selected and caption view element needs to be added. - this._addCaption(); - - // If selection is currently inside caption editable - store it to hide when empty. - const editableElement = selection.editableElement; - - if ( editableElement && isCaption( selection.editableElement ) ) { - this._lastSelectedEditable = selection.editableElement; - } - }, { priority: 'high' } ); + this.listenTo( viewDocument, 'render', () => this._updateView(), { priority: 'high' } ); } /** @@ -164,32 +129,46 @@ export default class ImageCaptionEngine extends Plugin { _addCaption() { const editing = this.editor.editing; const selection = editing.view.selection; - const imageFigure = selection.getSelectedElement(); + const viewImage = selection.getSelectedElement(); const mapper = editing.mapper; - if ( imageFigure && isImageWidget( imageFigure ) ) { - const modelImage = mapper.toModelElement( imageFigure ); + if ( viewImage && isImageWidget( viewImage ) ) { + const modelImage = mapper.toModelElement( viewImage ); const modelCaption = getCaptionFromImage( modelImage ); let viewCaption = mapper.toViewElement( modelCaption ); if ( !viewCaption ) { viewCaption = this._captionCreator(); - - const viewPosition = ViewPosition.createAt( imageFigure, 'end' ); - mapper.bindElements( modelCaption, viewCaption ); - viewWriter.insert( viewPosition, viewCaption ); + insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); } this._lastSelectedEditable = viewCaption; } } + + _updateView() { + const selection = this.editor.editing.view.selection; + + // Check if there is an empty caption view element to remove. + this._removeEmptyCaption(); + + // Check if image widget is selected and caption view element needs to be added. + this._addCaption(); + + // If selection is currently inside caption editable - store it to hide when empty. + const editableElement = selection.editableElement; + + if ( editableElement && isCaption( selection.editableElement ) ) { + this._lastSelectedEditable = selection.editableElement; + } + } } // Checks whether data inserted to the model document have image element that has no caption element inside it. // If there is none - adds it to the image element. // // @private -function insertMissingCaptionElement( evt, changeType, data, batch ) { +function insertMissingModelCaptionElement( evt, changeType, data, batch ) { if ( changeType !== 'insert' ) { return; } @@ -210,6 +189,24 @@ function insertMissingCaptionElement( evt, changeType, data, batch ) { } } +function insertMissingViewCaptionElement( creator, mapper ) { + return ( evt, data ) => { + if ( isInsideCaption( data.item ) ) { + const modelCaption = data.item.parent; + const modelImage = modelCaption.parent; + + const viewImage = mapper.toViewElement( modelImage ); + let viewCaption = mapper.toViewElement( modelCaption ); + + // Image should be already converted to the view. + if ( viewImage && !viewCaption ) { + viewCaption = creator(); + insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); + } + } + }; +} + // Creates a converter that converts image caption model element to view element. // // @private @@ -224,14 +221,12 @@ function captionModelToView( elementCreator ) { return; } - const imageFigure = conversionApi.mapper.toViewElement( data.range.start.parent ); - const viewElement = ( elementCreator instanceof ViewElement ) ? + const viewImage = conversionApi.mapper.toViewElement( data.range.start.parent ); + const viewCaption = ( elementCreator instanceof ViewElement ) ? elementCreator.clone( true ) : elementCreator(); - const viewPosition = ViewPosition.createAt( imageFigure, 'end' ); - conversionApi.mapper.bindElements( data.item, viewElement ); - viewWriter.insert( viewPosition, viewElement ); + insertViewCaptionAndBind( viewCaption, data.item, viewImage, conversionApi.mapper ); } }; } @@ -252,3 +247,17 @@ function matchImageCaption( element ) { return null; } + +// Inserts `viewCaption` at the end of `viewImage` and binds it to `modelCaption`. +// +// @private +// @param {module:engine/view/element~Element} viewCaption +// @param {module:engine/model/element~Element} modelCaption +// @param {module:engine/view/element~Element} viewImage +// @param {module:engine/conversion/mapper~Mapper} mapper +function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { + const viewPosition = ViewPosition.createAt( viewImage, 'end' ); + + viewWriter.insert( viewPosition, viewCaption ); + mapper.bindElements( modelCaption, viewCaption ); +} From 910f89419d2ffc66a9aca3c2cdbf4a4d49678c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 2 Mar 2017 12:16:44 +0100 Subject: [PATCH 3/9] Added tests to image caption util function isInsideCaption. --- src/imagecaption/utils.js | 8 +++++++- tests/imagecaption/utils.js | 40 ++++++++++++++++++++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/src/imagecaption/utils.js b/src/imagecaption/utils.js index 20629fb0..66b13dc5 100644 --- a/src/imagecaption/utils.js +++ b/src/imagecaption/utils.js @@ -62,6 +62,12 @@ export function getCaptionFromImage( imageModelElement ) { return null; } +/** + * Returns `true` if provided `node` is placed inside image's caption. + * + * @param {module:engine/model/node~Node} node + * @return {Boolean} + */ export function isInsideCaption( node ) { - return node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image'; + return !!( node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image' ); } diff --git a/tests/imagecaption/utils.js b/tests/imagecaption/utils.js index fd13827f..57a65a9f 100644 --- a/tests/imagecaption/utils.js +++ b/tests/imagecaption/utils.js @@ -5,7 +5,7 @@ import ViewDocument from '@ckeditor/ckeditor5-engine/src/view/document'; import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement'; -import { captionElementCreator, isCaption, getCaptionFromImage } from '../../src/imagecaption/utils'; +import { captionElementCreator, isCaption, getCaptionFromImage, isInsideCaption } from '../../src/imagecaption/utils'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; describe( 'image captioning utils', () => { @@ -65,4 +65,42 @@ describe( 'image captioning utils', () => { expect( getCaptionFromImage( image ) ).to.be.null; } ); } ); + + describe( 'isInsideCaption', () => { + it( 'should return false if node has no parent', () => { + const el = new ModelElement( 'test' ); + + expect( isInsideCaption( el ) ).to.be.false; + } ); + + it( 'should return false if node\'s parent is not caption', () => { + const el = new ModelElement( 'test' ); + new ModelElement( 'test', null, el ); + + expect( isInsideCaption( el ) ).to.be.false; + } ); + + it( 'should return false if parent`s parent node is not defined', () => { + const el = new ModelElement( 'test' ); + new ModelElement( 'caption', null, el ); + + expect( isInsideCaption( el ) ).to.be.false; + } ); + + it( 'should return false if parent\'s parent node is not an image', () => { + const el = new ModelElement( 'test' ); + const parent = new ModelElement( 'caption', null, el ); + new ModelElement( 'not-image', null, parent ); + + expect( isInsideCaption( el ) ).to.be.false; + } ); + + it( 'should return true if node is placed inside image\'s caption', () => { + const el = new ModelElement( 'test' ); + const parent = new ModelElement( 'caption', null, el ); + new ModelElement( 'image', null, parent ); + + expect( isInsideCaption( el ) ).to.be.true; + } ); + } ); } ); From 9dcf164436cb7e7546680cb1b185e4e02ff151fd Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 2 Mar 2017 16:44:08 +0100 Subject: [PATCH 4/9] Moved ImageCaptionEngine methods to utils file. Added tests. --- src/imagecaption/imagecaptionengine.js | 48 +++++----------- src/imagecaption/utils.js | 36 ++++++++++++ tests/imagecaption/utils.js | 79 +++++++++++++++++++++++++- 3 files changed, 129 insertions(+), 34 deletions(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index aab7c8ee..555891b4 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -12,13 +12,19 @@ import ModelTreeWalker from '@ckeditor/ckeditor5-engine/src/model/treewalker'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement'; import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; -import ViewPosition from '@ckeditor/ckeditor5-engine/src/view/position'; import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range'; import viewWriter from '@ckeditor/ckeditor5-engine/src/view/writer'; import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; import { isImage, isImageWidget } from '../image/utils'; -import { captionElementCreator, isCaption, getCaptionFromImage, isInsideCaption } from './utils'; +import { + captionElementCreator, + isCaption, + getCaptionFromImage, + isInsideCaption, + matchImageCaption, + insertViewCaptionAndBind +} from './utils'; /** * The image caption engine plugin. @@ -189,6 +195,13 @@ function insertMissingModelCaptionElement( evt, changeType, data, batch ) { } } +// Returns function that should be executed before model to view conversion is made. It checks if insertion is placed +// inside model caption and makes sure that corresponding view element exists. +// +// @private +// @param {function} creator Function that returns view caption element. +// @param {module:engine/conversion/mapper~Mapper} mapper +// @return {function} function insertMissingViewCaptionElement( creator, mapper ) { return ( evt, data ) => { if ( isInsideCaption( data.item ) ) { @@ -230,34 +243,3 @@ function captionModelToView( elementCreator ) { } }; } - -// Checks if given element is `figcaption` element and is placed inside image `figure` element. -// -// @private -// @param {module:engine/view/element~Element} element -// @returns {Object|null} Returns object accepted by {@link module:engine/view/matcher~Matcher} or `null` if element -// cannot be matched. -function matchImageCaption( element ) { - const parent = element.parent; - - // Convert only captions for images. - if ( element.name == 'figcaption' && parent && parent.name == 'figure' && parent.hasClass( 'image' ) ) { - return { name: true }; - } - - return null; -} - -// Inserts `viewCaption` at the end of `viewImage` and binds it to `modelCaption`. -// -// @private -// @param {module:engine/view/element~Element} viewCaption -// @param {module:engine/model/element~Element} modelCaption -// @param {module:engine/view/element~Element} viewImage -// @param {module:engine/conversion/mapper~Mapper} mapper -function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { - const viewPosition = ViewPosition.createAt( viewImage, 'end' ); - - viewWriter.insert( viewPosition, viewCaption ); - mapper.bindElements( modelCaption, viewCaption ); -} diff --git a/src/imagecaption/utils.js b/src/imagecaption/utils.js index 66b13dc5..3f528dea 100644 --- a/src/imagecaption/utils.js +++ b/src/imagecaption/utils.js @@ -9,6 +9,8 @@ import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; +import ViewPosition from '@ckeditor/ckeditor5-engine/src/view/position'; +import viewWriter from '@ckeditor/ckeditor5-engine/src/view/writer'; const captionSymbol = Symbol( 'imageCaption' ); @@ -71,3 +73,37 @@ export function getCaptionFromImage( imageModelElement ) { export function isInsideCaption( node ) { return !!( node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image' ); } + +/** + * {@link module:engine/view/matcher~Matcher} pattern. Checks if given element is `figcaption` element and is placed + * inside image `figure` element. + * + * @param {module:engine/view/element~Element} element + * @returns {Object|null} Returns object accepted by {@link module:engine/view/matcher~Matcher} or `null` if element + * cannot be matched. + */ +export function matchImageCaption( element ) { + const parent = element.parent; + + // Convert only captions for images. + if ( element.name == 'figcaption' && parent && parent.name == 'figure' && parent.hasClass( 'image' ) ) { + return { name: true }; + } + + return null; +} + +/** + * Inserts `viewCaption` at the end of `viewImage` and binds it to `modelCaption`. + * + * @param {module:engine/view/containerelement~ContainerElement} viewCaption + * @param {module:engine/model/element~Element} modelCaption + * @param {module:engine/view/containerelement~ContainerElement} viewImage + * @param {module:engine/conversion/mapper~Mapper} mapper + */ +export function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { + const viewPosition = ViewPosition.createAt( viewImage, 'end' ); + + viewWriter.insert( viewPosition, viewCaption ); + mapper.bindElements( modelCaption, viewCaption ); +} diff --git a/tests/imagecaption/utils.js b/tests/imagecaption/utils.js index 57a65a9f..aade1b76 100644 --- a/tests/imagecaption/utils.js +++ b/tests/imagecaption/utils.js @@ -5,8 +5,18 @@ import ViewDocument from '@ckeditor/ckeditor5-engine/src/view/document'; import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement'; -import { captionElementCreator, isCaption, getCaptionFromImage, isInsideCaption } from '../../src/imagecaption/utils'; +import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; +import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement'; +import { + captionElementCreator, + isCaption, + getCaptionFromImage, + isInsideCaption, + matchImageCaption, + insertViewCaptionAndBind +} from '../../src/imagecaption/utils'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; +import Mapper from '@ckeditor/ckeditor5-engine/src/conversion/mapper'; describe( 'image captioning utils', () => { let element, document; @@ -103,4 +113,71 @@ describe( 'image captioning utils', () => { expect( isInsideCaption( el ) ).to.be.true; } ); } ); + + describe( 'matchImageCaption', () => { + it( 'should return null for element that is not a figcaption', () => { + const element = new ViewElement( 'div' ); + + expect( matchImageCaption( element ) ).to.be.null; + } ); + + it( 'should return null if figcaption has no parent', () => { + const element = new ViewElement( 'figcaption' ); + + expect( matchImageCaption( element ) ).to.be.null; + } ); + + it( 'should return null if figcaption\'s parent is not a figure', () => { + const element = new ViewElement( 'figcaption' ); + new ViewElement( 'div', null, element ); + + expect( matchImageCaption( element ) ).to.be.null; + } ); + + it( 'should return null if parent has no image class', () => { + const element = new ViewElement( 'figcaption' ); + new ViewElement( 'figure', null, element ); + + expect( matchImageCaption( element ) ).to.be.null; + } ); + + it( 'should return object if element is a valid caption', () => { + const element = new ViewElement( 'figcaption' ); + new ViewElement( 'figure', { class: 'image' }, element ); + + expect( matchImageCaption( element ) ).to.deep.equal( { name: true } ); + } ); + } ); + + describe( 'insertViewCaptionAndBind', () => { + let viewCaption, modelCaption, viewImage, mapper; + + beforeEach( () => { + viewCaption = new ViewContainerElement( 'figcaption' ); + modelCaption = new ModelElement( 'caption' ); + viewImage = new ViewContainerElement( 'figure', { class: 'image' } ); + mapper = new Mapper(); + } ); + + it( 'should insert provided caption to provided image', () => { + insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); + + expect( viewImage.getChild( 0 ) ).to.equal( viewCaption ); + } ); + + it( 'should insert provided caption at the end of provided image', () => { + const dummyElement = new ViewElement( 'dummy' ); + viewImage.appendChildren( dummyElement ); + + insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); + + expect( viewImage.getChild( 1 ) ).to.equal( viewCaption ); + } ); + + it( 'should bind view caption to model caption using provided mapper', () => { + insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); + + expect( mapper.toModelElement( viewCaption ) ).to.equal( modelCaption ); + } ); + } ); } ); From c2abdeba53582d902e483deeea70903fa17ea5b8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 2 Mar 2017 17:10:59 +0100 Subject: [PATCH 5/9] Added tests to ImageCaptionEngine checking if view caption is added when something is inserted to model caption. --- src/imagecaption/imagecaptionengine.js | 2 +- tests/imagecaption/imagecaptionengine.js | 33 ++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index 555891b4..ba0cee71 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -195,7 +195,7 @@ function insertMissingModelCaptionElement( evt, changeType, data, batch ) { } } -// Returns function that should be executed before model to view conversion is made. It checks if insertion is placed +// Returns function that should be executed when model to view conversion is made. It checks if insertion is placed // inside model caption and makes sure that corresponding view element exists. // // @private diff --git a/tests/imagecaption/imagecaptionengine.js b/tests/imagecaption/imagecaptionengine.js index dde70b8c..81c62dc6 100644 --- a/tests/imagecaption/imagecaptionengine.js +++ b/tests/imagecaption/imagecaptionengine.js @@ -186,6 +186,39 @@ describe( 'ImageCaptionEngine', () => { } ); } ); + describe( 'inserting into image caption', () => { + it( 'should add view caption if insertion was made to model caption', () => { + setModelData( document, '' ); + const image = document.getRoot().getChild( 0 ); + const caption = image.getChild( 0 ); + + // Check if there is no caption in the view + expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal( + '
' + ); + + document.enqueueChanges( () => { + const batch = document.batch(); + const position = ModelPosition.createAt( caption ); + + batch.insert( position, 'foo bar baz' ); + } ); + + // Check if data is inside model. + expect( getModelData( document, { withoutSelection: true } ) ).to.equal( + 'foo bar baz' + ); + + // Check if view has caption. + expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal( + '
' + + '' + + '
foo bar baz
' + + '
' + ); + } ); + } ); + describe( 'editing view', () => { it( 'image should have empty figcaption element when is selected', () => { setModelData( document, '[]' ); From bdbc2f7f8e1d7ad5c96ef7ecb64af32b70a399f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Thu, 2 Mar 2017 19:10:30 +0100 Subject: [PATCH 6/9] Small doc fix. --- src/imagecaption/imagecaptionengine.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index ba0cee71..be84031f 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -152,6 +152,12 @@ export default class ImageCaptionEngine extends Plugin { } } + /** + * Updates view before each rendering, making sure that not needed empty captions are removed and focused ones are + * visible when needed. + * + * @private + */ _updateView() { const selection = this.editor.editing.view.selection; From 1c7caea8c2ce9b34b82fb29f9955b653462b700f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 6 Mar 2017 14:48:07 +0100 Subject: [PATCH 7/9] Added undo/redo integration test for image caption. --- tests/imagecaption/imagecaptionengine.js | 36 +++++++++++++++++++++++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/tests/imagecaption/imagecaptionengine.js b/tests/imagecaption/imagecaptionengine.js index 81c62dc6..5110b76a 100644 --- a/tests/imagecaption/imagecaptionengine.js +++ b/tests/imagecaption/imagecaptionengine.js @@ -12,6 +12,7 @@ import ModelRange from '@ckeditor/ckeditor5-engine/src/model/range'; import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; import ImageCaptionEngine from '../../src/imagecaption/imagecaptionengine'; import ImageEngine from '../../src/image/imageengine'; +import UndoEngine from '@ckeditor/ckeditor5-undo/src/undoengine'; import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model'; import { getData as getViewData } from '@ckeditor/ckeditor5-engine/src/dev-utils/view'; import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; @@ -22,7 +23,7 @@ describe( 'ImageCaptionEngine', () => { beforeEach( () => { return VirtualTestEditor.create( { - plugins: [ ImageCaptionEngine, ImageEngine ] + plugins: [ ImageCaptionEngine, ImageEngine, UndoEngine ] } ) .then( newEditor => { editor = newEditor; @@ -317,5 +318,38 @@ describe( 'ImageCaptionEngine', () => { ']' ); } ); + + describe( 'undo/redo integration', () => { + it( 'should create view element after redo', () => { + setModelData( document, '[foo bar baz]' ); + + const modelRoot = document.getRoot(); + const modelImage = modelRoot.getChild( 0 ); + const modelCaption = modelImage.getChild( 0 ); + + // Remove text and selection from caption. + document.enqueueChanges( () => { + const batch = document.batch(); + + batch.remove( ModelRange.createIn( modelCaption ) ); + document.selection.removeAllRanges(); + } ); + + // Check if there is no figcaption in the view. + expect( getViewData( viewDocument ) ).to.equal( + '[]
' + ); + + editor.execute( 'undo' ); + + // Check if figcaption is back with contents. + expect( getViewData( viewDocument ) ).to.equal( + '
' + + '' + + '
{foo bar baz}
' + + '
' + ); + } ); + } ); } ); } ); From e750c1795985fa5afa787ab0f57ace13304befd1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Mon, 6 Mar 2017 15:00:50 +0100 Subject: [PATCH 8/9] Encapsulate some utils functions inside ImageCaptionEngine as they have not much sense as public API. --- src/imagecaption/imagecaptionengine.js | 29 +++++++++- src/imagecaption/utils.js | 27 --------- tests/imagecaption/utils.js | 76 +------------------------- 3 files changed, 27 insertions(+), 105 deletions(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index be84031f..4e98ef00 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -15,15 +15,14 @@ import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; import ViewRange from '@ckeditor/ckeditor5-engine/src/view/range'; import viewWriter from '@ckeditor/ckeditor5-engine/src/view/writer'; import ModelPosition from '@ckeditor/ckeditor5-engine/src/model/position'; +import ViewPosition from '@ckeditor/ckeditor5-engine/src/view/position'; import buildViewConverter from '@ckeditor/ckeditor5-engine/src/conversion/buildviewconverter'; import { isImage, isImageWidget } from '../image/utils'; import { captionElementCreator, isCaption, getCaptionFromImage, - isInsideCaption, - matchImageCaption, - insertViewCaptionAndBind + matchImageCaption } from './utils'; /** @@ -249,3 +248,27 @@ function captionModelToView( elementCreator ) { } }; } + +// Returns `true` if provided `node` is placed inside image's caption. +// +// @private +// @param {module:engine/model/node~Node} node +// @return {Boolean} +function isInsideCaption( node ) { + return !!( node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image' ); +} + +// Inserts `viewCaption` at the end of `viewImage` and binds it to `modelCaption`. +// +// @private +// @param {module:engine/view/containerelement~ContainerElement} viewCaption +// @param {module:engine/model/element~Element} modelCaption +// @param {module:engine/view/containerelement~ContainerElement} viewImage +// @param {module:engine/conversion/mapper~Mapper} mapper +export function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { + const viewPosition = ViewPosition.createAt( viewImage, 'end' ); + + viewWriter.insert( viewPosition, viewCaption ); + mapper.bindElements( modelCaption, viewCaption ); +} + diff --git a/src/imagecaption/utils.js b/src/imagecaption/utils.js index 3f528dea..60bb2cbc 100644 --- a/src/imagecaption/utils.js +++ b/src/imagecaption/utils.js @@ -9,8 +9,6 @@ import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; -import ViewPosition from '@ckeditor/ckeditor5-engine/src/view/position'; -import viewWriter from '@ckeditor/ckeditor5-engine/src/view/writer'; const captionSymbol = Symbol( 'imageCaption' ); @@ -64,16 +62,6 @@ export function getCaptionFromImage( imageModelElement ) { return null; } -/** - * Returns `true` if provided `node` is placed inside image's caption. - * - * @param {module:engine/model/node~Node} node - * @return {Boolean} - */ -export function isInsideCaption( node ) { - return !!( node.parent && node.parent.name == 'caption' && node.parent.parent && node.parent.parent.name == 'image' ); -} - /** * {@link module:engine/view/matcher~Matcher} pattern. Checks if given element is `figcaption` element and is placed * inside image `figure` element. @@ -92,18 +80,3 @@ export function matchImageCaption( element ) { return null; } - -/** - * Inserts `viewCaption` at the end of `viewImage` and binds it to `modelCaption`. - * - * @param {module:engine/view/containerelement~ContainerElement} viewCaption - * @param {module:engine/model/element~Element} modelCaption - * @param {module:engine/view/containerelement~ContainerElement} viewImage - * @param {module:engine/conversion/mapper~Mapper} mapper - */ -export function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { - const viewPosition = ViewPosition.createAt( viewImage, 'end' ); - - viewWriter.insert( viewPosition, viewCaption ); - mapper.bindElements( modelCaption, viewCaption ); -} diff --git a/tests/imagecaption/utils.js b/tests/imagecaption/utils.js index aade1b76..bd50f6e2 100644 --- a/tests/imagecaption/utils.js +++ b/tests/imagecaption/utils.js @@ -6,17 +6,13 @@ import ViewDocument from '@ckeditor/ckeditor5-engine/src/view/document'; import ViewEditableElement from '@ckeditor/ckeditor5-engine/src/view/editableelement'; import ViewElement from '@ckeditor/ckeditor5-engine/src/view/element'; -import ViewContainerElement from '@ckeditor/ckeditor5-engine/src/view/containerelement'; import { captionElementCreator, isCaption, getCaptionFromImage, - isInsideCaption, - matchImageCaption, - insertViewCaptionAndBind + matchImageCaption } from '../../src/imagecaption/utils'; import ModelElement from '@ckeditor/ckeditor5-engine/src/model/element'; -import Mapper from '@ckeditor/ckeditor5-engine/src/conversion/mapper'; describe( 'image captioning utils', () => { let element, document; @@ -76,44 +72,6 @@ describe( 'image captioning utils', () => { } ); } ); - describe( 'isInsideCaption', () => { - it( 'should return false if node has no parent', () => { - const el = new ModelElement( 'test' ); - - expect( isInsideCaption( el ) ).to.be.false; - } ); - - it( 'should return false if node\'s parent is not caption', () => { - const el = new ModelElement( 'test' ); - new ModelElement( 'test', null, el ); - - expect( isInsideCaption( el ) ).to.be.false; - } ); - - it( 'should return false if parent`s parent node is not defined', () => { - const el = new ModelElement( 'test' ); - new ModelElement( 'caption', null, el ); - - expect( isInsideCaption( el ) ).to.be.false; - } ); - - it( 'should return false if parent\'s parent node is not an image', () => { - const el = new ModelElement( 'test' ); - const parent = new ModelElement( 'caption', null, el ); - new ModelElement( 'not-image', null, parent ); - - expect( isInsideCaption( el ) ).to.be.false; - } ); - - it( 'should return true if node is placed inside image\'s caption', () => { - const el = new ModelElement( 'test' ); - const parent = new ModelElement( 'caption', null, el ); - new ModelElement( 'image', null, parent ); - - expect( isInsideCaption( el ) ).to.be.true; - } ); - } ); - describe( 'matchImageCaption', () => { it( 'should return null for element that is not a figcaption', () => { const element = new ViewElement( 'div' ); @@ -148,36 +106,4 @@ describe( 'image captioning utils', () => { expect( matchImageCaption( element ) ).to.deep.equal( { name: true } ); } ); } ); - - describe( 'insertViewCaptionAndBind', () => { - let viewCaption, modelCaption, viewImage, mapper; - - beforeEach( () => { - viewCaption = new ViewContainerElement( 'figcaption' ); - modelCaption = new ModelElement( 'caption' ); - viewImage = new ViewContainerElement( 'figure', { class: 'image' } ); - mapper = new Mapper(); - } ); - - it( 'should insert provided caption to provided image', () => { - insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); - - expect( viewImage.getChild( 0 ) ).to.equal( viewCaption ); - } ); - - it( 'should insert provided caption at the end of provided image', () => { - const dummyElement = new ViewElement( 'dummy' ); - viewImage.appendChildren( dummyElement ); - - insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); - - expect( viewImage.getChild( 1 ) ).to.equal( viewCaption ); - } ); - - it( 'should bind view caption to model caption using provided mapper', () => { - insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); - - expect( mapper.toModelElement( viewCaption ) ).to.equal( modelCaption ); - } ); - } ); } ); From ef82bf2ef21b8533cf35f9f06b05535e5ee29d6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Piotrek=20Koszuli=C5=84ski?= Date: Wed, 8 Mar 2017 23:04:17 +0100 Subject: [PATCH 9/9] Minor naming and docs fixes. --- src/imagecaption/imagecaptionengine.js | 33 +++++++++++++------------- src/imagecaption/utils.js | 2 +- 2 files changed, 18 insertions(+), 17 deletions(-) diff --git a/src/imagecaption/imagecaptionengine.js b/src/imagecaption/imagecaptionengine.js index 4e98ef00..e89b5b6b 100644 --- a/src/imagecaption/imagecaptionengine.js +++ b/src/imagecaption/imagecaptionengine.js @@ -49,16 +49,17 @@ export default class ImageCaptionEngine extends Plugin { * Last selected caption editable. * It is used for hiding editable when is empty and image widget is no longer selected. * - * @member {module:image/imagecaption/imagecaptionengine~ImageCaptionEngine} #_lastSelectedEditable + * @private + * @member {module:engine/view/editableelement~EditableElement} #_lastSelectedEditable */ /** - * Function used to create caption element in editing view. + * Function used to create editable caption element in the editing view. * * @private * @member {Function} */ - this._captionCreator = captionElementCreator( viewDocument ); + this._createCaption = captionElementCreator( viewDocument ); // Schema configuration. schema.registerItem( 'caption' ); @@ -69,27 +70,28 @@ export default class ImageCaptionEngine extends Plugin { // Add caption element to each image inserted without it. document.on( 'change', insertMissingModelCaptionElement ); - // View to model converter for data pipeline. + // View to model converter for the data pipeline. buildViewConverter() .for( data.viewToModel ) .from( matchImageCaption ) .toElement( 'caption' ); - // Model to view converter for data pipeline. + // Model to view converter for the data pipeline. data.modelToView.on( 'insert:caption', captionModelToView( new ViewContainerElement( 'figcaption' ) ) ); - // Model to view converter for editing pipeline. - editing.modelToView.on( 'insert:caption', captionModelToView( this._captionCreator ) ); + // Model to view converter for the editing pipeline. + editing.modelToView.on( 'insert:caption', captionModelToView( this._createCaption ) ); // When inserting something to caption in the model - make sure that caption in the view is also present. - editing.modelToView.on( 'insert', insertMissingViewCaptionElement( this._captionCreator, mapper ), { priority: 'high' } ); + // See https://github.com/ckeditor/ckeditor5-image/issues/58. + editing.modelToView.on( 'insert', insertMissingViewCaptionElement( this._createCaption, mapper ), { priority: 'high' } ); // Update view before each rendering. this.listenTo( viewDocument, 'render', () => this._updateView(), { priority: 'high' } ); } /** - * Checks if there is an empty caption element to remove from view. + * Checks if there is an empty caption element to remove from the view. * * @private */ @@ -131,7 +133,7 @@ export default class ImageCaptionEngine extends Plugin { * * @private */ - _addCaption() { + _addCaptionWhenSelected() { const editing = this.editor.editing; const selection = editing.view.selection; const viewImage = selection.getSelectedElement(); @@ -143,7 +145,7 @@ export default class ImageCaptionEngine extends Plugin { let viewCaption = mapper.toViewElement( modelCaption ); if ( !viewCaption ) { - viewCaption = this._captionCreator(); + viewCaption = this._createCaption(); insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ); } @@ -152,8 +154,8 @@ export default class ImageCaptionEngine extends Plugin { } /** - * Updates view before each rendering, making sure that not needed empty captions are removed and focused ones are - * visible when needed. + * Updates view before each rendering, making sure that empty captions (so unnecessary ones) are removed + * and then re-added when the image is selected. * * @private */ @@ -164,7 +166,7 @@ export default class ImageCaptionEngine extends Plugin { this._removeEmptyCaption(); // Check if image widget is selected and caption view element needs to be added. - this._addCaption(); + this._addCaptionWhenSelected(); // If selection is currently inside caption editable - store it to hide when empty. const editableElement = selection.editableElement; @@ -265,10 +267,9 @@ function isInsideCaption( node ) { // @param {module:engine/model/element~Element} modelCaption // @param {module:engine/view/containerelement~ContainerElement} viewImage // @param {module:engine/conversion/mapper~Mapper} mapper -export function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { +function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper ) { const viewPosition = ViewPosition.createAt( viewImage, 'end' ); viewWriter.insert( viewPosition, viewCaption ); mapper.bindElements( modelCaption, viewCaption ); } - diff --git a/src/imagecaption/utils.js b/src/imagecaption/utils.js index 60bb2cbc..3a4dc7a9 100644 --- a/src/imagecaption/utils.js +++ b/src/imagecaption/utils.js @@ -47,7 +47,7 @@ export function isCaption( viewElement ) { } /** - * Returns caption's model element from given image element. Returns `null` if no caption is found. + * Returns caption model element from given image element. Returns `null` if no caption is found. * * @param {module:engine/model/element~Element} imageModelElement * @return {module:engine/model/element~Element|null}