Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed editor crash after pasting over previously pasted content and undoing both actions #17454

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed editor crash after pasting over previously pasted content and undoing both actions #17454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
68 changes: 60 additions & 8 deletions packages/ckeditor5-engine/src/model/operation/transform.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,10 +539,17 @@ class ContextFactory {
}
} else if ( opA instanceof MergeOperation ) {
if ( opB instanceof MergeOperation ) {
// There can be only relation set for a pair of operations, but it is fine for these cases here.
// There is only one scenario when a pair can fulfill two cases -- `mergeSameTarget` and `mergeSameElement` which happens
// if operations are equal. But in this case it is okay that `mergeSameElement` overwrites `mergeSameTarget`.
if ( !opA.targetPosition.isEqual( opB.sourcePosition ) ) {
this._setRelation( opA, opB, 'mergeTargetNotMoved' );
}

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

if ( opA.sourcePosition.isEqual( opB.targetPosition ) ) {
this._setRelation( opA, opB, 'mergeSourceNotMoved' );
}
Expand All @@ -559,6 +566,10 @@ class ContextFactory {
this._setRelation( opA, opB, 'mergeSourceAffected' );
}

// After changes introduced for issue #17267 this branch was not covered anymore by our tests, as the introduced fixes
// covered the test that was earlier covered by this `if`. However, it is not 100% clear, that this cannot happen anymore,
// hence it was decided to keep handling this scenario anyway.
/* istanbul ignore next -- @preserve */
if ( opA.targetPosition.isEqual( opB.sourcePosition ) ) {
this._setRelation( opA, opB, 'mergeTargetWasBefore' );
}
Expand Down Expand Up @@ -1381,14 +1392,32 @@ setTransformation( MergeOperation, MergeOperation, ( a, b, context ) => {
}

// The default case.
// TODO: Possibly, there's a missing case for same `targetPosition` but different `sourcePosition`.
//
if ( a.sourcePosition.hasSameParentAs( b.targetPosition ) ) {
a.howMany += b.howMany;
}

a.sourcePosition = a.sourcePosition._getTransformedByMergeOperation( b );
a.targetPosition = a.targetPosition._getTransformedByMergeOperation( b );

if ( a.targetPosition.isEqual( b.targetPosition ) ) {
// Case 3:
//
// Operations target to the same position (merge something into the same element). But unlike case 1, the operations are not equal,
// because they have different source position.
//
// In most cases, when operations point to the same merge target, the operations are equal (i.e. they also have the same source).
// However, in some more complex undo cases (including multiple steps) it happens that two different elements are merged into
// the same element. It only happens "temporarily" as the operation is being transformed during undo process.
// It doesn't seem that this can happen in real-time collaboration.
//
// In this case, simply move the `a` merge target position, so it still points to the end of the target element.
//
a.targetPosition = a.targetPosition.getShiftedBy( b.howMany );
} else {
// The default case.
//
a.targetPosition = a.targetPosition._getTransformedByMergeOperation( b );
}

// Handle positions in graveyard.
// If graveyard positions are same and `a` operation is strong - do not transform.
Expand Down Expand Up @@ -1438,6 +1467,10 @@ setTransformation( MergeOperation, MoveOperation, ( a, b, context ) => {
// [] is move operation, } is merge source position (sticks to previous by default):
// <p>A[b]}c</p> -> <p>A}c</p>
//
// After changes introduced for issue #17267 two of the else-if branches were not covered anymore by our tests, as the introduced fixes
// covered the test that was earlier covered by these branches. However, it is not 100% clear, that this cannot happen anymore,
// hence it was decided to keep handling these scenarios anyway.
/* istanbul ignore next -- @preserve */
if ( b.sourcePosition.getShiftedBy( b.howMany ).isEqual( a.sourcePosition ) ) {
a.sourcePosition.stickiness = 'toNone';
}
Expand Down Expand Up @@ -1609,11 +1642,10 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {
}

// 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.
// See that case in `MergeOperation` x `MergeOperation` transformation.
//
// 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.
// Now, the merge operation is transformed by the split which has undone that previous merge operation. Make sure to set
// correct properties on merge operation `a`, just like the earlier merge and this split did not happen.
//
if ( context.abRelation == 'mergeSameElement' || a.sourcePosition.offset > 0 ) {
a.sourcePosition = b.moveTargetPosition.clone();
Expand All @@ -1626,11 +1658,31 @@ setTransformation( MergeOperation, SplitOperation, ( a, b, context ) => {
// The default case.
//
if ( a.sourcePosition.hasSameParentAs( b.splitPosition ) ) {
a.howMany = b.splitPosition.offset;
if ( b.splitPosition.offset >= a.sourcePosition.offset ) {
a.howMany = b.splitPosition.offset - a.sourcePosition.offset;
}
}

a.sourcePosition = a.sourcePosition._getTransformedBySplitOperation( b );
a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );

if ( a.targetPosition.hasSameParentAs( b.splitPosition ) && context.abRelation == 'mergeSameTarget' ) {
// Case 3:
//
// Merge operation targets into an element which is being split, but unlike case 1, the split happens at different (earlier)
// position. This is a regular case. By default, the merge target position is moved to the right-hand split part of the split
// element: <p>Foobar^</p><p>XYZ</p> --> <p>Foo</p><p>bar^</p><p>XYZ</p>. This happens in the default handling below.
//
// However, in some cases, we need to do something different. If this split operation is undoing a merge operation, and both
// `a` merge operation and the undone merge operation were targeting to the same element, we will acknowledge, that this
// `a` merge operation at the very beginning was supposed to merge into the original element. We will keep the target position in
// the original element, to ensure proper processing during undo: <p>Foobar^</p><p>XYZ</p> --> <p>Foo^</p><p>bar</p><p>XYZ</p>.
//
a.targetPosition = a.targetPosition.getShiftedBy( -b.howMany );
} else {
// The default case.
//
a.targetPosition = a.targetPosition._getTransformedBySplitOperation( b );
}

return [ a ];
} );
Expand Down
46 changes: 46 additions & 0 deletions packages/ckeditor5-engine/tests/model/operation/transform/undo.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -930,5 +930,51 @@ describe( 'transform', () => {

expectClients( '<paragraph>ABCD</paragraph>' );
} );

it( 'paste on text, then paste on that paste, then undo, undo, redo, redo', () => {
john.setData( '<paragraph>[]Some text to work with. A sentence to replace. More text to work with</paragraph>' );

// Paste over 'A sentence to replace. '.
john.editor.model.change( writer => {
const root = john.editor.model.document.getRoot();
const range = writer.createRange(
writer.createPositionFromPath( root, [ 0, 24 ] ),
writer.createPositionFromPath( root, [ 0, 47 ] )
);

john.editor.model.insertContent(
new DocumentFragment( [ new Element( 'paragraph', null, new Text( 'First text change. ' ) ) ] ),
range
);
} );

// Paste over 'First text change. '.
john.editor.model.change( writer => {
const root = john.editor.model.document.getRoot();
const range = writer.createRange(
writer.createPositionFromPath( root, [ 0, 24 ] ),
writer.createPositionFromPath( root, [ 0, 43 ] )
);

john.editor.model.insertContent(
new DocumentFragment( [ new Element( 'paragraph', null, new Text( 'Second text change. ' ) ) ] ),
range
);
} );

expectClients( '<paragraph>Some text to work with. Second text change. More text to work with</paragraph>' );

john.undo();
expectClients( '<paragraph>Some text to work with. First text change. More text to work with</paragraph>' );

john.undo();
expectClients( '<paragraph>Some text to work with. A sentence to replace. More text to work with</paragraph>' );

john.redo();
expectClients( '<paragraph>Some text to work with. First text change. More text to work with</paragraph>' );

john.redo();
expectClients( '<paragraph>Some text to work with. Second text change. More text to work with</paragraph>' );
} );
} );
} );