Renderer – take 2

[Reinmar]

We apparently came to the tipping point of what we can do with the current Renderer's implementation.

For the last 3 years, we've been extending its initial implementation. We added a lot of bug fixes and improvements. But the architecture and the outline of its implementation remained the same.

What we see now are issues like:

• The domConverter should not modify DOM structure #4371
• Unnecessary logic in Renderer#render() #4370
• Always render text nodes via 'renderer._updateText' #4353

They show that it became really hard to work on it further. It's too much accidental development now, which, together with a high essential complexity results in reduced stability and potential for further improvements.

What we need to do is to design a new Renderer – both its API and the algorithm for updating the DOM. We should also rethink the idea of DomConverter because it proved to create confusion.

[Reinmar]

One design problem of the current Renderer came out when we were working on ckeditor/ckeditor5-engine#1455. I pushed some tests to t/1456 which shows the problem:

	// <ul><li1>Foo</li1><li2><b>Bar</b></li2></ul>
	// ->
	// <ul><li1>Foo<ul><li2><b>Bar</b><i>Baz</i></li2></ul></li1></ul>
	it( 'renders changes made in detached elements', () => {
		const ul = parse(
			'<container:ul>' +
				'<container:li>Foo</container:li>' +
				'<container:li><attribute:b>Bar</attribute:b>Baz</container:li>' +
			'</container:ul>' );

		// Render the initial content.
		view.change( writer => {
			writer.insert( Position.createAt( viewRoot ), ul );
		} );

		// Make some changes in a detached elements (li1, li2).
		view.change( writer => {
			const li1 = ul.getChild( 0 );
			const li2 = ul.getChild( 1 );

			writer.remove( Range.createIn( ul ) );

			const innerUl = writer.createContainerElement( 'ul' );

			writer.insert( Position.createAt( innerUl ), li2 );
			writer.insert( Position.createAt( li1, 'end' ), innerUl );

			const i = writer.createAttributeElement( 'i' );

			writer.wrap( Range.createFromParentsAndOffsets( li2, 1, li2, 2 ), i );

			writer.insert( Position.createAt( ul ), li1 );
		} );

		expect( domRoot.innerHTML ).to.equal( '<ul><li>Foo<ul><li><b>Bar</b><i>Baz</i></li></ul></li></ul>' );
	} );

What you'll get currently is <ul><li>Foo</li></ul>.

That's because all the operations in li1 are done when it's detached, so they are not bubbled up to the root, so the renderer is not notified about them. As a result, it doesn't know that it needs to update the li1's content.

This shows that we:

• either need to redesign the bubbling mechanism so it works with detached nodes too – e.g. we could always bubble to the document (although, what about document-less nodes which we use e.g. in data processors?)
• should make the Renderer watch for changes in nodes which it once rendered – not only these which are in the tree.

Right now, the "nodes watching" logic is implemented in the View. It seems that it should be moved to the Renderer so it's responsible for tracking everything it needs for its job. It would better encapsulate the logic of the Renderer, making it more independent from other classes.

This is in line with other observations that we had. Right now, all Renderer's tests are unit tests which call markToSync() manually. But that's completely artificial, because in real life that method is called automatically by View. It means that Renderer's tests are not realistic and changes in e.g. Writer's logic would not affect them. E.g. if Writer would detach some nodes, change them and then append them in a new place them instead of changing them and then moving to the new place, we'd get different markToSync() calls. That affects what Renderer will render but it'd be caught by the tests.

Regardless of what kind of tests are better for Renderer, if we'll better encapsulate it, we'll make it safer.

We discussed briefly with @pjasiun and @f1ames how the architecture could look and we came up with this:

• LiveRenderer – renderer used for the editing pipeline
  • bindings – an integral part of this renderer – it needs to maintain them; the end of a public DomConverter
  • DomConverter – the logic how to handle nodes which are already bound or not should be kept inside LiveRenderer (and not outside like today), so that means we'll only need a private DomConverter with a single responsibility – a state-less factory for creating DOM nodes out of view nodes and back
  • filler config – the only configurable thing in the renderer; similar to what we have today
