From 1ac58d2a479b8eee8490ea089575c9350174398a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Fri, 24 Mar 2017 12:55:42 +0100 Subject: [PATCH 1/3] Updated value and isEnabled calculation in heading command. --- src/headingcommand.js | 31 +++++++++++++++++---- tests/heading.js | 2 +- tests/headingcommand.js | 62 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 89 insertions(+), 6 deletions(-) diff --git a/src/headingcommand.js b/src/headingcommand.js index 846a01c..a276c61 100644 --- a/src/headingcommand.js +++ b/src/headingcommand.js @@ -39,7 +39,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._updateValue(); + this.refreshState(); + } ); /** * Unique identifier of the command, also element's name in the model. @@ -111,11 +114,29 @@ export default class HeadingCommand extends Command { * @private */ _updateValue() { - const block = this.editor.document.selection.getSelectedBlocks().next().value; + const block = this._getSelectedBlock(); + + this.value = !!block && block.is( this.modelElement ); + } + + /** + * @inheritDoc + */ + _checkEnabled() { + const block = this._getSelectedBlock(); + const schema = this.editor.document.schema; - if ( block ) { - this.value = block.is( this.modelElement ); - } + return !!block && schema.check( { name: this.modelElement, inside: block.parent.name } ) && !schema.objects.has( block.name ); + } + + /** + * Returns currently selected block. + * + * @private + * @returns {module:engine/model/element~Element} + */ + _getSelectedBlock() { + return this.editor.document.selection.getSelectedBlocks().next().value; } } diff --git a/tests/heading.js b/tests/heading.js index 4d5dc64..5a9889e 100644 --- a/tests/heading.js +++ b/tests/heading.js @@ -58,7 +58,7 @@ describe( 'Heading', () => { const dropdown = editor.ui.componentFactory.create( 'headings' ); expect( dropdown ).to.be.instanceOf( DropdownView ); - expect( dropdown.buttonView.isEnabled ).to.be.true; + expect( dropdown.buttonView.isEnabled ).to.be.false; expect( dropdown.buttonView.isOn ).to.be.undefined; expect( dropdown.buttonView.label ).to.equal( 'Choose heading' ); expect( dropdown.buttonView.tooltip ).to.equal( 'Heading' ); diff --git a/tests/headingcommand.js b/tests/headingcommand.js index 26dcdb0..35dfcc1 100644 --- a/tests/headingcommand.js +++ b/tests/headingcommand.js @@ -33,6 +33,15 @@ describe( 'HeadingCommand', () => { schema.registerItem( option.modelElement, '$block' ); } + schema.registerItem( 'notBlock' ); + schema.allow( { name: 'notBlock', inside: '$root' } ); + schema.allow( { name: '$text', inside: 'notBlock' } ); + + schema.registerItem( 'object' ); + schema.allow( { name: 'object', inside: '$root' } ); + schema.allow( { name: '$text', inside: 'object' } ); + schema.objects.add( 'object' ); + root = document.getRoot(); } ); } ); @@ -70,6 +79,19 @@ describe( 'HeadingCommand', () => { expect( commands[ modelElement ].value ).to.be.true; } ); + + it( `equals false if moved from ${ modelElement } to non-block element`, () => { + setData( document, `<${ modelElement }>[foo]foo` ); + const element = document.getRoot().getChild( 1 ); + + expect( commands[ modelElement ].value ).to.be.true; + + document.enqueueChanges( () => { + document.selection.setRanges( [ Range.createIn( element ) ] ); + } ); + + expect( commands[ modelElement ].value ).to.be.false; + } ); } } ); @@ -191,4 +213,44 @@ 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, 'f{}oo' ); + + expect( command.isEnabled ).to.be.true; + } ); + + it( 'should be disabled if inside non-block', () => { + setData( document, 'f{}oo' ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be disabled if inside object', () => { + setData( document, 'f{}oo' ); + + expect( command.isEnabled ).to.be.false; + } ); + + it( 'should be disabled if selection is placed on object', () => { + setData( document, '[foo]' ); + + expect( command.isEnabled ).to.be.false; + } ); + } ); + } + } ); } ); From 5182132a49d6915799f98f6fa4bfc8325e0f78b5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Fri, 24 Mar 2017 13:35:39 +0100 Subject: [PATCH 2/3] Fixed schema and tests in heading command. --- src/headingcommand.js | 27 ++++++++++++++------------- tests/heading.js | 12 ++++++++---- tests/headingcommand.js | 8 ++++++-- 3 files changed, 28 insertions(+), 19 deletions(-) diff --git a/src/headingcommand.js b/src/headingcommand.js index a276c61..540e887 100644 --- a/src/headingcommand.js +++ b/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. @@ -114,7 +116,7 @@ export default class HeadingCommand extends Command { * @private */ _updateValue() { - const block = this._getSelectedBlock(); + const block = first( this.editor.document.selection.getSelectedBlocks() ); this.value = !!block && block.is( this.modelElement ); } @@ -123,20 +125,19 @@ export default class HeadingCommand extends Command { * @inheritDoc */ _checkEnabled() { - const block = this._getSelectedBlock(); - const schema = this.editor.document.schema; + const block = first( this.editor.document.selection.getSelectedBlocks() ); - return !!block && schema.check( { name: this.modelElement, inside: block.parent.name } ) && !schema.objects.has( block.name ); - } + if ( !block ) { + return false; + } - /** - * Returns currently selected block. - * - * @private - * @returns {module:engine/model/element~Element} - */ - _getSelectedBlock() { - return this.editor.document.selection.getSelectedBlocks().next().value; + const schema = this.editor.document.schema; + const isAllowed = schema.check( { + name: this.modelElement, + inside: Position.createBefore( block ) + } ); + + return isAllowed && !schema.objects.has( block.name ); } } diff --git a/tests/heading.js b/tests/heading.js index 5a9889e..bbd9f46 100644 --- a/tests/heading.js +++ b/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, 'f{}oo' ); } ); } ); @@ -56,11 +60,11 @@ describe( 'Heading', () => { describe( 'init()', () => { it( 'should register options feature component', () => { const dropdown = editor.ui.componentFactory.create( 'headings' ); - + // expect( dropdown ).to.be.instanceOf( DropdownView ); - expect( dropdown.buttonView.isEnabled ).to.be.false; + 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, 'f{}oo' ); expect( listView.items.map( item => item.isActive ) ).to.deep.equal( [ false, diff --git a/tests/headingcommand.js b/tests/headingcommand.js index 35dfcc1..fc7cbe8 100644 --- a/tests/headingcommand.js +++ b/tests/headingcommand.js @@ -80,12 +80,16 @@ describe( 'HeadingCommand', () => { expect( commands[ modelElement ].value ).to.be.true; } ); + it( `equals false if inside to non-block element`, () => { + setData( document, `[foo]` ); + + expect( commands[ modelElement ].value ).to.be.false; + } ); + it( `equals false if moved from ${ modelElement } to non-block element`, () => { setData( document, `<${ modelElement }>[foo]foo` ); const element = document.getRoot().getChild( 1 ); - expect( commands[ modelElement ].value ).to.be.true; - document.enqueueChanges( () => { document.selection.setRanges( [ Range.createIn( element ) ] ); } ); From f6dbed5c0fb67500996c090c637ac84cfbf2d773 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Szymon=20Kup=C5=9B?= Date: Tue, 28 Mar 2017 15:24:35 +0200 Subject: [PATCH 3/3] Renamed HeadingCommand's private method _updateValue to public refreshValue. Removed schema.objects checking from _checkEnabled method. --- src/headingcommand.js | 15 +++------------ tests/heading.js | 2 +- tests/headingcommand.js | 28 +++++++++++++++------------- 3 files changed, 19 insertions(+), 26 deletions(-) diff --git a/src/headingcommand.js b/src/headingcommand.js index 540e887..8ad8854 100644 --- a/src/headingcommand.js +++ b/src/headingcommand.js @@ -42,7 +42,7 @@ export default class HeadingCommand extends Command { // Update current value each time changes are done on document. this.listenTo( editor.document, 'changesDone', () => { - this._updateValue(); + this.refreshValue(); this.refreshState(); } ); @@ -112,10 +112,8 @@ export default class HeadingCommand extends Command { /** * Updates command's {@link #value value} based on current selection. - * - * @private */ - _updateValue() { + refreshValue() { const block = first( this.editor.document.selection.getSelectedBlocks() ); this.value = !!block && block.is( this.modelElement ); @@ -127,17 +125,10 @@ export default class HeadingCommand extends Command { _checkEnabled() { const block = first( this.editor.document.selection.getSelectedBlocks() ); - if ( !block ) { - return false; - } - - const schema = this.editor.document.schema; - const isAllowed = schema.check( { + return !!block && this.editor.document.schema.check( { name: this.modelElement, inside: Position.createBefore( block ) } ); - - return isAllowed && !schema.objects.has( block.name ); } } diff --git a/tests/heading.js b/tests/heading.js index bbd9f46..f02912c 100644 --- a/tests/heading.js +++ b/tests/heading.js @@ -60,7 +60,7 @@ describe( 'Heading', () => { describe( 'init()', () => { it( 'should register options feature component', () => { const dropdown = editor.ui.componentFactory.create( 'headings' ); - // + expect( dropdown ).to.be.instanceOf( DropdownView ); expect( dropdown.buttonView.isEnabled ).to.be.true; expect( dropdown.buttonView.isOn ).to.be.undefined; diff --git a/tests/headingcommand.js b/tests/headingcommand.js index fc7cbe8..e348b3c 100644 --- a/tests/headingcommand.js +++ b/tests/headingcommand.js @@ -37,11 +37,6 @@ describe( 'HeadingCommand', () => { schema.allow( { name: 'notBlock', inside: '$root' } ); schema.allow( { name: '$text', inside: 'notBlock' } ); - schema.registerItem( 'object' ); - schema.allow( { name: 'object', inside: '$root' } ); - schema.allow( { name: '$text', inside: 'object' } ); - schema.objects.add( 'object' ); - root = document.getRoot(); } ); } ); @@ -96,6 +91,19 @@ describe( 'HeadingCommand', () => { expect( commands[ modelElement ].value ).to.be.false; } ); + + it( 'should be refreshed after calling refreshValue()', () => { + const command = commands[ modelElement ]; + setData( document, `<${ modelElement }>[foo]foo` ); + 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; + } ); } } ); @@ -243,14 +251,8 @@ describe( 'HeadingCommand', () => { expect( command.isEnabled ).to.be.false; } ); - it( 'should be disabled if inside object', () => { - setData( document, 'f{}oo' ); - - expect( command.isEnabled ).to.be.false; - } ); - - it( 'should be disabled if selection is placed on object', () => { - setData( document, '[foo]' ); + it( 'should be disabled if selection is placed on non-block', () => { + setData( document, '[foo]' ); expect( command.isEnabled ).to.be.false; } );