upcastStyleToAttribute() helper is called in wrong context

[pomek]

📝 Provide detailed reproduction steps (if any)

ckeditor5/packages/ckeditor5-table/src/converters/tableproperties.js

Lines 10 to 37 in 2eb5c73

	/**
	* Conversion helper for upcasting attributes using normalized styles.
	*
	* @param {module:engine/conversion/conversion~Conversion} conversion
	* @param {Object} options
	* @param {String} options.modelElement
	* @param {String} options.modelAttribute
	* @param {String} options.styleName
	* @param {Boolean} [options.reduceBoxSides=false]
	*/
	export function upcastStyleToAttribute( conversion, { modelElement, modelAttribute, styleName, reduceBoxSides = false } ) {
	conversion.for( 'upcast' ).attributeToAttribute( {
	view: {
	styles: {
	[ styleName ]: /[\s\S]+/
	}
	},
	model: {
	name: modelElement,
	key: modelAttribute,
	value: viewElement => {
	const normalized = viewElement.getNormalizedStyle( styleName );
	return reduceBoxSides ? reduceBoxSidesValue( normalized ) : normalized;
	}
	}
	} );
	}

Replace the model.value function with the snippet:

viewElement => {
	const normalized = viewElement.getNormalizedStyle( styleName );

	console.log( {
		viewElementName: viewElement.name,
		modelElementToSet: modelElement,
		modelAttributeToSet: styleName,
		attributeValue: reduceBoxSides ? reduceBoxSidesValue( normalized ) : normalized
	} );

	return reduceBoxSides ? reduceBoxSidesValue( normalized ) : normalized;
}

Then, open the table properties manual test and set the editor data with the following table:

editor.setData(`<table style="float: left;width:300px;height:250px;border: dashed 3px hsl(0, 0%, 60%);background: #00f"> <tr> <td>Foo.</td> <td style="width:100px">Foo.</td> </tr> <tr> <td>Foo.</td> <td>Foo.</td> </tr> </table>`);

	<table style="float: left;width:300px;height:250px;border: dashed 3px hsl(0, 0%, 60%);background: #00f">
		<tr>
			<td>Foo.</td>
			<td style="width:100px">Foo.</td>
		</tr>
		<tr>
			<td>Foo.</td>
			<td>Foo.</td>
		</tr>
	</table>

The helper function is used for upcasting all table and table cell properties. I'd like to focus on a single property, e.g. "width".

✔️ Expected result

I'd expect two logs for the width property.

1. {viewElementName: "td", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "100px"}
2. {viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "300px"}

These logs are displayed in proper context: when upcasting the table (view) element, set the table (model) element. When upcasting the td (view) element, set the the tableCell (model) element.

Combinations (view->model): td->table and table->tableCell are invalid and should not be logged.

❌ Actual result

"width" property

{viewElementName: "td", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "100px"}
{viewElementName: "td", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "100px"}
{viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "300px"}
{viewElementName: "table", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "300px"}

Full log

{viewElementName: "td", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "100px"}
{viewElementName: "td", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "100px"}
{viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "300px"}
{viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "height", attributeValue: "250px"}
{viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "background-color", attributeValue: "#00f"}
{viewElementName: "table", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "300px"}
{viewElementName: "table", modelElementToSet: "tableCell", modelAttributeToSet: "height", attributeValue: "250px"}
{viewElementName: "table", modelElementToSet: "tableCell", modelAttributeToSet: "background-color", attributeValue: "#00f"}

📃 Other details

While working on the default table properties, I wanted to disallow upcasting properties if their values are equal to default properties specified in the editor configuration.

So, I passed the default value for each property and updated the console.log:

console.log( {
	viewElementName: viewElement.name,
	modelElementToSet: modelElement,
	modelAttributeToSet: styleName,
	attributeValue: reduceBoxSides ? reduceBoxSidesValue( normalized ) : normalized,
	shouldSetValue: value !== defaultValue
} );

Input data:

<table style="float: left;width:300px;height:250px;border: dashed 3px hsl(0, 0%, 60%);background: #00f">
	<tr>
		<td>Foo.</td>
	</tr>
</table>

Just a single table cell. And filtred console.logs calls (only width property is displayed):

{viewElementName: "table", modelElementToSet: "table", modelAttributeToSet: "width", attributeValue: "300px", shouldSetValue: false}
{viewElementName: "table", modelElementToSet: "tableCell", modelAttributeToSet: "width", attributeValue: "300px", shouldSetValue: true}

The main question is – why the converter was called for the tableCell element if the input data does not contain any "style" attribute?

---

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

[pomek]

After changing the conversion to event-based, everything works as I expected:

export function upcastStyleToAttribute( conversion, options ) {
	const {
		viewElementName,
		modelAttribute,
		styleName,
		defaultValue
	} = options;

	conversion.for( 'upcast' ).add( dispatcher => dispatcher.on( 'element:' + viewElementName, ( evt, data, conversionApi ) => {
		if ( !data.modelRange ) {
			return;
		}

		if ( !data.viewItem.hasStyle( styleName ) ) {
			return;
		}

		const matcherPattern = {
			styles: [ styleName ]
		};

		// Try to consume appropriate values from consumable values list.
		if ( !conversionApi.consumable.test( data.viewItem, matcherPattern ) ) {
			return;
		}

		const modelElement = [ ...data.modelRange.getItems( { shallow: true } ) ].pop();
		const value = data.viewItem.getNormalizedStyle( styleName );

		conversionApi.consumable.consume( data.viewItem, matcherPattern );

		if ( value !== defaultValue ) {
			conversionApi.writer.setAttribute( modelAttribute, value, modelElement );
		}
	} ) );
}

[pomek]

The event-based conversion resolves my case, but it does not fix the issue.

Please, look at the code:

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

Lines 871 to 878 in 41bf7a3

	const modelKey = config.model.key;
	const modelValue = typeof config.model.value == 'function' ?
	config.model.value( data.viewItem, conversionApi ) : config.model.value;
	// Do not convert if attribute building function returned falsy value.
	if ( modelValue === null ) {
	return;
	}

modelValue is called too early. Instead of parsing the model value when the attribute would be converted, it is called every time, even if the conversion will not happen.

Moving the above code below consumable.test() resolving the original issue:

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

Lines 871 to 890 in 41bf7a3

	const modelKey = config.model.key;
	const modelValue = typeof config.model.value == 'function' ?
	config.model.value( data.viewItem, conversionApi ) : config.model.value;
	// Do not convert if attribute building function returned falsy value.
	if ( modelValue === null ) {
	return;
	}
	if ( onlyViewNameIsDefined( config.view, data.viewItem ) ) {
	match.match.name = true;
	} else {
	// Do not test or consume `name` consumable.
	delete match.match.name;
	}
	// Try to consume appropriate values from consumable values list.
	if ( !conversionApi.consumable.test( data.viewItem, match.match ) ) {
	return;
	}