Ckeditor5: One-way elementToElement() converters could have access to conversionApi

[Reinmar]

📝 Provide a description of the improvement

I'm writing an upcast converter that needs to consume all children of the element being converted to ensure that they are skipped.

I could use for( 'upcast' ).elementToElement() converter with a callback for the model property, but I don't have the access to the conversion API there. This detail forces me to write an event-based converter which is a pity.

There may be more things that the higher-level converters could know about.

---

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

[Reinmar]

cc @scofalik @jodator

[scofalik]

I was pretty sure this is available in the callback. Check if it is available in downcasting.

[jodator]

AFAIR most of the one-way converters has a minimal set of information in the callback. For instance attribute converters do not have access to the model element on which this attribute is set. This might be similar story in element to element converters.

ps.: maybe a simplified use-case for your converter could be helpful to check what we can do there?

[Reinmar]

I'm writing an upcast converter that needs to consume all children of the element being converted to ensure that they are skipped.

Actually, I was wrong – there's no method for consuming all children. So the solution needs to be a bit different – not calling convertChildren() will have a similar effect. But then it's a bit different issue: #7336.

Still, this issue could be valid because I might've wanted to call convertChildren().

[Reinmar]

Conclusion: Make conversionApi accessible from model/view callbacks of elementToElement() and other converters for upcast and downcast pipelines.

[jodator]

Looks it is a way to do this: https://github.com/ckeditor/ckeditor5/issues/7336#issuecomment-654704526.

[jodator]

Intro

Current element-to-element upcast converter lacks simple, but powerful addition that would allow for a simpler upcast converters (check the example of RawHTML POC simplification).

editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'div',
    model: ( viewElement, writer ) => {
        return writer.createElement( 'div' );
    }
} );

So we need to add whole conversionApi to the elementToElement()'s model() callback.

Options

Option 1: Backwards compatible

editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'div',
    model: ( viewElement, writer, conversionApi ) => {
        // API usage
        conversionApi.consumable.consume( viewElement );

        // Ambigous writer usage:
        return writer.createElement( 'div' ); // or
        return conversionApi.writer.createElement( 'div' );
    }
} );

Option 2: Backwards in-compatible

// Using object destructuring:
editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'div',
    model: ( viewElement, { writer, consumabe } ) => {
        consumable.consume( viewElement );

        return writer.createElement( 'div' );
    }
} );

// Old-fashined objects:
editor.conversion.for( 'upcast' ).elementToElement( {
    view: 'div',
    model: ( viewElement, conversionApi ) => {
        conversionApi.consumable.consume( viewElement );

        return conversionApi.writer.createElement( 'div' );
    }
} );

Pros & cons of options

Option 1 (Backwards compatible):

• Pros:
  
  
  
  • Backward compatible.
  
  
  • Fast access to most used writer, a conversionApi is an "advanced"/less common use-case.
  
  
• Cons:
  
  
  
  • Duplicated writer as parameter and in conversionAPI.
  
  

Option 2 (Backwards in-compatible):

• Pros:
  
  
  
  • No ambiguity in a callback.
  
  
  • Simple upgrade path using object destructuring model: ( viewEl, writer ) => {} -> model: ( viewEl, { writer } ) => {}.
  
  
• Cons:
  
  
  
  • Backward in-compatible.
  
  
  • Might require tedious refactoring of existing code-base.
  
  

[jodator]

I'd see option 2 to be implemented, but backwards incompatibility and some additional work is need.

[oleq]

I also like 2 the most. It's simple and future-proof (if more tools/steps arrive to the conversion pipeline).

What I'm worried about, though, is the backwards compatibility issue. Upgrading to this editor version will certainly require some work if an integration brings its own editing plugins. That's a huge cost (although the migration path is simple).

[Reinmar]

What do other callbacks (`view/model`) in other converters accept? If we change it here, we'll need to change it in other places as well. Does it make sense in all those examples?

[jodator]

@Reinmar -  as for now I see that we could unify upcast/downcast for element conversion:

Downcast

• elementToElement
  
  
  
  • ( element, writer ) => {}
  
  
  • must return element
  
  
• attributeToElement
  
  
  
  • ( attributeValue, writer ) => {}
  
  
  • AFAIR it must be writer.createAttributeElement() anyway... dunno for now :man_shrugging:
  
  
• attributeToAttribute
  
  
  
  • ( attributeValue ) => ( {} )
  
  
  • must return key, value but uses I'm lost...
  
  

Upcast

• elementToElement
  
  
  
  • ( element, writer ) => {}
  
  
  • returns element
  
  
• attributeToElement
  
  
  
  • ( element ) => {}
  
  
  • returns value
  
  
• attributeToAttribute
  
  
  
  • ( element ) => {}
  
  
  • returns value
  
  

[Reinmar]

In the topic: #7774.

[oskarwrobel]

I also vote for the option 2 for all of both upcast and downcast helpers. If we have a green light I can handle it.

[scofalik]

How about having elementValue, writer, conversionApi in all those calls. I understand that the writer is duplicated but I can live with it.

Downcast

• elementToElement
  
  
  
  • ( element, writer, conversionApi ) => {}
  
  
• attributeToElement
  
  
  
  • ( attributeValue, writer, conversionApi ) => {}
  
  
• attributeToAttribute
  
  
  
  • ( attributeValue, writer, conversionApi ) => ( {} )
  
  

Upcast

• elementToElement
  
  
  
  • ( element, writer, conversionApi ) => {}
  
  
• attributeToElement
  
  
  
  • ( element, writer, conversionApi ) => {}
  
  
• attributeToAttribute
  
  
  
  • ( element, writer, conversionApi ) => {}
  
  

OTOH, I don't see anything wrong with breaking change either. The change is cosmetic, people should be able to change their custom code easily.

Hm.

Now, as I look at it, I actually like having a breaking change more :D.

BTW. Don't forget about markers conversion. We might want to have conversionApi there too, for example, to consume the range in order to halt ("disable") conversion (to be researched if it is possible to "disable conversion" right now, and how).

[jodator]

To sum up:

1. We agree that we want callback( elementOrAttribute, conversionApi ).
2. We want to unify this for element & attribute converters.
3. Last step: adding to markers converters:

BTW. Don't forget about markers conversion. We might want to have conversionApi there too, for example, to consume the range in order to halt ("disable") conversion (to be researched if it is possible to "disable conversion" right now, and how).

AFAICS, markers callbacks are similar, so adding conversionApi there could potentially make sense. As I didn't use marker converters much, I'm relaying on you in this.

[jodator]

The simplest upgrade path is to use Object destructuring for existing callbacks:

Before:

editor.conversion.for( 'downcast' ).attributeToElement( {
    model: 'attribute',
    view: ( attributeValue, writer ) => {
        return writer.createAttributeElement( 'span', { style: 'color:blue' } );
    }
} );

After:

editor.conversion.for( 'downcast' ).attributeToElement( {
    model: 'attribute',
    view: ( attributeValue, { writer } ) => {
        return writer.createAttributeElement( 'span', { style: 'color:blue' } );
    }
} );

Or using variable name:

editor.conversion.for( 'downcast' ).attributeToElement( {
    model: 'attribute',
    view: ( attributeValue, { writer: viewWriter } ) => {
        return viewWriter.createAttributeElement( 'span', { style: 'color:blue' } );
    }
} );