Rendering improvements

[Reinmar]

The below is an overview of possible rendering improvement. Most of them are related to IME handling, but we also have other issues (like #4174).

Avoid unnecessary structure updates (#1417)

Background: #4296.

How does selection conversion work?

1. MODEL: <p><$text bold=true>xx</text></p> + selection[bold=true]
2. VIEW:
   1. <p><b>xx</b></p>
   2. <p>[]<b>xx</b></p> (after position conversion)
   3. <p><B>[]</B><b>xx</b></p> (during writer.wrap( selection ) )
   4. <p><B>[]xx</B></p> (after attribute conversion; we're merging to the left, so a new element <B> is left)
3. RENDERING:
   • actual DOM: <p><b>xx</b></p>
   • expected DOM <p><B>XX</B></p> (we need to replace the entire <b> element; IDEA: don't replace <b> with <B> because they are similar!)

BUT... WHAT IF SOMETHING ELSE HAS CHANGED?

• actual DOM: <p><b>xx</b></p>
• expected DOM <p><B>XXY</B></p> (don't replace <b> with <B> but recursively go deeper to apply other possible changes, see Rendering deeper changes fails to update text nodes #4174)

Possible solution:

1. Make diff( actual, expected, sameNode ) skip elements which have the same names and attributes (see view.Element#isSimilar()).
2. As a result, we won't replace <b> with <B>, which means that the view to DOM mapping will be outdated – the new view <B> element will be mapped to the new DOM <B> element which is not in the tree (because the old DOM <b> element was left there). Which means that we should update bindings after skipping such replacements. How to do that? Perhaps we can simply and blindly set new mappings after rendering: purge existing bindngs (in a limited scope), updatedDomElements.forEach( mapToUpdatedViewElement ).

---

Deep structure updates

Background: https://github.com/ckeditor/ckeditor5-engine/issues/1125

1. <p><a>Text<b>X[]</b></a></p>
2. <p><a>Text<b>X</b></a><b>Y[]</b></p> (after typing "Y"; a lot mutations)
3. <p><a>Text<b>XY</b></a></p> (the model and the view after inserting "Y")

• ACTUAL DOM: <p><a>Text<b>X</b></a><b>Y[]</b></p>
• EXPECTED DOM: <p><a>Text<b>XY</b></a></p>

---

Fillers

1. Insert the inline filler when selection is at an inline element boundary. Makes sure that typed text will be inserted into the element we want it to be. Case: typing at the end of a link natively inserts text outside that link.

   • <p>x<a>[]ZWS</a>x</p>
   • <p>x<a>yyyy[]ZWS</a>x</p>
   • <p>x<a>ZWS[]yyyy</a>x</p>

   It's still unclear where should be selection – on the left or on the right side of the filler. Perhaps, it will depend on a case.

2. Do not touch fillers when composition takes place. Case: typing in empty <b>. Assuming that we don't do unnecessary DOM changes, removing the filler immediately after a letter was typed will always break the composition - https://github.com/ckeditor/ckeditor5-engine/issues/898.

---

Misc

• Use smarter text nodes methods to update them. Case: hard to tell. But feels like the right thing to do - https://github.com/ckeditor/ckeditor5-engine/issues/403.
• Avoid selection rendering during composition. Case: may fix some issues where we incorrectly update model's selection during composition (may be frequent during more complicated autocorrect) - https://github.com/ckeditor/ckeditor5-engine/issues/460.
• Do not replace whole text nodes when only part was modified - ckeditor/ckeditor5-engine#1425.

[f1ames]

One thing regarding Avoid unnecessary structure updates, I noticed. Having content like:

<p>
    Foo
    <span class="ck-link_selected">
        <a href="https://ckeditor.com">
            <i>Li<strong>​​​​​​​{}</strong>nk</i>
        </a>
    </span>
    Bar
</p>

and then typing something (let's say A), results in renderer having 4 elements in renderer.markedChidlren: strong, p, a, i (in such order). The reason for this is that strong is marked by mutation observer and other elements by view.

And then rendering goes as follows:

1. strong -> actions: delete - it deletes inserted A,
2. p -> actions: equal, insert, delete, equal - reinserts whole link span (see below),
3. a -> actions: equal- doesn't touch DOM,
4. i -> actions: equal, insert, delete, equal - reinserts strong with proper content.

As for 2, link DOM node (which sits inside highligthing span) is removed in https://github.com/ckeditor/ckeditor5-engine/blob/f6de5f5b69947aba95a7ac07731d46772da1cf02/src/view/renderer.js#L479
line so before any actual DOM modifications 🤔 It happens because in domconverter we are unable to map it from an existing view element so the new element is created and then all children are moved from exisitng DOM element to a newly created one. As the new element is not attached to the DOM all moved children also becomes detached. It seems related to what @scofalik mentioned in https://github.com/ckeditor/ckeditor5-engine/issues/1125#issuecomment-332790058.

---

It's just an observation, haven't analyzed it in depth, and I'm not sure if it is intended behaviour or if we could/should optimize something here.

[f1ames]

Extracted renderer task to ckeditor/ckeditor5-engine#1417.

[f1ames]

After reviewing and revisiting inline fillers PR (issue #4033, PR ckeditor/ckeditor5-engine#1355) we have reached some interesting conclusions. The fact that inline filler is removed while typing inside empty text nodes is in fact caused by two things:

1. Broken mappings.
2. Unnecessary elements rerendering.

1. Broken mappings

There is a moment between start of rendering (actually between updating the View - check this https://github.com/ckeditor/ckeditor5-engine/issues/1417#issuecomment-388809745 which describes what happens and when mappings in DomConverter gets outdated) and DOM structure rendering when mappings are outdated due to View modifications (which desynchronises it with DOM). During this moment, the inline filler management takes place inside renderer and the fact that mappings are outdated causes inline filler to be removed (because it is not possible to map selection position).
If the mappings could be fixed on the beginning of the rendering so inline filler will not be removed, the significant part of #4033 PR will not be necessary.

2. Unnecessary elements rerendering

It is covered in ckeditor/ckeditor5-engine#1417 and the proposed solution will basically try to avoid elements rerendering by fixing mappings of elements, which in fact did not change, instead of rerendering them (see https://github.com/ckeditor/ckeditor5-engine/issues/1417#issuecomment-390615612 and https://github.com/ckeditor/ckeditor5-engine/issues/1417#issuecomment-390948118).

---

So there are two ways to approach the inline filler issue. Go with #4033 and ckeditor/ckeditor5-engine#1417 which means adding additional logic which in fact covers the issue caused by outdated mappings. Or try to approach it from a different angle and implement mappings updating at the beginning of the rendering. It should cover both #4033 and ckeditor/ckeditor5-engine#1417 which means quite a big improvement. We agreed with @Reinmar during F2F talk that it seems like a valid approach for both issues (see ckeditor/ckeditor5-engine#1355 (comment) for his comment).

[CKEditorBot]

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]

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).