Redesign and introduce new reconversion system

[niegowski]

EDIT (Summary of changes)

You can find a summary of changes in the conversion system in this post: #11268 (comment)

There's also a migration guide that covers related changes: Migration to CKEditor 5 v33.0.0.

Why

What do we have now

There is a downcast helper for elementToElement with a triggerBy option that triggers reconversion of a specific model element by removing its view representation (but keeping mappings), triggering the new view structure conversion callbacks (for that model element only), and reusing the child elements' old view representations. 

What’s wrong with what we have now

To downcast a single model element to a slightly more complex structure we need to use the low-level API (so we need to take care of consuming what we converted and to bind model elements with view elements). That's because we are converting a subtree of a model structure. The reusing of view elements works sort of in a "magical" way. 

There is also an issue about the current implementation conflicting different triggerBy converters: #9641.

What could we improve in the current solution

It could be more understandable if a single elementToElement converter would handle a single model element and do not touch its children elements. The conversion callback could provide a view structure with the special placeholders (slots) that model child elements should be downcasted into.

Why do we think it can be done better?

The current triggerBy usage is very confusing. It requires to convert (in a single converter) not only an element itself but also its model child elements (and only their content is reused). This would be more declarative if we could provide conversion for a single model element to a bigger view structure with placeholders/slots for model child elements so that it could take care of inserting view elements in the correct positions in the view structure (not only as a direct child of the main element).

That slots could also have conditions for example to handle specific table rows (thead vs tbody that depends on headingRows attribute value).

What do we have prototyped so far? Where’s that code?

The early PoC is in the list reconversion branch (that includes a lot of other experiments for rangeToStructure conversion). That branch is currently broken (because of changes in progress). The most interesting function is insertSlotted that is supposed to get triggered after the old view was removed (but the view mapping was temporarily kept until the end of the conversion process to be reused while converting slots' content).

What

A high-level overview of a new mechanism

The solution could be split into layers:

• DowncastDispatcher could fire a new reduceChanges event before the conversion process starts to allow features (or conversion helpers) to alter the list of changes provided by the Differ so it could detect cases where the reconversion should be used. The re-conversion would be split into 2 events - one to remove the old view representation of a model element and one to insert a new representation. The old view elements would get removed from the view and unbound in the Mapper but it could keep temporal mapping to allow reusing view elements that got removed in the same conversion process. Those would get flushed at the end of the whole conversion.
• DowncastHelpers elementToElement and elementToStructure would listen for the reduceChanges event to register callbacks for triggerBy changes to trigger re-conversion is a specified situation.
• elementToStructure downcast helper could provide a factory for placeholders/slots so the simple view generator could put those into the structure and the helper itself would take care of converting those slot's content or reusing the view that was previously there.

Example API proposal

The high-level API:

editor.conversion.for( 'downcast' ).elementToStructure( {
    model: 'table', 
    view: ( modelElement, conversionApi ) => {
        const viewElement = conversionApi.writer.createContainerElement( 'div' );

        // ...

        writer.insert( writer.createPositionAt( viewElement, 0 ), conversionApi.slotFor( modelElement, 'children' ) );

        // ...

        return viewElement;
    }, 
    triggerBy: { 
        attributes: [ 'headingRows' ]
    }
} );

Success criteria

• Simple conversion for a single model element to a bigger view structure without taking care of mapping or consumables.
• Can be used in tables to simplify the current downcast of the whole table and enable its reconversion with the preservation of table rows and cells.
• Should be adjustable to use with a rangeToStructure future helper that will be used for the document list conversion.

Risks, other things to change.

• Temporal mappings kept for the conversion process are not yet in PoC (it was a simple one converter solution).
• It seems that markers downcasted to view should properly reconvert themself (as it works for the current reconversion).

When

Why now

The PoC was prepared while working on #9784 and #9783. This could be extracted from that PoC as it's a separate feature that would be later used by rangeToStructure downcast helper.

What’s next

The next step would be to implement rangeToStructure downcast helper and use it for document list conversion.

---

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

[niegowski]

The elementToStructure PoC is now extracted to a new branch: https://github.com/ckeditor/ckeditor5/compare/ck/10294-elementToStructure

It also implements the logic of remove + reconvert diff items.

[Reinmar]

There is also an issue about the current implementation conflicting different triggerBy converters: #9641.

Another issue: #10306 (not sure if related).

[niegowski]

I just noticed another bug in the current reconversion implementation: #10314

[dawidurbanski]

I have couple thoughts after working on adding support for reconversion in elementToElement downcast helper.

Let me put them all together in this ticket so they are at least noted somewhere.

Children prop Boolean|Function

The new API for describing model in elementToElement config object looks like this:

model: {
	name: 'items',
	attributes: [ 'mode' ],
	children: true
},

I found some obscure use cases where it could be useful if children prop accepted not only boolean but also a function returning boolean (ideally with modelElement passed). This way we could set children dynamically based on model element attribute / element type / it's children type etc.

model: {
	name: 'items',
	attributes: [ 'mode' ],
	children: ( modelElement ) => modelElement.getAttribute( 'mode' ) !== 'no-reconvert'
},

I don't have any realistic use cases for this at the moment, so it's just for the records. Probably not very important nor needed.

Docs / comments

ckeditor5/packages/ckeditor5-engine/src/conversion/downcasthelpers.js

Lines 229 to 230 in e10b4ea

	* @param {Array.<String>} [config.model.attributes] The list of attribute names that should be consumed while creating
	* the view structure. Note that the view will be reconverted if any of the listed attributes will change.

Do we have to mention consumables here? Isn't enough to say the second part:

View will be reconverted if any of the listed attributes will change.

Also, to keep it consistent with other parts of this API, shouldn't we accept both array of strings and string for single attribute?

@param {String|Array.<String>} [config.model.attributes]

---

ckeditor5/packages/ckeditor5-engine/src/conversion/downcasthelpers.js

Lines 219 to 220 in e10b4ea

	* Note: The children of a model element that's being converted must be allocated in the same order in the view
	* in which they are placed in the model.

I know it has been added as part of "improving docs" commit, but this note is still a bit unclear to me. Yeah it describes the constraint but does not say why nor what happens if you don't follow it. Will it raise an error? Will it crash?

I guess this probably might be a part of improving conversion docs and be just a matter of putting a link near this note.

@Reinmar @niegowski FYI

[niegowski]

Also, to keep it consistent with other parts of this API, shouldn't we accept both array of strings and string for single attribute?

@param {String|Array.<String>} [config.model.attributes]

The normalization accepts it, maybe it's just missing JSDoc?

[Inviz]

Hopefully this will solve issues like #10306

[dawidurbanski]

Crosslinking the summary of changes for this huge feature: #11268 (comment)