Conversion utilities: abandon multiples values attribute converters

[pjasiun]

We based our conversion utils, among others, on definition-based-converters. This util was focused on the specific case, where the model attribute is an enum with several possible values. This lead to the API which is very inconvenient in other cases.

Refactoring converters we realized that attributes values come in 4 types:

• boolean (like bold),
• string (link img href),
• object (like img srcset),
• enum (like font family).

Most of them need only a single converter (often a callback).

Because we focus on creating conversion utilities the way that you can define multiple values they look like this for boolean:

editor.conversion.for( 'downcast' ).add( downcastAttributeToElement( 'bold', { view: 'strong' } ) );
editor.conversion.for( 'upcast' ).add( upcastElementToAttribute( { view: 'strong', model: 'bold' } ) )

And like this for callbacks:

editor.conversion.for( 'downcast' )
	.add( downcastAttributeToElement( 'linkHref', { view: linkHref => createLinkElement( linkHref ) } ) );

editor.conversion.for( 'upcast' )
	.add( upcastElementToAttribute( {
		view: { name: 'a', attribute: { href: true } },
		model: {
			key: 'linkHref',
			value: viewElement => viewElement.getAttribute( 'href' )
		}
	} ) );

They are very unsymmetrical: in downcastElementToElement and all upcast* methods you pass object with model and view properties, but for downcastAttributeToElement and downcastAttributeToAttribute parameters are very different. I updated multiple converters to the new API and had to check docs every time to be sure which one should be defined which way because as long as you do not use enum it is not natural to write converters like this.

Also, since upcastElementToAttribute accepts only a single attribute value to be set, this does not solve any problem.

My proposal is to do it this way.

For downcastAttributeToElement:

downcastAttributeToElement( { model: 'bold', view: 'strong' } );

downcastAttributeToElement( { model: 'bold', view: 'strong' }, 'high' );

downcastAttributeToElement( { model: 'bold', view: new ViewAttributeElement( 'strong' ) } );

downcastAttributeToElement( {
	model: 'bold',
	view: {
		name: 'span',
		class: 'bold'
	}
} );

downcastAttributeToElement( {
	model: {
		key: 'styled',
		value: 'dark'
	}
	view: {
		name: 'span',
		class: [ 'styled', 'styled-dark' ]
	}
} );

downcastAttributeToElement( {
	model: {
		key: 'fontSize',
	 	value: 'big'
	}
	view: {
		name: 'span',
		style: {
			'font-size': '1.2em'
		}
	}
} );
downcastAttributeToElement( {
	model: {
		key: 'fontSize',
	 	value: 'small'
	}
	view: {
		name: 'span',
		style: {
			'font-size': '0.8em'
		}
	}
} );

downcastAttributeToElement( {
	model: 'bold',
	view: modelAttributeValue => new ViewAttributeElement( 'span', { style: 'font-weight:' + modelAttributeValue } )
} );

Check the current API here: https://github.com/ckeditor/ckeditor5-engine/blob/fd128a1c7ee5aef3e5aab788726af89c88059b64/src/conversion/downcast-converters.js#L78-L122

For downcastAttributeToAttribute:

downcastAttributeToAttribute( { model: 'source', view: 'src' } );

downcastAttributeToAttribute( { model: 'source', view: 'src' }, 'high' );

downcastAttributeToAttribute( {
	model: 'stylish',
	view: {
		key: 'class',
		value: 'styled'
	}
} );

downcastAttributeToAttribute( {
	model: {
		key: 'styled',
		value: 'dark'
	} 
	view: {
		key: 'class',
		value: 'styled styled-dark'
	}
} );

downcastAttributeToAttribute( {
	model: {
		key: 'styled',
		value: 'dark'
	} 
	view: {
		key: 'class',
		value: 'styled-dark'
	}
} );
downcastAttributeToAttribute( {
	model: {
		key: 'styled',
		value: 'light'
	}
	view: {
		key: 'class',
		value: 'styled-light'
	}
} );

