Merge pull request #1372 from ckeditor/t/1369

Fixed: conversion.downcast-converters.changeAttribute should not consume if element creators returned null. Closes #1369.

src/conversion/downcast-converters.js

@@ -654,6 +654,13 @@ export function changeAttribute( attributeCreator ) {
  attributeCreator = attributeCreator || ( ( value, data ) => ( { value, key: data.attributeKey } ) );
 
  return ( evt, data, conversionApi ) => {
+ const oldAttribute = attributeCreator( data.attributeOldValue, data );
+ const newAttribute = attributeCreator( data.attributeNewValue, data );
+
+ if ( !oldAttribute && !newAttribute ) {
+ return;
+ }
+
  if ( !conversionApi.consumable.consume( data.item, evt.name ) ) {
  return;
  }
@@ -662,8 +669,6 @@ export function changeAttribute( attributeCreator ) {
  const viewWriter = conversionApi.writer;
 
  // First remove the old attribute if there was one.
- const oldAttribute = attributeCreator( data.attributeOldValue, data );
-
  if ( data.attributeOldValue !== null && oldAttribute ) {
  if ( oldAttribute.key == 'class' ) {
  const classes = Array.isArray( oldAttribute.value ) ? oldAttribute.value : [ oldAttribute.value ];
@@ -682,9 +687,7 @@ export function changeAttribute( attributeCreator ) {
  }
  }
 
- // Then, if conversion was successful, set the new attribute.
- const newAttribute = attributeCreator( data.attributeNewValue, data );
-
+ // Then set the new attribute.
  if ( data.attributeNewValue !== null && newAttribute ) {
  if ( newAttribute.key == 'class' ) {
  const classes = Array.isArray( newAttribute.value ) ? newAttribute.value : [ newAttribute.value ];

tests/conversion/downcast-converters.js

@@ -646,7 +646,7 @@ describe( 'downcast-converters', () => {
  } );
  } );
 
- describe( 'setAttribute', () => {
+ describe( 'changeAttribute', () => {
  it( 'should convert attribute insert/change/remove on a model node', () => {
  const modelElement = new ModelElement( 'paragraph', { class: 'foo' }, new ModelText( 'foobar' ) );
 
@@ -717,6 +717,22 @@ describe( 'downcast-converters', () => {
  // No attribute set.
  expect( viewToString( viewRoot ) ).to.equal( '<div><p>foobar</p></div>' );
  } );
+
+ it( 'should not convert or consume if element creator returned null', () => {
+ const callback = sinon.stub().returns( null );
+
+ dispatcher.on( 'attribute:class', changeAttribute( callback ) );
+
+ const modelElement = new ModelElement( 'paragraph', { class: 'foo' }, new ModelText( 'foobar' ) );
+
+ model.change( writer => {
+ writer.insert( modelElement, modelRootStart );
+ } );
+
+ expect( viewToString( viewRoot ) ).to.equal( '<div><p class="foo">foobar</p></div>' );
+
+ sinon.assert.called( callback );
+ } );
  } );
 
  describe( 'wrap', () => {