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

Jump to bottom

Fixed content not restored on undo when multiple blocks and widgets were removed #8872

Merged
merged 3 commits into from
Feb 2, 2021
Merged

Fixed content not restored on undo when multiple blocks and widgets were removed #8872

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
81fd7ac
Fix: Fixed content not restored on undo when multiple blocks and widg…
scofalik Jan 20, 2021
2b52b40
Test: Added test for the fixed scenario.
scofalik Jan 20, 2021
4127a0c
Merge branch 'master' into i/8870
scofalik Feb 2, 2021
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
3 changes: 1 addition & 2 deletions packages/ckeditor5-engine/src/model/operation/splitoperation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ export default class SplitOperation extends Operation {
* @member {module:engine/model/position~Position} module:engine/model/operation/splitoperation~SplitOperation#insertionPosition
*/
this.insertionPosition = SplitOperation.getInsertionPosition( splitPosition );
this.insertionPosition.stickiness = 'toNone';

/**
* Position in the graveyard root before the element which should be used as a parent of the nodes after `position`.
Expand Down Expand Up @@ -230,7 +229,7 @@ export default class SplitOperation extends Operation {
const path = splitPosition.path.slice( 0, -1 );
path[ path.length - 1 ]++;

return new Position( splitPosition.root, path );
return new Position( splitPosition.root, path, 'toPrevious' );
}

/**
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-engine/src/model/operation/transform.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1981,7 +1981,7 @@ setTransformation( SplitOperation, InsertOperation, ( a, b ) => {
}

a.splitPosition = a.splitPosition._getTransformedByInsertOperation( b );
a.insertionPosition = SplitOperation.getInsertionPosition( a.splitPosition );
a.insertionPosition = a.insertionPosition._getTransformedByInsertOperation( b );

return [ a ];
} );
Expand Down
2 changes: 1 addition & 1 deletion packages/ckeditor5-engine/tests/model/operation/transform/merge.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ describe( 'transform', () => {

john.undo();
syncClients();
expectClients( '<paragraph>Foo</paragraph><paragraph>Bar</paragraph><paragraph></paragraph>' );
expectClients( '<paragraph>Foo</paragraph><paragraph></paragraph><paragraph>Bar</paragraph>' );
Copy link
Contributor Author

@scofalik scofalik Jan 20, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The old expectation was incorrect (worse UX wise) than the new one. The new expectation actually returns to the state before any changes...


kate.undo();
syncClients();
Expand Down
24 changes: 24 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 @@ -686,4 +686,28 @@ describe( 'transform', () => {

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

// https://github.com/ckeditor/ckeditor5/issues/8870
it( 'object, p, p, p, remove, undo', () => {
john.setData( '<image></image><paragraph>A</paragraph><paragraph>B[]</paragraph>' );

// I couldn't use delete command because simply executing this command several times does not
// work correctly when selection reaches <image></image><p>[]</p>.
// At this point we have custom control for firing delete event on view and I didn't want to
// simulate that.
//
john.remove( [ 2, 0 ], [ 2, 1 ] );
john.merge( [ 2 ] );
john.remove( [ 1, 0 ], [ 1, 1 ] );
john.remove( [ 1 ], [ 2 ] );
john.remove( [ 0 ], [ 1 ] );

john.undo();
john.undo();
john.undo();
john.undo();
john.undo();

expectClients( '<image></image><paragraph>A</paragraph><paragraph>B</paragraph>' );
} );
} );
5 changes: 4 additions & 1 deletion packages/ckeditor5-engine/tests/model/operation/transform/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ 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 TableEditing from '@ckeditor/ckeditor5-table/src/tableediting';
import ImageEditing from '@ckeditor/ckeditor5-image/src/image/imageediting';

import { getData, parse } from '../../../../src/dev-utils/model';
import { transformSets } from '../../../../src/model/operation/transform';
Expand All @@ -38,7 +39,9 @@ export class Client {
// UndoEditing is needed for undo command.
// Block plugins are needed for proper data serializing.
// BoldEditing is needed for bold command.
plugins: [ Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing, BoldEditing, TableEditing ]
plugins: [
Typing, Paragraph, ListEditing, UndoEditing, BlockQuoteEditing, HeadingEditing, BoldEditing, TableEditing, ImageEditing
]
} ).then( editor => {
this.editor = editor;
this.document = editor.model.document;
Expand Down