Merge pull request #1405 from ckeditor/t/1404

Fix: `model.Differ` did not handle attribute change transformation correctly. Closes #1404.
ckeditor · Apr 11, 2018 · 3769a02 · 3769a02
2 parents 817a746 + 03469be
commit 3769a02
Show file tree

Hide file tree

Showing 2 changed files with 143 additions and 0 deletions.
diff --git a/src/model/differ.js b/src/model/differ.js
@@ -690,6 +690,7 @@ export default class Differ {
 			}
 
 			if ( inc.type == 'attribute' ) {
+				// In case of attribute change, `howMany` should be kept same as `nodesToHandle`. It's not an error.
 				if ( old.type == 'insert' ) {
 					if ( inc.offset < old.offset && incEnd > old.offset ) {
 						if ( incEnd > oldEnd ) {
@@ -712,6 +713,7 @@ export default class Differ {
 						}
 
 						inc.nodesToHandle = old.offset - inc.offset;
+						inc.howMany = inc.nodesToHandle;
 					} else if ( inc.offset >= old.offset && inc.offset < oldEnd ) {
 						if ( incEnd > oldEnd ) {
 							inc.nodesToHandle = incEnd - oldEnd;
@@ -723,8 +725,15 @@ export default class Differ {
 				}
 
 				if ( old.type == 'attribute' ) {
+					// There are only two conflicting scenarios possible here:
 					if ( inc.offset >= old.offset && incEnd <= oldEnd ) {
+						// `old` change includes `inc` change, or they are the same.
 						inc.nodesToHandle = 0;
+						inc.howMany = 0;
+						inc.offset = 0;
+					} else if ( inc.offset <= old.offset && incEnd >= oldEnd ) {
+						// `inc` change includes `old` change.
+						old.howMany = 0;
 					}
 				}
 			}

diff --git a/tests/model/differ.js b/tests/model/differ.js
@@ -782,6 +782,140 @@ describe( 'Differ', () => {
 			} );
 		} );
 
+		it( 'attribute changes intersecting #1', () => {
+			const parent = root.getChild( 1 );
+
+			// Be aware that you cannot make an intersecting changes with the same attribute key,
+			// cause the value would be incorrect for the common part of the ranges.
+			const ranges = [
+				[ 0, 2, null, true, 'foo' ],
+				[ 1, 3, null, true, 'bar' ]
+			];
+
+			model.change( () => {
+				for ( const item of ranges ) {
+					const range = Range.createFromParentsAndOffsets( parent, item[ 0 ], parent, item[ 1 ] );
+
+					attribute( range, item[ 4 ], item[ 2 ], item[ 3 ] );
+				}
+
+				expectChanges( [
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 0, parent, 2 ),
+						attributeKey: 'foo',
+						attributeOldValue: null,
+						attributeNewValue: true
+					},
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 1, parent, 3 ),
+						attributeKey: 'bar',
+						attributeOldValue: null,
+						attributeNewValue: true
+					}
+				] );
+			} );
+		} );
+
+		it( 'attribute changes intersecting #2', () => {
+			const parent = root.getChild( 1 );
+
+			// Be aware that you cannot make an intersecting changes with the same attribute key,
+			// cause the value would be incorrect for the common part of the ranges.
+			const ranges = [
+				[ 1, 3, null, true, 'foo' ],
+				[ 0, 2, null, true, 'bar' ]
+			];
+
+			model.change( () => {
+				for ( const item of ranges ) {
+					const range = Range.createFromParentsAndOffsets( parent, item[ 0 ], parent, item[ 1 ] );
+
+					attribute( range, item[ 4 ], item[ 2 ], item[ 3 ] );
+				}
+
+				expectChanges( [
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 0, parent, 1 ),
+						attributeKey: 'bar',
+						attributeOldValue: null,
+						attributeNewValue: true
+					},
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 1, parent, 2 ),
+						attributeKey: 'foo',
+						attributeOldValue: null,
+						attributeNewValue: true
+					},
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 1, parent, 2 ),
+						attributeKey: 'bar',
+						attributeOldValue: null,
+						attributeNewValue: true
+					},
+					{
+						type: 'attribute',
+						range: Range.createFromParentsAndOffsets( parent, 2, parent, 3 ),
+						attributeKey: 'foo',
+						attributeOldValue: null,
+						attributeNewValue: true
+					}
+				] );
+			} );
+		} );
+
+		it( 'attribute changes included in an attribute change #1 - changes are reversed at the end', () => {
+			const parent = root.getChild( 1 );
+
+			const ranges = [
+				[ 0, 1, null, true ],
+				[ 1, 2, null, true ],
+				[ 0, 2, true, null ]
+			];
+
+			model.change( () => {
+				for ( const item of ranges ) {
+					const range = Range.createFromParentsAndOffsets( parent, item[ 0 ], parent, item[ 1 ] );
+
+					attribute( range, attributeKey, item[ 2 ], item[ 3 ] );
+				}
+
+				expectChanges( [] );
+			} );
+		} );
+
+		it( 'attribute changes included in an attribute change #2 - changes are re-applied at the end', () => {
+			const parent = root.getChild( 1 );
+
+			const ranges = [
+				[ 0, 1, null, true ],
+				[ 1, 2, null, true ],
+				[ 0, 2, true, null ],
+				[ 0, 1, null, true ],
+				[ 1, 2, null, true ]
+			];
+
+			model.change( () => {
+				for ( const item of ranges ) {
+					const range = Range.createFromParentsAndOffsets( parent, item[ 0 ], parent, item[ 1 ] );
+
+					attribute( range, attributeKey, item[ 2 ], item[ 3 ] );
+				}
+
+				expectChanges( [ {
+					type: 'attribute',
+					range: Range.createFromParentsAndOffsets( parent, 0, parent, 2 ),
+					attributeKey,
+					attributeOldValue: null,
+					attributeNewValue: true
+				} ] );
+			} );
+		} );
+
 		it( 'on multiple non-consecutive characters in multiple operations', () => {
 			const parent = root.getChild( 0 );