T/460 Selection rendering breaks composition

[f1ames]

Fix: Rendering is suspended during composition. Closes ckeditor/ckeditor5#3742.

---

Additional information

The fix introduces CompositionObserver which listens to native composition events. If composition is in progress the renderer will not update the DOM.

[pjasiun]

Looks like asking for serious troubles with collaboration. If you allow changing the view, but prevent changes in DOM, you may have a very different state of view and DOM if another user changed the same part of the document. Mutations calculate a diff between DOM and view and if there is something from the OT in the view it will be marked to be removed since it does not appear in the DOM.

[pjasiun]

You can say that it's an edge case, but it is exactly the same edge case you want to fix with this return (OT and composition in the same block). It means that to prevent breaking composition, you breaks OT.

[pjasiun]

For sure it need some tests with collaboration begore merging this PR because it can causes more bad than good.

[scofalik]

Have you tested how it would work with OT / collaborative editing? I am worried it will not work properly. Consider following scenario:

1. You start composition in <p>foo^bar</p> -> <p>foo[@]bar</p>. @ is composed char.
2. Rendering is blocked.
3. You get a change from server that inserts letter a at the beginning of <p>.

Your model is:
<paragraph>afoo[@]bar</paragraph>
Your view is:
<p>afoo[@]bar</p>
But your DOM is:
<p>foo[@]bar</p>

5. Now, when you type another part of composed char, the mutation will be handled by typing and when DOM and view are diffed, it will appear that letter a should be removed from DOM. Amirite?

