The findOptimalInsertionPosition() should use getTopMostBlocks() instead getSelectedBlocks()

[jodator]

I'm still battling with a more generic issue but it seems that selection.getSelectedBlocks() is causing some problems (https://github.com/ckeditor/ckeditor5-indent/issues/11, https://github.com/ckeditor/ckeditor5-table/issues/126 and the latest: https://github.com/ckeditor/ckeditor5-table/issues/199).

Generally speaking the below use of this method:

const firstBlockInSelection = model.selection.getSelectedBlocks().next().value

in most cases will return the first block but in case of tables it will return first inner child of a parent block and the table.

So if a selection contains a table and a heading it will return:

["paragraph", "paragraph", "paragraph", "paragraph", "table", "heading1"]

For some algorithms (most?) we want to have top-most blocks (so the selection.getTopMostBlocks() should be used. Which would return only:

["table", "heading1"]

@Reinmar I didn't dig much into it looks like most similar issues are related to that algorithm so either:

1. The selection.getSelectedBlocks() should return parent first
2. (preferable) We should use selection.getTopMostBlocks() rather than selection.getSelectedBlocks().

[jodator]

ps.: Changing the selection.getSelectedBlocks() to selection.getTopMostBlocks() does fix the issue in tables (ckeditor/ckeditor5-table#199) and does not break the tests so it might be quick fix.

[Reinmar]

Haha :D Check out the history and timing of this issue: https://github.com/ckeditor/ckeditor5-engine/issues/826

[Reinmar]

I didn't know that we introduced getTopMostBlocks() and unfortunately I think it was wrong. For two reasons:

• The role of getSelectedBlocks() was to be useful in those cases. It was introduced to be used in those features because most of them work in the same way. If we now see that most of them need to work differently than the current implementation of getSelectedBlocks() then, IMO, we should be tuning the original method for that.
• If we see use cases for returning all blocks inside the selection (even nested ones), then I think this should be a param of the original method.

cc @scofalik

[Reinmar]

@scofalik shares the same sentiments. Block quote may be a use case for the current behaviour of getSelectedBlocks() so most likely we need to keep it. It also makes sense to not remove a functionality that we had. But we can make it a non-default one, the default being the top most blocks.

[jodator]

I couldn't find any feature that uses that deep structure, to be honest. The block quote feature actually uses the getTopMostBlocks() now.

After renaming getTopMostBlocks() to getSelectedBlocks() and removing the latter everything passes except for the tests for getSelectedBlocks()... I'm not sure if this is used anywhere.