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

Commit efe3987

Browse files
authored
Merge pull request #856 from ckeditor/t/841
Fix: Fixed a bug where `LiveRange` position would be lost when using wrap and unwrap deltas. Closes #841.
2 parents 9aa4913 + 81bdf6d commit efe3987

File tree

5 files changed

+510
-45
lines changed

5 files changed

+510
-45
lines changed

src/model/delta/basic-transformations.js

Lines changed: 10 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -97,33 +97,20 @@ addTransformationCase( InsertDelta, MergeDelta, ( a, b, isStrong ) => {
9797
return defaultTransform( a, b, isStrong );
9898
} );
9999

100-
// Add special case for MarkerDelta x SplitDelta
101-
addTransformationCase( MarkerDelta, SplitDelta, ( a, b, isStrong ) => {
102-
// If marked range is split, we need to fix it:
103-
// ab[cdef]gh ==> ab[cd
104-
// ef]gh
105-
// To mimic what normally happens with LiveRange if you split it.
106-
107-
// Mote: MarkerDelta can't get split to two deltas, neither can MarkerOperation.
108-
const transformedDelta = defaultTransform( a, b, isStrong )[ 0 ];
100+
function transformMarkerDelta( a, b ) {
101+
const transformedDelta = a.clone();
109102
const transformedOp = transformedDelta.operations[ 0 ];
110103

111-
// Fix positions, if needed.
112-
const markerOp = a.operations[ 0 ];
113-
114-
const source = b.position;
115-
const target = b._moveOperation.targetPosition;
116-
117-
if ( markerOp.oldRange.containsPosition( b.position ) ) {
118-
transformedOp.oldRange.end = markerOp.oldRange.end._getCombined( source, target );
119-
}
120-
121-
if ( markerOp.newRange.containsPosition( b.position ) ) {
122-
transformedOp.newRange.end = markerOp.newRange.end._getCombined( source, target );
123-
}
104+
transformedOp.oldRange = transformedOp.oldRange.getTransformedByDelta( b )[ 0 ];
105+
transformedOp.newRange = transformedOp.newRange.getTransformedByDelta( b )[ 0 ];
124106

125107
return [ transformedDelta ];
126-
} );
108+
}
109+
110+
addTransformationCase( MarkerDelta, SplitDelta, transformMarkerDelta );
111+
addTransformationCase( MarkerDelta, MergeDelta, transformMarkerDelta );
112+
addTransformationCase( MarkerDelta, WrapDelta, transformMarkerDelta );
113+
addTransformationCase( MarkerDelta, UnwrapDelta, transformMarkerDelta );
127114

