Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Commit

Permalink
Merge pull request #1702 from ckeditor/t/1701
Browse files Browse the repository at this point in the history
Fix: Editor crashed during undo in some pasting+remove scenarios. Closes #1701.
  • Loading branch information
Piotr Jasiun authored Mar 21, 2019
2 parents 45cd071 + 71af35a commit ca619e7
Show file tree
Hide file tree
Showing 2 changed files with 91 additions and 20 deletions.
58 changes: 47 additions & 11 deletions src/model/operation/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -508,6 +508,10 @@ class ContextFactory {
this._setRelation( opA, opB, 'mergeTargetNotMoved' );
}

if ( opA.sourcePosition.isEqual( opB.targetPosition ) ) {
this._setRelation( opA, opB, 'mergeSourceNotMoved' );
}

if ( opA.sourcePosition.isEqual( opB.sourcePosition ) ) {
this._setRelation( opA, opB, 'mergeSameElement' );
}
Expand Down Expand Up @@ -1434,19 +1438,34 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {

// Case 2:
//
// Merge source is at the same position as split position. This sometimes happen during undo. This merge operation
// might have been earlier transformed by a merge operation which both merged the same element. See case in
// `MergeOperation` x `MergeOperation` transformation. In that case, if the merge operation has been undone, the special
// case is not applied.
//
// In this scenario the merge operation is now transformed by the split which has undone the previous merge operation.
// So now we are fixing situation which was skipped in `MergeOperation` x `MergeOperation` case.
// Merge source is at the same position as split position. This sometimes happen, mostly during undo.
// The decision here is mostly to choose whether merge source position should stay where it is (so it will be at the end of the
// split element) or should be move to the beginning of the new element.
//
if ( a.sourcePosition.isEqual( b.splitPosition ) && ( context.abRelation == 'mergeSameElement' || a.sourcePosition.offset > 0 ) ) {
a.sourcePosition = b.moveTargetPosition.clone();
a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );
if ( a.sourcePosition.isEqual( b.splitPosition ) ) {
// Use context to check if `SplitOperation` is not undoing a merge operation, that didn't change the `a` operation.
// This scenario happens the undone merge operation moved nodes at the source position of `a` operation.
// In that case `a` operation source position should stay where it is.
if ( context.abRelation == 'mergeSourceNotMoved' ) {
a.howMany = 0;
a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );

return [ a ];
return [ a ];
}

// This merge operation might have been earlier transformed by a merge operation which both merged the same element.
// See that case in `MergeOperation` x `MergeOperation` transformation. In that scenario, if the merge operation has been undone,
// the special case is not applied.
//
// Now, the merge operation is transformed by the split which has undone that previous merge operation.
// So now we are fixing situation which was skipped in `MergeOperation` x `MergeOperation` case.
//
if ( context.abRelation == 'mergeSameElement' || a.sourcePosition.offset > 0 ) {
a.sourcePosition = b.moveTargetPosition.clone();
a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );

return [ a ];
}
}

// The default case.
Expand Down Expand Up @@ -2165,6 +2184,9 @@ setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => {
//
// So we cancel split operation only if it was really identical.
//
// Also, there is additional case, where split operations aren't identical and should not be cancelled, however the
// default transformation is incorrect too.
//
if ( a.splitPosition.isEqual( b.splitPosition ) ) {
if ( !a.graveyardPosition && !b.graveyardPosition ) {
return [ new NoOperation( 0 ) ];
Expand All @@ -2173,6 +2195,20 @@ setTransformation( SplitOperation, SplitOperation, ( a, b, context ) => {
if ( a.graveyardPosition && b.graveyardPosition && a.graveyardPosition.isEqual( b.graveyardPosition ) ) {
return [ new NoOperation( 0 ) ];
}

// Use context to know that the `a.splitPosition` should stay where it is.
// This happens during undo when first a merge operation moved nodes to `a.splitPosition` and now `b` operation undoes that merge.
if ( context.abRelation == 'splitBefore' ) {
// Since split is at the same position, there are no nodes left to split.
a.howMany = 0;

// Note: there was `if ( a.graveyardPosition )` here but it was uncovered in tests and I couldn't find any scenarios for now.
// That would have to be a `SplitOperation` that didn't come from undo but is transformed by operations that were undone.
// It could happen if `context` is enabled in collaboration.
a.graveyardPosition = a.graveyardPosition._getTransformedBySplitOperation( b );

return [ a ];
}
}

// Case 2:
Expand Down
53 changes: 44 additions & 9 deletions tests/model/operation/transform/undo.js
Original file line number Diff line number Diff line change
Expand Up @@ -616,15 +616,13 @@ describe( 'transform', () => {

// https://github.com/ckeditor/ckeditor5/issues/1540
it( 'paste, select all, paste, undo, undo, redo, redo, redo', () => {
const model = john.editor.model;

john.setData( '<paragraph>[]</paragraph>' );

model.insertContent( getPastedContent() );
pasteContent();

john.setSelection( [ 0, 0 ], [ 1, 3 ] );

model.insertContent( getPastedContent() );
pasteContent();

expectClients( '<heading1>Foo</heading1><paragraph>Bar</paragraph>' );

Expand All @@ -644,11 +642,48 @@ describe( 'transform', () => {

expectClients( '<heading1>Foo</heading1><paragraph>Bar</paragraph>' );

function getPastedContent() {
return new DocumentFragment( [
new Element( 'heading1', null, new Text( 'Foo' ) ),
new Element( 'paragraph', null, new Text( 'Bar' ) )
] );
function pasteContent() {
john.editor.model.insertContent(
new DocumentFragment( [
new Element( 'heading1', null, new Text( 'Foo' ) ),
new Element( 'paragraph', null, new Text( 'Bar' ) )
] )
);
}
} );

// Happens in track changes. Emulated here.
// https://github.com/ckeditor/ckeditor5-engine/issues/1701
it( 'paste, remove, undo, undo, redo, redo', () => {
john.setData( '<paragraph>Ab[]cd</paragraph><paragraph>Wxyz</paragraph>' );

john.editor.model.insertContent(
new DocumentFragment( [
new Element( 'paragraph', null, new Text( 'Foo' ) ),
new Element( 'paragraph', null, new Text( 'Bar' ) )
] )
);

john.setSelection( [ 1, 3 ], [ 2, 2 ] );

john._processExecute( 'delete' );

expectClients( '<paragraph>AbFoo</paragraph><paragraph>Baryz</paragraph>' );

john.undo();

expectClients( '<paragraph>AbFoo</paragraph><paragraph>Barcd</paragraph><paragraph>Wxyz</paragraph>' );

john.undo();

expectClients( '<paragraph>Abcd</paragraph><paragraph>Wxyz</paragraph>' );

john.redo();

expectClients( '<paragraph>AbFoo</paragraph><paragraph>Barcd</paragraph><paragraph>Wxyz</paragraph>' );

john.redo();

expectClients( '<paragraph>AbFoo</paragraph><paragraph>Baryz</paragraph>' );
} );
} );

0 comments on commit ca619e7

Please sign in to comment.