Writing an event-based elementToElement upcast converter should be simpler

[Reinmar]

📝 Provide a description of the improvement

Right now, if you need to do something at least slightly custom (so elementToElement() is not an option to you), you need to copy 90% of this code:

https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L544-L618

We should clean up and split this algorithm into set of function that one could pick from.

• https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L572 – why do we check whether we can consume it after calling getModelElement()? Can't the order be changed? (edit: yep, already refactored)
• https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L578-L587 + https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L595-L616 => conversionApi.safeInsert()?
• https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L590 – position could be replaced with modelElement to make it shorter
• https://github.com/ckeditor/ckeditor5/blob/79221ae/packages/ckeditor5-engine/src/conversion/upcasthelpers.js#L593 – I'd check whether this cannot be moved closer to the top or other code related to consuming the element.

---

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

[Reinmar]

cc @scofalik @jodator

[Reinmar]

BTW, the best proof that this needs to be simplified is https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-code-block/src/converters.js#L197-L239.

[jodator]

@Reinmar I'm reading this issue backwards so I'll touch the copy-paste example. When I was starting with table I was convinced that this is the proper way of doing it. Lately we was proposing "additive" converters - ie such that adds something to a previously converted element.

Compare my proposal (not yet issued) for changing the table cell upcast conversion:

https://github.com/ckeditor/ckeditor5/compare/i/6XXX-simplify-table-cell-upcast?expand=1

This is much simpler example than code block. The table cell converter would only ensure that upcasted table has <paragraph> inside. This can be also arguable whether to use a model post-fixer but for the sake of this conversation I'll assume that we need clean model and post-fixing is a pattern to avoid.

The side question (or a thing to consider) is maybe some of the converters can be written differently?

Other than that I think that such converters could be simplified and better utils for those tasks are welcome :)

[Reinmar]

TODO:

• Propose a better API for prepareToElementConverter() and prepateToAttribute() converter.
• Update existing copies of that converter (code-block, tables, etc.).
• Documentation – in API docs and some more in conversion deep dive.

[jodator]

🚧 WiP 🚧

Right this post will act as a placeholder for longer notes - ATM I've only checked where the custom element-to-element conversion happens and what's happening there.

---

Upcast

upcastElementToElement()

Notable "overriders":

• ckeditor5-code-block/src/codeblockediting.js
• ckeditor5-heading/src/title.js
• packages/ckeditor5-image/src/image/converters.js
• ckeditor5-link/src/linkimageediting.js
• ckeditor5-list/src/converters.js
• ckeditor5-table/src/converters/upcasttable.js (2x)

Common points

• consuming & testing view elements (might be merged). Two flavours: I. test+consume, II. consume only.
• custom model element creation/calculations - the actual converter logic (+consuming)
• split to allowed parent & inserting new element (safeInsert())

Other potential improvements

1. Consume name by default: consume( data.viewItem, { name: true } ) -> consume( data.viewItem ) (but only handful of occurrences).
2. Shorten convertChildren( data.viewItem, modelElement )

upcastElementToAttribute() / upcastAttributeToAttribute()

• ckeditor5-list/src/todolistediting.js
• ckeditor5-restricted-editing/src/restrictededitingmode/converters.js
• ckeditor5-table/src/converters/tableproperties.js
• ckeditor5-image/src/imagestyle/imagestyleediting.js (combo element + attribute)

POC examples

RawHtml

• copies upcast element converter
• consumes an element & do not convert children

---

Other API improvements

config.view = undefined

Introduced alongside getSplitParts PR.

That one was tricky and used only for to <paragraph> conversion.

IMO The problem with this that we try to convert any view element to a paragraph but we made that in the low level converter. It gives two weird if() / return constructs.

Proposed changes:

1. Disallow (or remove support for) empty config.view.
2. Optionally introduce Matcher.any object - will match any view item explicitly.

[jodator]

I've added two new methods:

1. conversionApi.sefeInsert( modelElement, positionOrParentElement )
2. conversionApi.updateConversionResult( modeleElement, data )

The usage is:

const wasInserted = conversionApi.safeInsert( modelElement, data.modelCursor ) );

if ( !wasInserted ) {
    return;
}

conversionApi.consumable.consume( modelElement, { name: true } ); // might be anywhere...

conversionApi.convertChildren( modelElement );

conversionApi.updateConversionResult( modelElement, data );

I've used it in conversion of:

• table feature: b8ba0aa
• figure upcast: ad8e93a (small though)
• title upcast: 0e8bbca
• code block: 4b71659

