Do not run AttributeToAttribute downcast conversion on TextProxy nodes

[f1ames]

Suggested merge commit message (convention)

Fix: Do not run AttributeToAttribute downcast conversion on TextProxy nodes. Closes ckeditor/ckeditor5#4440.

---

Additional information

I wanted to elaborate a little on this fix, because it looks like "Here we got a null exception, lets add an if, easy-peasy, done". But it is a little more complex:

The cause of this issue is described in https://github.com/ckeditor/ckeditor5-engine/issues/1587#issuecomment-441062367 - in short attribute to attribute downcast conversion is run for model TextProxy which is downcasted to a view Text node which cannot have attributes directly (but converter assumes it can).

The reasonable approach will be to downcast (via attribute to attribute converters) only model elements which can have attributes. However, in the model, TextProxy is the element which allows attributes. So checking model element for attributes acceptance doesn't make sense. Also model doesn't have a direct knowledge to what view element the model element will be downcasted to (that is the responsibility of converters). So basically to check if element can be processed by attribute to attribute conversion, view element should be checked. And this is what exactly happens here. Since conversionApi.mapper.toViewElement( data.item ) is the first place where to be converted model element is mapped to view, it is also a proper place to check if element directly accepts attributes. In the view all Element nodes can have attributes and other types cannot (so basically Text and TextProxy). So any node different than Element should not be converted and since conversionApi.mapper.toViewElement() method works only on Element nodes, it returns null for other elements.

[coveralls]

Coverage remained the same at 100.0% when pulling 83f7091 on t/1587 into 71c4c19 on master.

[scofalik]

I discussed this issue with @f1ames.

The problem with this is that it works correctly, however it is easy to mess up. attributeToAttribute (in short aToA) conversion is suited only for model elements. It does not and should not work for text nodes, so the crash is not surprising.

Basically, the situation when the crash occurs is when aToA is defined but the same attribute is used on a text. We already had two questions from the community about configuring converters to have the same attribute on elements and texts. The trick is to provide attributeToElement converter for texts ( model: { key: 'key', name: '$text' }) and have it fire before aToA converter (using converterPriority or proper code ordering). The problem is that this is not straightforward and is not explained anywhere.

For that reason, I am actually for having the fix as proposed in this PR. I'd just add a log.warn() informing that attribute to attribute downcast conversion was skipped because model attribute was set on model text node (or something similar that makes sense, I leave it up to you to format it in a short and informative way :P).

Also, please make it clear in API docs that aToA is for element-to-element conversion and is not suitable for text nodes and that aToE should be used to convert text attributes.

[f1ames]

@scofalik Ready for another review.

[scofalik]

I've added two small things, other than that it is good.

[scofalik]

text node's attribute

attribute-to-attribute

[scofalik]

text node's attribute