浏览代码

Merge pull request #81 from ckeditor/t/77

Fix: Image captions in the view are hidden instead of being removed. Closes #77.
Szymon Cofalik 8 年之前
父节点
当前提交
88cf7fbf02

+ 76 - 109
packages/ckeditor5-image/src/imagecaption/imagecaptionengine.js

@@ -12,7 +12,6 @@ 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 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';
@@ -43,16 +42,16 @@ 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.
 		 * It is used for hiding editable when is empty and image widget is no longer selected.
 		 *
 		 * @private
-		 * @member {module:engine/view/editableelement~EditableElement} #_lastSelectedEditable
+		 * @member {module:engine/view/editableelement~EditableElement} #_lastSelectedCaption
 		 */
 
+		this._viewCaptionsToUpdate = [];
+
 		/**
 		 * Function used to create editable caption element in the editing view.
 		 *
@@ -77,102 +76,77 @@ export default class ImageCaptionEngine extends Plugin {
 			.toElement( 'caption' );
 
 		// Model to view converter for the data pipeline.
-		data.modelToView.on( 'insert:caption', captionModelToView( new ViewContainerElement( 'figcaption' ) ) );
+		data.modelToView.on( 'insert:caption', captionModelToView( new ViewContainerElement( 'figcaption' ), false ) );
 
 		// 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.
-		// See https://github.com/ckeditor/ckeditor5-image/issues/58.
-		editing.modelToView.on( 'insert', insertMissingViewCaptionElement( this._createCaption, mapper ), { priority: 'high' } );
+		// Always show caption in view when something is inserted in model.
+		editing.modelToView.on( 'insert', ( evt, data ) => this._fixCaptionVisibility( data.item ), { priority: 'high' } );
+
+		// Hide caption when everything is removed from it.
+		editing.modelToView.on( 'remove', ( evt, data ) => this._fixCaptionVisibility( data.sourcePosition.parent ), { priority: 'high' } );
 
 		// Update view before each rendering.
-		this.listenTo( viewDocument, 'render', () => this._updateView(), { priority: 'high' } );
+		this.listenTo( viewDocument, 'render', () => this._updateCaptionVisibility(), { priority: 'high' } );
 	}
 
 	/**
-	 * Checks if there is an empty caption element to remove from the view.
+	 * Updates view before each rendering, making sure that empty captions (so unnecessary ones) are hidden
+	 * and then visible when the image is selected.
 	 *
 	 * @private
 	 */
