The image style command should be augmentable

packages/ckeditor5-image/src/imagestyle/imagestylecommand.js

@@ -93,32 +93,45 @@ export default class ImageStyleCommand extends Command {
 	 * {@link module:image/imagestyle~ImageStyleConfig#options}).
 	 * @fires execute
 	 */
-	execute( options ) {
+	execute( options = {} ) {
 		const editor = this.editor;
 		const model = editor.model;
 		const imageUtils = editor.plugins.get( 'ImageUtils' );
 
 		model.change( writer => {
 			const requestedStyle = options.value;
-			const supportedTypes = this._styles.get( requestedStyle ).modelElements;
 
 			let imageElement = imageUtils.getClosestSelectedImageElement( model.document.selection );
 
 			// Change the image type if a style requires it.
-			if ( !supportedTypes.includes( imageElement.name ) ) {
-				this.editor.execute( !supportedTypes.includes( 'imageBlock' ) ? 'imageTypeInline' : 'imageTypeBlock' );
+			if ( requestedStyle && this.shouldConvertImageType( requestedStyle, imageElement ) ) {
+				this.editor.execute( imageUtils.isBlockImage( imageElement ) ? 'imageTypeInline' : 'imageTypeBlock' );
 
 				// Update the imageElement to the newly created image.
 				imageElement = imageUtils.getClosestSelectedImageElement( model.document.selection );
 			}
 
 			// Default style means that there is no `imageStyle` attribute in the model.
 			// https://github.com/ckeditor/ckeditor5-image/issues/147
-			if ( this._styles.get( requestedStyle ).isDefault ) {
+			if ( !requestedStyle || this._styles.get( requestedStyle ).isDefault ) {
 				writer.removeAttribute( 'imageStyle', imageElement );
 			} else {
 				writer.setAttribute( 'imageStyle', requestedStyle, imageElement );
 			}
 		} );
 	}
+
+	/**
+	 * Returns `true` if requested style change would trigger the image type change.
+	 *
+	 * @param {module:image/imagestyle~ImageStyleOptionDefinition} requestedStyle The name of the style (as configured in
+	 * {@link module:image/imagestyle~ImageStyleConfig#options}).
+	 * @param {module:engine/model/element~Element} imageElement The image model element.
+	 * @returns {Boolean}
+	 */
+	shouldConvertImageType( requestedStyle, imageElement ) {
+		const supportedTypes = this._styles.get( requestedStyle ).modelElements;
+
+		return !supportedTypes.includes( imageElement.name );
+	}
 }

packages/ckeditor5-image/src/imagestyle/imagestyleediting.js

@@ -9,8 +9,9 @@
 
 import { Plugin } from 'ckeditor5/src/core';
 import ImageStyleCommand from './imagestylecommand';
-import { viewToModelStyleAttribute, modelToViewStyleAttribute } from './converters';
+import ImageUtils from '../imageutils';
 import utils from './utils';
+import { viewToModelStyleAttribute, modelToViewStyleAttribute } from './converters';
 
 /**
  * The image style engine plugin. It sets the default configuration, creates converters and registers
@@ -26,6 +27,13 @@ export default class ImageStyleEditing extends Plugin {
 		return 'ImageStyleEditing';
 	}
 
+	/**
+	 * @inheritDoc
+	 */
+	static get requires() {
+		return [ ImageUtils ];
+	}
+
 	/**
 	 * @inheritDoc
 	 */
@@ -56,6 +64,7 @@ export default class ImageStyleEditing extends Plugin {
 		} );
 
 		this._setupConversion( isBlockPluginLoaded, isInlinePluginLoaded );
+		this._setupPostFixer();
 
 		// Register imageStyle command.
 		editor.commands.add( 'imageStyle', new ImageStyleCommand( editor, this.normalizedStyles ) );
@@ -96,4 +105,51 @@ export default class ImageStyleEditing extends Plugin {
 			editor.data.upcastDispatcher.on( 'element:img', viewToModelConverter, { priority: 'low' } );
 		}
 	}
+
+	/**
+	 * Registers a post-fixer that will make sure that the style attribute value is correct for a specific image type (block vs inline).
+	 *
+	 * @private
+	 */
+	_setupPostFixer() {
+		const editor = this.editor;
+		const document = editor.model.document;
+
+		const imageUtils = editor.plugins.get( ImageUtils );
+		const stylesMap = new Map( this.normalizedStyles.map( style => [ style.name, style ] ) );
+
+		// Make sure that style attribute is valid for the image type.
+		document.registerPostFixer( writer => {
+			let changed = false;
+
+			for ( const change of document.differ.getChanges() ) {
+				if ( change.type == 'insert' || change.type == 'attribute' && change.attributeKey == 'imageStyle' ) {
+					let element = change.type == 'insert' ? change.position.nodeAfter : change.range.start.nodeAfter;
+
+					if ( element && element.is( 'element', 'paragraph' ) && element.childCount > 0 ) {
+						element = element.getChild( 0 );
+					}
+
+					if ( !imageUtils.isImage( element ) ) {
+						continue;
+					}
+
+					const imageStyle = element.getAttribute( 'imageStyle' );
+
+					if ( !imageStyle ) {
+						continue;
+					}
+
+					const imageStyleDefinition = stylesMap.get( imageStyle );
+
+					if ( !imageStyleDefinition || !imageStyleDefinition.modelElements.includes( element.name ) ) {
+						writer.removeAttribute( 'imageStyle', element );
+						changed = true;
+					}
+				}
+			}
+
+			return changed;
+		} );
+	}
 }

packages/ckeditor5-image/src/imageutils.js

@@ -150,7 +150,6 @@ export default class ImageUtils extends Plugin {
 	/**
 	 * Returns a image model element if one is selected or is among the selection's ancestors.
 	 *
-	 * @protected
 	 * @param {module:engine/model/selection~Selection|module:engine/model/documentselection~DocumentSelection} selection
 	 * @returns {module:engine/model/element~Element|null}
 	 */
@@ -212,7 +211,6 @@ export default class ImageUtils extends Plugin {
 	/**
 	 * Checks if the provided model element is an `image`.
 	 *
-	 * @protected
 	 * @param {module:engine/model/element~Element} modelElement
 	 * @returns {Boolean}
 	 */
@@ -223,7 +221,6 @@ export default class ImageUtils extends Plugin {
 	/**
 	 * Checks if the provided model element is an `imageInline`.
 	 *
-	 * @protected
 	 * @param {module:engine/model/element~Element} modelElement
 	 * @returns {Boolean}
 	 */

packages/ckeditor5-image/tests/imagestyle/imagestylecommand.js

@@ -357,6 +357,15 @@ describe( 'ImageStyleCommand', () => {
 					expect( getData( model ) ).to.equal( '<paragraph>[<imageInline></imageInline>]</paragraph>' );
 					expect( command.value ).to.equal( defaultInline.name );
 				} );
+
+				it( 'should set default style if no style specified', () => {
+					setData( model, '<paragraph>[<imageInline imageStyle="${ anyImage.name }"></imageInline>]</paragraph>' );
+
+					command.execute();
+
+					expect( getData( model ) ).to.equal( '<paragraph>[<imageInline></imageInline>]</paragraph>' );
+					expect( command.value ).to.equal( defaultInline.name );
+				} );
 			} );
 
 			describe( 'when a block image is selected', () => {
@@ -406,6 +415,15 @@ describe( 'ImageStyleCommand', () => {
 					expect( getData( model ) ).to.equal( '[<imageBlock><caption></caption></imageBlock>]' );
 					expect( command.value ).to.equal( defaultBlock.name );
 				} );
+
+				it( 'should set default style if no style specified', () => {
+					setData( model, '[<imageBlock imageStyle="${ anyImage.name }"><caption></caption></imageBlock>]' );
+
+					command.execute();
+
+					expect( getData( model ) ).to.equal( '[<imageBlock><caption></caption></imageBlock>]' );
+					expect( command.value ).to.equal( defaultBlock.name );
+				} );
 			} );
 
 			it( 'should set the style if the selection is inside a caption', () => {
@@ -417,4 +435,44 @@ describe( 'ImageStyleCommand', () => {
 			} );
 		} );
 	} );
