Procházet zdrojové kódy

Merge pull request #1286 from ckeditor/t/1281

Fix: `DocumenSelection#change:range` event will be fired only once after multiple selection live ranges have changed. Closes #1281.
Szymon Cofalik před 7 roky
rodič
revize
e4236994ce

+ 31 - 8
packages/ckeditor5-engine/src/model/documentselection.js

@@ -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() ) {
@@ -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() {
@@ -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;
@@ -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 } );
 	}
 }
 

+ 27 - 0
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -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', () => {

+ 216 - 0
packages/ckeditor5-engine/tests/tickets/1281.js

@@ -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 );
+	}
+} );