浏览代码

Merge branch 'master' into i/7521

Aleksander Nowodzinski 5 年之前
父节点
当前提交
e483a34dc5

+ 3 - 0
packages/ckeditor5-core/src/command.js

@@ -208,6 +208,9 @@ export default class Command {
 	 *
 	 * In order to see how to disable a command from "outside" see the {@link #isEnabled} documentation.
 	 *
+	 * This method may return a value, which would be forwarded all the way down to the
+	 * {@link module:core/editor/editor~Editor#execute `editor.execute()`}.
+	 *
 	 * @fires execute
 	 */
 	execute() {}

+ 2 - 1
packages/ckeditor5-core/src/commandcollection.js

@@ -51,6 +51,7 @@ export default class CommandCollection {
 	 *
 	 * @param {String} commandName The name of the command.
 	 * @param {*} [...commandParams] Command parameters.
+	 * @returns {*} The value returned by the {@link module:core/command~Command#execute `command.execute()`}.
 	 */
 	execute( commandName, ...args ) {
 		const command = this.get( commandName );
@@ -65,7 +66,7 @@ export default class CommandCollection {
 			throw new CKEditorError( 'commandcollection-command-not-found: Command does not exist.', this, { commandName } );
 		}
 
-		command.execute( ...args );
+		return command.execute( ...args );
 	}
 
 	/**

+ 2 - 1
packages/ckeditor5-core/src/editor/editor.js

@@ -284,10 +284,11 @@ export default class Editor {
 	 *
 	 * @param {String} commandName The name of the command to execute.
 	 * @param {*} [...commandParams] Command parameters.
+	 * @returns {*} The value returned by the {@link module:core/commandcollection~CommandCollection#execute `commands.execute()`}.
 	 */
 	execute( ...args ) {
 		try {
-			this.commands.execute( ...args );
+			return this.commands.execute( ...args );
 		} catch ( err ) {
 			// @if CK_DEBUG // throw err;
 			/* istanbul ignore next */

+ 3 - 1
packages/ckeditor5-core/src/multicommand.js

@@ -57,11 +57,13 @@ export default class MultiCommand extends Command {
 
 	/**
 	 * Executes the first of it registered child commands.
+	 *
+	 * @returns {*} The value returned by the {@link module:core/command~Command#execute `command.execute()`}.
 	 */
 	execute( ...args ) {
 		const command = this._getFirstEnabledCommand();
 
-		command.execute( args );
+		return command.execute( args );
 	}
 
 	/**

+ 14 - 0
packages/ckeditor5-core/tests/commandcollection.js

@@ -55,6 +55,20 @@ describe( 'CommandCollection', () => {
 			expect( command.execute.args[ 0 ] ).to.deep.equal( [ 1, 2 ] );
 		} );
 
+		it( 'returns the result of command\'s execute()', () => {
+			const command = new SomeCommand( editor );
+
+			const commandResult = { foo: 'bar' };
+			sinon.stub( command, 'execute' ).returns( commandResult );
+
+			collection.add( 'foo', command );
+
+			const collectionResult = collection.execute( 'foo' );
+
+			expect( collectionResult, 'collection.execute()' ).to.equal( commandResult );
+			expect( collectionResult, 'collection.execute()' ).to.deep.equal( { foo: 'bar' } );
+		} );
+
 		it( 'throws an error if command does not exist', () => {
 			const command = new SomeCommand( editor );
 			collection.add( 'bar', command );

+ 19 - 0
packages/ckeditor5-core/tests/editor/editor.js

@@ -486,6 +486,25 @@ describe( 'Editor', () => {
 			expect( command.execute.calledOnce ).to.be.true;
 		} );
 
+		it( 'should return the result of command\'s execute()', () => {
+			class SomeCommand extends Command {
+				execute() {}
+			}
+
+			const editor = new TestEditor();
+			const command = new SomeCommand( editor );
+
+			const commandResult = { foo: 'bar' };
+			sinon.stub( command, 'execute' ).returns( commandResult );
+
+			editor.commands.add( 'someCommand', command );
+
+			const editorResult = editor.execute( 'someCommand' );
+
+			expect( editorResult, 'editor.execute()' ).to.equal( commandResult );
+			expect( editorResult, 'editor.execute()' ).to.deep.equal( { foo: 'bar' } );
+		} );
+
 		it( 'should throw an error if specified command has not been added', () => {
 			const editor = new TestEditor();
 

+ 15 - 0
packages/ckeditor5-core/tests/multicommand.js

@@ -98,6 +98,21 @@ describe( 'MultiCommand', () => {
 			sinon.assert.calledOnce( spyC );
 		} );
 
+		it( 'returns the result of command\'s execute()', () => {
+			const command = new Command( editor );
+			const commandResult = { foo: 'bar' };
+			sinon.stub( command, 'execute' ).returns( commandResult );
+
+			multiCommand.registerChildCommand( command );
+
+			command.isEnabled = true;
+
+			const multiCommandResult = multiCommand.execute();
+
+			expect( multiCommandResult, 'multiCommand.execute()' ).to.equal( commandResult );
+			expect( multiCommandResult, 'multiCommand.execute()' ).to.deep.equal( { foo: 'bar' } );
+		} );
+
 		it( 'executes first registered command if many are enabled', () => {
 			const commandA = new Command( editor );
 			const commandB = new Command( editor );

+ 11 - 0
packages/ckeditor5-engine/src/controller/datacontroller.js

@@ -133,6 +133,7 @@ export default class DataController {
 		this.upcastDispatcher.on( 'documentFragment', convertToModelFragment(), { priority: 'lowest' } );
 
 		this.decorate( 'init' );
+		this.decorate( 'set' );
 
 		// Fire `ready` event when initialisation has completed. Such low level listener gives possibility
 		// to plug into initialisation pipeline without interrupting the initialisation flow.
@@ -317,6 +318,7 @@ export default class DataController {
 	 *
 	 *		dataController.set( { main: '<p>Foo</p>', title: '<h1>Bar</h1>' } ); // Sets data on the `main` and `title` roots.
 	 *
+	 * @fires set
 	 * @param {String|Object.<String,String>} data Input data as a string or an object containing `rootName` - `data`
 	 * pairs to set data on multiple roots at once.
 	 */
@@ -452,6 +454,15 @@ export default class DataController {
 	 *
 	 * @event init
 	 */
+
+	/**
+	 * Event fired after {@link #set set() method} has been run.
+	 *
+	 * The `set` event is fired by decorated {@link #set} method.
+	 * See {@link module:utils/observablemixin~ObservableMixin#decorate} for more information and samples.
+	 *
+	 * @event set
+	 */
 }
 
 mix( DataController, ObservableMixin );

+ 24 - 42
packages/ckeditor5-engine/src/model/documentselection.js

@@ -567,7 +567,6 @@ mix( DocumentSelection, EmitterMixin );
 //
 // @extends module:engine/model/selection~Selection
 //
-
 class LiveSelection extends Selection {
 	// Creates an empty live selection for given {@link module:engine/model/document~Document}.
 	// @param {module:engine/model/document~Document} doc Document which owns this selection.
@@ -602,10 +601,10 @@ 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.
+		// Position to which the selection should be set if the last selection range was moved to the graveyard.
 		// @private
-		// @member {Array} module:engine/model/liveselection~LiveSelection#_fixGraveyardRangesData
-		this._fixGraveyardRangesData = [];
+		// @member {module:engine/model/position~Position} module:engine/model/liveselection~LiveSelection#_selectionRestorePosition
+		this._selectionRestorePosition = null;
 
 		// Flag that informs whether the selection ranges have changed. It is changed on true when `LiveRange#change:range` event is fired.
 		// @private
@@ -628,12 +627,14 @@ class LiveSelection extends Selection {
 				return;
 			}
 
-			while ( this._fixGraveyardRangesData.length ) {
-				const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();
-
-				this._fixGraveyardSelection( liveRange, sourcePosition );
+			// Fix selection if the last range was removed from it and we have a position to which we can restore the selection.
+			if ( this._ranges.length == 0 && this._selectionRestorePosition ) {
+				this._fixGraveyardSelection( this._selectionRestorePosition );
 			}
 
+			// "Forget" the restore position even if it was not "used".
+			this._selectionRestorePosition = null;
+
 			if ( this._hasChangedRange ) {
 				this._hasChangedRange = false;
 				this.fire( 'change:range', { directChange: false } );
@@ -827,15 +828,17 @@ class LiveSelection extends Selection {
 
 		const liveRange = LiveRange.fromRange( range );
 
+		// If selection range is moved to the graveyard remove it from the selection object.
+		// Also, save some data that can be used to restore selection later, on `Model#applyOperation` event.
 		liveRange.on( 'change:range', ( evt, oldRange, data ) => {
 			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._fixGraveyardRangesData.push( {
-					liveRange,
-					sourcePosition: data.deletionPosition
-				} );
+				this._selectionRestorePosition = data.deletionPosition;
+
+				const index = this._ranges.indexOf( liveRange );
+				this._ranges.splice( index, 1 );
+				liveRange.detach();
 			}
 		} );
 
@@ -1117,36 +1120,20 @@ class LiveSelection extends Selection {
 		return attrs;
 	}
 
-	// Fixes a selection range after it ends up in graveyard root.
+	// Fixes the selection after all its ranges got removed.
 	//
 	// @private
-	// @param {module:engine/model/liverange~LiveRange} liveRange The range from selection, that ended up in the graveyard root.
-	// @param {module:engine/model/position~Position} removedRangeStart Start position of a range which was removed.
-	_fixGraveyardSelection( liveRange, removedRangeStart ) {
-		// The start of the removed range is the closest position to the `liveRange` - the original selection range.
-		// This is a good candidate for a fixed selection range.
-		const positionCandidate = removedRangeStart.clone();
-
-		// Find a range that is a correct selection range and is closest to the start of removed range.
-		const selectionRange = this._model.schema.getNearestSelectionRange( positionCandidate );
-
-		// Remove the old selection range before preparing and adding new selection range. This order is important,
-		// because new range, in some cases, may intersect with old range (it depends on `getNearestSelectionRange()` result).
-		const index = this._ranges.indexOf( liveRange );
-		this._ranges.splice( index, 1 );
-		liveRange.detach();
+	// @param {module:engine/model/position~Position} deletionPosition Position where the deletion happened.
+	_fixGraveyardSelection( deletionPosition ) {
+		// Find a range that is a correct selection range and is closest to the position where the deletion happened.
+		const selectionRange = this._model.schema.getNearestSelectionRange( deletionPosition );
 
 		// If nearest valid selection range has been found - add it in the place of old range.
-		// If range is equal to any other selection ranges then it is probably due to contents
-		// of a multi-range selection being removed. See ckeditor/ckeditor5#6501.
-		if ( selectionRange && !isRangeCollidingWithSelection( selectionRange, this ) ) {
+		if ( selectionRange ) {
 			// Check the range, convert it to live range, bind events, etc.
-			const newRange = this._prepareRange( selectionRange );
-
-			// Add new range in the place of old range.
-			this._ranges.splice( index, 0, newRange );
+			this._pushRange( selectionRange );
 		}
-		// If nearest valid selection range cannot be found or is intersecting with other selection ranges removing the old range is fine.
+		// If nearest valid selection range cannot be found don't add any range. Selection will be set to the default range.
 	}
 }
 
@@ -1191,8 +1178,3 @@ function clearAttributesStoredInElement( model, batch ) {
 		}
 	}
 }
-
-// Checks if range collides with any of selection ranges.
-function isRangeCollidingWithSelection( range, selection ) {
-	return !selection._ranges.every( selectionRange => !range.isEqual( selectionRange ) );
-}

+ 10 - 0
packages/ckeditor5-engine/tests/controller/datacontroller.js

@@ -258,6 +258,16 @@ describe( 'DataController', () => {
 	} );
 
 	describe( 'set()', () => {
+		it( 'should be decorated', () => {
+			const spy = sinon.spy();
+
+			data.on( 'set', spy );
+
+			data.set( 'foo bar' );
+
+			sinon.assert.calledWithExactly( spy, sinon.match.any, [ 'foo bar' ] );
+		} );
+
 		it( 'should set data to default main root', () => {
 			schema.extend( '$text', { allowIn: '$root' } );
 			data.set( 'foo' );

+ 28 - 6
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -1779,7 +1779,7 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
 			} );
 
-			it( 'handles multi-range selection in a text node by merging it into one range (resulting in collapsed ranges)', () => {
+			it( 'handles multi-range selection in a text node by merging it into one range (resulting in a collapsed range)', () => {
 				const ranges = [
 					new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ),
 					new Range( new Position( root, [ 1, 3 ] ), new Position( root, [ 1, 4 ] ) )
@@ -1789,19 +1789,19 @@ describe( 'DocumentSelection', () => {
 
 				model.applyOperation(
 					new MoveOperation(
-						new Position( root, [ 1, 1 ] ),
-						4,
+						new Position( root, [ 1, 0 ] ),
+						5,
 						new Position( doc.graveyard, [ 0 ] ),
 						doc.version
 					)
 				);
 
 				expect( selection.rangeCount ).to.equal( 1 );
-				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 1 ] );
-				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 1 ] );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 0 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 0 ] );
 			} );
 
-			it( 'handles multi-range selection on object nodes by merging it into one range (resulting in non-collapsed ranges)', () => {
+			it( 'handles multi-range selection on object nodes by merging it into one range (resulting in a non-collapsed range)', () => {
 				model.schema.register( 'outer', {
 					isObject: true
 				} );
@@ -1835,6 +1835,28 @@ describe( 'DocumentSelection', () => {
 				expect( selection.getFirstPosition().path ).to.deep.equal( [ 0, 0 ] );
 				expect( selection.getLastPosition().path ).to.deep.equal( [ 0, 1 ] );
 			} );
+
+			it( 'should not fix the selection if not all ranges were removed', () => {
+				const ranges = [
+					new Range( new Position( root, [ 1, 1 ] ), new Position( root, [ 1, 2 ] ) ),
+					new Range( new Position( root, [ 1, 3 ] ), new Position( root, [ 1, 4 ] ) )
+				];
+
+				selection._setTo( ranges );
+
+				model.applyOperation(
+					new MoveOperation(
+						new Position( root, [ 1, 1 ] ),
+						1,
+						new Position( doc.graveyard, [ 0 ] ),
+						doc.version
+					)
+				);
+
+				expect( selection.rangeCount ).to.equal( 1 );
+				expect( selection.getFirstPosition().path ).to.deep.equal( [ 1, 2 ] );
+				expect( selection.getLastPosition().path ).to.deep.equal( [ 1, 3 ] );
+			} );
 		} );
 
 		it( '`DocumentSelection#change:range` event should be fire once even if selection contains multi-ranges', () => {

+ 34 - 0
packages/ckeditor5-table/tests/tableselection-integration.js

@@ -220,6 +220,40 @@ describe( 'TableSelection - integration', () => {
 				'</table>'
 			);
 		} );
+
+		// https://github.com/ckeditor/ckeditor5/issues/7659.
+		// The fix is in the `DocumentSelection` class but this test is here to make sure that the fix works
+		// and that the behavior won't change in the future.
+		it( 'should not fix selection if not all ranges were removed', () => {
+			// [ ][ ][ ]
+			// [x][x][ ]
+			// [x][x][ ]
+			tableSelection.setCellSelection(
+				modelRoot.getNodeByPath( [ 0, 1, 0 ] ),
+				modelRoot.getNodeByPath( [ 0, 2, 1 ] )
+			);
+
+			editor.model.change( writer => {
+				// Remove second row.
+				writer.remove( modelRoot.getNodeByPath( [ 0, 1 ] ) );
+			} );
+
+			assertEqualMarkup(
+				getModelData( model ),
+				'<table>' +
+					'<tableRow>' +
+						'<tableCell><paragraph>11</paragraph></tableCell>' +
+						'<tableCell><paragraph>12</paragraph></tableCell>' +
+						'<tableCell><paragraph>13</paragraph></tableCell>' +
+					'</tableRow>' +
+					'<tableRow>' +
+						'[<tableCell><paragraph>31</paragraph></tableCell>]' +
+						'[<tableCell><paragraph>32</paragraph></tableCell>]' +
+						'<tableCell><paragraph>33</paragraph></tableCell>' +
+					'</tableRow>' +
+				'</table>'
+			);
+		} );
 	} );
 
 	describe( 'with undo', () => {

+ 1 - 1
packages/ckeditor5-typing/tests/inputcommand.js

@@ -259,7 +259,7 @@ describe( 'InputCommand', () => {
 				model.change( writer => {
 					let rangeSelection;
 
-					for ( const range of selection.getRanges() ) {
+					for ( const range of Array.from( selection.getRanges() ) ) {
 						rangeSelection = writer.createSelection( range );
 
 						model.deleteContent( rangeSelection );

+ 2 - 0
packages/ckeditor5-undo/src/basecommand.js

@@ -41,6 +41,8 @@ export default class BaseCommand extends Command {
 
 		// Refresh state, so the command is inactive right after initialization.
 		this.refresh();
+
+		this.listenTo( editor.data, 'set', () => this.clearStack() );
 	}
 
 	/**

+ 8 - 0
packages/ckeditor5-undo/tests/redocommand.js

@@ -288,5 +288,13 @@ describe( 'RedoCommand', () => {
 				expect( undoSpy.firstCall.args[ 1 ] ).to.equal( redoingBatch );
 			} );
 		} );
+
+		it( 'should clear stack on DataController set()', () => {
+			const spy = sinon.stub( redo, 'clearStack' );
+
+			editor.setData( 'foo' );
+
+			sinon.assert.called( spy );
+		} );
 	} );
 } );

+ 8 - 0
packages/ckeditor5-undo/tests/undocommand.js

@@ -347,5 +347,13 @@ describe( 'UndoCommand', () => {
 			expect( getCaseText( root ) ).to.equal( 'adbcef' );
 			expect( editor.model.document.selection.getFirstRange().isEqual( r( 1, 4 ) ) ).to.be.true;
 		} );
+
+		it( 'should clear stack on DataController set()', () => {
+			const spy = sinon.stub( undo, 'clearStack' );
+
+			editor.setData( 'foo' );
+
+			sinon.assert.called( spy );
+		} );
 	} );
 } );