+
+	describe( 'shouldConvertImageType()', () => {
+		const imgSrc = 'assets/sample.png';
+
+		describe( 'for an inline image', () => {
+			it( 'should return true if the requested type is other than imageInline', () => {
+				setData( model, `<paragraph>[<imageInline src="${ imgSrc }"></imageInline>]</paragraph>` );
+
+				const image = model.document.selection.getSelectedElement();
+
+				expect( command.shouldConvertImageType( onlyBlock.name, image ) ).to.be.true;
+			} );
+
+			it( 'should return false if the requested type equals imageInline', () => {
+				setData( model, `<paragraph>[<imageInline src="${ imgSrc }"></imageInline>]</paragraph>` );
+
+				const image = model.document.selection.getSelectedElement();
+
+				expect( command.shouldConvertImageType( defaultInline.name, image ) ).to.be.false;
+			} );
+		} );
+
+		describe( 'for a block image', () => {
+			it( 'should return true if the requested type is other than imageBlock', () => {
+				setData( model, `[<imageBlock src="${ imgSrc }"></imageBlock>]` );
+
+				const image = model.document.selection.getSelectedElement();
+
+				expect( command.shouldConvertImageType( onlyInline.name, image ) ).to.be.true;
+			} );
+
+			it( 'should return false if the requested type equals imageBlock', () => {
+				setData( model, `[<imageBlock src="${ imgSrc }"><caption></caption></imageBlock>]` );
+
+				const image = model.document.selection.getSelectedElement();
+
+				expect( command.shouldConvertImageType( onlyBlock.name, image ) ).to.be.false;
+			} );
+		} );
+	} );
 } );

