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

Always render text nodes via 'renderer._updateText' #4353

Closed
f1ames opened this issue Jun 1, 2018 · 6 comments
Closed

Always render text nodes via 'renderer._updateText' #4353

f1ames opened this issue Jun 1, 2018 · 6 comments
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). package:engine resolution:expired This issue was closed due to lack of feedback. type:task This issue reports a chore (non-production change) and other types of "todos".

Comments

@f1ames
Copy link
Contributor

f1ames commented Jun 1, 2018

Extracted from https://github.com/ckeditor/ckeditor5-engine/issues/1417#issuecomment-389191257.

The simplest case to check this behaviour:

  1. Open editor instance, could be e.g. http://localhost:8125/ckeditor5-core/tests/manual/article.html.
  2. Select one character on the end of the paragraph like <p>Paragrap[h]</p>.
  3. Type something.

Paragraph is marked to update in the renderer and renderer._updateChildren replaces the whole text node with the new one (containing modified text) - no matter how long text is, so it happens also for very long paragraphs. Since we have introduced smart text nodes updates (#3703) it will be nice to utilize it in such cases too.

The main benefit should be visible during composition with collaboration. That's the reason I mentioned paragraph length. Having a long one may result in a few people composing inside it at once which will break quite quickly.

There are also similar cases (like bolding part of the text) which also may utilize smart text nodes updates. Case with bold is already described in the linked issue.


I'm not sure if this issue should be solved by the renderer itself or if it would be possible to adjust the type of elements which are marked to update in a View somewhere.

@f1ames
Copy link
Contributor Author

f1ames commented Jun 4, 2018

One important think I just realised. When talking about benefits of this improvement:

The main benefit should be visible during composition with collaboration...

I have only checked how renderer behaves in the above situation without collaboration (single player mode 😃), but as the fix will mainly improve collaboration I will recheck if during collaborative scenarios this behaviour is also present (so the fix makes sense).

@Reinmar
Copy link
Member

Reinmar commented Jun 28, 2018

It could be implemented this way (in _updateChildren()):

const { structureUpdates, textNodeUpdates } = this._diffChildren( viewElement, inlineFillerPosition );

structureUpdates would still be an array of delete and insert actions. But if diffChildren() found that text nodes are different, instead of saying that the old one should be removed and a new one needs to be inserted, it would return an entry in textNodeUpdates (a pair of a text node to update and its new value).

@Reinmar
Copy link
Member

Reinmar commented Jun 28, 2018

Another idea is to enhance how we test the renderer. The _diffChildren() function contains a nice piece of logic that can be tested in separation from the final "update DOM" logic. Those tests would have to prepare an expected DOM plus a new view.

This will become especially useful once _diffChildren() will start to return more complex values (as proposed above).

@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the backlog milestone Oct 9, 2019
@mlewand mlewand added module:view type:task This issue reports a chore (non-production change) and other types of "todos". package:engine labels Oct 9, 2019
@Reinmar Reinmar added the domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). label Nov 4, 2021
@pomek pomek removed this from the backlog milestone Feb 21, 2022
@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

There has been no activity on this issue for the past year. We've marked it as stale and will close it in 30 days. We understand it may still be relevant, so if you're interested in the solution, leave a comment or reaction under this issue.

@CKEditorBot
Copy link
Collaborator

We've closed your issue due to inactivity over the last year. We understand that the issue may still be relevant. If so, feel free to open a new one (and link this issue to it).

@CKEditorBot CKEditorBot added resolution:expired This issue was closed due to lack of feedback. and removed status:stale labels Jan 15, 2024
@CKEditorBot CKEditorBot closed this as not planned Won't fix, can't repro, duplicate, stale Jan 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:typing/ime This issue reports a problem with standard typing & IME (typing method for CJK languages). package:engine resolution:expired This issue was closed due to lack of feedback. type:task This issue reports a chore (non-production change) and other types of "todos".
Projects
None yet
Development

No branches or pull requests

5 participants