Merge pull request #133 from ckeditor/t/132

Fix: ImageStyleCommand should switch properly between any two non-null styles. Closes #132.

Author: szymonkups

src/imagestyle/converters.js

@@ -30,7 +30,11 @@ export function modelToViewStyleAttribute( styles ) {
 		const oldStyle = getStyleByValue( data.attributeOldValue, styles );
 		const viewElement = conversionApi.mapper.toViewElement( data.item );
 
-		if ( handleRemoval( eventType, oldStyle, viewElement ) || handleAddition( eventType, newStyle, viewElement ) ) {
+		const isRemovalHandled = handleRemoval( eventType, oldStyle, viewElement );
+		const isAdditionHandled = handleAddition( eventType, newStyle, viewElement );
+
+		// https://github.com/ckeditor/ckeditor5-image/issues/132
+		if ( isRemovalHandled || isAdditionHandled ) {
 			consumable.consume( data.item, consumableType );
 		}
 	};

tests/imagestyle/imagestyleengine.js

@@ -56,25 +56,36 @@ describe( 'ImageStyleEngine', () => {
 		editor.setData( '<figure class="image side-class"><img src="foo.png" /></figure>' );
 
 		expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image imageStyle="side" src="foo.png"></image>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget side-class" contenteditable="false">' +
+				'<img src="foo.png"></img>' +
+			'</figure>' );
 	} );
 
 	it( 'should not convert from view to model if class is not defined', () => {
 		editor.setData( '<figure class="image foo-bar"><img src="foo.png" /></figure>' );
 
 		expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image src="foo.png"></image>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should not convert from view to model when not in image figure', () => {
 		editor.setData( '<figure class="side-class"></figure>' );
 
 		expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal( '' );
 	} );
 
 	it( 'should not convert from view to model if schema prevents it', () => {
 		document.schema.disallow( { name: 'image', attributes: 'imageStyle' } );
 		editor.setData( '<figure class="image side-class"><img src="foo.png" /></figure>' );
 
 		expect( getModelData( document, { withoutSelection: true } ) ).to.equal( '<image src="foo.png"></image>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should convert model to view: adding attribute', () => {
@@ -87,6 +98,9 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image side-class"><img src="foo.png"></figure>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget side-class" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should convert model to view: removing attribute', () => {
@@ -99,6 +113,9 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image"><img src="foo.png"></figure>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should convert model to view: change attribute', () => {
@@ -111,6 +128,22 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image side-class"><img src="foo.png"></figure>' );
+
+		// https://github.com/ckeditor/ckeditor5-image/issues/132
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget side-class" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
+
+		document.enqueueChanges( () => {
+			batch.setAttribute( image, 'imageStyle', 'dummy' );
+		} );
+
+		expect( editor.getData() ).to.equal( '<figure class="image dummy-class"><img src="foo.png"></figure>' );
+
+		// https://github.com/ckeditor/ckeditor5-image/issues/132
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget dummy-class" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should not convert from model to view if already consumed: adding attribute', () => {
@@ -177,6 +210,9 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image"><img src="foo.png"></figure>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should not convert from model to view if style is not present: change attribute', () => {
@@ -189,6 +225,9 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image"><img src="foo.png"></figure>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 
 	it( 'should not convert from model to view if style is not present: remove attribute', () => {
@@ -201,5 +240,8 @@ describe( 'ImageStyleEngine', () => {
 		} );
 
 		expect( editor.getData() ).to.equal( '<figure class="image"><img src="foo.png"></figure>' );
+		expect( getViewData( viewDocument, { withoutSelection: true } ) ).to.equal(
+			'<figure class="image ck-widget" contenteditable="false"><img src="foo.png"></img></figure>'
+		);
 	} );
 } );