packages/ckeditor5-image/tests/imagestyle/imagestyleediting.js

@@ -3,21 +3,21 @@
  * For licensing, see LICENSE.md or https://ckeditor.com/legal/ckeditor-oss-license
  */
 
+import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import VirtualTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/virtualtesteditor';
 import ModelTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/modeltesteditor';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+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 ImageStyleEditing from '../../src/imagestyle/imagestyleediting';
 import ImageBlockEditing from '../../src/image/imageblockediting';
 import ImageInlineEditing from '../../src/image/imageinlineediting';
 import ImageStyleCommand from '../../src/imagestyle/imagestylecommand';
 import imageStyleUtils from '../../src/imagestyle/utils';
-import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
-
-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 testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
 import ImageEditing from '../../src/image/imageediting';
 import ImageResizeEditing from '../../src/imageresize/imageresizeediting';
+import ImageUtils from '../../src/imageutils';
 
 describe( 'ImageStyleEditing', () => {
 	describe( 'plugin', () => {
@@ -37,6 +37,10 @@ describe( 'ImageStyleEditing', () => {
 			expect( ImageStyleEditing.pluginName ).to.equal( 'ImageStyleEditing' );
 		} );
 
+		it( 'requires ImageUtils ', () => {
+			expect( ImageStyleEditing.requires ).to.deep.equal( [ ImageUtils ] );
+		} );
+
 		afterEach( () => {
 			editor.destroy();
 		} );
@@ -201,6 +205,93 @@ describe( 'ImageStyleEditing', () => {
 		} );
 	} );
 
+	describe( 'model post-fixer', () => {
+		let editor, model, document;
+
+		beforeEach( async () => {
+			editor = await ModelTestEditor.create( {
+				plugins: [ ImageBlockEditing, ImageInlineEditing, ImageStyleEditing, Paragraph ],
+				image: {
+					styles: {
+						options: [
+							{ name: 'forBlock', modelElements: [ 'imageBlock' ] },
+							{ name: 'forInline', modelElements: [ 'imageInline' ] }
+						]
+					}
+				}
+			} );
+
+			model = editor.model;
+			document = model.document;
+		} );
+
+		afterEach( () => {
+			editor.destroy();
+		} );
+
+		it( 'should remove imageStyle attribute with invalid value', () => {
+			setModelData( model, '<imageBlock src="/assets/sample.png" imageStyle="foo"></imageBlock>' );
+
+			const image = document.getRoot().getChild( 0 );
+
+			expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
+		} );
+
+		it( 'should remove imageStyle attribute with invalid value (after changing attribute value)', () => {
+			setModelData( model, '<imageBlock src="/assets/sample.png"></imageBlock>' );
+
+			const image = document.getRoot().getChild( 0 );
+
+			model.change( writer => {
+				writer.setAttribute( 'imageStyle', 'foo', image );
+			} );
+
+			expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
+		} );
+
+		it( 'should remove imageStyle attribute with invalid value (after changing image type)', () => {
+			setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="full"></imageBlock>]' );
+
+			editor.execute( 'imageTypeInline' );
+
+			const image = document.getRoot().getChild( 0 ).getChild( 0 );
+
+			expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
+		} );
+
+		it( 'should remove imageStyle attribute with value not allowed for a block image', () => {
+			setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="forInline"></imageBlock>]' );
+
+			const image = document.getRoot().getChild( 0 );
+
+			expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
+		} );
+
+		it( 'should remove imageStyle attribute with value not allowed for an inline image', () => {
+			setModelData( model, '<paragraph>[<imageInline src="/assets/sample.png" imageStyle="forBlock"></imageInline>]</paragraph>' );
+
+			const image = document.getRoot().getChild( 0 ).getChild( 0 );
+
+			expect( image.hasAttribute( 'imageStyle' ) ).to.be.false;
+		} );
+
+		it( 'should not remove imageStyle attribute with value allowed for a block image', () => {
+			setModelData( model, '[<imageBlock src="/assets/sample.png" imageStyle="forBlock"></imageBlock>]' );
+
+			const image = document.getRoot().getChild( 0 );
+
+			expect( image.getAttribute( 'imageStyle' ) ).to.equal( 'forBlock' );
+		} );
+
+		it( 'should not remove imageStyle attribute with value allowed for an inline image', () => {
+			setModelData( model, '<paragraph>[<imageInline src="/assets/sample.png" imageStyle="forInline"></imageInline>]</paragraph>' );
+
+			const image = document.getRoot().getChild( 0 ).getChild( 0 );
+
+			expect( image.getAttribute( 'imageStyle' ) ).to.equal( 'forInline' );
+		} );
+	} );
+
 	describe( 'conversion', () => {
 		let editor, model, viewDocument, document;