128115
// Add special case for MoveDelta x MergeDelta transformation.
129116
addTransformationCase( MoveDelta, MergeDelta, ( a, b, isStrong ) => {

src/model/range.js

Lines changed: 49 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -447,20 +447,58 @@ export default class Range {
447447
* @returns {Array.<module:engine/model/range~Range>}
448448
*/
449449
_getTransformedByDocumentChange( type, deltaType, targetPosition, howMany, sourcePosition ) {
450+
// IMPORTANT! Every special case added here has to be reflected in MarkerDelta transformations!
451+
// Check /src/model/delta/basic-transformations.js.
450452
if ( type == 'insert' ) {
451453
return this._getTransformedByInsertion( targetPosition, howMany, false, false );
452454
} else {
453455
const ranges = this._getTransformedByMove( sourcePosition, targetPosition, howMany );
454456

455-
if ( deltaType == 'split' && this.containsPosition( sourcePosition ) ) {
456-
// Special case for splitting element inside range.
457-
// <p>f[ooba]r</p> -> <p>f[oo</p><p>ba]r</p>
458-
ranges[ 0 ].end = ranges[ 1 ].end;
459-
ranges.pop();
460-
} else if ( deltaType == 'merge' && type == 'move' && this.isCollapsed && ranges[ 0 ].start.isEqual( sourcePosition ) ) {
461-
// Special case when collapsed range is in merged element.
462-
// <p>foo</p><p>[]bar{}</p> -> <p>foo[]bar{}</p>
463-
ranges[ 0 ] = new Range( targetPosition.getShiftedBy( this.start.offset ) );
457+
// Don't ask. Just debug.
458+
// Like this: https://github.com/ckeditor/ckeditor5-engine/issues/841#issuecomment-282706488.
459+
//
460+
// In following cases, in examples, the last step is the fix step.
461+
// When there are multiple ranges in an example, ranges[] array indices are represented as follows:
462+
// * [] is ranges[ 0 ],
463+
// * {} is ranges[ 1 ],
464+
// * () is ranges[ 2 ].
465+
if ( type == 'move' ) {
466+
const sourceRange = Range.createFromPositionAndShift( sourcePosition, howMany );
467+
468+
if ( deltaType == 'split' && this.containsPosition( sourcePosition ) ) {
469+
// Range contains a position where an element is split.
470+
// <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>
471+
return [ new Range( ranges[ 0 ].start, ranges[ 1 ].end ) ];
472+
} else if ( deltaType == 'merge' && this.isCollapsed && ranges[ 0 ].start.isEqual( sourcePosition ) ) {
473+
// Collapsed range is in merged element.
474+
// Without fix, the range would end up in the graveyard, together with removed element.
475+
// <p>foo</p><p>[]bar</p> -> <p>foobar</p><p>[]</p> -> <p>foobar</p> -> <p>foo[]bar</p>
476+
return [ new Range( targetPosition.getShiftedBy( this.start.offset ) ) ];
477+
} else if ( deltaType == 'wrap' ) {
478+
// Range intersects (at the start) with wrapped element (<p>ab</p>).
479+
// <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>
480+
if ( sourceRange.containsPosition( this.start ) && this.containsPosition( sourceRange.end ) ) {
481+
return [ new Range( ranges[ 2 ].start, ranges[ 1 ].end ) ];
482+
}
483+
// Range intersects (at the end) with wrapped element (<p>cd</p>).
484+
// <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>
485+
else if ( sourceRange.containsPosition( this.end ) && this.containsPosition( sourceRange.start ) ) {
486+
return [ new Range( ranges[ 0 ].start, ranges[ 1 ].end ) ];
487+
}
488+
} else if ( deltaType == 'unwrap' ) {
489+
// Range intersects (at the beginning) with unwrapped element (<w></w>).
490+
// <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>
491+
// <w></w> is removed in next operation, but the remove does not mess up ranges.
492+
if ( sourceRange.containsPosition( this.start ) && this.containsPosition( sourceRange.end ) ) {
493+
return [ new Range( ranges[ 1 ].start, ranges[ 0 ].end ) ];
494+
}
495+
// Range intersects (at the end) with unwrapped element (<w></w>).
496+
// <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>
497+
// <w></w> is removed in next operation, but the remove does not mess up ranges.
498+
else if ( sourceRange.containsPosition( this.end ) && this.containsPosition( sourceRange.start ) ) {
499+
return [ new Range( ranges[ 0 ].start, ranges[ 2 ].end ) ];
500+
}
501+
}
464502
}
465503

466504
return ranges;
@@ -652,11 +690,10 @@ export default class Range {
652690
* Combines all ranges from the passed array into a one range. At least one range has to be passed.
653691
* Passed ranges must not have common parts.
654692
*
655-
* The first range from the array is a reference range. If other ranges starts or ends on the exactly same position where
693+
* The first range from the array is a reference range. If other ranges start or end on the exactly same position where
656694
* the reference range, they get combined into one range.
657695
*
658-
* [ ][] [ ][ ][ ref range ][ ][] [ ] // Passed ranges, shown sorted. "Ref range" was the first range in original array.
659-
* [ returned range ] [ ] // The combined range.
696+
* [ ][] [ ][ ][ ][ ][] [ ] // Passed ranges, shown sorted
660697
* [ ] // The result of the function if the first range was a reference range.
661698
* [ ] // The result of the function if the third-to-seventh range was a reference range.
662699
* [ ] // The result of the function if the last range was a reference range.

tests/model/delta/transform/markerdelta.js

Lines changed: 145 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -19,7 +19,10 @@ import {
1919
expectDelta,
2020
getFilledDocument,
2121
getMarkerDelta,
22-
getSplitDelta
22+
getSplitDelta,
23+
getMergeDelta,
24+
getWrapDelta,
25+
getUnwrapDelta
2326
} from '../../../model/delta/transform/_utils/utils';
2427

2528
describe( 'transform', () => {
@@ -33,16 +36,16 @@ describe( 'transform', () => {
3336
} );
3437

3538
describe( 'MarkerDelta by', () => {
36-
let markerDelta;
39+
describe( 'SplitDelta', () => {
40+
let markerDelta;
3741

38-
beforeEach( () => {
39-
const oldRange = new Range( new Position( root, [ 3, 0 ] ), new Position( root, [ 3, 3 ] ) );
40-
const newRange = new Range( new Position( root, [ 3, 3, 3, 2 ] ), new Position( root, [ 3, 3, 3, 6 ] ) );
42+
beforeEach( () => {
43+
const oldRange = new Range( new Position( root, [ 3, 0 ] ), new Position( root, [ 3, 3 ] ) );
44+
const newRange = new Range( new Position( root, [ 3, 3, 3, 2 ] ), new Position( root, [ 3, 3, 3, 6 ] ) );
4145

42-
markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
43-
} );
46+
markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
47+
} );
4448

45-
describe( 'SplitDelta', () => {
4649
it( 'split inside oldRange', () => {
4750
let splitDelta = getSplitDelta( new Position( root, [ 3, 1 ] ), new Element( 'div' ), 3, baseVersion );
4851
let transformed = transform( markerDelta, splitDelta );
@@ -93,5 +96,139 @@ describe( 'transform', () => {
9396
} );
9497
} );
9598
} );
99+
100+
describe( 'MergeDelta', () => {
101+
it( 'collapsed marker in merged element', () => {
102+
// MarkerDelta with collapsed range, which changes from the beginning of merged element to the end.
103+
const oldRange = new Range( new Position( root, [ 3, 3, 3, 0 ] ) );
104+
const newRange = new Range( new Position( root, [ 3, 3, 3, 12 ] ) );
105+
106+
const markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
107+
108+
// MergeDelta merges the element in which is collapsed marker range with the previous element.
109+
const mergeDelta = getMergeDelta( new Position( root, [ 3, 3, 3 ] ), 4, 12, baseVersion );
110+
111+
const transformed = transform( markerDelta, mergeDelta );
112+
113+
// It is expected, that ranges in MarkerDelta got correctly transformed:
114+
// from start of merged element to the place where merged nodes where moved in the previous element,
115+
// from end of merged element to the end of previous element.
116+
const expectedOldRange = new Range( new Position( root, [ 3, 3, 2, 4 ] ), new Position( root, [ 3, 3, 2, 4 ] ) );
117+
const expectedNewRange = new Range( new Position( root, [ 3, 3, 2, 16 ] ), new Position( root, [ 3, 3, 2, 16 ] ) );
118+
119+
expectDelta( transformed[ 0 ], {
120+
type: MarkerDelta,
121+
operations: [
122+
{
123+
type: MarkerOperation,
124+
name: 'name',
125+
oldRange: expectedOldRange,
126+
newRange: expectedNewRange,
127+
baseVersion: baseVersion + 2
128+
}
129+
]
130+
} );
131+
} );
132+
} );
133+
134+
describe( 'WrapDelta', () => {
135+
it( 'ranges intersecting with wrapped range', () => {
136+
// MarkerDelta with ranges that intersects with wrapped range.
137+
const oldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 2 ] ) );
138+
const newRange = new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 2, 2 ] ) );
139+
140+
const markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
141+
142+
// Wrap delta wraps element on position ( root [ 1 ] ), which intersects with both `oldRange` and `newRange`.
143+
const wrapElement = new Element( 'w' );
144+
const wrapRange = new Range( new Position( root, [ 1 ] ), new Position( root, [ 2 ] ) );
145+
const wrapDelta = getWrapDelta( wrapRange, wrapElement, baseVersion );
146+
147+
const transformed = transform( markerDelta, wrapDelta );
148+
149+
// It is expected, that ranges in MarkerDelta got correctly transformed:
150+
// `oldRange` end is in wrapped element,
151+
// `newRange` start is in wrapped element.
152+
const expectedOldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 0, 2 ] ) );
153+
const expectedNewRange = new Range( new Position( root, [ 1, 0, 2 ] ), new Position( root, [ 2, 2 ] ) );
154+
155+
expectDelta( transformed[ 0 ], {
156+
type: MarkerDelta,
157+
operations: [
158+
{
159+
type: MarkerOperation,
160+
name: 'name',
161+
oldRange: expectedOldRange,
162+
newRange: expectedNewRange,
163+
baseVersion: baseVersion + 2
164+
}
165+
]
166+
} );
167+
} );
168+
} );
169+
170+
describe( 'UnwrapDelta', () => {
171+
it( 'ranges intersecting with unwrapped element', () => {
172+
// MarkerDelta with ranges that intersects with unwrapped element.
173+
const oldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 0, 2 ] ) );
174+
const newRange = new Range( new Position( root, [ 1, 0, 2 ] ), new Position( root, [ 2, 2 ] ) );
175+
176+
const markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
177+
178+
// Unwrap delta unwraps element on position ( root [ 1, 0 ] ), which intersects with both `oldRange` and `newRange`.
179+
const unwrapPosition = new Position( root, [ 1, 0 ] );
180+
const unwrapDelta = getUnwrapDelta( unwrapPosition, 4, baseVersion );
181+
182+
const transformed = transform( markerDelta, unwrapDelta );
183+
184+
// It is expected, that ranges in MarkerDelta got correctly transformed.
185+
const expectedOldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 2 ] ) );
186+
const expectedNewRange = new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 2, 2 ] ) );
187+
188+
expectDelta( transformed[ 0 ], {
189+
type: MarkerDelta,
190+
operations: [
191+
{
192+
type: MarkerOperation,
193+
name: 'name',
194+
oldRange: expectedOldRange,
195+
newRange: expectedNewRange,
196+
baseVersion: baseVersion + 2
197+
}
198+
]
199+
} );
200+
} );
201+
202+
it( 'ranges intersecting with unwrapped element #2', () => {
203+
// MarkerDelta with ranges that intersects with unwrapped element.
204+
const oldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 1, 2 ] ) );
205+
const newRange = new Range( new Position( root, [ 1, 2 ] ), new Position( root, [ 2, 2 ] ) );
206+
207+
const markerDelta = getMarkerDelta( 'name', oldRange, newRange, baseVersion );
208+
209+
// Unwrap delta unwraps element on position ( root [ 1 ] ), which intersects with both `oldRange` and `newRange`.
210+
const unwrapPosition = new Position( root, [ 1 ] );
211+
const unwrapDelta = getUnwrapDelta( unwrapPosition, 4, baseVersion );
212+
213+
const transformed = transform( markerDelta, unwrapDelta );
214+
215+
// It is expected, that ranges in MarkerDelta got correctly transformed.
216+
const expectedOldRange = new Range( new Position( root, [ 0, 2 ] ), new Position( root, [ 3 ] ) );
217+
const expectedNewRange = new Range( new Position( root, [ 3 ] ), new Position( root, [ 5, 2 ] ) );
218+
219+
expectDelta( transformed[ 0 ], {
220+
type: MarkerDelta,
221+
operations: [
222+
{
223+
type: MarkerOperation,
224+
name: 'name',
225+
oldRange: expectedOldRange,
226+
newRange: expectedNewRange,
227+
baseVersion: baseVersion + 2
228+
}
229+
]
230+
} );
231+
} );
232+
} );
96233
} );
97234
} );

0 commit comments

Comments
 (0)