Error during conversion of text inside element when specific attribute converters are defined for both elements

[f1ames]

Follow up of #1317.

Considering conversions defined like:

editor.conversion.for( 'downcast' ).add( downcastAttributeToElement( {
    model: {
        key: 'test',
        name: '$text'
    },
    view: ( value, writer ) => {
        return writer.createAttributeElement( 'span', { test: value } );
    }
} ) );

editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
    model: 'test',
    view: 'test'
} ) );

and setting model data like:

const root = editor.model.document.getRoot();
const p = writer.createElement( 'paragraph' );
const text = writer.createText( 'FooBar', { 'test': 3 } );

writer.insert( text, p );
writer.insert( p, root );

throws an Cannot read property '_setAttribute' of undefined error:

Works fine if text is inserted into root, but when inserted as a child of p the error is thrown. The interesting thing is when generic conversion is removed from the example above:

// editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
//    model: 'test',
//    view: 'test'
// } ) );

everything starts to work fine.

[pomek]

If you disable the first converter:

editor.conversion.for( 'downcast' ).add( downcastAttributeToElement( {
    model: {
        key: 'test',
        name: '$text'
    },
    view: ( value, writer ) => {
        return writer.createAttributeElement( 'span', { test: value } );
    }
} ) );

the error still occur. And it even make a sense because in the second converter:

editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
    model: 'test',
    view: 'test'
} ) );

we want to convert every [test] attribute in the model to view. Text without any wrapper cannot have any attribute.

When I changed a little the test scenario:

editor.model.change( writer => {
	const root = editor.model.document.getRoot();
	const p = root.getChild( 0 );
	const text = writer.createText( 'FooBar', /*{ 'test': 3 }*/ );

	writer.setAttribute( 'test', 3, p );
	writer.insert( text, p );
} );

In the view I got: <p test="3">FooBar</p> so it works.

[f1ames]

And it even make a sense because in the second converter we want to convert every [test] attribute in the model to view. Text without any wrapper cannot have any attribute.

☝️ It looks like the "generic" converter treats all nodes as elements and when encounters a text nodes it fails. IMHO when text node is encountered, the generic converter should act the same way as "text specific" one - wrapping text in a span with an attribute applied.

The second idea we have discussed with @pomek was to apply the attribute to text parent element but it may become tricky for more complex scenarios (e.g. same attribute with different values for text and its parent).

[scofalik]

Your converters are fine. It should work correctly, as you intended, so there is a bug somewhere. You shouldn't do anything like this:

IMHO when text node is encountered, the generic converter should act the same way as "text specific" one - wrapping text in a span with an attribute applied.

First, I think that the "generic" (or let's say attributeToAttribute converter) should not do anything because attributeToElement converter should have already consumed the attribute.

[pomek]

Recently I talked with @scofalik about the bug. I checked which converters are executed during for the test scenario. I put a console.log between those lines https://github.com/ckeditor/ckeditor5-engine/blob/bafd6359441651a5a32a7673f1538dcb0cdcc1c4/src/conversion/modelconsumable.js#L164-L166 and I printed out the item and the type.

For such scenario:

conversion.for( 'downcast' ).add( downcastAttributeToElement( {
	model: {
		key: 'test',
		name: '$text'
	},
	view: ( value, writer ) => {
		return writer.createAttributeElement( 'span', { test: value } );
	},
} ) );

conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
	model: 'test',
	view: 'test'
} ) );

model.change( writer => {
	const foo = writer.createText( 'FooBar', { test: 3 } );
	writer.insert( foo, modelRoot );
} );

Calls look:

After removing the aToA converter, calls look:

So it seems that the aToA converter is called for DocumentSelection. It sounds like an issue with an attribute that can be added for both nodes – for a text and an element.

[f1ames]

If you disable the first converter (...) the error still occur.

I have checked it with a minimal example like:

editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
	model: 'test',
	view: 'test'
} ) );

editor.model.change( writer => {
	const root = editor.model.document.getRoot();
	const p = root.getChild( 1 );
	const text = writer.createText( 'FooBar', { 'test': 3 } );

	writer.insert( text, p );
} );

and indeed the above is enough to reproduce the issue. So second converted mentioned in the initial report does not affect it at all.

It even make a sense because in the second converter (...) we want to convert every [test] attribute in the model to view. Text without any wrapper cannot have any attribute.

☝️ This seems to be cause of this issue, that AttributeToAttribute conversion is fired and processed for Text node the same as for Element node.

When AttributeToAttribute conversion is used, it uses internally changeAttribute() function on a given node. It seems this function assumes that it operates on a node which can have attributes directly (which is not true for Text nodes).

When AttributeToElementconversion is used, it uses internally wrap() function, which wraps given node in an additional node which directly accepts attributes.

I assume:

editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
	model: 'test',
	view: 'test'
} ) );

converter should not be executed for nodes which cannot directly have attributes. But I don't see any check for this in the code 🤔

---

So it seems that the aToA converter is called for DocumentSelection.

Btw. I have checked the above and it seems conversion is called for the selection only if text aToA converted is defined and it is not related to this issue.

[f1ames]

One more thing to clarify, the direct cause of the error is the fact that changeAttribute() function uses internally conversionApi.mapper.toViewElement() method, passing Text node (TextProxy object to be precise) and this method operates only on Element's: https://github.com/ckeditor/ckeditor5-engine/blob/71c4c199d0fbe228a74efc272d00b13f522f36ad/src/conversion/mapper.js#L209-L217

so null is returned.