Ckeditor5: Improve selection API

[pjasiun]

Every time we want to set the collapsed selection to the position or range we do it in a different way. This is because there is no easy way to set the selection to the position. We should make it simpler and setTo seems to be the good method for it. It should accept not only other selection but also the position (the selection will be collapsed them), a single range or collection of ranges. Alternatively we could have a separate method: setToPosition( postion ) or setCollapsed( postion ).

[pjasiun]

To show why is this a problem, in the last PR I reviewed @ma2ciek used:

selection.setFocus( element );
selection.collapseToEnd();

To put the selection at the beginnging of the element, what is strange, but it's hard to find a simple method which does what he wanted to do in 2 lines of code.

[Reinmar]

The problem is that you need Position's createAt() to do so easily. I think that Range and Selection both miss such a method. You shouldn't need to import Position and Range to move a selection to a chosen place.

[pjasiun]

So I think we should have:
selection.setCollapsedAt( itemOrPosition, offset ) -> Postion.createAt
selection.setCollapsedAfter( item ) -> Postion.createAfter
selection.setCollapsedBefore( item ) -> Postion.createBefore
selection.setIn( element ) -> Range.createIn()
selection.setOn( item ) -> Range.createOn()
selection.setTo -> accepts position, range, iterable of ranges or another seletion

Range.createCollapsedAt -> Postion.createAt
Range.createCollapsedAfter -> Postion.createAfter
Range.createCollapsedBefore -> Postion.createBefore

WDYT?

[Reinmar]

LGTM 👍

[ma2ciek]

Should I rename selection.collapse to selection.setCollapsedAt? I didn't notice that selection.collapse takes some arguments.

[pjasiun]

I think so. If no one remembers that we have selection.collapse it means that it's a wrong name for the method. When I want to set selection I'm looking for the set* method.

[ma2ciek]

Do we need the selection.setCollapsedAfter( item ) and selection.setCollapsedBefore( item )? They'll be aliases to the selection.setCollapsedAfter( item, 'after' ) & selection.setCollapsedAfter( item, 'before' )

[pjasiun]

Then we can skip setCollapsedAfter and setCollapsedBefore. At least for now.

[ma2ciek]

Should I add a possibility to set backward selection in the selection.setTo method when passing Range or iterable of Ranges?

[Reinmar]

Should I add a possibility to set backward selection in the selection.setTo method when passing Range or iterable of Ranges?

I have a problem with this... I now think that we should not have any selection method which allows you to set a non-collapsed selection at once. Why? Because people will tend to ignore the directionality of the selection. This happened in CKEditor 4 where the API ignored it and hence the direction was never set.

So, what I think we should have are methods to set:

• a collapsed selection,
• selection's anchor (to position, to "node+start/end/offset", etc.) and then extending its focus to a given position

So, examples:

selection.setCollapsedAt( item, 'start' );

selection.setCollapsedAt( item, 'start' );
selection.setFocusTo( position );
selection.setFocusTo( item, 'end' );

These two methods – setCollapsedAt() and setFocusTo() (or we could call it like the native extend() method) will be enough. Eventually we could also have like the native setBaseAndExtent().

As for setOn() and setIn()– the first one makes sense because when selecting elements (like images) you don't care about directionality in 90% of cases. setIn() might exist as well because it has a narrow use case to select element's content (e.g. inserted table cell). In such cases directionality doesn't matter as well.

[Reinmar]

Should I rename selection.collapse to selection.setCollapsedAt? I didn't notice that selection.collapse takes some arguments.

Hm... I forgot about it too. Please mind that collapse() and extend() are native methods so it'd be pity if invented our own API which does nearly the same thing but using different names. So, I'd rather go with collapse() and extend().

[Reinmar]

BTW, Selection.collapse() is used dozens of times (in the model and in the view). So the fact that we forgot about it (when we saw an issue report that there's no such method) doesn't mean that we need to suddenly change its name. We just got confused by someone's confusion – it happens. Don't panic.

[Reinmar]

Ok, after a short talk with PJ we decided to drift away from native names and have Selection.setCollapsedAt() and Selection.moveFocusTo(). The rationale is to have all methods which ignore the previous state starting with set* and instead of extend() have moveFocusTo() to clarify what this method actually does.

[Reinmar]

So, from the original proposal, do we need this method?

selection.setTo -> accepts position, range, iterable of ranges or another seletion\

I'm against, as I explained in https://github.com/ckeditor/ckeditor5-engine/issues/1074#issuecomment-321969644.