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 #879 from ckeditor/t/877
Browse files Browse the repository at this point in the history 
Fix: Live ranges, selections and markers no longer lose content when using the move delta. Closes #877.

The base algorithm implemented in `Range#_getTransformedByDocumentChange()` will now include all model items between the old and new range boundary. See https://github.com/ckeditor/ckeditor5-engine/issues/877#issuecomment-287740021 for more details.
  • Loading branch information
@Reinmar
Reinmar authored Mar 20, 2017
2 parents 79c2bfe + 3f4ef3e commit e08b019
Show file tree
Hide file tree
Showing 5 changed files with 125 additions and 59 deletions.
2 changes: 2 additions & 0 deletions src/model/delta/basic-transformations.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,8 @@ addTransformationCase( MarkerDelta, SplitDelta, transformMarkerDelta );
addTransformationCase( MarkerDelta, MergeDelta, transformMarkerDelta );
addTransformationCase( MarkerDelta, WrapDelta, transformMarkerDelta );
addTransformationCase( MarkerDelta, UnwrapDelta, transformMarkerDelta );
addTransformationCase( MarkerDelta, MoveDelta, transformMarkerDelta );
addTransformationCase( MarkerDelta, RenameDelta, transformMarkerDelta );

// Add special case for MoveDelta x MergeDelta transformation.
addTransformationCase( MoveDelta, MergeDelta, ( a, b, isStrong ) => {
Expand Down
80 changes: 31 additions & 49 deletions src/model/range.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -447,61 +447,43 @@ export default class Range {
* @returns {Array.<module:engine/model/range~Range>}
*/
_getTransformedByDocumentChange( type, deltaType, targetPosition, howMany, sourcePosition ) {
// IMPORTANT! Every special case added here has to be reflected in MarkerDelta transformations!
// Check /src/model/delta/basic-transformations.js.
if ( type == 'insert' ) {
return this._getTransformedByInsertion( targetPosition, howMany, false, false );
} else {
const ranges = this._getTransformedByMove( sourcePosition, targetPosition, howMany );

// Don't ask. Just debug.
// Like this: https://github.com/ckeditor/ckeditor5-engine/issues/841#issuecomment-282706488.
//
// In following cases, in examples, the last step is the fix step.
// When there are multiple ranges in an example, ranges[] array indices are represented as follows:
// * [] is ranges[ 0 ],
// * {} is ranges[ 1 ],
// * () is ranges[ 2 ].
if ( type == 'move' ) {
const sourceRange = Range.createFromPositionAndShift( sourcePosition, howMany );

if ( deltaType == 'split' && this.containsPosition( sourcePosition ) ) {
// Range contains a position where an element is split.
// <p>f[ooba]r</p> -> <p>f[ooba]r</p><p></p> -> <p>f[oo]</p><p>{ba}r</p> -> <p>f[oo</p><p>ba]r</p>
return [ new Range( ranges[ 0 ].start, ranges[ 1 ].end ) ];
} else if ( deltaType == 'merge' && this.isCollapsed && ranges[ 0 ].start.isEqual( sourcePosition ) ) {
// Collapsed range is in merged element.
// Without fix, the range would end up in the graveyard, together with removed element.
// <p>foo</p><p>[]bar</p> -> <p>foobar</p><p>[]</p> -> <p>foobar</p> -> <p>foo[]bar</p>
return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ];
} else if ( deltaType == 'wrap' ) {
// Range intersects (at the start) with wrapped element (<p>ab</p>).
// <p>a[b</p><p>c]d</p> -> <p>a[b</p><w></w><p>c]d</p> -> [<w>]<p>a(b</p>){</w><p>c}d</p> -> <w><p>a[b</p></w><p>c]d</p>
if ( sourceRange.containsPosition( this.start ) && this.containsPosition( sourceRange.end ) ) {
return [ new Range( ranges[ 2 ].start, ranges[ 1 ].end ) ];
}
// Range intersects (at the end) with wrapped element (<p>cd</p>).
// <p>a[b</p><p>c]d</p> -> <p>a[b</p><p>c]d</p><w></w> -> <p>a[b</p>]<w>{<p>c}d</p></w> -> <p>a[b</p><w><p>c]d</p></w>
else if ( sourceRange.containsPosition( this.end ) && this.containsPosition( sourceRange.start ) ) {
return [ new Range( ranges[ 0 ].start, ranges[ 1 ].end ) ];
}
} else if ( deltaType == 'unwrap' ) {
// Range intersects (at the beginning) with unwrapped element (<w></w>).
// <w><p>a[b</p></w><p>c]d</p> -> <p>a{b</p>}<w>[</w><p>c]d</p> -> <p>a[b</p><w></w><p>c]d</p>
// <w></w> is removed in next operation, but the remove does not mess up ranges.
if ( sourceRange.containsPosition( this.start ) && this.containsPosition( sourceRange.end ) ) {
return [ new Range( ranges[ 1 ].start, ranges[ 0 ].end ) ];
}
// Range intersects (at the end) with unwrapped element (<w></w>).
// <p>a[b</p><w><p>c]d</p></w> -> <p>a[b</p>](<p>c)d</p>{<w>}</w> -> <p>a[b</p><p>c]d</p><w></w>
// <w></w> is removed in next operation, but the remove does not mess up ranges.
else if ( sourceRange.containsPosition( this.end ) && this.containsPosition( sourceRange.start ) ) {
return [ new Range( ranges[ 0 ].start, ranges[ 2 ].end ) ];
}
const sourceRange = Range.createFromPositionAndShift( sourcePosition, howMany );

if ( deltaType == 'merge' && this.isCollapsed && ( this.start.isEqual( sourceRange.start ) || this.start.isEqual( sourceRange.end ) ) ) {
// Collapsed range is in merged element.
// Without fix, the range would end up in the graveyard, together with removed element.
// <p>foo</p><p>[]bar</p> -> <p>foobar</p><p>[]</p> -> <p>foobar</p> -> <p>foo[]bar</p>
return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ];
} else if ( type == 'move' ) {
// In all examples `[]` is `this` and `{}` is `sourceRange`, while `^` is move target position.
//
// Example:
// <p>xx</p>^<w>{<p>a[b</p>}</w><p>c]d</p> --> <p>xx</p><p>a[b</p><w></w><p>c]d</p>
// ^<p>xx</p><w>{<p>a[b</p>}</w><p>c]d</p> --> <p>a[b</p><p>xx</p><w></w><p>c]d</p> // Note <p>xx</p> inclusion.
// <w>{<p>a[b</p>}</w>^<p>c]d</p> --> <w></w><p>a[b</p><p>c]d</p>
if ( sourceRange.containsPosition( this.start ) && this.containsPosition( sourceRange.end ) && this.end.isAfter( targetPosition ) ) {
let start = this.start._getCombined( sourcePosition, targetPosition._getTransformedByDeletion( sourcePosition, howMany ) );
const end = this.end._getTransformedByMove( sourcePosition, targetPosition, howMany, false, false );

return [ new Range( start, end ) ];
}

// Example:
// <p>c[d</p><w>{<p>a]b</p>}</w>^<p>xx</p> --> <p>c[d</p><w></w><p>a]b</p><p>xx</p>
// <p>c[d</p><w>{<p>a]b</p>}</w><p>xx</p>^ --> <p>c[d</p><w></w><p>xx</p><p>a]b</p> // Note <p>xx</p> inclusion.
// <p>c[d</p>^<w>{<p>a]b</p>}</w> --> <p>c[d</p><p>a]b</p><w></w>
if ( sourceRange.containsPosition( this.end ) && this.containsPosition( sourceRange.start ) && this.start.isBefore( targetPosition ) ) {
const start = this.start._getTransformedByMove( sourcePosition, targetPosition, howMany, true, false );
let end = this.end._getCombined( sourcePosition, targetPosition._getTransformedByDeletion( sourcePosition, howMany ) );

return [ new Range( start, end ) ];
}
}

return ranges;
return this._getTransformedByMove( sourcePosition, targetPosition, howMany );
}
}

Expand Down
6 changes: 3 additions & 3 deletions tests/model/liverange.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -292,7 +292,7 @@ describe( 'LiveRange', () => {
doc.fire( 'change', 'move', changes, null );

expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
expect( live.end.path ).to.deep.equal( [ 0, 2, 1 ] );
expect( live.end.path ).to.deep.equal( [ 2, 1 ] ); // Included some nodes.
expect( spy.calledOnce ).to.be.true;
} );

Expand All @@ -307,7 +307,7 @@ describe( 'LiveRange', () => {
doc.fire( 'change', 'move', changes, null );

expect( live.start.path ).to.deep.equal( [ 0, 1, 4 ] );
expect( live.end.path ).to.deep.equal( [ 0, 2, 6 ] );
expect( live.end.path ).to.deep.equal( [ 0, 2, 1 ] );
expect( spy.calledOnce ).to.be.true;
} );

Expand Down Expand Up @@ -357,7 +357,7 @@ describe( 'LiveRange', () => {
};
doc.fire( 'change', 'move', changes, null );

expect( live.start.path ).to.deep.equal( [ 0, 1, 2 ] );
expect( live.start.path ).to.deep.equal( [ 0, 1, 9 ] );
expect( live.end.path ).to.deep.equal( [ 0, 1, 12 ] );
expect( spy.calledOnce ).to.be.true;
} );
Expand Down
2 changes: 1 addition & 1 deletion tests/model/liveselection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -453,7 +453,7 @@ describe( 'LiveSelection', () => {
let range = selection.getFirstRange();

expect( range.start.path ).to.deep.equal( [ 0, 2 ] );
expect( range.end.path ).to.deep.equal( [ 1, 3 ] );
expect( range.end.path ).to.deep.equal( [ 5 ] );
expect( spyRange.calledOnce ).to.be.true;
} );

Expand Down
94 changes: 88 additions & 6 deletions tests/model/range.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -750,8 +750,8 @@ describe( 'Range', () => {
describe( 'by MoveDelta', () => {
it( 'move before range', () => {
const start = new Position( root, [ 0 ] );
const end = new Position( otherRoot, [ 0 ] );
const delta = getMoveDelta( start, 2, end, 1 );
const target = new Position( otherRoot, [ 0 ] );
const delta = getMoveDelta( start, 2, target, 1 );

const transformed = range.getTransformedByDelta( delta );

Expand All @@ -760,8 +760,8 @@ describe( 'Range', () => {

it( 'move intersecting with range (and targeting before range)', () => {
const start = new Position( root, [ 4 ] );
const end = new Position( root, [ 0 ] );
const delta = getMoveDelta( start, 2, end, 1 );
const target = new Position( root, [ 0 ] );
const delta = getMoveDelta( start, 2, target, 1 );

const transformed = range.getTransformedByDelta( delta );

Expand All @@ -772,15 +772,80 @@ describe( 'Range', () => {
it( 'move inside the range', () => {
range.end.offset = 6;
const start = new Position( root, [ 3 ] );
const end = new Position( root, [ 5 ] );
const delta = getMoveDelta( start, 1, end, 1 );
const target = new Position( root, [ 5 ] );
const delta = getMoveDelta( start, 1, target, 1 );

const transformed = range.getTransformedByDelta( delta );

expectRange( transformed[ 0 ], 2, 4 );
expectRange( transformed[ 1 ], 5, 6 );
expectRange( transformed[ 2 ], 4, 5 );
} );

// #877.
it( 'moved element contains range start and is moved towards inside of range', () => {
// Initial state:
// <w><p>abc</p><p>x[x</p></w><p>d]ef</p>
// Expected state after moving `<p>` out of `<w>`:
// <w><p>abc</p></w><p>x[x</p><p>d]ef</p>

const range = new Range( new Position( root, [ 0, 1, 1 ] ), new Position( root, [ 1, 1 ] ) );
const delta = getMoveDelta( new Position( root, [ 0, 1 ] ), 1, new Position( root, [ 1 ] ), 1 );

const transformed = range.getTransformedByDelta( delta );

expect( transformed.length ).to.equal( 1 );
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 1, 1 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 2, 1 ] );
} );

it( 'moved element contains range start and is moved out of range', () => {
// Initial state:
// <p>abc</p><p>x[x</p><p>d]ef</p>
// Expected state after moving:
// <p>x[x</p><p>abc</p><p>d]ef</p>

const range = new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 2, 1 ] ) );
const delta = getMoveDelta( new Position( root, [ 1 ] ), 1, new Position( root, [ 0 ] ), 1 );

const transformed = range.getTransformedByDelta( delta );

expect( transformed.length ).to.equal( 1 );
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 1 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 2, 1 ] );
} );

it( 'moved element contains range end and is moved towards range', () => {
// Initial state:
// <p>a[bc</p><p>def</p><p>x]x</p>
// Expected state after moving:
// <p>a[bc</p><p>x]x</p><p>def</p>

const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 2, 1 ] ) );
const delta = getMoveDelta( new Position( root, [ 2 ] ), 1, new Position( root, [ 1 ] ), 1 );

const transformed = range.getTransformedByDelta( delta );

expect( transformed.length ).to.equal( 1 );
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 1 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 1, 1 ] );
} );

it( 'moved element contains range end and is moved out of range', () => {
// Initial state:
// <p>a[bc</p><p>x]x</p><p>def</p>
// Expected state after moving:
// <p>a[bc</p><p>def</p><p>x]x</p>

const range = new Range( new Position( root, [ 0, 1 ] ), new Position( root, [ 1, 1 ] ) );
const delta = getMoveDelta( new Position( root, [ 1 ] ), 1, new Position( root, [ 3 ] ), 1 );

const transformed = range.getTransformedByDelta( delta );

expect( transformed.length ).to.equal( 1 );
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 1 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 2, 1 ] );
} );
} );

describe( 'by RemoveDelta', () => {
Expand Down Expand Up @@ -858,6 +923,23 @@ describe( 'Range', () => {
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 3 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 3 ] );
} );

// #877.
it( 'merge elements that contain elements with range boundaries', () => {
// Initial state:
// <w><p>x[x</p></w><w><p>y]y</p></w>
// Expected state after merge:
// <w><p>x[x</p><p>y]y</p></w>

const range = new Range( new Position( root, [ 0, 0, 1 ] ), new Position( root, [ 1, 0, 1 ] ) );
const delta = getMergeDelta( new Position( root, [ 1 ] ), 1, 1, 1 );

const transformed = range.getTransformedByDelta( delta );

expect( transformed.length ).to.equal( 1 );
expect( transformed[ 0 ].start.path ).to.deep.equal( [ 0, 0, 1 ] );
expect( transformed[ 0 ].end.path ).to.deep.equal( [ 0, 1, 1 ] );
} );
} );

describe( 'by WrapDelta', () => {
Expand Down

0 comments on commit e08b019

Please sign in to comment.