Conversion: Re-converting the entire "thing" when it changes

[Reinmar]

Background

How does conversion work right now? Whenever a thing changes in the model, we buffer those changes with the differ to get a minimal set of them and then convert those by saying how the view should change.

This makes converters extremely complex in cases where the model and view structures diverge (take lists, todo lists or tables). In fact, even in close to 1:1 cases like the image feature there's awful lot of complexity in them and very little flexibility.

We have an ongoing discussion whether the model and view structure should diverge in the first place. If they'd be identical, converters would be quite clear. However, then a lot of it would end up in commands and, from my experience, leak to other features too.

My experience from CKEditor 4 times is that a list feature itself adds tremendous amount of problems to other algorithms such as backspacing, the enter key, inserting new content, etc. You want to insert your image in the middle of a list so the list is split? Good luck! And I'm not joking – we've been regularly seeing issues with handling lists by backspace/enter in CKEditor 4 after 6 years since that code was written. That does not happen in CKEditor 5.

In CKEditor 5 we managed to keep nearly all the complexity in a single place which is the list converters. And when tested, we don't get back to that. It's there, it works, we can forget about it. I don't think that any other feature does anything special about lists. This is a huge win. Complexity is kept under the hood of this feature and we maintain nicely decoupled solution.

Of course, there are drawbacks too. For instance, <ol/ul> attributes are a problem because these elements do not exist in the model.

Proposed direction

Quoting myself:

I always wanted to make the conversion base on as simple mechanism as possible. The complexity should be kept under the hood and should not leak to converters.

For non 1:1 mappings, the only reasonable solution I can see is doing a "reconversion" whenever we cannot handle a model change with an easy 1:1 converter.

For instance, in case of tables, a table style can be converted with attrribute-to-attribute mapping. However, table heading rows count cannot. It should re-trigger conversion of the entire table.

How could that look from code perspective? There are many possible APIs, but something that seems most imaginable today would look like this:

editor.conversion.for( 'downcast' ).elementToElement( {
	model: 'table',     // the root thing to be reconverted
	triggerBy: [
		'insert:table', // not sure about remove:table
		'insert:tableRow',
		'remove:tableRow',
		'insert:tableCell',
		'remove:tableCell',
		'attribute:headingRows:table',
		'attribute:headingColumns:table',
		'attribute:colspan:tableCell',
		'attribute:rowspan:tableCell',
	],
	view: ( conversionApi, modelTable ) => {
		return createViewTable( conversionApi, modelTable );
	}
} );

Of course, there's a lot that would have to happen under the hood and a inside createViewTable, but that'd be the gist.

That API, however, would not be sufficient to handle lists where there's no single "root". When a paragraph is inserted in the middle of a list (so, between two listItems), the conversion of a model range (a fragment of the model) would have to triggered. This could be expressed like this:

editor.conversion.for( 'downcast' ).add( dispatcher => {
	dispatcher.on( 'insert', ( evt, data, conversionApi ) => {
		if ( isElementBetweenTwoListItems( data.modelElement ) ) {
			conversionApi.reconvert( findRangeConveringListBeforeAndAfter( data.modelElement ) );
		}
	} );
} );

However, we could perhaps also change the list implementation so it would have a <list> wrapper, while still being flat (indentation kept as an attribute). In this case, the list converter would look like this:

editor.conversion.for( 'downcast' ).elementToElement( {
	model: 'list',
	triggerBy: [
		'insert:list', // not sure about remove:list
		'insert:listItem',
		'remove:listItem',
		'attribute:listIndentation:listItem',
		'attribute:listType:listItem'
	],
	view: ( modelList, conversionApi ) => {
		return createViewList( modelList, conversionApi );
	}
} );

Of course, again, a lot of complexity would need to be handled by the conversion API, like reconverting the right things (markers?), handling re-mapping, consuming everything, etc. But that code is there today anyway and I hope it would be managable.

Doubts

I know that there are certain doubts about this direction. That's most likely the reason why it wasn't implemented this way in the first place.

The main doubts I heard are:

• Flickering
  • What flickering? We talk about recreating the view structure. Plus, we don't have to recreate the entire structure – we could reuse e.g. the content of cells when reconverting the table. It does not have to be converted again. Support for slot conversion would probably help here too.
  • Now, the interesting part is the rendering. Our renderer is one of the places which is seriously under-thought. It's a piece of code worth seriously redesign and if we consider that, then reusing existing DOM structures when rendering a new one is within our limits. This can be similarity based, or id based, or mapping based. It definitely requires research, but it's what modern frameworks handle and what we handle right now too, so I'm not especially worried about this.
  • But even if we won't touch the renderer, no big deal should happen. DOM is changed synchronously. The user will not see the state between removing the previous table and inserting the new one. The only issue is with the scroll position but this can be dealt with relatively easily.
• Memory consumption will increase.
  • Not true.
  • It may increase memory pressure (make GC run more frequently), but we need to make sure there are no leaks anyway and rewriting things make it easier to control when a thing can be completely unmapped because mapping is the only thing where elements are cross-referencing themselves (all references can be cleared easily here).
