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

Commit

Permalink
Merge branch 'master' into t/1320b
Browse files Browse the repository at this point in the history
  • Loading branch information
@Reinmar
Reinmar committed Feb 26, 2018
2 parents ebb82ea + e70ab96 commit 0aa5590
Show file tree
Hide file tree
Showing 2 changed files with 121 additions and 47 deletions.
111 changes: 64 additions & 47 deletions src/model/differ.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -229,13 +229,6 @@ export default class Differ {

// Check all changed elements.
for ( const element of this._changesInElement.keys() ) {
// Each item in `this._changesInElement` describes changes of the _children_ of that element.
// If the element itself has been inserted we should skip all the changes in it because the element will be reconverted.
// If the element itself has been removed we should skip all the changes in it because they would be incorrect.
if ( this._isInsertedOrRemoved( element ) ) {
continue;
}

// Get changes for this element and sort them.
const changes = this._changesInElement.get( element ).sort( ( a, b ) => {
if ( a.offset === b.offset ) {
Expand Down Expand Up @@ -394,46 +387,6 @@ export default class Differ {
this._cachedChanges = null;
}

/**
* Checks whether a given element is inserted or removed or one of its ancestors is inserted or removed. Used to
* filter out sub-changes in elements that are changed.
*
* @private
* @param {module:engine/model/element~Element} element An element to check.
* @returns {Boolean}
*/
_isInsertedOrRemoved( element ) {
let parent = element.parent;

// Check all ancestors of given element.
while ( parent ) {
// Get the checked element's offset.
const offset = element.startOffset;

if ( this._changesInElement.has( parent ) ) {
const changes = this._changesInElement.get( parent );

// If there were changes in that element's ancestor, check all of them.
for ( const change of changes ) {
// Skip attribute changes. We are interested only if the element was inserted or removed.
if ( change.type == 'attribute' ) {
continue;
}

if ( change.offset <= offset && change.offset + change.howMany > offset ) {
return true;
}
}
}

// Move up.
parent = parent.parent;
element = element.parent;
}

return false;
}

/**
* Saves and handles an insert change.
*
Expand All @@ -443,6 +396,10 @@ export default class Differ {
* @param {Number} howMany
*/
_markInsert( parent, offset, howMany ) {
if ( this._isInInsertedElement( parent ) ) {
return;
}

const changeItem = { type: 'insert', offset, howMany, count: this._changeCount++ };

this._markChange( parent, changeItem );
Expand All @@ -457,9 +414,15 @@ export default class Differ {
* @param {Number} howMany
*/
_markRemove( parent, offset, howMany ) {
if ( this._isInInsertedElement( parent ) ) {
return;
}

const changeItem = { type: 'remove', offset, howMany, count: this._changeCount++ };

this._markChange( parent, changeItem );

this._removeAllNestedChanges( parent, offset, howMany );
}

/**
Expand All @@ -469,6 +432,10 @@ export default class Differ {
* @param {module:engine/model/item~Item} item
*/
_markAttribute( item ) {
if ( this._isInInsertedElement( item.parent ) ) {
return;
}

const changeItem = { type: 'attribute', offset: item.startOffset, howMany: item.offsetSize, count: this._changeCount++ };

this._markChange( item.parent, changeItem );
Expand Down Expand Up @@ -831,6 +798,56 @@ export default class Differ {

return diffs;
}

/**
* Checks whether given element or any of its parents is an element that is buffered as an inserted element.
*
* @private
* @param {module:engine/model/element~Element} element Element to check.
* @returns {Boolean}
*/
_isInInsertedElement( element ) {
const parent = element.parent;

if ( !parent ) {
return false;
}

const changes = this._changesInElement.get( parent );
const offset = element.startOffset;

if ( changes ) {
for ( const change of changes ) {
if ( change.type == 'insert' && offset >= change.offset && offset < change.offset + change.howMany ) {
return true;
}
}
}

return this._isInInsertedElement( parent );
}

/**
* Removes deeply all buffered changes that are registered in elements from range specified by `parent`, `offset`
* and `howMany`.
*
* @private
* @param {module:engine/model/element~Element} parent
* @param {Number} offset
* @param {Number} howMany
*/
_removeAllNestedChanges( parent, offset, howMany ) {
const range = Range.createFromParentsAndOffsets( parent, offset, parent, offset + howMany );

for ( const item of range.getItems( { shallow: true } ) ) {
if ( item.is( 'element' ) ) {
this._elementSnapshots.delete( item );
this._changesInElement.delete( item );

this._removeAllNestedChanges( item, 0, item.maxOffset );
}
}
}
}

// Returns an array that is a copy of passed child list with the exception that text nodes are split to one or more
Expand Down
57 changes: 57 additions & 0 deletions tests/model/differ.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1193,6 +1193,63 @@ describe( 'Differ', () => {
] );
} );
} );

// ckeditor5#733.
it( 'proper filtering of changes in removed elements', () => {
// Before fix there was a buggy scenario described in ckeditor5#733.
// There was this structure: `<paragraph>foo[</paragraph><image /><blockQuote><p>te]xt</p></blockQuote>`
// On delete of above selection `image` and `paragraph` inside `blockQuote` are removed (it gets merged).
// However, since `image` was removed first, when checking if `paragraph` is in a removed element,
// it appeared that `blockQuote` looks like it is removed because it had the same path as the already removed `<image>`.
// In a result, removing `paragraph` was discarded.
// The mistake was that the checking for removing was done at incorrect moment.
root.removeChildren( 0, root.childCount );
root.appendChildren( [
new Element( 'paragraph', null, new Text( 'foo' ) ),
new Element( 'image' ),
new Element( 'blockQuote', null, [
new Element( 'paragraph', null, new Text( 'text' ) )
] )
] );

model.change( () => {
// Remove `"te"`.
remove( new Position( root, [ 2, 0, 0 ] ), 2 );
// Remove `image` before `blockQuote`.
remove( new Position( root, [ 1 ] ), 1 );
// Move `"xt"` to first `paragraph` (merging).
move( new Position( root, [ 1, 0, 0 ] ), 2, new Position( root, [ 0, 3 ] ) );
// Remove `paragraph` inside `blockQuote`.
remove( new Position( root, [ 1, 0 ] ), 1 );

expectChanges( [
{ type: 'insert', name: '$text', length: 2, position: new Position( root, [ 0, 3 ] ) },
{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 1 ] ) },
{ type: 'remove', name: 'paragraph', length: 1, position: new Position( root, [ 1, 0 ] ) }
] );
} );
} );

it( 'proper filtering of changes in inserted elements', () => {
root.removeChildren( 0, root.childCount );
root.appendChildren( new Element( 'image' ) );

const blockQuote = new Element( 'blockQuote', null, new Element( 'paragraph' ) );

model.change( () => {
// Insert `blockQuote` with `paragraph` after `image`.
insert( blockQuote, new Position( root, [ 1 ] ) );
// Remove `image` from before `blockQuote`.
remove( new Position( root, [ 0 ] ), 1 );
// Insert text into `paragraph` in `blockQuote`.
insert( new Text( 'foo' ), new Position( root, [ 0, 0, 0 ] ) );

expectChanges( [
{ type: 'remove', name: 'image', length: 1, position: new Position( root, [ 0 ] ) },
{ type: 'insert', name: 'blockQuote', length: 1, position: new Position( root, [ 0 ] ) }
] );
} );
} );
} );

describe( 'getChanges()', () => {
Expand Down

0 comments on commit 0aa5590

Please sign in to comment.