Merge pull request #13263 from ckeditor/ck/13262-table-property-window

Fix (table): Table and cell property commands were executed when the UI dialog opened.

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;
  }
 

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;
  }
 

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();

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' );