From 669c5fdd7f641f196b11dd3555aa54426a6a1ca8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Fri, 23 Nov 2018 12:00:21 +0100 Subject: [PATCH 1/2] Do not run 'AttributeToAttribute' conversion on 'TextProxy'. --- src/conversion/downcast-converters.js | 6 ++++ tests/conversion/downcast-converters.js | 43 +++++++++++++++++++++++++ 2 files changed, 49 insertions(+) diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js index 7424d75e0..71d6749d1 100644 --- a/src/conversion/downcast-converters.js +++ b/src/conversion/downcast-converters.js @@ -685,6 +685,12 @@ export function changeAttribute( attributeCreator ) { const viewElement = conversionApi.mapper.toViewElement( data.item ); const viewWriter = conversionApi.writer; + // If model item cannot be mapped to view element, it means item is not an `Element` instance (it is a `TextProxy`). + // Only elements can have attributes in a view so do not proceed for anything else (see #1587). + if ( !viewElement ) { + return; + } + // First remove the old attribute if there was one. if ( data.attributeOldValue !== null && oldAttribute ) { if ( oldAttribute.key == 'class' ) { diff --git a/tests/conversion/downcast-converters.js b/tests/conversion/downcast-converters.js index 0efc408cb..def013205 100644 --- a/tests/conversion/downcast-converters.js +++ b/tests/conversion/downcast-converters.js @@ -460,6 +460,49 @@ describe( 'downcast-helpers', () => { expectResult( '' ); } ); + + // #1587 + it( 'config.view and config.model as strings in generic conversion (elements only)', () => { + 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( '

' ); + + model.change( writer => { + writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); + } ); + + expectResult( '

' ); + } ); + + // #1587 + it( 'config.view and config.model as strings in generic conversion (elements + text)', () => { + 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( '

Foo

Bar

' ); + + model.change( writer => { + writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); + } ); + + expectResult( '

Foo

Bar

' ); + } ); } ); describe( 'downcastMarkerToElement', () => { From 50f78a014fe6f5f6f3a2b762efaa72dc048ff6c7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Krzysztof=20Krzto=C5=84?= Date: Tue, 27 Nov 2018 12:05:11 +0100 Subject: [PATCH 2/2] Add warning message to point out converters misconfiguration. --- src/conversion/downcast-converters.js | 40 +++++++++++++++++++++++-- tests/conversion/downcast-converters.js | 12 ++++++++ 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/src/conversion/downcast-converters.js b/src/conversion/downcast-converters.js index 71d6749d1..509ebc839 100644 --- a/src/conversion/downcast-converters.js +++ b/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,9 +686,44 @@ export function changeAttribute( attributeCreator ) { const viewElement = conversionApi.mapper.toViewElement( data.item ); const viewWriter = conversionApi.writer; - // If model item cannot be mapped to view element, it means item is not an `Element` instance (it is a `TextProxy`). - // Only elements can have attributes in a view so do not proceed for anything else (see #1587). + // 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} 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 } ); + * } + * } ), { priority: 'high' } ); + * + * @error conversion-attribute-to-attribute-on-text + */ + log.warn( 'conversion-attribute-to-attribute-on-text: Trying to convert text node with attribute to attribute converter.' ); + return; } diff --git a/tests/conversion/downcast-converters.js b/tests/conversion/downcast-converters.js index def013205..b61824d7b 100644 --- a/tests/conversion/downcast-converters.js +++ b/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' } ) ); } ); @@ -463,6 +468,8 @@ describe( 'downcast-helpers', () => { // #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' } ) ); @@ -473,6 +480,7 @@ describe( 'downcast-helpers', () => { } ); expectResult( '

' ); + expect( logSpy.callCount ).to.equal( 0 ); model.change( writer => { writer.removeAttribute( 'test', modelRoot.getChild( 1 ) ); @@ -483,6 +491,8 @@ describe( 'downcast-helpers', () => { // #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' } ) ); @@ -496,6 +506,8 @@ describe( 'downcast-helpers', () => { } ); expectResult( '

Foo

Bar

' ); + 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 ) );