Преглед на файлове

AttributeDelta and AttributePperation don't process leaving nodes during iterating over their ranges.

Szymon Cofalik преди 10 години
родител
ревизия
dd3bd420e0

+ 18 - 13
packages/ckeditor5-engine/src/document/delta/attributedelta.js

@@ -11,9 +11,10 @@ CKEDITOR.define( [
 	'document/operation/attributeoperation',
 	'document/position',
 	'document/range',
+	'document/positioniterator',
 	'document/attribute',
 	'document/element'
-], ( Delta, register, AttributeOperation, Position, Range, Attribute, Element ) => {
+], ( Delta, register, AttributeOperation, Position, Range, PositionIterator, Attribute, Element ) => {
 	/**
 	 * To provide specific OT behavior and better collisions solving, change methods ({@link document.Batch#setAttr}
 	 * and {@link document.Batch#removeAttr}) use `AttributeDelta` class which inherits from the `Delta` class and may
@@ -113,22 +114,26 @@ CKEDITOR.define( [
 		let next = iterator.next();
 
 		while ( !next.done ) {
-			valueAfter = next.value.node.getAttr( key );
-
-			// At the first run of the iterator the position in undefined. We also do not have a valueBefore, but
-			// because valueAfter may be null, valueBefore may be equal valueAfter ( undefined == null ).
-			if ( position && valueBefore != valueAfter ) {
-				// if valueBefore == value there is nothing to change, so we add operation only if these values are different.
-				if ( valueBefore != value ) {
-					addOperation();
+			// We check values only when the range contains given element, that is when the iterator "enters" the element.
+			// To prevent double-checking or not needed checking, we filter-out iterator values for ELEMENT_LEAVE position.
+			if ( next.value.type != PositionIterator.ELEMENT_LEAVE ) {
+				valueAfter = next.value.node.getAttr( key );
+
+				// At the first run of the iterator the position in undefined. We also do not have a valueBefore, but
+				// because valueAfter may be null, valueBefore may be equal valueAfter ( undefined == null ).
+				if ( position && valueBefore != valueAfter ) {
+					// if valueBefore == value there is nothing to change, so we add operation only if these values are different.
+					if ( valueBefore != value ) {
+						addOperation();
+					}
+
+					lastSplitPosition = position;
 				}
 
-				lastSplitPosition = position;
+				position = iterator.position;
+				valueBefore = valueAfter;
 			}
 
-			position = iterator.position;
-			valueBefore = valueAfter;
-
 			next = iterator.next();
 		}
 

+ 8 - 9
packages/ckeditor5-engine/src/document/operation/attributeoperation.js

@@ -76,7 +76,6 @@ CKEDITOR.define( [
 		_execute() {
 			const oldAttr = this.oldAttr;
 			const newAttr = this.newAttr;
-			let value;
 
 			if ( oldAttr !== null && newAttr !== null && oldAttr.key != newAttr.key ) {
 				/**
@@ -93,8 +92,8 @@ CKEDITOR.define( [
 
 			// Remove or change.
 			if ( oldAttr !== null ) {
-				for ( value of this.range ) {
-					if ( !value.node.hasAttr( oldAttr ) ) {
+				for ( let node of this.range.getNodes() ) {
+					if ( !node.hasAttr( oldAttr ) ) {
 						/**
 						 * The attribute which should be removed does not exists for the given node.
 						 *
@@ -104,21 +103,21 @@ CKEDITOR.define( [
 						 */
 						throw new CKEditorError(
 							'operation-attribute-no-attr-to-remove: The attribute which should be removed does not exists for given node.',
-							{ node: value.node, attr: oldAttr } );
+							{ node: node, attr: oldAttr } );
 					}
 
 					// There is no use in removing attribute if we will overwrite it later.
 					// Still it is profitable to run through the loop to check if all nodes in the range has old attribute.
 					if ( newAttr === null ) {
-						value.node.removeAttr( oldAttr.key );
+						node.removeAttr( oldAttr.key );
 					}
 				}
 			}
 
 			// Insert or change.
 			if ( newAttr !== null ) {
-				for ( value of this.range ) {
-					if ( oldAttr === null && value.node.hasAttr( newAttr.key ) ) {
+				for ( let node of this.range.getNodes() ) {
+					if ( oldAttr === null && node.hasAttr( newAttr.key ) ) {
 						/**
 						 * The attribute with given key already exists for the given node.
 						 *
@@ -128,10 +127,10 @@ CKEDITOR.define( [
 						 */
 						throw new CKEditorError(
 							'operation-attribute-attr-exists: The attribute with given key already exists.',
-							{ node: value.node, attr: newAttr } );
+							{ node: node, attr: newAttr } );
 					}
 
-					value.node.setAttr( newAttr );
+					node.setAttr( newAttr );
 				}
 			}
 

+ 47 - 21
packages/ckeditor5-engine/tests/document/deltas/attributedelta.js

@@ -132,7 +132,9 @@ describe( 'Batch', () => {
 				new Text( 'xxx', [ new Attribute( 'a', 1 ) ] ),
 				new Text( 'xxx', [ new Attribute( 'a', 2 ) ] ),
 				'xxx',
-				new Text( 'xxx', [ new Attribute( 'a', 1 ) ] )
+				new Text( 'xxx', [ new Attribute( 'a', 1 ) ] ),
+				new Element( 'e', [ new Attribute( 'a', 2 ) ], 'xxx' ),
+				'xxx'
 			] );
 		} );
 
