diff --git a/packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesui.js b/packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesui.js index 9a22248274d..bb08f895dc6 100644 --- a/packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesui.js +++ b/packages/ckeditor5-table/src/tablecellproperties/tablecellpropertiesui.js @@ -124,6 +124,15 @@ export default class TableCellPropertiesUI extends Plugin { */ this._undoStepBatch = null; + /** + * Flag used to indicate whether view is ready to execute update commands + * (it finished loading initial data). + * + * @private + * @member {Boolean} + */ + this._isReady = false; + editor.ui.componentFactory.add( 'tableCellProperties', locale => { const view = new ButtonView( locale ); @@ -312,6 +321,8 @@ export default class TableCellPropertiesUI extends Plugin { this.view.set( property, value ); } ); + + this._isReady = true; } /** @@ -359,6 +370,8 @@ export default class TableCellPropertiesUI extends Plugin { this.stopListening( editor.ui, 'update' ); + this._isReady = false; + // Blur any input element before removing it from DOM to prevent issues in some browsers. // See https://github.com/ckeditor/ckeditor5/issues/1501. this.view.saveButtonView.focus(); @@ -412,14 +425,12 @@ export default class TableCellPropertiesUI extends Plugin { * * @private * @param {String} commandName - * @param {String} defaultValue The default value of the command. * @returns {Function} */ - _getPropertyChangeCallback( commandName, defaultValue ) { - return ( evt, propertyName, newValue, oldValue ) => { - // If the "oldValue" is missing and "newValue" is set to the default value, do not execute the command. - // It is an initial call (when opening the table properties view). - if ( !oldValue && defaultValue === newValue ) { + _getPropertyChangeCallback( commandName ) { + return ( evt, propertyName, newValue ) => { + // Do not execute the command on initial call (opening the table properties view). + if ( !this._isReady ) { return; } @@ -441,21 +452,18 @@ export default class TableCellPropertiesUI extends Plugin { * @param {module:ui/view~View} options.viewField * @param {Function} options.validator * @param {String} options.errorText - * @param {String} options.defaultValue * @returns {Function} */ _getValidatedPropertyChangeCallback( options ) { - const { commandName, viewField, validator, errorText, defaultValue } = options; + const { commandName, viewField, validator, errorText } = options; const setErrorTextDebounced = debounce( () => { viewField.errorText = errorText; }, ERROR_TEXT_TIMEOUT ); - return ( evt, propertyName, newValue, oldValue ) => { + return ( evt, propertyName, newValue ) => { setErrorTextDebounced.cancel(); - - // If the "oldValue" is missing and "newValue" is set to the default value, do not execute the command. - // It is an initial call (when opening the table properties view). - if ( !oldValue && defaultValue === newValue ) { + // Do not execute the command on initial call (opening the table properties view). + if ( !this._isReady ) { return; } diff --git a/packages/ckeditor5-table/src/tableproperties/tablepropertiesui.js b/packages/ckeditor5-table/src/tableproperties/tablepropertiesui.js index a5c4a64335d..7e86cdd4892 100644 --- a/packages/ckeditor5-table/src/tableproperties/tablepropertiesui.js +++ b/packages/ckeditor5-table/src/tableproperties/tablepropertiesui.js @@ -116,6 +116,15 @@ export default class TablePropertiesUI extends Plugin { */ this._undoStepBatch = null; + /** + * Flag used to indicate whether view is ready to execute update commands + * (it finished loading initial data). + * + * @private + * @member {Boolean} + */ + this._isReady = false; + editor.ui.componentFactory.add( 'tableProperties', locale => { const view = new ButtonView( locale ); @@ -292,6 +301,8 @@ export default class TablePropertiesUI extends Plugin { this.view.set( property, value ); } ); + + this._isReady = true; } /** @@ -339,6 +350,8 @@ export default class TablePropertiesUI extends Plugin { this.stopListening( editor.ui, 'update' ); + this._isReady = false; + // Blur any input element before removing it from DOM to prevent issues in some browsers. // See https://github.com/ckeditor/ckeditor5/issues/1501. this.view.saveButtonView.focus(); @@ -394,14 +407,12 @@ export default class TablePropertiesUI extends Plugin { * * @private * @param {String} commandName The command that will be executed. - * @param {String} defaultValue The default value of the command. * @returns {Function} */ - _getPropertyChangeCallback( commandName, defaultValue ) { - return ( evt, propertyName, newValue, oldValue ) => { - // If the "oldValue" is missing and "newValue" is set to the default value, do not execute the command. - // It is an initial call (when opening the table properties view). - if ( !oldValue && defaultValue === newValue ) { + _getPropertyChangeCallback( commandName ) { + return ( evt, propertyName, newValue ) => { + // Do not execute the command on initial call (opening the table properties view). + if ( !this._isReady ) { return; } @@ -423,21 +434,19 @@ export default class TablePropertiesUI extends Plugin { * @param {module:ui/view~View} options.viewField * @param {Function} options.validator * @param {String} options.errorText - * @param {String} options.defaultValue * @returns {Function} */ _getValidatedPropertyChangeCallback( options ) { - const { commandName, viewField, validator, errorText, defaultValue } = options; + const { commandName, viewField, validator, errorText } = options; const setErrorTextDebounced = debounce( () => { viewField.errorText = errorText; }, ERROR_TEXT_TIMEOUT ); - return ( evt, propertyName, newValue, oldValue ) => { + return ( evt, propertyName, newValue ) => { setErrorTextDebounced.cancel(); - // If the "oldValue" is missing and "newValue" is set to the default value, do not execute the command. - // It is an initial call (when opening the table properties view). - if ( !oldValue && defaultValue === newValue ) { + // Do not execute the command on initial call (opening the table properties view). + if ( !this._isReady ) { return; } diff --git a/packages/ckeditor5-table/tests/tablecellproperties/tablecellpropertiesui.js b/packages/ckeditor5-table/tests/tablecellproperties/tablecellpropertiesui.js index 21039a622bd..a542c665023 100644 --- a/packages/ckeditor5-table/tests/tablecellproperties/tablecellpropertiesui.js +++ b/packages/ckeditor5-table/tests/tablecellproperties/tablecellpropertiesui.js @@ -295,6 +295,16 @@ describe( 'table cell properties', () => { expect( contextualBalloon.visibleView ).to.equal( tableCellPropertiesView ); } ); + it( 'should not reposition if view is not visible', () => { + const spy = sinon.spy( contextualBalloon, 'updatePosition' ); + + tableCellPropertiesButton.fire( 'execute' ); + tableCellPropertiesUI.view = false; + editor.ui.fire( 'update' ); + + expect( spy.called ).to.be.false; + } ); + it( 'should hide if clicked outside the balloon', () => { tableCellPropertiesButton.fire( 'execute' ); tableCellPropertiesView = tableCellPropertiesUI.view; @@ -605,6 +615,20 @@ describe( 'table cell properties', () => { } ); describe( 'initial data', () => { + it( 'should not execute commands before changing the data', () => { + const tableCellBackgroundCommand = editor.commands.get( 'tableCellBackgroundColor' ); + const spy = sinon.spy( tableCellBackgroundCommand, 'execute' ); + + tableCellPropertiesUI._showView(); + tableCellPropertiesView = tableCellPropertiesUI.view; + + expect( spy.called ).to.be.false; + + tableCellPropertiesView.backgroundColor = 'red'; + + expect( spy.called ).to.be.true; + } ); + it( 'should be set before adding the form to the the balloon to avoid unnecessary input animations', () => { // Trigger lazy init. tableCellPropertiesUI._showView(); diff --git a/packages/ckeditor5-table/tests/tableproperties/tablepropertiesui.js b/packages/ckeditor5-table/tests/tableproperties/tablepropertiesui.js index 26430350355..72f02834e9c 100644 --- a/packages/ckeditor5-table/tests/tableproperties/tablepropertiesui.js +++ b/packages/ckeditor5-table/tests/tableproperties/tablepropertiesui.js @@ -569,6 +569,20 @@ describe( 'table properties', () => { } ); describe( 'initial data', () => { + it( 'should not execute commands before changing the data', () => { + const tableBackgroundCommand = editor.commands.get( 'tableBackgroundColor' ); + const spy = sinon.spy( tableBackgroundCommand, 'execute' ); + + tablePropertiesUI._showView(); + tablePropertiesView = tablePropertiesUI.view; + + expect( spy.called ).to.be.false; + + tablePropertiesView.backgroundColor = 'red'; + + expect( spy.called ).to.be.true; + } ); + it( 'should be set before adding the form to the the balloon to avoid unnecessary input animations', () => { // Trigger lazy init. tablePropertiesUI._showView(); @@ -707,6 +721,15 @@ describe( 'table properties', () => { sinon.assert.calledOnce( spy ); } ); + it( 'should not reposition the baloon if view is not visible', () => { + const spy = sinon.spy( contextualBalloon, 'updatePosition' ); + + tablePropertiesUI.view = false; + editor.ui.fire( 'update' ); + + expect( spy.called ).to.be.false; + } ); + it( 'should hide the view and not reposition the balloon if table is no longer selected', () => { const positionSpy = sinon.spy( contextualBalloon, 'updatePosition' ); const hideSpy = sinon.spy( tablePropertiesUI, '_hideView' );