I think this has to be handled more cautiously. Some ideas (pretty wild so I don't know whether they are good):

• Maybe not only block rendering but also block typing? Then handle inserting composed character on compositionend and re-render view?
• Maybe block applying all operations when composing? But then we need to cache them and we would have to make some transformations.
• Maybe stop model->view conversion too and try to refresh things after composition has ended?

[f1ames]

Thanks for pointing that out, I was wondering about issues connected to OT. What is the proper way to test OT for such cases, do we already have some integration/manual tests for similar cases?

[scofalik]

You can test it directly by faking composition start (set isComposing by hand?) and apply correct InsertOperation (for example using document.batch().insert()).

Then you would have to somehow fire typing, though. I don't remember how TypingCommand#_execute works.

[f1ames]

Ok, first I will write some unit/integration test (using operations as @scofalik suggested) to check how it works with composition.
And then as @pjasiun suggested I will do some "real" collaboration testing.
Of course, having in mind that the current solution most likely will break things and needs to be implemented differently.

[Reinmar]

A question (without checking much of the code) – does this PR blocks the entire rendering? Looking at some comments, I'd say that we should make it more precise from the day one by blocking changing only a certain container's content.

[f1ames]

@Reinmar, yes it blocks the entire rendering. Blocking only the container (or part of it) in which the composition takes place may help to solve some cases with OT, however probably not all.
Do you suggest it only should block rendering of the node in which composition takes place?

[Reinmar]

Looking at some comments, I'd say that we should make it more precise from the day one by blocking changing only a certain container's content.

Although... I can imagine such situation:

1. One person is composing in <p>a[bc]def</p>.
2. The other person presses enter at this position: <p>abcde[]f</p>.

If we block rendering in that first <p>, the person who's composing will see something like this for some time:

<p>a[bc]def</p>
<p>f</p>

Mind the duplicated "f".

After more thinking I guess that blocking rendering smaller bits of content may lead to really weird situations ;|. So I'm not sure anymore if we shouldn't start with blocking everything.

[Reinmar]

@Reinmar, yes it blocks the entire rendering. Blocking only the container (or part of it) in which the composition takes place may help to solve some cases with OT, however probably not all.
Do you suggest it only should block rendering of the node in which composition takes place?

I don't know. Right now I can see only these issues with weird things happening around the blocked container. @scofalik may be able to imagine how (and if) this would have any effect on OT. I think not. The model will actually be live. The problems may be model+view <-> DOM related – e.g. mapping. If we'll block rendering then view will diverge from the view so mapping the composition positions to the positions in the model may become tricky.

[pjasiun]

I think that we should do as much as we can not to break anything, but when there is no other way, for changes we can not predict/handle, I think that it's better to break composition then OT. These will be edge cases, I hope, but if the composition will be broken, then it broken composition. If the OT is broken, it can be broken editor (or all connected editors in the worst scenario).

I think, still, we can handle, most obvious scenarios. But blocking rendering should go in pair with handing observers well when the view totally changed. You can not predict what kind of changes occurred in the meantime. It mostly meant that you need to handle mutations, but I can also imagine some bugs with selection observer, hope other observers will be fine.

This is why I think that less we block, safer we are. Again, if part of the not blocked rendering will break composition, then only composition is broken, but if we will have a crash because of the not handled mutation/selection/whatever observer of the blocked element, then everything is broken.

I would start from the smallest possible part, so blocking changing content of the text node. Then we can handle inserting a new text node if composition creates it. Then breaking text block, moving it, etc. Or not, we can decide that if someone else breaks the block, it's too big change to prevent and the composition should be broken.

For handling typing in the same block, you still need to somehow save changes in the view, which are not rendered. I believe it's doable. For instance:

foo [locked]

foxxxo [in DOM, because of composition]

fyyyoo [in View, because of OT]
// yyy on position 1-4 are saved with the information that they are not rendered

DOM mutation:
[ change 'foo' to 'foxxxo' ]

Mutation after comparing it to view:
[ change 'fyyyoo' to 'foxxxo' ]

Mutation after handling saved letters:
[ change 'fyyyoo' to 'fyyyoxxxo' ] // This is the mutation which should be fired from the view.MutationObserver

[pjasiun]

And, as I think about it, selection need to be handled as well. If I end composition by moving the selection to the end of foxxxo{}, then it should be moved to the fyyyoxxxo{} in the view.

[pjasiun]

Also, note that renderer role is to compare view and DOM and not render nodes which does not need to be rendered (to not break composition). So elements which are the same in the view and DOM should not be touched. It, most probably, does not mean that we do not need renderer blocking, but as long as it can prevent some bugs, we should improve renderers nodes comparing, so render as few as possible.

[Reinmar]

BTW, we can also improve how the text nodes are updated.

In w3c/editing#149 I commented that:

Touching the same text node (by using methods like CharacterData.replaceContent()) doesn't break the composition too: https://jsfiddle.net/gqnq4h5r/12/ (you can place caret inside "App*" and use IME there). This is super cool :).

I don't know if this applies to all browsers but if we want to go this way that OT is more important, then this would be the way to go.

BUT do note that composition can't have the lowest priority for us. E.g. if you want to render selections of other users in the text node in which someone is typing... you'll break the composition every time some other user moved the selection in that text node. This, in some situations, will not be an edge case, but a really popular scenario. It'd render composing text impossible which would be a huge issue.

[Reinmar]

Final words – I guess we know a bit more about the scope and complexity of this problem now. It looks to me that it may be a killer issue so we should consider trying to find some quick wins and leave the rest for the future.

What I'd like to now first is:

1. Whether we have any problems with composition and how bad they are. I'm talking here about 1 user scenarios because we can be sure that collaboration breaks the composition.
2. What kind of quick wins we could talk about. E.g. can we block rendering/converting just the markers? That would at least prevent issues caused by other users' selections. Can we use CharacterData.replaceContent() today to prevent issues with people typing in one text node?
3. Do we have any viable plan for the future? We should have some solution designed now, so the temporal steps we'll do now will lead us closer to that designed solution. Otherwise, we may be wasting our time.

[pjasiun]

BUT do note that composition can't have the lowest priority for us. E.g. if you want to render selections of other users in the text node in which someone is typing... you'll break the composition every time some other user moved the selection in that text node. This, in some situations, will not be an edge case, but a really popular scenario. It'd render composing text impossible which would be a huge issue.

You did not get my point. I did not say that we should not prevent rendering anything, like the selection of other users. Or that it is not important. I said that we can not introduce changes which prevent breaking composition but crash editor instead. We need to be very careful and be sure that every introduced change is well tested and will not cause more harm than good.

[f1ames]

Looks tough indeed. I start checking for failing 1 user scenarios while using composition. Checked MacOS accent balloon and Spanish accent composition for now.

MacOS accent scenarios

1. Long press + number
2. Long press + mouse
3. Long press + arrow keys + enter
4. Long press + arrow keys + esc
5. Long press + click outside
6. Long press + Bold (edge case) + choose accent if possible
7. Long press + other letter

Only failing scenarios are listed below

Chrome
6. Inserts bolded accented letter but throws an error - Uncaught CKEditorError: view-renderer-filler-was-lost: The inline filler node was lost.. In other browsers the accent panel is just closed when applying bold.

FF - All OK

Safari
3. and 4. This is more general issue: It is impossible to navigate with arrow keys inside accent panel. After pressing left arrow last accent is inserted. After pressing right arrow first accent is inserted.

Spanish-ISO scenarios

1. Accent (´) + normal letter (b)
2. Accent (´) + accentable letter (a)
3. Accent (´) + esc
4. Accent (´) + bold (edge case)

Only failing scenarios are listed below

Chrome - All OK

FF
4. After applying bold the second press inserts letter, first one does nothing.

Safari
The general issue is that accent (´) is removed when typed on non-collapsed selection.

3. Broken:

1. <h2>Headin[]g 1</h2>
2. Press accent
3. Press esc
4. Results in <h2>Heading'[]g 1</h2>

4. Broken:

1. <p>Hea[]ding 2</p>
2. Press accent
3. Press bold
4. Type y
5. Results in <p>He<strong>y</strong>´ding 2</p> and Throws TypeError: null is not an object (evaluating 'viewBlock.parent') in _defaultToModelPosition — mapper.js:222

To sum up, there is 1 issue in Chrome, 1 in FF and 4 in Safari.
I am not sure how many of this issues are caused by the fact that DOM is rerendered during the composition (will check this later, after checking Hiragana) but seems like it is the cause for some of them.

[Reinmar]

3. and 4. This is more general issue: It is impossible to navigate with arrow keys inside accent panel. After pressing left arrow last accent is inserted. After pressing right arrow first accent is inserted.

This is odd. Perhaps our handler for the filler kicks in and tries to handle the arrow keys. This is the only place where we listen to keys.

[Reinmar]

The general issue is that accent (´) is removed when typed on non-collapsed selection.

May be that our dirty unsafe key hack fails here :(

[scofalik]

My 5 cents: of course blocking rendering does not crash OT directly. What PJ (and me) meant by crashing OT is more crashing collaborative editing, because of reasons that we already discussed.

[f1ames]

Checked some scenarios with Hiragana too. Only a few but I think it is enough to give some general picture. The scenarios contains just a key sequences as it is the easiest way to describe it and the reproduce.

1. a + a + a + enter
2. a + e + down x3 + enter x2
3. a + e + down x3 + a + e + down x2 + enter x2
4. a + e + a + left + right + left + up x2
5. a + e + a + up hold
6. a + e + Bold + a + e + down x4
7. Bold + a + e + a

Only failing scenarios are listed below

Chrome
3. Broken - inserts two characters more than it should (逢え和え[]和え instead of 逢え和え[]).
4. Broken - composition ends but composition dialog stays and cannot be closed.
5. Broken - spans a lot of additional characters when up is hold long enough.
6. Broken - seems like bold just ends the composition and text is inserted properly. However, next composition is broken.
7. Broken - first typed character is not a part of the composition.

FF
7. Broken - only first character is inserted.

Safari
Navigating with arrow keys breaks the composition in most cases (breaks 2. - 6. scenario).
7. Seems like composition instantly ends inside styled element

I think it will be good to check which of the failing cases could be solved by blocking rendering during composition and or using CharacterData.replaceContent() (also I will take a quick look on #861 (comment) and #861 (comment)) and after that we could probably conclude something based on the results and all previous comments.

[pjasiun]

I think it will be good to check which of the failing cases could be solved by blocking rendering during composition

I think that first, we should check why these cases are failing. It might be simple a bug in the renderer which renders more than it needs and may be fixed. CharacterData.replaceContent() might help too. But blocking rendering is the final workaround, when we are not able to find a real solution.

[Reinmar]

I can see that the arrow keys break text input again. I think that this is the same issue as with what you described previously – that in e.g. Safari you can't use arrows to navigate the accent tooltip. I'd check this first. And it may not be related to rendering but to how we handle the filler.

[f1ames]

@Reinmar if by filler you mean https://github.com/ckeditor/ckeditor5-engine/blob/master/src/view/filler.js#L156, it seems this code does not have any impact on this issue (both in MacOS accent panel and Hiragana), so the cause must be someweher else.

Looks more like the rendering issue, every time the arrow is pressed, the content of text node changes (the different accent is inserted) and rendering (_updateChildren + _updateSelection) breaks composition.

[f1ames]

May be that our dirty unsafe key hack fails here :(

Yep -> https://github.com/ckeditor/ckeditor5-typing/issues/82

[f1ames]

As for #861 (comment) - while navigating with arrow keys inside balloon panel the selection changes so the renderer updates it (_updateSelection) which breaks composition.
Introducing e.g. blocking selection rendering during composition will prevent this issue but then, there is_updateChildren which is fired on accent change and replaces whole text node with the new one (which also breaks the composition).

I was also able to check how it works with replaceData. The replaceData does not break the composition only if the replaced part of text does not include the current selection. So using replace data might help in some cases but then it would require to use replaceData with diff to change only parts of text which were changes.

[Reinmar]

As for #861 (comment) - while navigating with arrow keys inside balloon panel the selection changes so the renderer updates it (_updateSelection) which breaks composition.
Introducing e.g. blocking selection rendering during composition will prevent this issue but then, there is _updateChildren which is fired on accent change and replaces whole text node with the new one (which also breaks the composition).

We have two options here:

1. Make sure no rendering is needed because the selection we've read from native selectionchange events and converted through view=>model=>view is identical with the one with which we had initially. The same must apply to the content itself. That was @pjasiun's plan and in ideal world that would be the thing, but I personally find this approach to idealistic. Too many things can go wrong. But it will still be the first thing to check – why does selection converted from the model differs from the one we have in the view. Perhaps we have some bug.
2. Indeed block selection rendering when composition takes place. This should be easier than blocking content rendering, but it also assumes that for the composition to work, the renderer must decide that the DOM is identical with the view. That can still go wrong (because the view may diverge with the DOM and rendering will happen), but at least we'd be half way closer.

[Reinmar]

I was also able to check how it works with replaceData. The replaceData does not break the composition only if the replaced part of text does not include the current selection. So using replace data might help in some cases but then it would require to use replaceData with diff to change only parts of text which were changes.

Yep, we'd need to use the diff function and diff results grouping. I was worried about performance impact, but I guess we'll use it only when updating existing text node (which is rather rare action). When rendering for the first time we'll be of course creating new text nodes so it should be fine.

[f1ames]

The two Safari issues (Spanish-ISO 3. and 4.) described in #861 (comment) are caused by the Safari auto correction mechanism so it is not releted to this issue. With auto correction off, it works fine.

[pjasiun]

We have two options here:

1. Make sure no rendering is needed...
2. Indeed block selection rendering when composition takes place...

I agree with both. For content, we should do our best to improve renderer comparison, so the renderer will see that the view and the DOM are the same and do not need rendering, since, at the moment all other options seems to be very painful. For selection, we should try it too, but preventing selection rendering seems to be far less dangerous than contents.

[pjasiun]

I was worried about performance impact

Yep, we had performance problems with diff. It appears when you compare 2 totally different collections (strings). The time needed to calculate the diff is linear if collections are the same and grows exponentially with the length of the different texts. There was an idea to improve the diff function so if the difference is too big to calculate it quickly (something like 30 different characters), we overwrite the whole string.

[Reinmar]

But do you think that it may be any performance issue if we use diffing only on strings which we know that got changed? This only affects typing and other kinds of text insertion which happen only a couple of times per second.

[pjasiun]

If one paste a long text into the paragraph we may get a serious browser freeze (more than 10 sec). It might not be that bad, but the performance, in this case, needs to be tested in the first place.

I remember we experienced it already, and it was not around 500 ms, but more than 10 sec. But then we used a linear model and calculated a diff on the whole model, so in this case in might be not that bad.

[f1ames]

Reported autocorrection issue (#861 (comment)): https://github.com/ckeditor/ckeditor5-typing/issues/85.

[f1ames]

I think that first, we should check why these cases are failing. It might be simple a bug in the renderer which renders more than it needs and may be fixed.

In case of selection during composition (MacOS accent panel) it is: https://github.com/ckeditor/ckeditor5-typing/issues/86.

[f1ames]

It looks like https://github.com/ckeditor/ckeditor5-typing/issues/86 also fixed most issues with Hiragana composition in Chrome (3. - 6.) mentioned above. And so for the above scenarios the remaining issues are:

MacOS accent scenarios

• Chrome - 6. - https://github.com/ckeditor/ckeditor5-typing/issues/90

Spanish-ISO scenarios

• FF - 4. - https://github.com/ckeditor/ckeditor5-typing/issues/91

Hiragana

• Chrome, FF, Safari - 7. - https://github.com/ckeditor/ckeditor5-typing/issues/89

All IME related found issues: https://github.com/ckeditor/ckeditor5-typing/issues?utf8=%E2%9C%93&q=is%3Aissue%20is%3Aopen%20IME

[Reinmar]

@pjasiun, as I told you, we don't close unmerged PRs if they have some value. I've been looking for this PR now, knowing that it was somewhere there previously, and I couldn't find it. Hence, I'm reopening it.

[f1ames]

Went through scenarios from #861 (comment) and #861 (comment) once again:

MacOS accent

6. Inserts bolded accented letter but throws an error - Uncaught CKEditorError: view-renderer-filler-was-lost: The inline filler node was lost.. In other browsers the accent panel is just closed when applying bold.

Only this one is still reproducible on Chrome.

3. and 4. This is more general issue: It is impossible to navigate with arrow keys inside accent panel. After pressing left arrow last accent is inserted. After pressing right arrow first accent is inserted.

The Safari one visible above related to key navigation inside accent panel was fixed.

Spanish-ISO

Both:

4. After applying bold the second press inserts letter, first one does nothing.

The general issue is that accent (´) is removed when typed on non-collapsed selection.

were fixed.

Hiragana

6. Broken - seems like bold just ends the composition and text is inserted properly. However, next composition is broken.
7. Broken - first typed character is not a part of the composition.

Still reproducible in Chrome and Safari, looks like Bold is the issue here as described in https://github.com/ckeditor/ckeditor5-typing/issues/89. And on Firefox only 7. case fails, seems to be related to the same issue. The rest of reported cases were fixed.

[f1ames]

Went once again through scenarios which are still reproducible (#861 (comment)) because of https://github.com/ckeditor/ckeditor5-engine/issues/1417.

MacOS accent

6. Inserts bolded accented letter but throws an error - Uncaught CKEditorError: view-renderer-filler-was-lost: The inline filler node was lost.. In other browsers the accent panel is just closed when applying bold.

Only this one is still reproducible on Chrome.

I extracted this issue to https://github.com/ckeditor/ckeditor5-typing/issues/147 so it is easier to track it.

Hiragana

There is only problem with Safari when:

Navigating with arrow keys when composition takes place inside empty inline element breaks it.

I suspect it is caused by the fact that when we navigate through composition panel, selection leaves text node (in which composition takes place) which causes inline filler to be removed and it breaks composition.

Extracted to https://github.com/ckeditor/ckeditor5-typing/issues/148 so it is easier to track.