I've failed with list feature conversion but from bird's eye it might be because the list converters doesn't use splitToAllowedParent() but I didn't dig much into it.

---

As for RawHtml POC, it could look like this:

editor.conversion.for( 'upcast' ).add( dispatcher => {
    dispatcher.on( 'element:raw-html', ( evt, data, conversionApi ) => {
        const viewItem = data.viewItem;
        const viewItemContent = stringifyViewItem( viewItem );

        const modelElement = conversionApi.writer.createElement(
            'rawHtml',
            { rawHtmlContent: viewItemContent }
        );

        conversionApi.safeInsert( modelElement, data.modelCursor );

        conversionApi.consumable.consume( viewItem, { name: true } );

        conversionApi.updateConversionResult( modelElement, data );
    } );
} );

But it could also look like this:

editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'raw-html',
    model: ( viewItem, writer, conversionApi ) => {
        const viewItemContent = stringifyViewItem( viewItem );
        const modelElement = conversionApi.writer.createElement(
            'rawHtml',
            { rawHtmlContent: viewItemContent }
        );

        for ( const child of viewItem.getChildren() ) {
            conversionApi.consumable.consume( child, { name: true } );
        }

        return modelElement;
    }
} );

ps.: To my surprise this also works:

editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'raw-html',
    model: ( viewItem, writer ) => {
        const viewItemContent = stringifyViewItem( viewItem );

        return writer.createElement( 'rawHtml', { rawHtmlContent: viewItemContent } );
    }
} );

Because the POC uses new UpcastWriter().createDocumentFragment( contentContainer.getChildren() ) which removes view children of the raw-html view element thus preventing them from a conversion 🤷‍♂️.

[scofalik]

safeInsert() would need to work in a way that it first checks if the insert can be performed if you want to change the order. But maybe it would be better to have that split into two functions, so this would be the pattern:

if ( !conversionApi.checkInsert( ... ) ) {
    return;
}

if ( !conversionApi.consumable.consume( ... ) ) {
    return;
}

conversionApi.safeInsert( ... );

Maybe then we could merge safeInsert() and updateConversionResult(). It would make sense that such inserting is operating on data.modelRange and data.modelCursor so it updates it too. WDYT?

[scofalik]

Okay, it seems that safeInsert() will not perform any changes on the model if inserting was not possible. In this case, you can change order and your changes are correct. Still, we could think about joining safeInsert() and updateConversionResult() into one function.

[jodator]

Still, we could think about joining safeInsert() and updateConversionResult() into one function.

AFAICS - Not doable because of convertChildren() and the reason that any child can break currently converted element. This means that you have to update the result ranges after children conversion in case currently converted element go split into parts and you need to set data.modelCursor in the second part of split result.

[scofalik]

Okay. I don't have any other remarks.

[scofalik]

conversionApi.safeInsert( modelElement, data.modelCursor ) );
conversionApi.convertChildren( modelElement );
conversionApi.updateConversionResult( modelElement, data );

But... cannot this be moved into one function? Assuming that we already checked that the element can be inserted.

[jodator]

conversionApi.safelyInsertAndConvertChildrenIfPossible( modelElement, data );

Looks weird - or I don't have a better name for that.

[Reinmar]

• figure upcast: ad8e93a (small though)

I was trying to understand the code in ckeditor5-image and I hate how hard to read this is ;/ It's doing some smart things, but I highly doubt I'd write it the same way myself. In fact, #2780 proves that it's non-obvious how to write such a converter. But I guess it's beyond the scope of this ticket. 

But... cannot this be moved into one function? Assuming that we already checked that the element can be inserted.

I also can't think of a reasonable API. Second, if you write a custom converter, there's usually something custom that you'll want to do at one of the stages or drop one of the stages. In fact, ACAICS, none of the cases that @jodator improved would benefit from this method.

[Reinmar]

So, anyway, it looks good to me so far. I think the direction makes sense.

The missing piece for me will be of course APi docs but also a chapter about writing an event based converters in https://ckeditor.com/docs/ckeditor5/latest/framework/guides/deep-dive/conversion/conversion-introduction.html. It's a broader topic than just e2e converters, so we'd need some short general intro. I can write that part. But then we need an example of switching from e2e to a custom converter. If you could prepare something, that'd speed up the rest.

Also, do we want to review other helpers in the same way? a2e, a2a? If so, perhaps it'd be good to do the research before actually committing changes done in this ticket. IDK if that'll be possible, but the APIs should be consistent.

[jodator]

To not forget - some API might be removed. For instance the getSplitParts is now not needed AFAICS.