• StaticRenderer – renderer used for the data output
  • DOMFactory – the same factory as used in LiveRenderer
  • filler config

The markToSync() method would be gone, leaving both renderers with a single render() method. Additionally, LiveRenderer will also have to expose methods for accessing bindings.

It's still to discuss how LiveRenderer should observe existing nodes, how and when to stop referencing them and what should be the general outline of the render()'s logic.

[Reinmar]

One more thing that I'd love to do before we'll work on this is a research on:

• performance of the current renderer – how does it affect general editor performance (see e.g. Editor Hangs When Loading Large Document #1041)
• good practices and some tricks used by other virtual DOM libraries (e.g. we never used the trick with reusing a pool of elements and cloning them instead of creating, which proved to be faster when we checked that 4 years ago :D)

[pjasiun]

As @Reinmar already mentioned DomConverter and Renderer were created a long time ago, when everything was simpler. DomConverter supposed to be the common part of the data and editing pipelines, and from the time perspective, I see it might be a mistake. What data pipeline (data processors) needs is a very simple tool to transform dom to view and view to dom. With no binding, no reusable elements, etc.. It might be just something like createDomFromView( viewElement, fillerElement );, nothing more.

Renderer on the other hand, for the editing pipeline, need to handle much more. The fact that big part of its logic is now in the DomConverter (i.e. bindings) does not help in writing algorithms. At the same time, another part of the renderer which is listening on view changes, is now in the main View class, what might be also wrong. After years of bug fixing I believe that renderer should have simple API: gets view root element to follow, DOM root element in which content should be rendered and render method to tell when the rendering should happen. And that's it. All of the logic should be kept in the renderer. It should give us much more control.

good practices and some tricks used by other virtual DOM libraries

Agree, but keep in mind that there is a significant difference between popular virtual DOM libraries and an editor. In the editor, based on the contenteditable attribute, HTML can be changed not only by the renderer but also by the user. And we can not ignore (overwrite) these changes, because we may break the composition. This this the problem most (all?) virtual DOM libraries do not need to care about.

[f1ames]

Recently we have encountered an issue described by @Reinmar in https://github.com/ckeditor/ckeditor5-engine/issues/1456#issuecomment-402424524 (manipulating detached nodes) - https://github.com/ckeditor/ckeditor5-engine/issues/1560.

[Reinmar]

Oh my god, I've just stumbled upon #4353 when debugging some IME/typing related scenarios with @oleq and it's a nightmare.

The role of the renderer should be to take any new view, even completely rebuilt, and apply reasonable (minimal) changes to the DOM.

Why do I write "reasonable" instead of "minimal"? Because of complexity.

There are cases where we have to go for minimal changes. But in many, especially if the view changed heavily, we can completely rebuild the DOM. 

When can we take shortcuts? For example, we could have an assumption that if an instance of a view element changes (e.g. one <td> is replaced with another <td>), even though if that's still the same element (same name and attributes), we replace it in the DOM with a completely new one. That's what @jodator postulated in #7729. Otherwise, the renderer would need to be too smart to reuse existing DOM elements if the view changed significantly. And I think that this is what happened to the current renderer. I'd rather think how we can move the complexity to conversion and @jodator is researching now how we could implement memoization there.

However, there are cases when we shouldn't cut corners. One case where that has hard to predict consequences is when you type. The smaller the changes in the DOM, the better and it shouldn't be rocket science. In case of text nodes, while diffing we should treat text nodes as "similar" and after updating the structure, make minimal changes to update outdated text nodes. In other words, #4353 (comment).

The approach to that must be SAFE. The current renderer is too reliant on what happens outside. Currently, the renderer assumes (incorrectly) that if a text node changed in the view from "foo" to "foob" it will be in Renderer#markedTexts. This isn't true, as only text nodes of which data were changed end up there... but there's no downcast writer method to change a text node value 🤦 . A text node is always replaced with a new one.

Let's therefore only mark elements to be checked and if we know that a text node changes in the view (may perhaps happen if two text nodes are merged), let's mark the parent element to be updated.

[niegowski]

Related comment from the community - #9376 (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]

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