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

Disable inline filling character removal during composition #4033

Closed
f1ames opened this issue Mar 30, 2017 · 5 comments · Fixed by ckeditor/ckeditor5-engine#1424
Closed

Disable inline filling character removal during composition #4033

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

Comments

@f1ames
Copy link
Contributor

f1ames commented Mar 30, 2017

Extracted from ckeditor/ckeditor5-engine#896 (comment).

I feel that the next thing we should do is work on disabling filling char and element removal when composition takes place. Since things are transparent to the view (they are not visible there) I think that they could be kept longer if needed without much of a trouble. Am I right @pjasiun?

Also, I remember that the filling char is removed on keydown so it doesn't affect caret movement. This should also be prevented during composition.

@f1ames f1ames self-assigned this Mar 30, 2017
@f1ames f1ames changed the title Disable filling character removal during composition. Disable filling character removal during composition Feb 28, 2018
@f1ames
Copy link
Contributor Author

f1ames commented Mar 8, 2018

Stumbled upon one issue in Chrome when removing inlineFiller (basically any text) after composition end moves selection (by the amount of removed characters) - reported here - https://bugs.chromium.org/p/chromium/issues/detail?id=820067.

chrome - mar-08-2018 14-27-02
(There are 5 ZWS and on compositionend 7 characters are removed)

The selection change is async so even though renderer updates it after inlineFiller removal it does not help.

Looks like more generic problem, something similar happens also while typing. I'm wondering why it does not happen when typing inside empty element (checking now). Also there is a difference how it behaves with just replacing filler node data fillerNode.data = ... and using CharacterData.deleteData.

@f1ames
Copy link
Contributor Author

f1ames commented Mar 8, 2018

As with removing inlineFiller while typing similar also happens (as described in the Chrome issue in this case selection moves to the beginning of the text node), but here somehow renderer._updateSelection fixes it properly (looks like in opposite to render triggered by composition in this case selection is moved by browser before renderer updates it).

@f1ames f1ames changed the title Disable filling character removal during composition Disable inline filling character removal during composition Mar 13, 2018
@f1ames
Copy link
Contributor Author

f1ames commented Mar 13, 2018

Extracted disabling block filler during composition to ckeditor/ckeditor5-engine#1353.

@f1ames
Copy link
Contributor Author

f1ames commented Apr 3, 2018

I started to dig deeper into inline filler management (how it will work after only blocking its removal during composition) and it seems there is some avoidable complexity due to unnecessary structure updates (see ckeditor/ckeditor5-engine#1342). For example, having initial HTML like <p>Foo<strong>INLINE_FILLER{}</strong>Bar</p> and then starting a composition and inserting a character will result in the following:

  1. Filler is not removed from the strong element due to composition.
  2. renderer._updateChildren rerenders strong and p (in this order) as both were marked to sync.
    • Rendering strong removes whole strong element (together with filler).
    • Rendering p renders new strong element (without filler).
  3. Filler is inserted in a new strong element.

Avoiding unnecessary rerendering of a strong element would keep the filler in place so the logic which reinserts it could be simplified (also the fact that filler is removed basically as a side effect of _updateChildren in this case looks troublesome).

@f1ames
Copy link
Contributor Author

f1ames commented Apr 5, 2018

When managing inline fillers there are basically 4 main cases which needs to be handled properly. Assuming starting HTML like <p>Foo<b>FILLER{}</b></p>:

  1. Inserting character/text should cause inline filler to be removed.
  2. Inserting character caused by composition. Here inline filler should not be touched (moved or removed). After composition ends, inline filler should be removed.
  3. Composition starts (characters may be inserted) and selection is extended (so it starts/ends in different node than the one in which composition was started). In this case composition is not ended and inline filler should not be moved or removed (see Disable inline filling character removal during composition ckeditor5-engine#1355 (comment) for detailed description and repro steps). After composition ends (by moving whole selection or inserting characters), inline filler should be removed (unless nothing was inserted so we end up with <p>Foo<b>FILLER{}</b></p> - here filler should be preserved).
  4. Moving selection or inserting text with isComposing set to true, but in the node different from one in which composition have been started. Theoretically, this situation should not have place as moving collapsed selection during composition to different node should end composition. However, due to some renderer issues (How to listen to composition events? #1342) it may happen that composition is not ended properly. In such case, inline filler (if it was left after composition was not properly ended) should be removed from its origin text node. If there will be another node requiring inline filler creation (e.g. <em></em>), inline filler should be inserted there and processed with regular flow (points 1 - 3 above).

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-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 18 milestone Oct 9, 2019
@mlewand mlewand added module:view type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
2 participants