-	_removeEmptyCaption() {
+	_updateCaptionVisibility() {
+		const mapper = this.editor.editing.mapper;
 		const viewSelection = this.editor.editing.view.selection;
-		const viewCaptionElement = this._lastSelectedEditable;
-
-		// No caption to hide.
-		if ( !viewCaptionElement ) {
-			return;
-		}
+		const selectedElement = viewSelection.getSelectedElement();
+		let viewCaption;
 
-		// If selection is placed inside caption - do not remove it.
-		if ( viewSelection.editableElement === viewCaptionElement ) {
-			return;
+		// Hide last selected caption if have no child elements.
+		if ( this._lastSelectedCaption && !this._lastSelectedCaption.childCount ) {
+			this._lastSelectedCaption.addClass( 'ck-hidden' );
 		}
 
-		// Do not remove caption if selection is placed on image that contains that caption.
-		const selectedElement = viewSelection.getSelectedElement();
-
+		// If whole image widget is selected.
 		if ( selectedElement && isImageWidget( selectedElement ) ) {
-			const viewImage = viewCaptionElement.findAncestor( element => element == selectedElement );
-
-			if ( viewImage ) {
-				return;
-			}
+			const modelImage = mapper.toModelElement( selectedElement );
+			const modelCaption = getCaptionFromImage( modelImage );
+			viewCaption =  mapper.toViewElement( modelCaption );
 		}
 
-		// Remove image caption if its empty.
-		if ( viewCaptionElement.childCount === 0 ) {
-			const mapper = this.editor.editing.mapper;
-			viewWriter.remove( ViewRange.createOn( viewCaptionElement ) );
-			mapper.unbindViewElement( viewCaptionElement );
+		// If selection is placed inside caption.
+		if ( isCaption( viewSelection.editableElement ) ) {
+			viewCaption = viewSelection.editableElement;
 		}
-	}
-
-	/**
-	 * Checks if selected image needs a new caption element inside.
-	 *
-	 * @private
-	 */
-	_addCaptionWhenSelected() {
-		const editing = this.editor.editing;
-		const selection = editing.view.selection;
-		const viewImage = selection.getSelectedElement();
-		const mapper = editing.mapper;
-
-		if ( viewImage && isImageWidget( viewImage ) ) {
-			const modelImage = mapper.toModelElement( viewImage );
-			const modelCaption = getCaptionFromImage( modelImage );
-			let viewCaption =  mapper.toViewElement( modelCaption );
 
-			if ( !viewCaption ) {
-				viewCaption = this._createCaption();
-				insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper );
-			}
-
-			this._lastSelectedEditable = viewCaption;
+		if ( viewCaption ) {
+			viewCaption.removeClass( 'ck-hidden' );
+			this._lastSelectedCaption = viewCaption;
 		}
 	}
 
 	/**
-	 * Updates view before each rendering, making sure that empty captions (so unnecessary ones) are removed
-	 * and then re-added when the image is selected.
+	 * Fixes caption visibility during model to view conversion.
+	 * Checks if changed node is placed inside caption element and fixes it's visibility in the view.
 	 *
 	 * @private
+	 * @param {module:engine/model/node~Node} node
 	 */
-	_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._addCaptionWhenSelected();
-
-		// 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;
+	_fixCaptionVisibility( node ) {
+		const modelCaption = getParentCaption( node );
+		const mapper = this.editor.editing.mapper;
+
+		if ( modelCaption ) {
+			const viewCaption = mapper.toViewElement( modelCaption );
+
+			if ( viewCaption ) {
+				if ( modelCaption.childCount ) {
+					viewCaption.removeClass( 'ck-hidden' );
+				} else {
+					viewCaption.addClass( 'ck-hidden' );
+				}
+			}
 		}
 	}
 }
@@ -201,42 +175,22 @@ function insertMissingModelCaptionElement( evt, changeType, data, batch ) {
 		}
 	}
 }
-
-// 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
-// @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 ) ) {
-			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
 // @param {Function|module:engine/view/element~Element} elementCreator
+// @param {Boolean} [hide=true] When set to `false` view element will not be inserted when it's empty.
 // @return {Function}
-function captionModelToView( elementCreator ) {
+function captionModelToView( elementCreator, hide = true ) {
 	return ( evt, data, consumable, conversionApi ) => {
 		const captionElement = data.item;
 
-		if ( isImage( captionElement.parent ) && ( captionElement.childCount > 0 ) ) {
+		// Return if element shouldn't be present when empty.
+		if ( !captionElement.childCount && !hide ) {
+			return;
+		}
+
+		if ( isImage( captionElement.parent ) ) {
 			if ( !consumable.consume( data.item, 'insert' ) ) {
 				return;
 			}
@@ -246,20 +200,16 @@ function captionModelToView( elementCreator ) {
 				elementCreator.clone( true ) :
 				elementCreator();
 
+			// Hide if empty.
+			if ( !captionElement.childCount ) {
+				viewCaption.addClass( 'ck-hidden' );
+			}
+
 			insertViewCaptionAndBind( viewCaption, data.item, viewImage, conversionApi.mapper );
 		}
 	};
 }
 
-// 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
@@ -273,3 +223,20 @@ function insertViewCaptionAndBind( viewCaption, modelCaption, viewImage, mapper
 	viewWriter.insert( viewPosition, viewCaption );
 	mapper.bindElements( modelCaption, viewCaption );
 }
+
+/**
+ * Checks if provided node or one of its ancestors is caption element and returns it.
+ *
+ * @param {module:engine/model/node~Node} node
+ * @returns {module:engine/model/element~Element|null}
+ */
+function getParentCaption( node ) {
+	const ancestors = node.getAncestors( { includeNode: true } );
+	const caption = ancestors.find( ancestor => ancestor.name == 'caption' );
+
+	if ( caption && caption.parent && caption.parent.name == 'image' ) {
+		return caption;
+	}
+
+	return null;
+}

+ 105 - 44
packages/ckeditor5-image/tests/imagecaption/imagecaptionengine.js

@@ -80,7 +80,7 @@ describe( 'ImageCaptionEngine', () => {
 				expect( editor.getData() ).to.equal( '<figure class="image"><img src="img.png"><figcaption>Foo bar baz.</figcaption></figure>' );
 			} );
 
-			it( 'should not convert caption if it\'s empty', () => {
+			it( 'should not convert caption to figcaption if it\'s empty', () => {
 				setModelData( document, '<image src="img.png"><caption></caption></image>' );
 
 				expect( editor.getData() ).to.equal( '<figure class="image"><img src="img.png"></figure>' );
@@ -106,11 +106,14 @@ describe( 'ImageCaptionEngine', () => {
 				);
 			} );
 
-			it( 'should not convert caption if it\'s empty', () => {
+			it( 'should convert caption to element with proper CSS class if it\'s empty', () => {
 				setModelData( document, '<image src="img.png"><caption></caption></image>' );
 
 				expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
-					'<figure class="image ck-widget" contenteditable="false"><img src="img.png"></img></figure>'
+					'<figure class="image ck-widget" contenteditable="false">' +
+						'<img src="img.png"></img>' +
+						'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
+					'</figure>'
 				);
 			} );
 
@@ -141,6 +144,60 @@ describe( 'ImageCaptionEngine', () => {
 					'<figure class="image ck-widget" contenteditable="false"><img src="img.png"></img><span></span>Foo bar baz.</figure>'
 				);
 			} );
+
+			it( 'should show caption when something is inserted inside', () => {
+				setModelData( document, '<image src="img.png"><caption></caption></image>' );
+				const image = document.getRoot().getChild( 0 );
+				const caption = image.getChild( 0 );
+
+				document.enqueueChanges( () => {
+					const batch = document.batch();
+					batch.insert( ModelPosition.createAt( caption ), 'foo bar' );
+				} );
+
+				expect( getViewData( viewDocument ) ).to.equal(
+					'[]<figure class="image ck-widget" contenteditable="false">' +
+						'<img src="img.png"></img>' +
+						'<figcaption class="ck-editable" contenteditable="true">foo bar</figcaption>' +
+					'</figure>'
+				);
+			} );
+
+			it( 'should hide when everything is removed from caption', () => {
+				setModelData( document, '<image src="img.png"><caption>foo bar baz</caption></image>' );
+				const image = document.getRoot().getChild( 0 );
+				const caption = image.getChild( 0 );
+
+				document.enqueueChanges( () => {
+					const batch = document.batch();
+					batch.remove( ModelRange.createIn( caption ) );
+				} );
+
+				expect( getViewData( viewDocument ) ).to.equal(
+					'[]<figure class="image ck-widget" contenteditable="false">' +
+					'<img src="img.png"></img>' +
+					'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
+					'</figure>'
+				);
+			} );
+
+			it( 'should show when not everything is removed from caption', () => {
+				setModelData( document, '<image src="img.png"><caption>foo bar baz</caption></image>' );
+				const image = document.getRoot().getChild( 0 );
+				const caption = image.getChild( 0 );
+
+				document.enqueueChanges( () => {
+					const batch = document.batch();
+					batch.remove( ModelRange.createFromParentsAndOffsets( caption, 0, caption, 8 ) );
+				} );
+
+				expect( getViewData( viewDocument ) ).to.equal(
+					'[]<figure class="image ck-widget" contenteditable="false">' +
+					'<img src="img.png"></img>' +
+					'<figcaption class="ck-editable" contenteditable="true">baz</figcaption>' +
+					'</figure>'
+				);
+			} );
 		} );
 	} );
 
@@ -153,12 +210,19 @@ describe( 'ImageCaptionEngine', () => {
 				batch.insert( new ModelPosition( document.getRoot(), [ 0 ] ), image );
 			} );
 
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal(
-				'<image alt="" src=""><caption></caption></image>'
+			expect( getModelData( document ) ).to.equal(
+				'[]<image alt="" src=""><caption></caption></image>'
+			);
+
+			expect( getViewData( viewDocument ) ).to.equal(
+				'[]<figure class="image ck-widget" contenteditable="false">' +
+					'<img alt="" src=""></img>' +
+					'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
+				'</figure>'
 			);
 		} );
 
-		it( 'should not add caption element if image does not have it', () => {
+		it( 'should not add caption element if image already have it', () => {
 			const caption = new ModelElement( 'caption', null, 'foo bar' );
 			const image = new ModelElement( 'image', { src: '', alt: '' }, caption );
 			const batch = document.batch();
@@ -167,8 +231,15 @@ describe( 'ImageCaptionEngine', () => {
 				batch.insert( new ModelPosition( document.getRoot(), [ 0 ] ), image );
 			} );
 
-			expect( getModelData( document, { withoutSelection: true } ) ).to.equal(
-				'<image alt="" src=""><caption>foo bar</caption></image>'
+			expect( getModelData( document ) ).to.equal(
+				'[]<image alt="" src=""><caption>foo bar</caption></image>'
+			);
+
+			expect( getViewData( viewDocument ) ).to.equal(
+				'[]<figure class="image ck-widget" contenteditable="false">' +
+					'<img alt="" src=""></img>' +
+					'<figcaption class="ck-editable" contenteditable="true">foo bar</figcaption>' +
+				'</figure>'
 			);
 		} );
 
@@ -187,39 +258,6 @@ describe( 'ImageCaptionEngine', () => {
 		} );
 	} );
 
-	describe( 'inserting into image caption', () => {
-		it( 'should add view caption if insertion was made to model caption', () => {
-			setModelData( document, '<image src=""><caption></caption></image>' );
-			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(
-				'<figure class="image ck-widget" contenteditable="false"><img src=""></img></figure>'
-			);
-
-			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(
-				'<image src=""><caption>foo bar baz</caption></image>'
-			);
-
-			// Check if view has caption.
-			expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
-				'<figure class="image ck-widget" contenteditable="false">' +
-					'<img src=""></img>' +
-					'<figcaption class="ck-editable" contenteditable="true">foo bar baz</figcaption>' +
-				'</figure>'
-			);
-		} );
-	} );
-
 	describe( 'editing view', () => {
 		it( 'image should have empty figcaption element when is selected', () => {
 			setModelData( document, '[<image src=""><caption></caption></image>]' );
@@ -232,12 +270,13 @@ describe( 'ImageCaptionEngine', () => {
 			);
 		} );
 
-		it( 'image should not have empty figcaption element when is not selected', () => {
+		it( 'image should have empty figcaption element with hidden class when not selected', () => {
 			setModelData( document, '[]<image src=""><caption></caption></image>' );
 
 			expect( getViewData( viewDocument ) ).to.equal(
 				'[]<figure class="image ck-widget" contenteditable="false">' +
 					'<img src=""></img>' +
+					'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
 				'</figure>'
 			);
 		} );
@@ -253,7 +292,7 @@ describe( 'ImageCaptionEngine', () => {
 			);
 		} );
 
-		it( 'should remove figcaption when caption is empty and image is no longer selected', () => {
+		it( 'should add hidden class to figcaption when caption is empty and image is no longer selected', () => {
 			setModelData( document, '[<image src=""><caption></caption></image>]' );
 
 			document.enqueueChanges( () => {
@@ -263,6 +302,7 @@ describe( 'ImageCaptionEngine', () => {
 			expect( getViewData( viewDocument ) ).to.equal(
 				'[]<figure class="image ck-widget" contenteditable="false">' +
 					'<img src=""></img>' +
+					'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
 				'</figure>'
 			);
 		} );
@@ -337,7 +377,10 @@ describe( 'ImageCaptionEngine', () => {
 
 				// Check if there is no figcaption in the view.
 				expect( getViewData( viewDocument ) ).to.equal(
-					'[]<figure class="image ck-widget" contenteditable="false"><img src=""></img></figure>'
+					'[]<figure class="image ck-widget" contenteditable="false">' +
+						'<img src=""></img>' +
+						'<figcaption class="ck-editable ck-hidden" contenteditable="true"></figcaption>' +
+					'</figure>'
 				);
 
 				editor.execute( 'undo' );
@@ -350,6 +393,24 @@ describe( 'ImageCaptionEngine', () => {
 					'</figure>'
 				);
 			} );
+
+			it( 'undo should work after inserting the image', () => {
+				const image = new ModelElement( 'image' );
+
+				setModelData( document, '[]' );
+
+				document.enqueueChanges( () => {
+					const batch = document.batch();
+
+					batch.insert( document.selection.anchor, image );
+				} );
+
+				editor.execute( 'undo' );
+
+				expect( getModelData( document ) ).to.equal(
+					'[]'
+				);
+			} );
 		} );
 	} );
 } );