[Chrome][Safari][IME] Starting composition on non-collapsed selection containing partially styled element throws an error.

[f1ames]

1. Go to http://localhost:8125/ckeditor5-typing/tests/manual/input.html.
2. Select <p><em>Th[is</em> is] an <strong>editor</strong> instance.</p>.
3. Start the composition (Spanish-ISO with ' or Hiragana with e.g. a).

Chrome
The character starting the composition is inserted (marked as X below), but the resulting html is
<p><em>ThX[]</em>] is an <strong>editor</strong> instance.</p>, which means only styled part of the selection was replaced. Also an error is throw Uncaught TypeError: Cannot read property 'parent' of null.

Safari
Resulting html is <p><em>ThX[]s</em>] an <strong>editor</strong> instance.</p>, no error thrown.

[f1ames]

Works fine on FF. On Chrome still behaves as described above. Safari seems to replace selection properly (resulting in <p><em>ThX[]</em>] an <strong>editor</strong> instance.</p>), however composition does not start.

[f1ames]

Rechecked with https://github.com/ckeditor/ckeditor5-engine/issues/1417, still reproducible as described in the above comment.

[f1ames]

I checked what is the difference in a rendering flow when regular character a and the one starting composition ´ is typed in a situation like:

<p><i>Th[is</i> is] CKEditor!</p>

For regular character a:

1. Initial model state: <paragraph><$text italic="true">Th[is</$text> is] CKEditor!</paragraph>.
2. The a key is pressed.
3. The input._handleKeydown() function triggers selected content deletion.
   • Model: <paragraph><$text italic="true">Th[]</$text> CKEditor!</paragraph>.
4. Text mutation is handled: {type: "text", oldText: "Th", newText: "Tha", node: Text}.
   • Model: <paragraph><$text italic="true">Tha[]</$text> CKEditor!</paragraph>.
5. Changes in a model a propagated to view and then correctly rendered.

For composition starting character ´ (Spanish accent):

1. Initial model state: <paragraph><$text italic="true">Th[is</$text> is] CKEditor!</paragraph>.
2. The ´ key is pressed.
3. The input._handleKeydown() function does not trigger selected content deletion because in this case selection is collapsed.
   • Model: <paragraph><$text italic="true">Th´[]</$text> is CKEditor!</paragraph>.
   • DOM: <p><i>Th´</i> CKEditor</p> (because natively whole selection was replaced with ´).
4. Now there are two text mutations to be handled:
   • {type: "text", oldText: "This", newText: "Th´", node: Text}
   • {type: "text", oldText: " is CKEditor!", newText: " CKEditor!", node: Text}
5. First mutation is handled and DOM rerendered via renderer.render() method.
   • While model/view content of 2nd text node is is CKEditor! and corresponding DOM text node content is CKEditor! the text node gets rerendered in rednerer._updateChildren() method. It replaces the whole text node with the new one¹.
6. Second mutation is handled. However, as it relates to the text node which was rerendered in a previous step, the mutation text node does not exists in a DOM (has no parent) and cannot be mapped which throws an error.

1. The rednerer._updateChildren() method works in a way that text nodes which content has changed are completely replaced by the new ones. This behaviour was reported in https://github.com/ckeditor/ckeditor5-engine/issues/1425.

---

So here basically rendering between handling mutations changes the DOM structure. The mutation which is processed as a second one contains reference to already detached DOM element which breaks whole flow. For now I see two possibilities:

• https://github.com/ckeditor/ckeditor5-engine/issues/1425 should solve this issue.
• We should think of processing all mutations provided by the same event before rerendering the DOM.

WDYT @Reinmar? The first one should be easier, but it feels a little like a workaround not the direct cause fix. However, since we were thinking of working on it, maybe we can give it a try?

[Reinmar]

Do I understand correctly that _handleKeydown() doesn't work in this case? That's bad... It exactly the thing that was supposed to prepare the DOM for upcoming composition.

[f1ames]

Do I understand correctly that _handleKeydown() doesn't work in this case? That's bad... It exactly the thing that was supposed to prepare the DOM for upcoming composition.

