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

Improve 'sameNodes' comparator inside 'renderer._updateChildren' #4296

Closed
f1ames opened this issue Mar 5, 2018 · 2 comments · Fixed by ckeditor/ckeditor5-engine#1424
Closed
Labels
package:engine type:improvement This issue reports a possible enhancement of an existing feature.
Milestone

Comments

@f1ames
Copy link
Contributor

f1ames commented Mar 5, 2018

Stumbled upon when debugging composition:

  1. Set editor content to <strong>^Bold</strong>:
Model: <paragraph><$text bold="true">[]Bold</$text></paragraph>
View: <container:p><attribute:strong>{}Bold</attribute:strong></container:p>
  1. Type anything, e.g. a

The renderer._updateChildren method checks diff between actualDomChildren and expectedDomChildren returning [ 'insert', 'delete' ] set of operations, while both actualDomChildren and expectedDomChildren represents exactly same HTML - <strong>aBold</strong>.

If the sameNodes comparator would be able to handle such situations we would be able to skip reinserting same nodes (should also help with composition). Preventing situations like (the intermediate state after insert and before delete):

image

Checked on ckeditor5-core/tests/manual/article.html manual test with Chrome 64.0.3282.167.

If this issue was already discussed in the past somewhere just point me to the right direction, but I haven't found any similar ones. Could help with #802

@Reinmar
Copy link
Member

Reinmar commented Mar 5, 2018

The diff is based on a simple instance comparison. I agree that it'd be perfect if it was smart enough to know that it should not change the DOM when the expected child is identical. However, this is really tricky.

One issue that we'd need to solve is updating the bindings. We maintain a view <-> DOM elements map. Since expectedDomChildren contains some new DOM element instances, it means that the view document contains new view element instances as well. If we'd keep the old DOM elements, then we'd need to fix the bindings so the existing view elements point to those old, existing DOM elements. That might actually be possible. But this is a big change. Plus, the diff would need to be improved.

However, you may also ask why does the view document contain new view element instances? Because conversion from model to view does not care about such things. It purges whatever it needs. And that's fine. During conversion, the entire view should change freely. The smart rendering should be hidden inside the renderer.

Which means that we'll need to make this change one day. Renderer must be smart. Renderer must avoid doing DOM changes. However, even if we'd land this there's a big chance that IME would be still broken in some cases. E.g. by inline and block filler removal and small selection fixes which will frequently happen on inline element boundaries. That's why those are first two points in #802 (comment).

@f1ames
Copy link
Contributor Author

f1ames commented Mar 6, 2018

I mentioned it because of two things I encountered while working on blocking selection during composition:

First one is the fact that even if we stop selection manipulation during composition, it may be affected by reinserting the DOM elements - I described it in more details in https://github.com/ckeditor/ckeditor5-engine/issues/460#issuecomment-370736424.

I also checked if preventing only selection manipulation during composition will solve any of the reported issue (from #802) and it seems that non of this issues can be solved by only this change (unless my validation method was invalid). In most cases both selection and element rerendering is the cause.
From what I see rerendering DOM also affects selection so this two mechanisms are strictly related (so modifying how updating nodes works will also affect how selection updating works - but not the other way around).

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 status:confirmed type:improvement This issue reports a possible enhancement of an existing feature. 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:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants