Merge pull request #70 from ckeditor/t/66

Fix: Drop-down should be inactive when none of the commands can be applied to the current selection. Closes #66.

src/headingcommand.js

@@ -10,6 +10,8 @@
 import Range from '@ckeditor/ckeditor5-engine/src/model/range';
 import Command from '@ckeditor/ckeditor5-core/src/command/command';
 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';
 
 /**
  * The heading command. It is used by the {@link module:heading/heading~Heading heading feature} to apply headings.
@@ -39,7 +41,10 @@ export default class HeadingCommand extends Command {
 		this.set( 'value', false );
 
 		// Update current value each time changes are done on document.
-		this.listenTo( editor.document, 'changesDone', () => this._updateValue() );
+		this.listenTo( editor.document, 'changesDone', () => {
+			this.refreshValue();
+			this.refreshState();
+		} );
 
 		/**
 		 * Unique identifier of the command, also element's name in the model.
@@ -107,15 +112,23 @@ export default class HeadingCommand extends Command {
 
 	/**
 	 * Updates command's {@link #value value} based on current selection.
-	 *
-	 * @private
 	 */
-	_updateValue() {
-		const block = this.editor.document.selection.getSelectedBlocks().next().value;
+	refreshValue() {
+		const block = first( this.editor.document.selection.getSelectedBlocks() );
+
+		this.value = !!block && block.is( this.modelElement );
+	}
+
+	/**
+	 * @inheritDoc
+	 */
+	_checkEnabled() {
+		const block = first( this.editor.document.selection.getSelectedBlocks() );
 
-		if ( block ) {
-			this.value = block.is( this.modelElement );
-		}
+		return !!block && this.editor.document.schema.check( {
+			name: this.modelElement,
+			inside: Position.createBefore( block )
+		} );
 	}
 }
 

tests/heading.js

@@ -11,6 +11,7 @@ import HeadingEngine from '../src/headingengine';
 import DropdownView from '@ckeditor/ckeditor5-ui/src/dropdown/dropdownview';
 import { add } from '@ckeditor/ckeditor5-utils/src/translation-service';
 import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+import { setData } from '@ckeditor/ckeditor5-engine/src/dev-utils/model';
 
 add( 'pl', {
 	'Choose heading': 'Wybierz nagłówek',
@@ -36,6 +37,9 @@ describe( 'Heading', () => {
 		.then( newEditor => {
 			editor = newEditor;
 			dropdown = editor.ui.componentFactory.create( 'headings' );
+
+			// Set data so the commands will be enabled.
+			setData( editor.document, '<paragraph>f{}oo</paragraph>' );
 		} );
 	} );
 
@@ -60,7 +64,7 @@ describe( 'Heading', () => {
 			expect( dropdown ).to.be.instanceOf( DropdownView );
 			expect( dropdown.buttonView.isEnabled ).to.be.true;
 			expect( dropdown.buttonView.isOn ).to.be.undefined;
-			expect( dropdown.buttonView.label ).to.equal( 'Choose heading' );
+			expect( dropdown.buttonView.label ).to.equal( 'Paragraph' );
 			expect( dropdown.buttonView.tooltip ).to.equal( 'Heading' );
 		} );
 
@@ -236,7 +240,7 @@ describe( 'Heading', () => {
 			it( 'reflects the #value of the commands', () => {
 				const listView = dropdown.listView;
 
-				editor.commands.get( 'heading2' ).value = true;
+				setData( editor.document, '<heading2>f{}oo</heading2>' );
 
 				expect( listView.items.map( item => item.isActive ) ).to.deep.equal( [
 					false,

tests/headingcommand.js

@@ -33,6 +33,10 @@ describe( 'HeadingCommand', () => {
 				schema.registerItem( option.modelElement, '$block' );
 			}
 
+			schema.registerItem( 'notBlock' );
+			schema.allow( { name: 'notBlock', inside: '$root' } );
+			schema.allow( { name: '$text', inside: 'notBlock' } );
+
 			root = document.getRoot();
 		} );
 	} );
@@ -70,6 +74,36 @@ describe( 'HeadingCommand', () => {
 
 				expect( commands[ modelElement ].value ).to.be.true;
 			} );
+
+			it( `equals false if inside to non-block element`, () => {
+				setData( document, `<notBlock>[foo]</notBlock>` );
+
+				expect( commands[ modelElement ].value ).to.be.false;
+			} );
+
+			it( `equals false if moved from ${ modelElement } to non-block element`, () => {
+				setData( document, `<${ modelElement }>[foo]</${ modelElement }><notBlock>foo</notBlock>` );
+				const element = document.getRoot().getChild( 1 );
+
+				document.enqueueChanges( () => {
+					document.selection.setRanges( [ Range.createIn( element ) ] );
+				} );
+
+				expect( commands[ modelElement ].value ).to.be.false;
+			} );
+
+			it( 'should be refreshed after calling refreshValue()', () => {
+				const command = commands[ modelElement ];
+				setData( document, `<${ modelElement }>[foo]</${ modelElement }><notBlock>foo</notBlock>` );
+				const element = document.getRoot().getChild( 1 );
+
+				// Purposely not putting it in `document.enqueueChanges` to update command manually.
+				document.selection.setRanges( [ Range.createIn( element ) ] );
+
+				expect( command.value ).to.be.true;
+				command.refreshValue();
+				expect( command.value ).to.be.false;
+			} );
 		}
 	} );
 
@@ -191,4 +225,38 @@ describe( 'HeadingCommand', () => {
 			}
 		} );
 	} );
+
+	describe( 'isEnabled', () => {
+		for ( let option of options ) {
+			test( option.modelElement );
+		}
+
+		function test( modelElement ) {
+			let command;
+
+			beforeEach( () => {
+				command = commands[ modelElement ];
+			} );
+
+			describe( `${ modelElement } command`, () => {
+				it( 'should be enabled when inside another block', () => {
+					setData( document, '<paragraph>f{}oo</paragraph>' );
+
+					expect( command.isEnabled ).to.be.true;
+				} );
+
+				it( 'should be disabled if inside non-block', () => {
+					setData( document, '<notBlock>f{}oo</notBlock>' );
+
+					expect( command.isEnabled ).to.be.false;
+				} );
+
+				it( 'should be disabled if selection is placed on non-block', () => {
+					setData( document, '[<notBlock>foo</notBlock>]' );
+
+					expect( command.isEnabled ).to.be.false;
+				} );
+			} );
+		}
+	} );
 } );