Renderer does not refresh nodes when they are reinserted.

[scofalik]

Currently, when node is changed in view, for it to be re-rendered in DOM, it has to be "marked to sync". All nodes in view fire change event which is bubbled to the root. When view.Document creates view.RootEditableElement it attaches syncing callback to it:

viewRoot.on( 'change:children', ( evt, node ) => this.renderer.markToSync( 'children', node ) );
viewRoot.on( 'change:attributes', ( evt, node ) => this.renderer.markToSync( 'attributes', node ) );
viewRoot.on( 'change:text', ( evt, node ) => this.renderer.markToSync( 'text', node ) );

So all nodes in this root, when changed, bubble event to RootEditableElement and then are makred to sync. This is not true for DocumentFragment. It makes sense -- view.DocumentFragment are not rendered to DOM after all.

And here comes problematic scenario. Assume view: <rootEditableElement><div><p>foo</p></div></rootEditableElement>.

Now three actions follow:

1. Remove <div> -> <rootEditableElement> is marked to sync it's children.
2. Remove <p> -> <div> is NOT marked to sync.
3. Reinsert <div> -> it is reinserted with <p> because it was not synced.

There are some possible fixes.

First what is coming to mind is "marking to sync" nodes that are in view.DocumentFragment. Seems simple but DocumentFragment does not have a reference to view.Document so it doesn't have a reference to view.Renderer. We can't implement it like this.

Digging deeper into issue I can see that interesting things happen when nodes are updated in view.Renderer. Synced view element's children are converted to DOM. New view elements are created but existing view elements use reference to their DOM counterparts. Elements that are no longer in view are removed from DOM.

We can solve the issue doing one of two:

1. When existing elements are converted, refresh their children.
2. Unbind elements that were removed, so they are re-created.

Since solution 1. would have to preform deep refreshing, it's actually less efficient and more difficult to implement than solution 2., so I'll go with solution 2. With solution 2. we know exactly which nodes were removed so we don't have to refresh deeply.

[scofalik]

Unfortunately I have to reopen this issue cause previous solution was not enough. The problem is that sometimes DOM parent element was unbound from view before it's children were updated. Those children then were reinserted but they were not updated.

The additional fix is to sort elements to update before they are update. The order should be so that first we should update view elements, which DOM elements are deepest in the DOM tree.

[scofalik]

Unfortunately the problem is more difficult than I thought.

I had an idea that sorting and updating view elements in proper order will be fine. It took me relatively small amount of time and solved the problematic issue, so I was quite happy.

Then I started to write a test for it. I tried to come up with least-complicated scenario. After two hours I still haven't got a scenario that fails without the fix and works with it, so I decided that I will copy the problematic scenario. However the issue was related with lists and undo so I figured that I will just analyze the problematic case and will do the same using just divs and view elements API.

And when I once again researched problematic case (around 12 steps of view writer actions and marking-to-sync) it appeared to me, that unfortunately the whole idea with unbounding elements between view and DOM won't solve some issues -- I was just lucky that it solved that one issue.

Here is scenario that is unsolvable:

// prepare view: root -> div "outer" -> div "child" -> p.
const viewP = new ViewElement( 'p' );
const viewDivChild = new ViewElement( 'div', null, p );
const viewDivOuter = new ViewElement( 'div', null, viewDivChild );
viewRoot.appendChildren( viewDivOuter );

// Render view tree to DOM.
renderer.markToSync( 'children', viewRoot );
renderer.render();

// Remove div "outer" from root and render it.
viewDivOuter.remove();
renderer.markToSync( 'children', viewRoot );
renderer.render();

// Remove p from div "child" -- div "child" won't be marked because it is in document fragment not view root.
viewP.remove();
// Add div "outer" back to root.
viewRoot.appendChildren( viewDivOuter );
renderer.markToSync( 'children', viewRoot );

// Render changes, view is: root -> div "outer" -> div "child".
renderer.render();
// But in DOM div "child" will have p element, because div "child" is never refreshed.

So I know this is very edgy, but on the other hand very similar problem already happened (as I mentioned I was lucky enough that my fix got it, but if the structure was a bit different may solution might have not worked).

So we either rethink marking to sync (nodes that end up in DocumentFragment have to be synced too) or existing DOM elements have to be refreshed when re-inserted (this is the other solution I proposed in the first post).

I'll go with latter solution (refreshing):

1. We should not be concerned with efficiency at the moment and rendering takes place only once, so even if there will be a lot of overhaul, it should not be that dramatic.
2. It will take less time to implement.

Edit: But TBH, I don't really like this solution, it's not renderer's fault that something did not get marked to sync... When renderer methods are called correctly, the problem would probably not exist.

[scofalik]

OTOH, its DomConverter who provides and creates DOM nodes and it does not know much about how, why or which of those nodes should be refreshed (and it should not know it)... So I'll think more about this issue...

EDIT: Okay, we can refresh while inserting the element to DOM... If that's the only solution, it will be CPU inefficient but we will have to live with it (I hate it :P).

EDIT2: I scratched that about DomConverter, actually it makes some sense to refresh nodes in DomConverter if this is the only place where we use view elements to create DOM elements.