downcastAttributeToAttribute( {
	model: 'style',
	view: attributeValue => ( { key: 'class', value: 'style-' + attributeValue } )
} );

Check the current API here: https://github.com/ckeditor/ckeditor5-engine/blob/fd128a1c7ee5aef3e5aab788726af89c88059b64/src/conversion/downcast-converters.js#L155-L195.

Then we have plugins configuration and two-way config.

Besides the problems mentioned above (inconvenient configuration for booleans, string and object attributes) there are 2 more issues. First, the config might be giant https://github.com/ckeditor/ckeditor5-engine/blob/fd128a1c7ee5aef3e5aab788726af89c88059b64/src/conversion/two-way-converters.js#L169-L230 or https://github.com/ckeditor/ckeditor5-engine/blob/fd128a1c7ee5aef3e5aab788726af89c88059b64/src/conversion/two-way-converters.js#L298-L327.

Second, is that if we will keep model string value to be the attribute key, we will get 2 confusing configuration:

• in one configuration model attributes key,
• in the other configuration model attributes value.

This is why, plugins configuration should be defined different way, to make it clear that this is attribute value and make it compatible with the downcast* configuration:

[
	{
		model: {
			value: 'right'
		},
		view: {
			key: 'class',
			value: 'align-right'
		}
	},
	{
		model: {
			value: 'center'
		},
		view: {
			key: 'class',
			value: 'align-center'
		},
		upcastAlso: {
			style: {
				'text-align': 'center'
			}
		}
	}
]

Then, we may have a util to add model.key property to the configuration and execute it. As soon as someone finds a good name for such helper, I am fine with it, but since all it will do is:

for( const config in editor.config.get( 'align' ) ) {
	config.model.key = 'align';

	editor.conversion.attributeToElement( config );
}

I think that all plugins which have to do so can do it directly in the plugin code.

Note that some plugins may do more and this code might be very specific with all normalization. Also, in some cases we need to define additionally model.name: https://github.com/ckeditor/ckeditor5-engine/issues/1290.

[jodator]

I have doubts from the configuration POV only. I think that whole idea behind configuration based converters was to give a common way to allow in a convenient way to override plugin's conversion defaults. So the examples with callbacks are overkills for that matter.

The model.key & model.value makes plugin configuration bigger. I don't have problem that in one plugin the model refers to attribute but in other plugin it is an element because the plugin will work either on attributes or on elements or will not accept multiple model values at all. Calling it simply a model in plugin configuration makes the config shorter. How this transpiles to engine methods is a second thing.

The ViewElementDefinition primary focus is to extend default plugin configuration by how one would see that plugin to behave.

Some configuration examples:

// Here the `model` is a model element
const headingConfig = [
    { model: 'heading1', view: 'h1', acceptsAlso: [ 'h2', 'h3', 'h4' ] },
    { model: 'heading2', view: { element: 'p', style: { 'font-weight': 'bold' } } }
];

// Here the `model` is a `fontSize` attribute
const fontSizeConfig = [
    { model: 'big', view: { class: 'font-big' } },
    { model: 'small', view: { class: 'font-small' } }
];

// Here there is no `model` as it is boolean type
const boldConfig = { view: 'strong' };
const anotherBoldConfig = { view: { element: 'span', style: { 'font-weight': 'bold' } } };

This way we do not enforce users to check whether it should be model: { key: 'big' } or model: { value: 'big' } it's also independent on future changes in conversion helpers API (let's say someone will have problems with model.key vs model.attribute in some time in the future).

Now how the plugin converts configuration to a conversion utilities depends on the plugin itself. Probably in most of the cases it will be some callback for enum type of configuration.

Also allowing existing markup to be cleaned up by the editor is by definition asymmetric as n view elements will be matched to 1 model value (element or attribute) but 1 model value will be always represented as 1 view element.