Well, it looks like. I mean, it does nothing because selection is collapsed in this case. So if it was the assumption that it will "remove" selection contents before composition is started then yes, it does not work. I wonder why selection is collapsed in this case, looking at events order, keydown should be handled first so selection still should be not affected by any composition mechanisms 🤔

[Reinmar]

It's worth reporting upstream. But we'll need to figure something out too. The idea was that we don't want to allow character insertion on a non collapsed selection because this may lead to a lot more mutations that we're able to handle. E.g. when blocks merge it's a disaster for us. Therefore, it would be great if there was some way to handle this also when composition starts. Can we do that on compositionstart itself? I guess not - because our DOM changes will break composition. So is there any other event?

…

Sent from my iPhone

On 5 Jun 2018, at 17:50, Krzysztof Krztoń ***@***.***> wrote: Do I understand correctly that _handleKeydown() doesn't work in this case? That's bad... It exactly the thing that was supposed to prepare the DOM for upcoming composition. Well, it looks like. I mean, it does nothing because selection is collapsed in this case. So if it was the assumption that it will "remove" selection contents before composition is started then yes, it does not work. I wonder why selection is collapsed in this case, looking at events order, keydown should be handled first so selection still should be not affected by any composition mechanisms 🤔 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[f1ames]

This behaviour is somehow strange. I prepared a codepen to check on what events selection is still non-collapse. For regular typing and composition I get very similar results. For regular typing (boolean value after event type shows if selection is collapsed):

and for Spanish accent:

which means in our keydown handler selection should still be non-collapsed one. However, I also noticed strange behaviour while debugging:

Which looks like debugger doesn't break in a place it should 🤔

So at least one of this things deserves an upstream (reported strange debugging sequnece).

[f1ames]

Ok, the cause is slightly different here 😬

Since while debugging, selection seems to be collapsed (because of some strange behaviour shown in the comment above) I thought this is the case here. However, when using good ol' console.log I can see selection is not collapsed. The code exits because of isSafeKeystroke( evtData ) call.

So 229 key code (which means composition start/end) is treated as safe keystroke here and that's the problem. I'm not sure what was the reason for it and if we could change that.

Btw. I did a super quick test and removing 229 from safe keystrokes helps in this case, but it will break every case when selection is non-collapsed on composition end (e.g. Japanese Hiragana) as it will remove newly composed sequence (because you confirm composition with key press like Enter) 😞

[Reinmar]

Try git blame. I think we added this at some point... Perhaps even you did that :D

[f1ames]

Try git blame. I think we added this at some point... Perhaps even you did that :D

Yes, indeed 😅 One of the reasons is what I mentioned in a previous comment probably:

... it will break every case when selection is non-collapsed on composition end (e.g. Japanese Hiragana) as it will remove newly composed sequence (because you confirm composition with key press like Enter) 😞

So I think the potential fix can work like:

• If not during composition 229 key code should also trigger content deletion in a _handleKeydown().
• If during composition 229 key code should not trigger content deletion in a _handleKeydown().

From what I see (at least for Hiragana) compositionend event is fired after keydown so it may work like that. But it's just a quick idea and should be investigated further - but seems like a good place to start.

[f1ames]

So I went with the approach mentioned in the https://github.com/ckeditor/ckeditor5-typing/issues/83#issuecomment-395027323 above and it has some potential. I have implemented the fix and tested it on Chrome, Firefox and Safari with macOS accent panel, Spanish, Japanese (Hiragana) and Korean (2-Set) keyboards.

It works fine on Chrome (which means fixes the issue) and Firefox. However, there are some issues on Safari - the order of events is different in some cases:

• keydown is fired after compositionend so it removes inserted characters (happens especially for macOS accent panel).
• keydown is fired after compositionstart so its handler would not remove selected content (for Spanish, Japanese and Korean).

---

I think for Safari we may try one more think in the keydown handler. If keydown with 229 key code is fired during composition it should additionally check if there is non-collapsed selection which spans over more than one element.
For other browser there will be only collapsed selection as initial fix covers it. If something was already composed, the selection is non-collapsed but it contains text only. So the above check will be triggered only for Safari cases. The only important think is to make sure if DOM manipulation in such state of composition doesn't break it.

[f1ames]

The only important think is to make sure if DOM manipulation in such state of composition doesn't break it.

