Ckeditor5: model.insertContentent( content, element ) does not work (and related issues)

[Reinmar]

It does not work because you cannot do this:

const sel = new Selection( element );

Unfortunately, Selection.setTo() forces you to pass the offset. I think it's unnecessary, but we can keep this limitation for clarity purposes.

However, in both cases, the additional problem with insertContent() is that it does not allow you to specify the offset:

model.insertContent( content, element, 0 );
model.insertContent( content, element, 'end' );

All this should be doable, but it's not.

[Reinmar]

Unfortunately, Selection.setTo() forces you to pass the offset. I think it's unnecessary, but we can keep this limitation for clarity purposes.

cc @pjasiun. Do you want to keep this limitation?

PS. This also needs to be improved in writer.setSelection() if we'll decide to change this (so offset is not required).

[Reinmar]

I'm reporting this as a bug/enhancement because right not insertContent() says it accepts Element as the second param. And in one or the other way that'd be highly useful.

[Reinmar]

BTW, offset defaults to 0 in Position.createAt( el, offset ), so you can do this:

const pos = Position.createAt( el );

[pjasiun]

cc @pjasiun. Do you want to keep this limitation?

We had some issues because of developers using Selection.setTo( element ) expect that this element will be selected (the selection will be around this element). And I can understand this expectation, reading the code it looks like selecting the element. On the other hand, for some, it means "select the whole content of this element". It could also mean "put the selection in the element at the beginning" (this is how it works with default 0) or "put the selection in the element at the end" (it makes a lot of sense when you think about pasting cases).

Since it is misleading I prefer to force developers to precisely define what they want to move and do not use any default value there. One need to use one of these:

new Selection( element, 'on' );
new Selection( element, 'in' );
new Selection( element, 0 );
new Selection( element, 'end' );

[pjasiun]

Unfortunately, Selection.setTo() forces you to pass the offset. I think it's unnecessary, but we can keep this limitation for clarity purposes.

And, to be honest, I do not now know about which of this behaviour you are thinking based on the ticket description. Do you expect the selection will be on the element or in it, at the beginning? This is why I think that clarification is useful.

[Reinmar]

This is why I think that clarification is useful.

OK, I'm fine with this as well. Let's always require the offset. So, to sum up:

1. Model#insertContent() should accept the 3rd parm (offset) and passed it to new Selection().
2. Position.createAt() and Range.createCollapsedAt(), Selection#setFocus(), etc (search for [offset=0]) should not default their second params to 0. This is a breaking change so let's make it now (since this release is big anyway).

WDYT?

[jodator]

1. should not default their second params to 0

@Reinmar / @pjasiun: So we should throw if no offset is passed? It will be similar to selection behavior.

[pjasiun]

I think so.