Unnecessary logic in Renderer#render()

[Reinmar]

After yesterday's small refactoring in the renderer a CC dropped in the renderer:

The change I did yesterday was supposed to not touch any logic but apparently, it did. Namely, I think that _updateChildren() started rendering the filler itself. I tried to understand why it didn't render the filler previously but the code got complicated before the refactoring and I have problems tracking it down.

Anyway, it seems completely accidental that it didn't do it previously. According to the logic, _diffChildren() got the filler position as a param and so it was injecting filler into the expectedDomChildren. However, the _updateChildren() method didn't use the expectedDomChildren returned by _diffChildren() but another array which it created earlier... That array should also contain the filler, but maybe the duplication caused some problems. IDK. But the logic didn't make sense, for sure.

Now it seems that _updateChildren() and _updateText() always take care of rendering the filler by themselves. It seems logical that there's no need to render the filler in render() if these two functions took care of everything.

[Reinmar]

cc @f1ames

[Reinmar]

I'm analysing this comment for clues:

		// Check whether the inline filler is required and where it really is in the DOM.
		// At this point in most cases it will be in the DOM, but there are exceptions.
		// For example, if the inline filler was deep in the created DOM structure, it will not be created.
		// Similarly, if it was removed at the beginning of this function and then neither text nor children were updated,
		// it will not be present.
		// Fix those and similar scenarios.

[Reinmar]

At this point in most cases it will be in the DOM, but there are exceptions.

OK, are there?

For example, if the inline filler was deep in the created DOM structure, it will not be created.

We fixed deep structure rendering recently, so most likely that's gone now. Will write a test though.

Similarly, if it was removed at the beginning of this function and then neither text nor children were updated, it will not be present.

According to the below code, after it gets removed, we then check if we need it again and if we do, we mark the children to be rendered. So the comment seems outdated too.

		if ( this._inlineFiller && !this._isSelectionInInlineFiller() ) {
			this._removeInlineFiller();
		}

		// If we've got the filler, let's try to guess its position in the view.
		if ( this._inlineFiller ) {
			inlineFillerPosition = this._getInlineFillerPosition();
		}
		// Otherwise, if it's needed, create it at the selection position.
		else if ( this._needsInlineFillerAtSelection() ) {
			inlineFillerPosition = this.selection.getFirstPosition();

			// Do not use `markToSync` so it will be added even if the parent is already added.
			this.markedChildren.add( inlineFillerPosition.parent );
		}

[f1ames]

There was exactly one unit test covering this case earlier:

and it was: https://github.com/ckeditor/ckeditor5-engine/blob/26673a0c8cd62244d9ccdb6967f50d4e0e47414d/tests/view/renderer.js#L1315-L1351 which leads to https://github.com/ckeditor/ckeditor5-engine/issues/1014.

[Reinmar]

Cool :) So I don't even have to write a new test. This one is good.

[Reinmar]

OK, @f1ames pointed out that I lost withChildren: false in ckeditor/ckeditor5-engine@601f5f4#diff-4c87133ed830fc4ea0646426deba32cfR259.

Adding it back there indeed shows that the code which I've removed in ckeditor/ckeditor5-engine#1452 was needed.

However, it means that we need a test which proves that withChildren: false is needed in that place. @f1ames I leave that to you. If you can figure out a test which proves that, then you can add it on master (together with the missing withChildren param. We cannot guess here.

[f1ames]

Just to add some insights here to show how the code works with withChildren: false and without it (default value is withChildren: true). The mentioned unit test starts with the HTML structure:

<ul1>
    <li1>Foobar.</li1>
    <li2>FILLER[]<div></div></li2>
</ul1>

and then it moves li2 inside new ul element (lets name it ul2) and places it inside li1 resulting in:

<ul1>
    <li1>
        Foobar.
        <ul2>
            <li2>FILLER[]<div></div></li2>
        </ul2>
    </li1>    
</ul1>

then ul1 and li1 elements are marked to be rendered. Now what happens during the renderer.render() call:

withChidlren: false

1. The renderer._updateChildrenMappings() method is called, nothing gets remapped here as there are no similar elements modified.
2. Then the renderer._updateChildren() method is called for ul1 element. As one of its children was removed (li2), the diff results in equal, delete - li1 stays in the DOM and li2 is removed (here the inline filler is lost).
3. Then renderer._updateChildren() method is called for li1. Since its children has changed, the diff result is equal, insert. The text:Foobar. stays as it is and the whole ul2 element is inserted¹².
4. Finally, as inline filler is not present it gets inserted.

withChidlren: true

1. The renderer._updateChildrenMappings() method is called, nothing gets remapped here as there are no similar elements modified. However, while mapping li1 element to DOM also its children are mapped which detaches li2 from the DOM² resulting in changing DOM structure to:

<ul1>
    <li1>Foobar.</li1>
</ul1>

2. Then renderer._updateChildren() method is called for ul1 element. As there is only one child as expected, the diff result is equal and no rendering takes place.
3. Then renderer._updateChildren() method is called for li1. Since its children has changed, the diff result is equal, insert. The text:Foobar. stays as it is and the whole ul2 element is inserted. The ul2 already contains inline filler as it contains whole DOM structured (li2) moved during mapping in the 1st step above.

---

¹ Here, the mechanism inserting inline filler is not triggered as filler.parent is li2 and renderer._updateChildren() method operates on li1.

² When converting elements from view to DOM, the element itself is fetched from the mappings. If it doesn't exists there the new element is created. The same happens to its children if withChildren: true option is present. New elements are created or existing ones are used. If the parent is newly created element and children exists in the DOM (like in the example above), DOM children are attached to a new parent. Since new parent is detached from the DOM, moved children also becomes detached.

[Reinmar]

@f1ames So, does the old <li2> gets reused in the withChildren:false case? Or do we create a new one?

The second question – if it gets reused, then is anything wrong with withChildren: true? Doesn't withChildren: true do the same thing, just earlier?

If so, do you think that it makes sense for updateChildrenMappings() to work like it works with withChildren: true. The method name may not be matching well this, but we kinda wanted to reuse as much of existing DOM as possible. We could rename it to reuseExistingElements() :D

cc @scofalik

[f1ames]

So, does the old <li2> gets reused in the withChildren:false case? Or do we create a new one?

With withChildren: false inside the renderer._updateChildrenMappings(), li2 is not reused. It is removed and then whole <ul2> element is created. That's why inline filler needs to be reinserted.

If so, do you think that it makes sense for updateChildrenMappings() to work like it works with withChildren: true.

I'm really not sure about this. I stumbled upon this case at least two times in the past and it was quite confusing that the DomConverter was changing the actual DOM. True that it helps reusing existing DOM elements but still it doesn't fell right IMHO. And even though we reuse some part of the DOM structure, it is still detached (when attaching to a new parent) and then attached to the DOM so the number of insert/remove operations on the DOM is the same (the only difference is that we do not create new DOM structure).