Table rows will not be added on tab key press if the associated command is disabled #186
Conversation
…command is disabled.
tests/tableediting.js
Outdated
| @@ -294,6 +294,22 @@ describe( 'TableEditing', () => { | |||
| ] ) ); | |||
| } ); | |||
|
|
|||
| it( 'should not create another row and not move the caret if insertTableRowBelow command is disabled', () => { | |||
| setModelData( model, modelTable( [ | |||
| [ '11', '[12]' ] | |||
There was a problem hiding this comment.
maybe test should include also testing the selection so it should start with collapsed and should stay collapsed (not changed) after the tab.
There was a problem hiding this comment.
I copied this from the test above. I can change it to a collapsed selection.
src/tableediting.js
Outdated
| editor.plugins.get( 'TableUtils' ).insertRows( table, { at: table.childCount } ); | ||
| editor.execute( 'insertTableRowBelow' ); | ||
|
|
||
| // Re-evaluate `isLastRow`. If `insertTableRowBelow` execution didn't add any row (because it was disabled or it got |
There was a problem hiding this comment.
Too complex? Maybe check if the command is enabled - if not just return.
There was a problem hiding this comment.
I am not sure what is the case for "got it overwritten in some way" part so maybe I'm wrong here.
But still if the number of rows didn't change I'd exit here rather then in focus condition below.
There was a problem hiding this comment.
That was my first idea, but I wanted to check the content state which is the ultimate source of truth and the indicator of what to do next.
By "got overwritten" I mean that commands behavior (execution) may be overwritten by other features. The command might be enabled but for some reason, it might not add a row. Track changes overwrites commands a lot and some of them do not change the content after execution. I am not yet sure how integration with tables will look like (now you can only add a table but table utils are not supported). So I think I am on a safe side here with the way I coded it.
There was a problem hiding this comment.
So the part about earlier abort still applies - just return here not below. (number of rows remains the same). WDYT?
There was a problem hiding this comment.
But I agree that this is confusing. I am not a big fan of this approach but still, I'd rather be safe than sorry :P.
src/tableediting.js
Outdated
| } | ||
|
|
||
| let cellToFocus; | ||
|
|
||
| // Move to first cell in next row. | ||
| if ( isForward && isLastCellInRow ) { | ||
| if ( isLastRow ) { |
There was a problem hiding this comment.
I don't like the if in if here. I'd move that check as if( !command.isEnabled ) { return } to the previous check of inserting table row.
Suggested merge commit message (convention)
Fix: Table rows will not be added on tab key press if the associated command is disabled. Closes ckeditor/ckeditor5#3268.