Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[IME] Composition does not start with first typed character in empty inline style. #3084

Closed
f1ames opened this issue Mar 13, 2017 · 8 comments · Fixed by ckeditor/ckeditor5-engine#1424
Labels
package:typing type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Mar 13, 2017

  1. Go to http://localhost:8125/ckeditor5-typing/tests/manual/input.html.
  2. Press Bold to create empty bolded style.
  3. Type one character which should start a composition (e.g. ' in Spanish, a in Hiragana).

First character is inserted but does not start a composition.

@Reinmar
Copy link
Member

Reinmar commented Mar 13, 2017

Tadada, @pjasiun, yet another example we can't be rendering when composition takes place :P

Most likely, this is due to removing the block filler on first selection change.

@f1ames f1ames changed the title Composition does not start with first typed character in empty inline style. [IME] Composition does not start with first typed character in empty inline style. Mar 13, 2017
@pjasiun
Copy link

pjasiun commented Mar 13, 2017

I'm not saying we should do nothing. I'm saying we should break nothing.

@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2017

There are actually two cases here:

  1. When typing in an empty block.
  2. When typing in the middle of a text.

Those cases are a bit different because a different filler is used in both of them. In both cases it breaks the composition.

@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2017

This is an important bug because it makes it hard to type anything using composition after applying any inline style.

@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2017

OK, it may not be about the block filler and inline filler actually. If it would be a problem with removing the block filler, it would also happen in an empty paragraph without applying bold there. But it works fine in such a case.

Also, @f1ames told me that he observed that the entire inline element is being re-rendered. So maybe, somehow, we lose a reference to that node (we replace it in the view with a new attribute element when converting the first typed letter) so we remove it and re-insert a fresh one to the DOM.

If so, then this is a big problem. If, as I expected until being enlightened, the renderer just diffed the view and DOM elements, we would have no such issues. The conversion may often just wipe out the old view elements and reinsert nearly identical ones. In such cases, it'd be great if we didn't have to re-render them.

One idea that I have is that perhaps view<->DOM mapping could be smarter and check whether the new element resembles the old one (and if so, it would re-bind them). We need to talk about this on Monday.

cc @pjasiun

@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2017

Just a quick note. It may also be a problem with the view writer.

Let's say we have an initial situation with a bold applied to an empty selection:

<p><strong>{}</strong></p>

After converting insertion of the letter we have:

<p><strong>{}</strong>a</p>

And after converting an attribute of that letter we have:

<p><strong>{}</strong><strong>a</strong></p>

(or a reverse situation - a before {})

Now, the writer also needs to merge these two elements. If it merges the one in which the selection was to the newly created one then we lose the reference. Maybe it can be fixed there.

@pjasiun
Copy link

pjasiun commented Mar 17, 2017

The case of writing in the empty, bolded block should work fine in the current architecture. This is exactly why we have selection attributes. We should not block anything.

But, in fact, in the current implementation, writer split and merge attributes elements as many time as it wants and renderer can't handle replacing one attribute element with another. It might be the reason of bugs. We should make one of these 2 guys (writer or renderer) smarter. Since the writer is already very complicated improving renderer might be the way to go. But yes, we need to talk on Monday.

@Reinmar
Copy link
Member

Reinmar commented Mar 17, 2017

The case of writing in the empty, bolded block should work fine in the current architecture. This is exactly why we have selection attributes. We should not block anything.

It's not about selection attributes and rendering the empty <strong> with the selection inside before you start typing. The problem here, most likely, is that the entire <strong> is being removed and inserted again when you type the first letter. This should rather not happen (at least conceptually) because it may break the composition started by the first letter.

Reinmar referenced this issue in ckeditor/ckeditor5-engine Jun 18, 2018
Fix: Renderer should avoid doing unnecessary DOM structure changes. Ensuring that the DOM gets updated less frequently fixes many issues with text composition. Closes #1417. Closes #1409. Closes #1349. Closes #1334. Closes #898. Closes ckeditor/ckeditor5-typing#129. Closes ckeditor/ckeditor5-typing#89. Closes #1427.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-typing Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:typing labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:typing type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants