I/6406: Fix logic behind removing multiple columns or rows in a row #286
Conversation
|
I'm not going to lie to you – this is not something I want to verify by analysing the implementation 🤣I've briefly scanned the code but that's it. cc @ckeditor/qa-team, could you go through the various cases that were failing recently and perhaps some more? |
Sure - the change was minimal actually - regenerate |
|
Sometimes when the table has complex geometry and you remove a row ( with multiselect or not ), cells are changing their position in table:
|
If this happens for one row selection then it looks like a bug in the original remove row algorithm. So if nothing similar happens I'm confident that this change is OK. I'd move the above case as a general table issue because it looks like an edge case as those spans are tough. BTW, thanks for coloured samples - it is so easy to spot what's happening in the case :) |
|
We've tested it and changes look good 👍 |
src/commands/removerowcommand.js
Outdated
| const rowIndexes = tableMap.filter( entry => selectedCells.includes( entry.cell ) ).map( el => el.row ); | ||
| const minRowIndex = rowIndexes[ 0 ]; | ||
| const maxRowIndex = rowIndexes[ rowIndexes.length - 1 ]; | ||
| const removedRowIndexes = getRowIndexes( selectedCells ); |
There was a problem hiding this comment.
These cells are not yet removed, these are selected cells that are about to be removed.
Suggested merge commit message (convention)
Fix: Recalculate table geometry on each row or column removal. Closes ckeditor/ckeditor5#6406.
Additional information
I've noticed that the
tableMapwas passed but it is static. You must get the table map after each row/column removal for loop operations otherwise the remove row/column algorithm would work on obsolete data.