Browse Source

Fixed: Selection attributes should be cleared in an enqueueChanges block.

Szymon Cofalik 8 years ago
parent
commit
792bc45d79

+ 11 - 5
packages/ckeditor5-engine/src/model/documentselection.js

@@ -85,7 +85,7 @@ export default class DocumentSelection extends Selection {
 
 			// Whenever element which had selection's attributes stored in it stops being empty,
 			// the attributes need to be removed.
-			clearAttributesStoredInElement( changes, batch );
+			clearAttributesStoredInElement( changes, batch, this._document );
 		} );
 	}
 
@@ -691,7 +691,7 @@ function getAttrsIfCharacter( node ) {
 }
 
 // Removes selection attributes from element which is not empty anymore.
-function clearAttributesStoredInElement( changes, batch ) {
+function clearAttributesStoredInElement( changes, batch, document ) {
 	// Batch may not be passed to the document#change event in some tests.
 	// See https://github.com/ckeditor/ckeditor5-engine/issues/1001#issuecomment-314202352
 	// Ignore also transparent batches because they are... transparent.
@@ -707,9 +707,15 @@ function clearAttributesStoredInElement( changes, batch ) {
 		return;
 	}
 
-	const storedAttributes = Array.from( changeParent.getAttributeKeys() ).filter( key => key.startsWith( storePrefix ) );
+	const anyStoredAttributes = Array.from( changeParent.getAttributeKeys() ).some( key => key.startsWith( storePrefix ) );
 
-	for ( const key of storedAttributes ) {
-		batch.removeAttribute( changeParent, key );
+	if ( anyStoredAttributes ) {
+		document.enqueueChanges( () => {
+			const storedAttributes = Array.from( changeParent.getAttributeKeys() ).filter( key => key.startsWith( storePrefix ) );
+
+			for ( const key of storedAttributes ) {
+				batch.removeAttribute( changeParent, key );
+			}
+		} );
 	}
 }

+ 28 - 0
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -990,8 +990,12 @@ describe( 'DocumentSelection', () => {
 					batchTypes.set( batch, batch.type );
 				} );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				doc.batch().insert( rangeInEmptyP.start, 'x' );
 
+				expect( doc.enqueueChanges.calledOnce ).to.be.true;
+
 				expect( emptyP.hasAttribute( fooStoreAttrKey ) ).to.be.false;
 				expect( emptyP.hasAttribute( abcStoreAttrKey ) ).to.be.false;
 
@@ -1002,8 +1006,11 @@ describe( 'DocumentSelection', () => {
 				selection.setRanges( [ rangeInEmptyP ] );
 				selection.setAttribute( 'foo', 'bar' );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				doc.batch().move( fullP.getChild( 0 ), rangeInEmptyP.start );
 
+				expect( doc.enqueueChanges.calledOnce ).to.be.true;
 				expect( emptyP.hasAttribute( fooStoreAttrKey ) ).to.be.false;
 			} );
 
@@ -1014,9 +1021,13 @@ describe( 'DocumentSelection', () => {
 				emptyP.setAttribute( fooStoreAttrKey, 'bar' );
 				emptyP2.setAttribute( fooStoreAttrKey, 'bar' );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				// <emptyP>{}<emptyP2>
 				doc.batch().merge( Position.createAfter( emptyP ) );
 
+				expect( doc.enqueueChanges.calledOnce ).to.be.true;
+
 				expect( emptyP.hasAttribute( fooStoreAttrKey ) ).to.be.false;
 				expect( emptyP.parent ).to.equal( root ); // Just to be sure we're checking the right element.
 			} );
@@ -1025,6 +1036,8 @@ describe( 'DocumentSelection', () => {
 				const emptyP2 = new Element( 'p', null );
 				root.appendChildren( emptyP2 );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				emptyP.setAttribute( fooStoreAttrKey, 'bar' );
 				emptyP2.setAttribute( abcStoreAttrKey, 'bar' );
 
@@ -1034,6 +1047,8 @@ describe( 'DocumentSelection', () => {
 				// <emptyP>{}<emptyP2>
 				doc.batch().merge( Position.createAfter( emptyP ) );
 
+				expect( doc.enqueueChanges.called ).to.be.false;
+
 				expect( emptyP.getAttribute( fooStoreAttrKey ) ).to.equal( 'bar' );
 				expect( emptyP.parent ).to.equal( root ); // Just to be sure we're checking the right element.
 			} );
@@ -1043,8 +1058,11 @@ describe( 'DocumentSelection', () => {
 
 				selection.setRanges( [ rangeInFullP ] );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				doc.batch().insert( rangeInEmptyP.start, 'x' );
 
+				expect( doc.enqueueChanges.calledOnce ).to.be.true;
 				expect( emptyP.hasAttribute( fooStoreAttrKey ) ).to.be.false;
 			} );
 
@@ -1058,10 +1076,14 @@ describe( 'DocumentSelection', () => {
 				const batch = doc.batch();
 				const spy = sinon.spy( batch, 'removeAttribute' );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				// <emptyP>{}<emptyP2>
 				batch.merge( Position.createAfter( emptyP ) );
 
 				expect( emptyP.hasAttribute( fooStoreAttrKey ) ).to.be.false;
+
+				expect( doc.enqueueChanges.calledOnce ).to.be.true;
 				expect( spy.calledOnce ).to.be.true;
 			} );
 
@@ -1069,8 +1091,11 @@ describe( 'DocumentSelection', () => {
 				selection.setRanges( [ rangeInEmptyP ] );
 				selection.setAttribute( 'foo', 'bar' );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				doc.batch( 'transparent' ).insert( rangeInEmptyP.start, 'x' );
 
+				expect( doc.enqueueChanges.called ).to.be.false;
 				expect( emptyP.getAttribute( fooStoreAttrKey ) ).to.equal( 'bar' );
 			} );
 
@@ -1080,8 +1105,11 @@ describe( 'DocumentSelection', () => {
 				selection.setRanges( [ rangeInEmptyP ] );
 				selection.setAttribute( 'foo', 'bar' );
 
+				sinon.spy( doc, 'enqueueChanges' );
+
 				doc.batch().rename( emptyP, 'pnew' );
 
+				expect( doc.enqueueChanges.called ).to.be.false;
 				expect( emptyP.getAttribute( fooStoreAttrKey ) ).to.equal( 'bar' );
 			} );
 		} );