The domConverter should not modify DOM structure

[f1ames]

Extracted from https://github.com/ckeditor/ckeditor5-engine/issues/1451#issuecomment-401356437:

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, DOM children are attached to a new parent. Since new parent is detached from the DOM, moved children also becomes detached.

and also mentioned earlier in https://github.com/ckeditor/ckeditor5-engine/issues/1125#issuecomment-332790058:

There are two bugs connected with this and related tickets. Both are caused by the fact that DomConverter sometimes returns references (when elements are already bound) and sometimes create new elements. It's also confusing for the programmer, because it is hard to say what you can do with the results got from DomConverter. For example, you should not append returned elements to other elements - if those were already in DOM/View tree, you mess up the current structure. Another problem is that this indirectly mixes responsibilities of Renderer and DomConverter. If we go with the original idea to refresh nodes in viewToDom then why we need a Renderer in the first place -- the refreshed changes will be already applied in DOM.

The main issue here is that even though only renderer should be responsible for updating DOM structure (apart from external actions ofc), in some cases domConverter also modifies it before rendering which looks like rather unpleasant side effect, may be confusing and hard to debug and mixes responsibilities of renderer and domConverter. It may lead to some hard to understand cases too - #4370.

As we have introduced some renderer improvements, especially deep changes rendering (#1417), maybe such mechanism is not needed anymore and domConverter could always create new DOM elements. The renderer itself will detect and prevent rerendering of similar elements.
Or maybe there are other ideas how we can improve this situation? cc @Reinmar @pjasiun @scofalik

[pjasiun]

DomConverter is part of the logic shared between renderer and data processors. It means that the code which is common for both data and editing pipelines need to go to the dom converter. Data pipeline does not use the renderer, uses only DomConverter directly. The logic which is limited to editing pipeline can be kept in the renderer (but does not need to, if you feel that this code fit better DomConverter it is fine to put it there).

This is why DomConverter need to be able to create DOM tree based on the view tree. However, we may split viewToDom into two methods: getDomForView and createDomFromView. This is an old issue that it is not clear if viewToDom should or should not create something.

[f1ames]

This is why DomConverter need to be able to create DOM tree based on the view tree. However, we may split viewToDom into two methods: getDomForView and createDomFromView. This is an old issue that it is not clear if viewToDom should or should not create something.

Creating DOM tree itself is not a problem here and it could stay in a DomConverter. The problem is that in some cases when the DOM tree is created, already existing DOM elements are used and detached from the DOM (attached to newly created DOM parent). Is it important for other data processor that structure created by DomConverter contains DOM elements which already exists in the DOM or it could be just newly created elements?

[pjasiun]

All what data processors do is to create new DOM from view. Data processors do not reuse any nodes:

https://github.com/ckeditor/ckeditor5-engine/blob/66bf132bbb915515c18318cb7e2869670bb7b90a/src/dataprocessor/htmldataprocessor.js#L61

[Reinmar]

The problem is that in some cases when the DOM tree is created, already existing DOM elements are used and detached from the DOM (attached to newly created DOM parent).

Completely agree. DomConverter gets way too far in this. Its name and method names say that it's about "conversion", while it does a lot more. So, either we should split responsibilities here or think about better naming.

[pjasiun]

To make it clear I also agree that DomConverter is messy. There is no need to keep it that complicated, methods should be clear and should not do renderer work. However, there should be a method to create DOM structure based on the view structure (and vice versa) for data processors usages.

[f1ames]

DomConverter gets way too far in this. Its name and method names say that it's about "conversion", while it does a lot more. So, either we should split responsibilities here or think about better naming.

Agree 👍 Renaming could be easier, but probably splitting some methods will be needed to.

However, there should be a method to create DOM structure based on the view structure (and vice versa) for data processors usages.

Yes, I agree that it should stay inside DomConverter. The main reason this issue was created is that during view to DOM conversion, in some cases, existing DOM is modified and some its elements detached (withChildren: true flag):

https://github.com/ckeditor/ckeditor5-engine/blob/66bf132bbb915515c18318cb7e2869670bb7b90a/src/view/domconverter.js#L229-L233

This is described in more details in https://github.com/ckeditor/ckeditor5-engine/issues/1451#issuecomment-401356437.

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