@@ -148,7 +150,7 @@ describe( 'Batch', () => {
 
 			for ( let delta of batch.deltas ) {
 				for ( let operation of delta.operations ) {
-					count += getIteratorCount( operation.range );
+					count += getIteratorCount( operation.range.getNodes() );
 				}
 			}
 
@@ -156,10 +158,10 @@ describe( 'Batch', () => {
 		}
 
 		function getCompressedAttrs() {
-			// default: 111---111222---111
+			// default: 111---111222---1112------
 			const range = Range.createFromElement( root );
 
-			return Array.from( range ).map( value => value.node.getAttr( 'a' ) || '-' ).join( '' );
+			return Array.from( range.getNodes() ).map( node => node.getAttr( 'a' ) || '-' ).join( '' );
 		}
 
 		describe( 'setAttr', () => {
@@ -167,48 +169,60 @@ describe( 'Batch', () => {
 				batch.setAttr( 'a', 3, getRange( 3, 6 ) );
 				expect( getOperationsCount() ).to.equal( 1 );
 				expect( getChangesAttrsCount() ).to.equal( 3 );
-				expect( getCompressedAttrs() ).to.equal( '111333111222---111' );
+				expect( getCompressedAttrs() ).to.equal( '111333111222---1112------' );
 			} );
 
 			it( 'should split the operations if parts of the range have different attributes', () => {
 				batch.setAttr( 'a', 3, getRange( 4, 14 ) );
 				expect( getOperationsCount() ).to.equal( 4 );
 				expect( getChangesAttrsCount() ).to.equal( 10 );
-				expect( getCompressedAttrs() ).to.equal( '111-3333333333-111' );
+				expect( getCompressedAttrs() ).to.equal( '111-3333333333-1112------' );
 			} );
 
 			it( 'should split the operations if parts of the part of the range have the attribute', () => {
 				batch.setAttr( 'a', 2, getRange( 4, 14 ) );
 				expect( getOperationsCount() ).to.equal( 3 );
 				expect( getChangesAttrsCount() ).to.equal( 7 );
-				expect( getCompressedAttrs() ).to.equal( '111-2222222222-111' );
+				expect( getCompressedAttrs() ).to.equal( '111-2222222222-1112------' );
 			} );
 
 			it( 'should strip the range if the beginning have the attribute', () => {
 				batch.setAttr( 'a', 1, getRange( 1, 5 ) );
 				expect( getOperationsCount() ).to.equal( 1 );
 				expect( getChangesAttrsCount() ).to.equal( 2 );
-				expect( getCompressedAttrs() ).to.equal( '11111-111222---111' );
+				expect( getCompressedAttrs() ).to.equal( '11111-111222---1112------' );
 			} );
 
 			it( 'should strip the range if the ending have the attribute', () => {
 				batch.setAttr( 'a', 1, getRange( 13, 17 ) );
 				expect( getOperationsCount() ).to.equal( 1 );
 				expect( getChangesAttrsCount() ).to.equal( 2 );
-				expect( getCompressedAttrs() ).to.equal( '111---111222-11111' );
+				expect( getCompressedAttrs() ).to.equal( '111---111222-111112------' );
 			} );
 
 			it( 'should do nothing if the range has attribute', () => {
 				batch.setAttr( 'a', 1, getRange( 0, 3 ) );
 				expect( getOperationsCount() ).to.equal( 0 );
-				expect( getCompressedAttrs() ).to.equal( '111---111222---111' );
+				expect( getCompressedAttrs() ).to.equal( '111---111222---1112------' );
+			} );
+
+			it( 'should not check range\'s start position node when creating operations', () => {
+				let range = new Range(
+					new Position( root, [ 18, 1 ] ),
+					new Position( root, [ 19 ] )
+				);
+
+				batch.setAttr( 'a', 1, range );
+				expect( getOperationsCount() ).to.equal( 1 );
+				expect( getChangesAttrsCount() ).to.equal( 2 );
+				expect( getCompressedAttrs() ).to.equal( '111---111222---1112-11---' );
 			} );
 
 			it( 'should create a proper operations for the mixed range', () => {
-				batch.setAttr( 'a', 1, getRange( 0, 18 ) );
-				expect( getOperationsCount() ).to.equal( 3 );
-				expect( getChangesAttrsCount() ).to.equal( 9 );
-				expect( getCompressedAttrs() ).to.equal( '111111111111111111' );
+				batch.setAttr( 'a', 1, getRange( 0, 20 ) );
+				expect( getOperationsCount() ).to.equal( 5 );
+				expect( getChangesAttrsCount() ).to.equal( 14 );
+				expect( getCompressedAttrs() ).to.equal( '11111111111111111111111--' );
 			} );
 
 			it( 'should be chainable', () => {
@@ -222,48 +236,60 @@ describe( 'Batch', () => {
 				batch.removeAttr( 'a', getRange( 0, 2 ) );
 				expect( getOperationsCount() ).to.equal( 1 );
 				expect( getChangesAttrsCount() ).to.equal( 2 );
-				expect( getCompressedAttrs() ).to.equal( '--1---111222---111' );
+				expect( getCompressedAttrs() ).to.equal( '--1---111222---1112------' );
 			} );
 
 			it( 'should split the operations if parts of the range have different attributes', () => {
 				batch.removeAttr( 'a', getRange( 7, 11 ) );
 				expect( getOperationsCount() ).to.equal( 2 );
 				expect( getChangesAttrsCount() ).to.equal( 4 );
-				expect( getCompressedAttrs() ).to.equal( '111---1----2---111' );
+				expect( getCompressedAttrs() ).to.equal( '111---1----2---1112------' );
 			} );
 
 			it( 'should split the operations if parts of the part of the range have no attribute', () => {
 				batch.removeAttr( 'a', getRange( 1, 7 ) );
 				expect( getOperationsCount() ).to.equal( 2 );
 				expect( getChangesAttrsCount() ).to.equal( 3 );
-				expect( getCompressedAttrs() ).to.equal( '1------11222---111' );
+				expect( getCompressedAttrs() ).to.equal( '1------11222---1112------' );
 			} );
 
 			it( 'should strip the range if the beginning have no attribute', () => {
 				batch.removeAttr( 'a', getRange( 4, 12 ) );
 				expect( getOperationsCount() ).to.equal( 2 );
 				expect( getChangesAttrsCount() ).to.equal( 6 );
-				expect( getCompressedAttrs() ).to.equal( '111------------111' );
+				expect( getCompressedAttrs() ).to.equal( '111------------1112------' );
 			} );
 
 			it( 'should strip the range if the ending have no attribute', () => {
 				batch.removeAttr( 'a', getRange( 7, 15 ) );
 				expect( getOperationsCount() ).to.equal( 2 );
 				expect( getChangesAttrsCount() ).to.equal( 5 );
-				expect( getCompressedAttrs() ).to.equal( '111---1--------111' );
+				expect( getCompressedAttrs() ).to.equal( '111---1--------1112------' );
 			} );
 
 			it( 'should do nothing if the range has no attribute', () => {
 				batch.removeAttr( 'a', getRange( 4, 5 ) );
 				expect( getOperationsCount() ).to.equal( 0 );
-				expect( getCompressedAttrs() ).to.equal( '111---111222---111' );
+				expect( getCompressedAttrs() ).to.equal( '111---111222---1112------' );
+			} );
+
+			it( 'should not check range\'s start position node when creating operations', () => {
+				let range = new Range(
+					new Position( root, [ 18, 3 ] ),
+					new Position( root, [ 19 ] )
+				);
+
+				batch.removeAttr( 'a', range );
+				expect( getOperationsCount() ).to.equal( 0 );
+				expect( getChangesAttrsCount() ).to.equal( 0 );
+				expect( getCompressedAttrs() ).to.equal( '111---111222---1112------' );
 			} );
 
 			it( 'should create a proper operations for the mixed range', () => {
 				batch.removeAttr( 'a', getRange( 3, 15 ) );
 				expect( getOperationsCount() ).to.equal( 2 );
 				expect( getChangesAttrsCount() ).to.equal( 6 );
-				expect( getCompressedAttrs() ).to.equal( '111------------111' );
+				expect( getCompressedAttrs() ).to.equal( '111------------1112------' );
 			} );
 
 			it( 'should be chainable', () => {

+ 43 - 1
packages/ckeditor5-engine/tests/document/operation/attributeoperation.js

@@ -14,6 +14,7 @@ const getIteratorCount = bender.tools.core.getIteratorCount;
 
 const modules = bender.amd.require(
 	'document/document',
+	'document/element',
 	'document/operation/attributeoperation',
 	'document/position',
 	'document/range',
@@ -24,10 +25,11 @@ const modules = bender.amd.require(
 );
 
 describe( 'AttributeOperation', () => {
-	let Document, AttributeOperation, Position, Range, Character, Attribute, Text, CKEditorError;
+	let Document, Element, AttributeOperation, Position, Range, Character, Attribute, Text, CKEditorError;
 
 	before( () => {
 		Document = modules[ 'document/document' ];
+		Element = modules[ 'document/element' ];
 		AttributeOperation = modules[ 'document/operation/attributeoperation' ];
 		Position = modules[ 'document/position' ];
 		Range = modules[ 'document/range' ];
@@ -211,6 +213,46 @@ describe( 'AttributeOperation', () => {
 		expect( getIteratorCount( root.getChild( 2 ).getAttrs() ) ).to.equal( 0 );
 	} );
 
+	it( 'should not set attribute of element if change range starts in the middle of that element', () => {
+		let fooAttr = new Attribute( 'foo', true );
+
+		let eleA = new Element( 'a', [], 'abc' );
+		let eleB = new Element( 'b', [], 'xyz' );
+
+		root.insertChildren( 0, [ eleA, eleB ] );
+
+		doc.applyOperation(
+			new AttributeOperation(
+				new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 2 ] ) ),
+				null,
+				fooAttr,
+				doc.version
+			)
+		);
+
+		expect( root.getChild( 0 ).hasAttr( fooAttr ) ).to.be.false;
+	} );
+
+	it( 'should not remove attribute of element if change range starts in the middle of that element', () => {
+		let fooAttr = new Attribute( 'foo', true );
+
+		let eleA = new Element( 'a', [ fooAttr ], 'abc' );
+		let eleB = new Element( 'b', [ fooAttr ], 'xyz' );
+
+		root.insertChildren( 0, [ eleA, eleB ] );
+
+		doc.applyOperation(
+			new AttributeOperation(
+				new Range( new Position( root, [ 0, 3 ] ), new Position( root, [ 1, 0 ] ) ),
+				fooAttr,
+				null,
+				doc.version
+			)
+		);
+
+		expect( root.getChild( 0 ).hasAttr( fooAttr ) ).to.be.true;
+	} );
+
 	it( 'should undo changing attribute by applying reverse operation', () => {
 		let oldAttr = new Attribute( 'isNew', false );
 		let newAttr = new Attribute( 'isNew', true );