In other words I would not enforce plugin configuration definition by how lower lever API is constructed. (my main thought on this)

As In my opinion there will be majority of developers that will only need to adapt CKEditor input/output plugin markup then there will be developers writing plugins. So majority of them will not need to know how conversion helpers really work. So for the majority will be that for that config option magic stuff will create that HTML element instead of default HTML element (I've wrote HTML not view element on purpose here).

ps.: Another option is to completely remove model part from configuration and name it in each plugin differently:

const headingConfig = [
    { heading: 'heading1', view: 'h1', acceptsAlso: [ 'h2', 'h3', 'h4' ] },
    { heading: 'heading2', view: { element: 'p', style: { 'font-weight': 'bold' } } }
];

const fontSizeConfig = [
    { size: 'big', view: { class: 'font-big' } },
    { size: 'small', view: { class: 'font-small' } }
];

[scofalik]

As much as I agree, I have to mention two things that I had on my mind when leaving the API as it was in definition-based-converters:

1. I didn't want to have a need for another helper. two-way-converters are already helpers and now we need a helper to use a helper...
2. In downcasting, if there is enum-type attribute, if we pass all the options together in one helper call (like it is now), only one converter function will be created. This means less events calls (although more logic inside one event). Less events calls means easier debugging.

Of course above aren't necessarily big enough issues to be deciders, but I had them on my mind.

[Reinmar]

We agreed on a couple of design decisions:

• The one-way and two-way converters should be kept developer-oriented and require clear model definition. However, each of the helper functions defines its own param shape and their semantics might slightly differ so it doesn't seem to make sense to define a typedef(s) for them. Alternatively, each of these functions might use its own typedef extending the ConverterDefinition typedef defined for feature configurations (see the next point).
• Feature will use their own typedefs to document their configuration options. These typedefs should extend the ConvertionDefinition typedef defined in two-way-converters.js. This typedef will be a simplified version of what each of the converter functions accepts as it won't define the model property. The model property will be added by each feature in its own typedef because it may have a different meaning.
• We'll add priority to ConverterDefinition. This will affect the base converter but also all upcastAlso definitions. We think that in great majority of cases, when talking about configuration, the whole ConverterDefinition object can base on one priority value (the same for the base converter and upcastAlso converters). If more flexibility is needed – use the API.
• We agreed that it doesn't make sense to design a complex ConverterDefinition format with support for cases like link conversion (where value is *). This would require implementing support for patterns (aka templates) and goes too far into designing our own language. One idea here could be that features like link will need to allow configuring both – the upcast and downcast conversion separately. The config would look like the contents of the LinkEditing plugin now. Then, we can avoid templates and complicating the definition.

PS. Some clarification regarding the typedef mentioned above:

// can be defined in two-way-converters.js or directly in conversion/converterdefinition.jsdoc
@typedef ConverterDefinition
	@property view {view.ElementDefinition}
	@property [model] {*}
	@property upcastAlso {MatcherPattern|Array.<MatcherPattern>}
	@property priority {Priority}

@typedef MatcherPattern {Object|Function}

[Reinmar]

BTW, this was an incorrect change: ckeditor/ckeditor5-heading@b62d5cb.

It means that, according to the docs, right now you cannot use upcastAlso in heading's config. And that if we'd use support for priority, you wouldn't have it documented as well.

That's why we'll define the generic ConverterDefinition typedef to be extended by features. This way, whatever we'll add in there, it will be automatically "broadcasted" everywhere in the docs.

[scofalik]

Some clarifications:

1. We moved two-way-converters.js to conversion.js. This awaits in a PR. This means that ConverterDefinition will be kept in conversion.js.
2. Do I understand correctly, that ConverterDefinition is a typedef for the parameter accepted by two-way-converters? I am not sure if it will fit one-way converters (I think it won't cause we allow callbacks there).

[Reinmar]

We'll close https://github.com/ckeditor/ckeditor5-engine/issues/1243 here as well.