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 #1286 from ckeditor/t/1281
Browse files Browse the repository at this point in the history 
Fix: `DocumenSelection#change:range` event will be fired only once after multiple selection live ranges have changed. Closes #1281.
  • Loading branch information
@scofalik
scofalik authored Feb 19, 2018
2 parents 8eba5e9 + 8521dbe commit b26935c
Show file tree
Hide file tree
Showing 3 changed files with 274 additions and 8 deletions.
39 changes: 31 additions & 8 deletions src/model/documentselection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -455,6 +455,16 @@ class LiveSelection extends Selection {
// @member {Map} module:engine/model/liveselection~LiveSelection#_attributePriority
this._attributePriority = new Map();

// Contains data required to fix ranges which have been moved to the graveyard.
// @private
// @member {Array} module:engine/model/liveselection~LiveSelection#_fixGraveyardRangesData
this._fixGraveyardRangesData = [];

// Flag that informs whether the selection ranges have changed. It is changed on true when `LiveRange#change:range` event is fired.
// @private
// @member {Array} module:engine/model/liveselection~LiveSelection#_hasChangedRange
this._hasChangedRange = false;

// Add events that will ensure selection correctness.
this.on( 'change:range', () => {
for ( const range of this.getRanges() ) {
Expand Down Expand Up @@ -497,6 +507,20 @@ class LiveSelection extends Selection {
clearAttributesStoredInElement( operation, this._model, batch );
}
}, { priority: 'low' } );

this.listenTo( this._model, 'applyOperation', () => {
while ( this._fixGraveyardRangesData.length ) {
const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();

this._fixGraveyardSelection( liveRange, sourcePosition );
}

if ( this._hasChangedRange ) {
this._hasChangedRange = false;

this.fire( 'change:range', { directChange: false } );
}
}, { priority: 'lowest' } );
}

get isCollapsed() {
Expand Down Expand Up @@ -618,13 +642,15 @@ class LiveSelection extends Selection {
const liveRange = LiveRange.createFromRange( range );

liveRange.on( 'change:range', ( evt, oldRange, data ) => {
// If `LiveRange` is in whole moved to the graveyard, fix that range.
this._hasChangedRange = true;

// If `LiveRange` is in whole moved to the graveyard, save necessary data. It will be fixed on `Model#applyOperation` event.
if ( liveRange.root == this._document.graveyard ) {
this._fixGraveyardSelection( liveRange, data.sourcePosition );
this._fixGraveyardRangesData.push( {
liveRange,
sourcePosition: data.sourcePosition
} );
}

// Whenever a live range from selection changes, fire an event informing about that change.
this.fire( 'change:range', { directChange: false } );
} );

return liveRange;
Expand Down Expand Up @@ -890,9 +916,6 @@ class LiveSelection extends Selection {
this._ranges.splice( index, 0, newRange );
}
// If nearest valid selection range cannot be found - just removing the old range is fine.

// Fire an event informing about selection change.
this.fire( 'change:range', { directChange: false } );
}
}

Expand Down
27 changes: 27 additions & 0 deletions tests/model/documentselection.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -740,6 +740,33 @@ describe( 'DocumentSelection', () => {
expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
} );
} );

it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {
root.removeChildren( 0, root.childCount );
root.insertChildren( 0, [
new Element( 'p', [], new Text( 'abcdef' ) ),
new Element( 'p', [], new Text( 'foobar' ) ),
new Text( 'xyz #2' )
] );

selection._setTo( [
Range.createIn( root.getNodeByPath( [ 0 ] ) ),
Range.createIn( root.getNodeByPath( [ 1 ] ) )
] );

spyRange = sinon.spy();
selection.on( 'change:range', spyRange );

model.applyOperation( wrapInDelta(
new InsertOperation(
new Position( root, [ 0 ] ),
'xyz #1',
doc.version
)
) );

expect( spyRange.calledOnce ).to.be.true;
} );
} );

