Merge pull request #1595 from ckeditor/t/1587

Fix: Do not run attribute-to-attribute downcast conversion on text node attributes. Closes #1587.

src/conversion/conversion.js

@@ -419,6 +419,9 @@ export default class Conversion {
 	/**
 	 * Sets up converters between the model and the view that convert a model attribute to a view attribute (and vice versa).
 	 * For example, `<image src='foo.jpg'></image>` is converted to `<img src='foo.jpg'></img>` (the same attribute key and value).
+	 * This type of converters is intended to be used with {@link module:engine/model/element~Element model element} nodes.
+	 * To convert text attributes {@link module:engine/conversion/conversion~Conversion#attributeToElement `attributeToElement converter`}
+	 * should be set up.
 	 *
 	 *		// A simple conversion from the `source` model attribute to the `src` view attribute (and vice versa).
 	 *		conversion.attributeToAttribute( { model: 'source', view: 'src' } );

src/conversion/downcast-converters.js

@@ -10,6 +10,7 @@ import ModelElement from '../model/element';
 import ViewAttributeElement from '../view/attributeelement';
 import DocumentSelection from '../model/documentselection';
 
+import log from '@ckeditor/ckeditor5-utils/src/log';
 import { cloneDeep } from 'lodash-es';
 
 /**
@@ -685,6 +686,49 @@ export function changeAttribute( attributeCreator ) {
 		const viewElement = conversionApi.mapper.toViewElement( data.item );
 		const viewWriter = conversionApi.writer;
 
+		// If model item cannot be mapped to a view element, it means item is not an `Element` instance but a `TextProxy` node.
+		// Only elements can have attributes in a view so do not proceed for anything else (#1587).
+		if ( !viewElement ) {
+			/**
+			 * This error occurs when a {@link module:engine/model/textproxy~TextProxy text node's} attribute is to be downcasted
+			 * by {@link module:engine/conversion/conversion~Conversion#attributeToAttribute `Attribute to Attribute converter`}.
+			 * In most cases it is caused by converters misconfiguration when only "generic" converter is defined:
+			 *
+			 *		editor.conversion.for( 'downcast' ).add( downcastAttributeToAttribute( {
+			 *			model: 'attribute-name',
+			 *			view: 'attribute-name'
+			 *		} ) );
+			 *
+			 * and given attribute is used on text node, for example:
+			 *
+			 *		model.change( writer => {
+			 *			writer.insertText( 'Foo', { 'attribute-name': 'bar' }, parent, 0 );
+			 *		} );
+			 *
+			 * In such cases, to convert the same attribute for both {@link module:engine/model/element~Element}
+			 * and {@link module:engine/model/textproxy~TextProxy `Text`} nodes, text specific
+			 * {@link module:engine/conversion/conversion~Conversion#attributeToElement `Attribute to Element converter`}
+			 * with higher {@link module:utils/priorities~PriorityString priority} must also be defined:
+			 *
+			 *		conversion.for( 'downcast' ).add( downcastAttributeToElement( {
+			 *			model: {
+			 *				key: 'attribute-name',
+			 *				name: '$text'
+			 *			},
+			 *			view: ( value, writer ) => {
+			 *				return writer.createAttributeElement( 'span', { 'attribute-name': value } );
+			 *			},
+			 *			converterPriority: 'high'
+			 *		} ) );
+			 *
+			 * @error conversion-attribute-to-attribute-on-text
+			 */
+			log.warn( 'conversion-attribute-to-attribute-on-text: ' +
+				'Trying to convert text node\'s attribute with attribute-to-attribute converter.' );
+
+			return;
+		}
+
 		// First remove the old attribute if there was one.
 		if ( data.attributeOldValue !== null && oldAttribute ) {
 			if ( oldAttribute.key == 'class' ) {

tests/conversion/downcast-converters.js

@@ -17,6 +17,9 @@ import ViewContainerElement from '../../src/view/containerelement';
 import ViewUIElement from '../../src/view/uielement';
 import ViewText from '../../src/view/text';
 
+import log from '@ckeditor/ckeditor5-utils/src/log';
+import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils';
+
 import {
 	downcastElementToElement, downcastAttributeToElement, downcastAttributeToAttribute, downcastMarkerToElement, downcastMarkerToHighlight,
 	insertElement, insertUIElement, changeAttribute, wrap, removeUIElement,
@@ -264,6 +267,8 @@ describe( 'downcast-helpers', () => {
 	} );
 
 	describe( 'downcastAttributeToAttribute', () => {
+		testUtils.createSinonSandbox();
+
 		beforeEach( () => {
 			conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'image', view: 'img' } ) );
 		} );
@@ -460,6 +465,56 @@ describe( 'downcast-helpers', () => {
 
 			expectResult( '<img class="styled-pull-out"></img>' );
 		} );
+
+		// #1587
+		it( 'config.view and config.model as strings in generic conversion (elements only)', () => {
+			const logSpy = testUtils.sinon.spy( log, 'warn' );
+
+			conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'paragraph', view: 'p' } ) );
+
+			conversion.for( 'downcast' ).add( downcastAttributeToAttribute( { model: 'test', view: 'test' } ) );
+
+			model.change( writer => {
+				writer.insertElement( 'paragraph', { test: '1' }, modelRoot, 0 );
+				writer.insertElement( 'paragraph', { test: '2' }, modelRoot, 1 );
+			} );
+
+			expectResult( '<p test="1"></p><p test="2"></p>' );
+			expect( logSpy.callCount ).to.equal( 0 );
+
+			model.change( writer => {
+				writer.removeAttribute( 'test', modelRoot.getChild( 1 ) );
+			} );
+
+			expectResult( '<p test="1"></p><p></p>' );
+		} );
+
+		// #1587
+		it( 'config.view and config.model as strings in generic conversion (elements + text)', () => {
+			const logSpy = testUtils.sinon.spy( log, 'warn' );
+
+			conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'paragraph', view: 'p' } ) );
+
+			conversion.for( 'downcast' ).add( downcastAttributeToAttribute( { model: 'test', view: 'test' } ) );
+
+			model.change( writer => {
+				writer.insertElement( 'paragraph', modelRoot, 0 );
+				writer.insertElement( 'paragraph', { test: '1' }, modelRoot, 1 );
+
+				writer.insertText( 'Foo', { test: '2' }, modelRoot.getChild( 0 ), 0 );
+				writer.insertText( 'Bar', { test: '3' }, modelRoot.getChild( 1 ), 0 );
+			} );
+
+			expectResult( '<p>Foo</p><p test="1">Bar</p>' );
+			expect( logSpy.callCount ).to.equal( 2 );
+			expect( logSpy.alwaysCalledWithMatch( 'conversion-attribute-to-attribute-on-text' ) ).to.true;
+
+			model.change( writer => {
+				writer.removeAttribute( 'test', modelRoot.getChild( 1 ) );
+			} );
+
+			expectResult( '<p>Foo</p><p>Bar</p>' );
+		} );
 	} );
 
 	describe( 'downcastMarkerToElement', () => {