Fix infinite selection change loop introduced in #623

[Reinmar]

We agreed to merge ckeditor/ckeditor5-engine#623 with a small regression – the infinite loop check kicks in when moving selection over boundaries of inline elements. It needs to be fixed ASAP.

This is a broader issue, though, and includes also:

• https://github.com/ckeditor/ckeditor5-engine/issues/612
• https://github.com/ckeditor/ckeditor5-engine/issues/535

It's a bit unclear now whether all of them are caused by the same issue or they are separate. In this ticket we must focus on the regression (if it's regression at all). But perhaps, at the same time we'll fix also the rest.

[scofalik]

At this moment I cannot reproduce the situation that inf-loop kicks in when selection is placed at the boundary of inline element. It does occur, however, when you bold a part of text, so I will investigate that.

[scofalik]

Okay I know the cause:

DOM selection: a<strong>[b]</strong>c is converted to...
View selection: a<strong>[b]</strong>c is converted to...
Model selection a[b]c is converted to...
View selection: a[<strong>b</strong>]c which is different than the first view selection and it causes the loop.

[scofalik]

Okay I analyzed how exactly the selection process looks like:

Let me explain a bit:

1. I click "Bold".
2. Changes are done on the document.
3. changesDone is fired.
4. Model selection (a[b]c) is converted to view selection. View selection is ON inline element (a[<b>b</b>]c).
5. Changes are rendered.
6. DOM selection is dirty - moved somehwere because text node in which it was is removed/changed.
7. View selection created basing on DOM selection (oldViewSelection) is also dirty.
8. oldViewSelection is different than current view selection.
9. DOM selection is set basing on current view selection. DOM selection still "fixes" itself to a<b>[b]</b>c while current view selection is a[<b>b</b>]c.
10. Now first weird thing happens: SelectionObserver fires selectionchange. Shouldn't it be disabled? view.render disables observers.
11. DOM selection (a<b>[b]</b>c) is converted to view selection (a<b>[b]</b>c) which is different than current view selection.
12. New view selection is converted to model selection (a[b]c).
13. No ranges change on model selection, but since view->model conversion has to be in enqueueChanges block, changesDone is fired.
14. We go back to point 3., however selection will not be dirty later on. Still it will be different than current view selection, because when converted from DOM it is set INSIDE inline element and when converted from model it is set ON inline element. We have a loop.

So I see multiple things that could be changed.

1. First of all, why selectionchange happened if observers are disabled? Is it correct? If it didn't fire, AFAICS the loop would not happen.
2. It feels bad that model selection is converted to view selection even though nothing changed. However changing that would not fix the problem anyway because actual view selection does not change in the whole process (at least not now).
3. Instead of checking whether view selections are equal (in _updateSelection or selectionchange) we could check whether they are similar or touching. Touching is already a term in model.Position, where two positions touch if there are no characters or empty nodes between them.
4. We also could just "repair" current view selection when we recognize that DOM selection fixed itself and view selection created from fixed DOM selection is touching current view selection.
5. Another way would be forcing view selection to prefer positions inside inline elements. However I don't like this solution because: different browsers may have different preferences and this might cause problems in different areas. Overall I feel like this would be a too big change.

[scofalik]

I like third solution most because it is least invasive. If we "repair" view selection, then model selection change will make it "wrong" again.

[pjasiun]

First of all, why selectionchange happened if observers are disabled? Is it correct? If it didn't fire, AFAICS the loop would not happen.

Because the native event is asyc, so it is fired after observer is reenabled.

Instead of checking whether view selections are equal (in _updateSelection or selectionchange) we could check whether they are similar or touching. Touching is already a term in model.Position, where two positions touch if there are no characters or empty nodes between them.

The whole idea about the selection attributes is that it matters if the selection is in the <b> or before it. Otherwise, we may have problems with composition.

[pjasiun]

Instead of checking whether view selections are equal (in _updateSelection or selectionchange) we could check whether they are similar or touching. Touching is already a term in model.Position, where two positions touch if there are no characters or empty nodes between them.

The whole idea about the selection attributes is that it matters if the selection is in the <b> or before it. Otherwise, we may have problems with composition.

But selection attributes are important only in the case of the collapsed selection. It case of the non-collapsed selection we could check whether they are similar or touching. It what renderer/observers should do: accept irrelevant differences between view and DOM and do not rerender/fire events.

[Reinmar]

Fixed by ckeditor/ckeditor5-engine#671.