Browse Source

Fix: Split x merge transformation different content bug.

Szymon Cofalik 7 years ago
parent
commit
4430ab2194

+ 68 - 1
packages/ckeditor5-engine/src/model/operation/transform.js

@@ -2031,7 +2031,74 @@ setTransformation( SplitOperation, InsertOperation, ( a, b ) => {
 	return [ a ];
 } );
 
-setTransformation( SplitOperation, MergeOperation, ( a, b ) => {
+setTransformation( SplitOperation, MergeOperation, ( a, b, context ) => {
+	// Case 1:
+	//
+	// Split element got merged. If two different elements were merged, clients will have different content.
+	//
+	// Example. Merge at `{}`, split at `[]`:
+	// <heading>Foo</heading>{}<paragraph>B[]ar</paragraph>
+	//
+	// On merge side it will look like this:
+	// <heading>FooB[]ar</heading>
+	// <heading>FooB</heading><heading>ar</heading>
+	//
+	// On split side it will look like this:
+	// <heading>Foo</heading>{}<paragraph>B</paragraph><paragraph>ar</paragraph>
+	// <heading>FooB</heading><paragraph>ar</paragraph>
+	//
+	// Clearly, the second element is different for both clients.
+	//
+	// We could use the removed merge element from graveyard as a split element but then clients would have a different
+	// model state (in graveyard), because the split side client would still have an element in graveyard (removed by merge).
+	//
+	// To overcome this, in `SplitOperation` x `MergeOperation` transformation we will add additional `SplitOperation`
+	// in the graveyard, which will actually clone the merged-and-deleted element. Then, that cloned element will be
+	// used for splitting. Example below.
+	//
+	// Original state:
+	// <heading>Foo</heading>{}<paragraph>B[]ar</paragraph>
+	//
+	// Merge side client:
+	//
+	// After merge:
+	// <heading>FooB[]ar</heading>                                 graveyard: <paragraph></paragraph>
+	//
+	// Extra split:
+	// <heading>FooB[]ar</heading>                                 graveyard: <paragraph></paragraph><paragraph></paragraph>
+	//
+	// Use the "cloned" element from graveyard:
+	// <heading>FooB</heading><paragraph>ar</paragraph>            graveyard: <paragraph></paragraph>
+	//
+	// Split side client:
+	//
+	// After split:
+	// <heading>Foo</heading>{}<paragraph>B</paragraph><paragraph>ar</paragraph>
+	//
+	// After merge:
+	// <heading>FooB</heading><paragraph>ar</paragraph>            graveyard: <paragraph></paragraph>
+	//
+	// This special case scenario only applies if the original split operation clones the split element.
+	// If the original split operation has `graveyardPosition` set, it all doesn't have sense because split operation
+	// knows exactly which element it should use. So there would be no original problem with different contents.
+	//
+	// Additionally, the special case applies only if the merge wasn't already undone.
+	//
+	if ( !a.graveyardPosition && !context.bWasUndone && a.position.hasSameParentAs( b.sourcePosition ) ) {
+		const splitPath = b.graveyardPosition.path.slice();
+		splitPath.push( 0 );
+
+		const additionalSplit = new SplitOperation( new Position( b.graveyardPosition.root, splitPath ), 0, null, 0 );
+
+		a.position = a.position._getTransformedByMergeOperation( b );
+		a.graveyardPosition = Position.createFromPosition( additionalSplit.insertionPosition );
+		a.graveyardPosition.stickiness = 'toNext';
+
+		return [ additionalSplit, a ];
+	}
+
+	// The default case.
+	//
 	if ( a.position.hasSameParentAs( b.deletionPosition ) && !a.position.isAfter( b.deletionPosition ) ) {
 		a.howMany--;
 	}

+ 44 - 0
packages/ckeditor5-engine/tests/model/operation/transform/merge.js

@@ -60,6 +60,50 @@ describe( 'transform', () => {
 			} );
 		} );
 
+		describe( 'by remove', () => {
+			it( 'remove merged element', () => {
+				john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
+				kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );
+
+				john.merge();
+				kate.remove();
+
+				syncClients();
+
+				expectClients( '<paragraph>Foo</paragraph>' );
+			} );
+
+			it( 'remove merged element then undo #1', () => {
+				john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
+				kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );
+
+				john.merge();
+				kate.remove();
+
+				syncClients();
+				expectClients( '<paragraph>Foo</paragraph>' );
+
+				kate.undo();
+
+				syncClients();
+
+				expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
+			} );
+
+			it( 'remove merged element then undo #2', () => {
+				john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );
+				kate.setData( '<paragraph>Foo</paragraph>[<paragraph>Bar</paragraph>]' );
+
+				john.merge();
+				kate.remove();
+				kate.undo();
+
+				syncClients();
+
+				expectClients( '<paragraph>FooBar</paragraph>' );
+			} );
+		} );
+
 		describe( 'by delete', () => {
 			it( 'text from two elements', () => {
 				john.setData( '<paragraph>Foo</paragraph>[]<paragraph>Bar</paragraph>' );

+ 50 - 0
packages/ckeditor5-engine/tests/model/operation/transform/split.js

@@ -403,6 +403,56 @@ describe( 'transform', () => {
 				syncClients();
 				expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph>' );
 			} );
+
+			it( 'element into heading', () => {
+				john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
+				kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );
+
+				john.split();
+				kate.merge();
+
+				syncClients();
+				expectClients(
+					'<heading1>FooB</heading1>' +
+					'<paragraph>ar</paragraph>'
+				);
+			} );
+
+			it( 'element into heading with undo #1', () => {
+				john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
+				kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );
+
+				john.split();
+				kate.merge();
+
+				syncClients();
+				expectClients(
+					'<heading1>FooB</heading1>' +
+					'<paragraph>ar</paragraph>'
+				);
+
+				john.undo();
+				kate.undo();
+
+				syncClients();
+				expectClients( '<heading1>Foo</heading1><paragraph>Bar</paragraph>' );
+			} );
+
+			it( 'element into heading with undo #2', () => {
+				john.setData( '<heading1>Foo</heading1><paragraph>B[]ar</paragraph>' );
+				kate.setData( '<heading1>Foo</heading1>[]<paragraph>Bar</paragraph>' );
+
+				john.split();
+				kate.merge();
+				kate.undo();
+
+				syncClients();
+				expectClients(
+					'<heading1>Foo</heading1>' +
+					'<paragraph>B</paragraph>' +
+					'<paragraph>ar</paragraph>'
+				);
+			} );
 		} );
 
 		describe( 'by delete', () => {

+ 2 - 1
packages/ckeditor5-engine/tests/model/operation/transform/utils.js

@@ -5,6 +5,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
 import Typing from '@ckeditor/ckeditor5-typing/src/typing';
 import UndoEditing from '@ckeditor/ckeditor5-undo/src/undoediting';
 import BlockQuoteEditing from '@ckeditor/ckeditor5-block-quote/src/blockquoteediting';
+import HeadingEditing from '@ckeditor/ckeditor5-heading/src/headingediting';
 
 import { getData, parse } from '../../../../src/dev-utils/model';
 import { transformSets } from '../../../../src/model/operation/transform';
@@ -29,7 +30,7 @@ export class Client {
 			// Typing is needed for delete command.
 			// UndoEditing is needed for undo command.
 			// Block plugins are needed for proper data serializing.
-			plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing ]
+			plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing ]
 		} ).then( editor => {
 			this.editor = editor;
 			this.document = editor.model.document;