• But it will make the engine more complex.
  • I'm fine with that. Every hour we spend on the engine is paid off later on when feature code is simple, maintainable and stable because there's one engine and hundreds features. That's what CKEditor 5's engine is for – it should hide the complexity under its hood.

---

If you'd like to see this improvement implemented, add a 👍 reaction to this post.

[Reinmar]

Another doubt that came to my mind: if we'd go with an API similar to what I mentioned, will features still be extensible?

That worries me, but the truth is – they are not extensible currently. E.g. #2780 that's caused by the fact that implementer couldn't possibly know how to implement the media embed converter to make it extensible, or #507 which is hard to resolve because composing block features is super tricky due to the number of events and ways that would have to consider.

[jodator]

Idea for sample/guide: some kind of icon with multiple attributes in the model and the view has conditional elements (depending on attribute value). The problem is that writing atomic converter for each attribute might be hard. Having one simple converter for the whole thing would ease that. Change of the attribute requires different rendering of the main element contents. For instance do not render some label/output if attribute is falsy - ie in a counter display only numeric values bigger then 0.

[jodator]

Slot conversion API

Technical Story: #7729

Context and Problem Statement

Converting complex model->view structures...

Decision Drivers

• Developer friendly API.

Considered Options

• Option 1: A triggerBy option that issues element re-render.
• Option 2: A triggerBy option + special <slot> view element.

Pros and Cons of the Options

Option 1: A triggerBy option that issues element re-render.

As proposed in #6510 a triggerBy is clean solution. Would work out of the box for a simple model elements, especially those without children, with many attributes triggering complex re-rendering.

Additionally, slot conversion is possible, but it seems that it would require additional API for the mapper.

editor.conversion.for( 'downcast' ).elementToElement( {
    model: 'box',
    view: ( modelBox, { writer, mapper } ) => {
        const viewBox = writer.createContainerElement( 'div', { class: 'box' } );

        // ... complex inner View for attributes.

        // Create "slots"
        for( const field of modelBox.getChildren() ) {
            const viewField = writer.createContainerElement( 'div', { class: 'box-content-field' } );

            writer.insert( writer.createPositionAt( contentWrap, field.index ), viewField );

            // This binding must be "special" -> we can't override the mapping just yet.
            mapper.bindSlotElements( field, viewField );
        }

        mapper.bindElements( modelBox, viewBox); // Seems redundant...

        return box;
    },
    triggerBy: [
        'attribute:meta:box',
        'insert:boxField',
        'remove:boxField'
    ]
} )

• 👍 - simpler API for fairly complex scenario.
• 👍 - complex view structure created in one converter.
• 👍 - all children of "slot" from the existing view don't need to be re-rendered.
• 👍 - might ease introduce JSX-like syntax for conversion.
• 👎 - because everything is done inside one converter it might lead to poorly written converters (too big callback, single responsibilty concern, hard to read).
• 👎 - the bindSlotElements looks odd to me, as whole binding mechanism here.

Option 2: A triggerBy option + special <slot> view element.

On top of Option 1 we can build a concept of "slot" elements. Here, top-level converter (if needed) might define a placeholder element to which render child.

editor.conversion.for( 'downcast' )
    // 1. Wrapper converter with intermediate "slot" elements.
    .elementToElement( {
        model: 'box',
        view: ( modelBox, { writer, mapper } ) => {
            const viewBox = writer.createContainerElement( 'div', { class: 'box' } );

            // ... complex inner View for attributes.

            // Create "slots"
            for ( const field of modelBox.getChildren() ) {
                 writer.defineSlot( field, viewBox, field.index );
            }

            mapper.bindElements( modelBox, viewBox); // Seems redundant...

            return box;
        },
        triggerBy: [
            'attribute:meta:box',
            'insert:boxField',
            'remove:boxField'
        ]
    } )
    // 2. Slot element converter.
    .elementToElement( {
        model: 'boxField',
        view: { name: 'div', class: 'box-content-field' }
    } );

• 👍 - Top-level converter is simpler.
• 👍 - Separation of concerns - provides a way to organize complex converters.
• 👍 - Potential to re-use whole rendered view element instead of only it's children if the element itself did not change.
• 👎 - Re-use of slot element might be hard to achieve if the slot element must also change (<td> vs <th> problem).

[jodator]

The base reconversion API using triggerBy is defined and implemented. It allows to reconvert simple or complex model structures. However,

• reconversion must include child views as well in the triggerBy definition because otherwise the remove converter removes too much (details here) - block reconversion use with tables
• there is no programmatic API for reconversion (table cell post-fixer sitll uses Differ.refreshItem()but it could use a new programmatic API (8252).
• docs should be written after the above issues resolved (I can start writing them now for the triggerBy
• the triggerByhandles only direct children add/remove and does not support attributes.still

[niegowski]

Outdated. Current work is tracked in #10294