Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Do not run AttributeToAttribute downcast conversion on TextProxy nodes #1595

Merged
merged 5 commits into from
Nov 29, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 38 additions & 2 deletions src/conversion/downcast-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -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';

/**
Expand Down Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text node's attribute

* 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.' );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

text node's attribute

attribute-to-attribute


return;
}

Expand Down
12 changes: 12 additions & 0 deletions tests/conversion/downcast-converters.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -264,6 +267,8 @@ describe( 'downcast-helpers', () => {
} );

describe( 'downcastAttributeToAttribute', () => {
testUtils.createSinonSandbox();

beforeEach( () => {
conversion.for( 'downcast' ).add( downcastElementToElement( { model: 'image', view: 'img' } ) );
} );
Expand Down Expand Up @@ -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' } ) );
Expand All @@ -473,6 +480,7 @@ describe( 'downcast-helpers', () => {
} );

expectResult( '<p test="1"></p><p test="2"></p>' );
expect( logSpy.callCount ).to.equal( 0 );

model.change( writer => {
writer.removeAttribute( 'test', modelRoot.getChild( 1 ) );
Expand All @@ -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' } ) );
Expand All @@ -496,6 +506,8 @@ describe( 'downcast-helpers', () => {
} );

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 ) );
Expand Down