Checked and it removes also a character which started a composition 🤔

[f1ames]

So the final solution I came up with works as follows:

• If there is a keydown with 229 code outside composition, selection content should be removed.
• If there is a keydown with 229 code during composition, selection content should not touched.

This covers Chrome where events order is keydown:229, compositionstart, keydown:229, compositionend.

However, on Safari the order in most cases is compositionstart, keydown:229, compositionend, keydown:229. The above solution will not work and break the composition so additional checks were needed:

• If on compositionstart there is a non-collpased, not flat selection it means that the keydown handler didn't kick in. Therefore, selection content should be removed. This change (deleting the content) works well and does not break the composition. This mechanism covers the following starting sequence: compositionstart, keydown:229.
• If there is a keydown:229 outside composition, but the selection is the same as upon ending the last composition (saved on compositionend event), the selected content should not be removed (otherwise, the composed text would be removed). This covers the following ending sequence: keydown:229, compositionend.

[f1ames]

There is an issue in Edge browser. Composition there works slightly different:

• Spanish accent does not fire composition events at all, but _handleKeydown() work well in this case.
• Composing Korean works as in Safari (compositionstart, keydown) so compositionstart handler takes care of it.
• Composing Japanese (Hiragana) works as in Chrome (keydown, compositionstart), however removing content on keydown results in composed character not showing. It seems that it may be caused byt the fact that content removal changes the selection so composed character does not appear 🤔

---

I also noticed one issue on Chrome caused by the current implementation. When starting Japanese composition (only native macOS IME keyborad, works fine with Google ones and on Windows) over non-collapsed selection, you have to then press left/right arrow because navigating through IME panel after starting composition with up/down arrow keys is not possible.

[f1ames]

The problem in Chrome originates from the fact that after selection contents removal, selection is rerendered:

however as DOM is modified it is necessary to update the selection, so I'm not sure yet if there is any reasonable workaround 🤔

---

As for Edge (tested with 16.16299) issue with Japanese typing also seems tricky. Without the mechanism which removes selection contents before composition, composing with non-collapsed selection (over more than one block element) will not work.

---

The one workaround I see is to adjust the current mechanism:

If there is a keydown with 229 code outside composition, selection content is removed.

in a way that selection content is removed on keydown with 229 only if selection is not flat (different start/end elements). It seems that with flat selection, generated mutations are handled properly and composing works well. So by removing selection contents only when really necessary we can narrow the number of cases when above issue happens. Still it is not an ideal solution 🤔 cc @Reinmar

[f1ames]

So the above problem with Chrome is caused by the fact that whole text nodes gets replaced in the renderer._updateChildren() method (see https://github.com/ckeditor/ckeditor5-engine/issues/1425 issue which is exactly about this). Text node gets replaced and then selection is put inside a new text node which apparently disturbs the composition flow.

I did a quick check and fixing https://github.com/ckeditor/ckeditor5-engine/issues/1425 seems to help.

[f1ames]

I did a test on plain contenteditable on Edge to see if removing selection contents on keydown breaks the composition as described above and it does (https://codepen.io/f1ames/pen/vrRqba?editors=1010).

This is not identical behaviour to what happens in the editor (content is not removed directly, changes are passes through a model and then are rendered), but the end result is the same so it seems this is more native behaviour (I also checked it with quick-fix for ckeditor/ckeditor5-engine#1425 mentioned in the comment above, but it doesn't change anything).

[f1ames]

Some thoughts not directly connected with this issue, but more with a fix and how things works. W3 documentation describes compositionstart event and in particular:

When a keyboard is used to feed an input method editor, this event type is generated after a keydown event, but speech or handwriting recognition systems MAY send this event type without keyboard events.

So clearing composition contents on keydown may not work in some mentioned cases (speech or handwriting recognition systems), but these are really edge cases so we may get back to it when needed.

[f1ames]

I have reported the Edge issue https://developer.microsoft.com/en-us/microsoft-edge/platform/issues/17997826/. ATM I'm not sure if it will be fixed by the browser vendor, because I haven't found a part describing this specific behaviour in any W3 specs, which may mean it is not an incorrect behaviour. Still, it works different/better in other browsers so I gave it a try.