Inserted blocks should be positioned the same way across the features

[pjasiun]

Since I recently reported a bug in tables and media embed features I believe we need to establish some rules how block widgets like tables, image or media should be inserted.

What is clear for me:

• if the position is at the beginning of the paragraph then the block should be inserted before the paragraph,
• if the position is at the end of the paragraph then the block should be inserted after the paragraph,
• if the position is in the empty paragraph then the block should replace the paragraph.

The question is: what should happen if the position is in the middle of the block. In the image feature we agreed that it will be better to insert it before the paragraph because of 2 reasons:

• if you change the image to be is a side image you need to insert it before the paragraph to make it an illustration to this paragraph, so the image is somehow connected with the next, not previous paragraph,
• inserting image before the paragraph works well for the block toolbar, which is displayed at the beginning of the paragraph (however, I believe we should be able to force it in the block toolbar, we can move the position to the beginning of the paragraph before executing command, if we need to).

Correct:

Correct:

Incorrect:

Incorrect:

[oleq]

Can we create a utility for that? (e.g. it would return the new Position of insertion for the given Selection).

[Reinmar]

This logic is already available in the editor. I'll just copy here my comment from

It is exceptionally bad when I insert into an empty paragraph: table feature leaves an empty paragraph instead of replacing it:

Just so you don't need to worry: model.insertContent() takes care of inserting things into an empty paragraph.

We added tables feature to Letters and now it is inconsistent that image is inserted before and table after the paragraph.

And for that we need to expose the findOptimalInsertionPosition() from image upload utils. I wonder... where should it end? Maybe it could become a part of model.insertContent() itself? An option maybe?

So, there's a utility for that already. It's just hidden in the ckeditor5-image package. Also, the second half of the task is already handled by model.insertContent() which all these features should be using.

Let's not create new tools for that, but instead let's find a way to expose the existing ones. As I wrote above – perhaps all this should be enclosed in model.insertContent()? However, what if you'd like then to only use the "find optimal position" logic without inserting anything? It seems that even if we'll enclose this in model.insertContent() (to make it convenient) we'd then need to figure out where to expose that util anyway.

[oleq]

IMO

• findOptimalInsertionPosition() should consider both empty blocks and start/end of block logic,
• findOptimalInsertionPosition() should land in engine/model/utils or engine/utils so it can be used standalone if there's such a need,
• model.insertContent() should use findOptimalInsertionPosition() by default,
• findOptimalInsertionPosition -> getInsertionPosition

[pjasiun]

where should it end?

Maybe it should be focused on widgets and be part of the widget repository as a util?

And maybe it should be focused on inserting blocks? findOptimalInsertionPositionForBlock()?

[pjasiun]

In the f2f we agreed that we will do the following changes.

1. Introduce findOptimalInsertionPosition() in the ckeditor5-widget package. It will work the same way it works now for the image plugin (so the way described in the first post). All 3 widgets (media, table, and image) will use it. However, it should work well for any widget-like block, it should not be expected that it will be used only for an element which is a widget.

2. We will change the way insertContetnt works. The second parameter will be optional. If not provided, model.document.selection will be used. If the second parameter is provided model.document.selection should not be used. If position or range will be used, insertContetnt should create selection from it and use this selection instead of document.selection:

model.insertContent( content ); // = insertContent( content, model.document.selection )
model.insertContent( content, position ); // = insertContent( content, new Selection( new Range( position ) ) );
model.insertContent( content, range ); // = insertContent( content, new Selection( range ) );

We realised that it means that if you use position or range as the second parameter you will not be able to get the selection after insertion (insertContetnt modify the selection parameter, but for position selection will be created inside insertContetnt method and will not be accesible). However, we agreed that it is fine. If one need to use the value returned as the second parameter he can use selection anyway.

This way, findOptimalInsertionPosition() will work as the second parameter of model.insertContetnt.