Merge pull request #84 from ckeditor/t/83

Other: Heading dropdown items should never revert the state, apply only. Closes #83.

src/headingcommand.js

@@ -8,9 +8,6 @@
  */
 
 import Command from '@ckeditor/ckeditor5-core/src/command';
-
-import Range from '@ckeditor/ckeditor5-engine/src/model/range';
-import Selection from '@ckeditor/ckeditor5-engine/src/model/selection';
 import Position from '@ckeditor/ckeditor5-engine/src/model/position';
 import first from '@ckeditor/ckeditor5-utils/src/first';
 
@@ -70,9 +67,6 @@ export default class HeadingCommand extends Command {
 		const editor = this.editor;
 		const document = editor.document;
 
-		// If current option is same as new option - toggle already applied option back to default one.
-		const shouldRemove = this.value;
-
 		document.enqueueChanges( () => {
 			const batch = options.batch || document.batch();
 			const blocks = Array.from( document.selection.getSelectedBlocks() )
@@ -81,20 +75,7 @@ export default class HeadingCommand extends Command {
 				} );
 
 			for ( const block of blocks ) {
-				// When removing applied option.
-				if ( shouldRemove ) {
-					if ( block.is( this.modelElement ) ) {
-						// Apply paragraph to the selection withing that particular block only instead
-						// of working on the entire document selection.
-						const selection = new Selection();
-						selection.addRange( Range.createIn( block ) );
-
-						// Share the batch with the paragraph command.
-						editor.execute( 'paragraph', { selection, batch } );
-					}
-				}
-				// When applying new option.
-				else if ( !block.is( this.modelElement ) ) {
+				if ( !block.is( this.modelElement ) ) {
 					batch.rename( block, this.modelElement );
 				}
 			}

tests/headingcommand.js

@@ -148,19 +148,6 @@ describe( 'HeadingCommand', () => {
 
 				expect( batch.deltas.length ).to.be.above( 0 );
 			} );
-
-			it( 'should use provided batch (converting to default option)', () => {
-				const batch = editor.document.batch();
-				const command = commands.heading1;
-
-				setData( document, '<heading1>foo[]bar</heading1>' );
-
-				expect( batch.deltas.length ).to.equal( 0 );
-
-				command.execute( { batch } );
-
-				expect( batch.deltas.length ).to.be.above( 0 );
-			} );
 		} );
 
 		describe( 'collapsed selection', () => {
@@ -171,13 +158,11 @@ describe( 'HeadingCommand', () => {
 				convertTo = option;
 			}
 
-			it( 'converts to default option when executed with already applied option', () => {
-				const command = commands.heading1;
-
+			it( 'does nothing when executed with already applied option', () => {
 				setData( document, '<heading1>foo[]bar</heading1>' );
-				command.execute();
 
-				expect( getData( document ) ).to.equal( '<paragraph>foo[]bar</paragraph>' );
+				commands.heading1.execute();
+				expect( getData( document ) ).to.equal( '<heading1>foo[]bar</heading1>' );
 			} );
 
 			it( 'converts topmost blocks', () => {
@@ -209,20 +194,30 @@ describe( 'HeadingCommand', () => {
 			}
 
 			it( 'converts all elements where selection is applied', () => {
-				setData( document, '<heading1>foo[</heading1><heading2>bar</heading2><heading3>]baz</heading3>' );
+				setData( document, '<heading1>foo[</heading1><heading2>bar</heading2><heading3>baz]</heading3>' );
+
 				commands.heading3.execute();
 
 				expect( getData( document ) ).to.equal(
-					'<heading3>foo[</heading3><heading3>bar</heading3><heading3>]baz</heading3>'
+					'<heading3>foo[</heading3><heading3>bar</heading3><heading3>baz]</heading3>'
+				);
+			} );
+
+			it( 'does nothing to the elements with same option (#1)', () => {
+				setData( document, '<heading1>[foo</heading1><heading1>bar]</heading1>' );
+				commands.heading1.execute();
+
+				expect( getData( document ) ).to.equal(
+					'<heading1>[foo</heading1><heading1>bar]</heading1>'
 				);
 			} );
 
-			it( 'resets to default value all elements with same option', () => {
-				setData( document, '<heading1>foo[</heading1><heading1>bar</heading1><heading2>baz</heading2>]' );
+			it( 'does nothing to the elements with same option (#2)', () => {
+				setData( document, '<heading1>[foo</heading1><heading1>bar</heading1><heading2>baz]</heading2>' );
 				commands.heading1.execute();
 
 				expect( getData( document ) ).to.equal(
-					'<paragraph>foo[</paragraph><paragraph>bar</paragraph><heading2>baz</heading2>]'
+					'<heading1>[foo</heading1><heading1>bar</heading1><heading1>baz]</heading1>'
 				);
 			} );
 

tests/integration.js

@@ -11,6 +11,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Enter from '@ckeditor/ckeditor5-enter/src/enter';
 import Image from '@ckeditor/ckeditor5-image/src/image';
 import ImageCaption from '@ckeditor/ckeditor5-image/src/imagecaption';
+import Undo from '@ckeditor/ckeditor5-undo/src/undo';
 
 import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
 import { getData as getModelData, setData as setModelData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
@@ -23,7 +24,7 @@ describe( 'Heading integration', () => {
 		document.body.appendChild( element );
 
 		return ClassicTestEditor.create( element, {
-			plugins: [ Paragraph, Heading, Enter, Image, ImageCaption ]
+			plugins: [ Paragraph, Heading, Enter, Image, ImageCaption, Undo ]
 		} )
 		.then( newEditor => {
 			editor = newEditor;
@@ -80,4 +81,24 @@ describe( 'Heading integration', () => {
 			);
 		} );
 	} );
+
+	describe( 'with the undo feature', () => {
+		it( 'does not create undo steps when applied to an existing heading (collapsed selection)', () => {
+			setModelData( editor.document, '<heading1>foo[]bar</heading1>' );
+
+			editor.execute( 'heading1' );
+			expect( getModelData( editor.document ) ).to.equal( '<heading1>foo[]bar</heading1>' );
+
+			expect( editor.commands.get( 'undo' ).isEnabled ).to.be.false;
+		} );
+
+		it( 'does not create undo steps when applied to an existing heading (non–collapsed selection)', () => {
+			setModelData( editor.document, '<heading1>[foo</heading1><heading1>bar]</heading1>' );
+
+			editor.execute( 'heading1' );
+			expect( getModelData( editor.document ) ).to.equal( '<heading1>[foo</heading1><heading1>bar]</heading1>' );
+
+			expect( editor.commands.get( 'undo' ).isEnabled ).to.be.false;
+		} );
+	} );
 } );