describe( 'attributes', () => {
Expand Down
216 changes: 216 additions & 0 deletions tests/tickets/1281.js
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,216 @@
/**
* @license Copyright (c) 2003-2018, CKSource - Frederico Knabben. All rights reserved.
* For licensing, see LICENSE.md.
*/

/* globals document */

import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor';
import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph';
import Position from '../../src/model/position';

import { setData as setModelData, getData as getModelData } from '../../src/dev-utils/model';

describe( 'Bug ckeditor5-engine#1281', () => {
let element, editor, model;

beforeEach( () => {
element = document.createElement( 'div' );
document.body.appendChild( element );

return ClassicTestEditor
.create( element, { plugins: [ Paragraph ] } )
.then( newEditor => {
editor = newEditor;
model = editor.model;
} );
} );

afterEach( () => {
element.remove();

return editor.destroy();
} );

it( 'loads content that contains multi-range selection', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);

const root = model.document.getRoot();
const thirdParagraph = root.getNodeByPath( [ 2 ] );
const fourthParagraph = root.getNodeByPath( [ 3 ] );
const selRanges = Array.from( model.document.selection.getRanges() );

expect( selRanges.length ).to.equal( 2 );

assertPositions( Position.createAt( thirdParagraph ), selRanges[ 0 ].start );
assertPositions( Position.createAt( thirdParagraph, 'end' ), selRanges[ 0 ].end );

assertPositions( Position.createAt( fourthParagraph ), selRanges[ 1 ].start );
assertPositions( Position.createAt( fourthParagraph, 'end' ), selRanges[ 1 ].end );
} );

it( 'does not throw an error when content before the selection is being removed (last element is selected)', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const firstParagraph = root.getNodeByPath( [ 0 ] );

expect( () => {
writer.remove( firstParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);
} );
} );

it( 'does not throw an error when content before the selection is being removed (last element is not selected)', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const firstParagraph = root.getNodeByPath( [ 0 ] );

expect( () => {
writer.remove( firstParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);
} );
} );

it( 'does not throw an error when content after the selection is being removed (first element is selected)', () => {
setModelData( model,
'<paragraph>[Paragraph 1.]</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>Paragraph 3.</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const lastParagraph = root.getNodeByPath( [ 4 ] );

expect( () => {
writer.remove( lastParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>[Paragraph 1.]</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>Paragraph 3.</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);
} );
} );

it( 'does not throw an error when content after the selection is being removed (first element is not selected)', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const lastParagraph = root.getNodeByPath( [ 4 ] );

expect( () => {
writer.remove( lastParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>Paragraph 2.</paragraph>' +
'<paragraph>[Paragraph 3.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);
} );
} );

it( 'does not throw an error when content between the selection\'s ranges is being removed (last element is selected)', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>[Paragraph 2.]</paragraph>' +
'<paragraph>Paragraph 3.</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const thirdParagraph = root.getNodeByPath( [ 2 ] );

expect( () => {
writer.remove( thirdParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>[Paragraph 2.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>'
);
} );
} );

it( 'does not throw an error when content between the selection\'s ranges is being removed (last element is not selected)', () => {
setModelData( model,
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>[Paragraph 2.]</paragraph>' +
'<paragraph>Paragraph 3.</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);

model.change( writer => {
const root = model.document.getRoot();
const thirdParagraph = root.getNodeByPath( [ 2 ] );

expect( () => {
writer.remove( thirdParagraph );
} ).to.not.throw();

assertOutput(
'<paragraph>Paragraph 1.</paragraph>' +
'<paragraph>[Paragraph 2.]</paragraph>' +
'<paragraph>[Paragraph 4.]</paragraph>' +
'<paragraph>Paragraph 5.</paragraph>'
);
} );
} );

function assertPositions( firstPosition, secondPosition ) {
expect( firstPosition.isEqual( secondPosition ) ).to.be.true;
}

function assertOutput( output ) {
expect( getModelData( model ) ).to.equal( output );
}
} );

0 comments on commit b26935c

Please sign in to comment.