Browse Source

Added mechanism that prevents from conflicts when selection gravity is overridden more than once.

Oskar Wróbel 7 years ago
parent
commit
8ca54dcbfa

+ 39 - 21
packages/ckeditor5-engine/src/model/documentselection.js

@@ -142,11 +142,13 @@ export default class DocumentSelection {
 	/**
 	/**
 	 * Describes whether gravity is overridden (using {@link ~DocumentSelection#_overrideGravity}) or not.
 	 * Describes whether gravity is overridden (using {@link ~DocumentSelection#_overrideGravity}) or not.
 	 *
 	 *
+	 * Note that gravity remains overridden as long as won't be restored the same number of times as was overridden.
+	 *
 	 * @readonly
 	 * @readonly
 	 * @return {boolean}
 	 * @return {boolean}
 	 */
 	 */
 	get isGravityOverridden() {
 	get isGravityOverridden() {
-		return this._selection._isGravityOverriden;
+		return this._selection.isGravityOverridden;
 	}
 	}
 
 
 	/**
 	/**
@@ -417,6 +419,8 @@ export default class DocumentSelection {
 	/**
 	/**
 	 * Restores {@link ~DocumentSelection#_overrideGravity overridden gravity}.
 	 * Restores {@link ~DocumentSelection#_overrideGravity overridden gravity}.
 	 *
 	 *
+	 * Note that gravity remains overridden as long as won't be restored the same number of times as was overridden.
+	 *
 	 * @see module:engine/model/writer~Writer#restoreSelectionGravity
 	 * @see module:engine/model/writer~Writer#restoreSelectionGravity
 	 * @protected
 	 * @protected
 	 */
 	 */
@@ -501,12 +505,12 @@ class LiveSelection extends Selection {
 		// @member {Array} module:engine/model/liveselection~LiveSelection#_hasChangedRange
 		// @member {Array} module:engine/model/liveselection~LiveSelection#_hasChangedRange
 		this._hasChangedRange = false;
 		this._hasChangedRange = false;
 
 
-		// When is set as `true` then selection attributes on node before the caret won't be taken
-		// into consideration while updating selection attributes.
-		//
-		// @protected
-		// @type {Boolean}
-		this._isGravityOverriden = false;
+		// Each overriding gravity increase the counter and each restoring decrease it.
+		// Gravity is overridden when counter is greater than 0. This is to prevent conflicts when
+		// gravity is overridden by more than one feature at the same time.
+		// @private
+		// @type {Number}
+		this._overriddenGravityCounter = 0;
 
 
 		// Add events that will ensure selection correctness.
 		// Add events that will ensure selection correctness.
 		this.on( 'change:range', () => {
 		this.on( 'change:range', () => {
@@ -593,6 +597,15 @@ class LiveSelection extends Selection {
 		return this._ranges.length > 0;
 		return this._ranges.length > 0;
 	}
 	}
 
 
+	// When set to `true` then selection attributes on node before the caret won't be taken
+	// into consideration while updating selection attributes.
+	//
+	// @protected
+	// @type {Boolean}
+	get isGravityOverridden() {
+		return this._overriddenGravityCounter > 0;
+	}
+
 	// Unbinds all events previously bound by live selection.
 	// Unbinds all events previously bound by live selection.
 	destroy() {
 	destroy() {
 		for ( let i = 0; i < this._ranges.length; i++ ) {
 		for ( let i = 0; i < this._ranges.length; i++ ) {
@@ -645,23 +658,28 @@ class LiveSelection extends Selection {
 	}
 	}
 
 
 	overrideGravity( customRestore ) {
 	overrideGravity( customRestore ) {
-		this._isGravityOverriden = true;
+		this._overriddenGravityCounter++;
+
+		if ( this._overriddenGravityCounter == 1 ) {
+			if ( !customRestore ) {
+				this.on( 'change:range', ( evt, data ) => {
+					if ( data.directChange ) {
+						this.restoreGravity();
+						evt.off();
+					}
+				} );
+			}
 
 
-		if ( !customRestore ) {
-			this.on( 'change:range', ( evt, data ) => {
-				if ( data.directChange ) {
-					this.restoreGravity();
-					evt.off();
-				}
-			} );
+			this._updateAttributes();
 		}
 		}
-
-		this._updateAttributes();
 	}
 	}
 
 
 	restoreGravity() {
 	restoreGravity() {
-		this._isGravityOverriden = false;
-		this._updateAttributes();
+		this._overriddenGravityCounter--;
+
+		if ( !this.isGravityOverridden ) {
+			this._updateAttributes();
+		}
 	}
 	}
 
 
 	// Removes all attributes from the selection and sets attributes according to the surrounding nodes.
 	// Removes all attributes from the selection and sets attributes according to the surrounding nodes.
@@ -915,7 +933,7 @@ class LiveSelection extends Selection {
 			const nodeAfter = position.textNode ? position.textNode : position.nodeAfter;
 			const nodeAfter = position.textNode ? position.textNode : position.nodeAfter;
 
 
 			// When gravity is overridden then don't take node before into consideration.
 			// When gravity is overridden then don't take node before into consideration.
-			if ( !this._isGravityOverriden ) {
+			if ( !this.isGravityOverridden ) {
 				// ...look at the node before caret and take attributes from it if it is a character node.
 				// ...look at the node before caret and take attributes from it if it is a character node.
 				attrs = getAttrsIfCharacter( nodeBefore );
 				attrs = getAttrsIfCharacter( nodeBefore );
 			}
 			}
@@ -927,7 +945,7 @@ class LiveSelection extends Selection {
 
 
 			// 4. If not, try to find the first character on the left, that is in the same node.
 			// 4. If not, try to find the first character on the left, that is in the same node.
 			// When gravity is overridden then don't take node before into consideration.
 			// When gravity is overridden then don't take node before into consideration.
-			if ( !this._isGravityOverriden && !attrs ) {
+			if ( !this.isGravityOverridden && !attrs ) {
 				let node = nodeBefore;
 				let node = nodeBefore;
 
 
 				while ( node && !attrs ) {
 				while ( node && !attrs ) {

+ 7 - 1
packages/ckeditor5-engine/src/model/writer.js

@@ -1033,6 +1033,10 @@ export default class Writer {
 	 * move caret) but you can pass customRestore = true in which case you will have to call
 	 * move caret) but you can pass customRestore = true in which case you will have to call
 	 * {@link ~Writer#restoreSelectionGravity} manually.
 	 * {@link ~Writer#restoreSelectionGravity} manually.
 	 *
 	 *
+	 * When the selection's gravity is overridden more than once without restoring meanwhile then needs to be restored
+	 * the same number of times as was overridden to revert default behavior. This is to prevent conflicts when
+	 * more than one feature want independently override and restore selection's gravity.
+	 *
 	 * @param {Boolean} [customRestore=false] When `true` then gravity won't be restored until
 	 * @param {Boolean} [customRestore=false] When `true` then gravity won't be restored until
 	 * {@link ~Writer#restoreSelectionGravity} will be called directly. When `false` then gravity is restored
 	 * {@link ~Writer#restoreSelectionGravity} will be called directly. When `false` then gravity is restored
 	 * after selection is moved by user.
 	 * after selection is moved by user.
@@ -1042,7 +1046,9 @@ export default class Writer {
 	}
 	}
 
 
 	/**
 	/**
-	 * Restores overridden gravity to default.
+	 * Restores {@link ~Writer#overrideSelectionGravity} gravity to default.
+	 *
+	 * Note that selection's gravity remains overridden as long as won't be restored the same number of times as was overridden.
 	 */
 	 */
 	restoreSelectionGravity() {
 	restoreSelectionGravity() {
 		this.model.document.selection._restoreGravity();
 		this.model.document.selection._restoreGravity();

+ 29 - 2
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -757,6 +757,14 @@ describe( 'DocumentSelection', () => {
 			} );
 			} );
 		} );
 		} );
 
 
+		it( 'should mark gravity as overridden', () => {
+			expect( selection.isGravityOverridden ).to.false;
+
+			selection._overrideGravity();
+
+			expect( selection.isGravityOverridden ).to.true;
+		} );
+
 		it( 'should not inherit attributes from node before the caret', () => {
 		it( 'should not inherit attributes from node before the caret', () => {
 			setData( model, '<$text bold="true" italic="true">foo[]</$text>' );
 			setData( model, '<$text bold="true" italic="true">foo[]</$text>' );
 
 
@@ -813,16 +821,18 @@ describe( 'DocumentSelection', () => {
 			} );
 			} );
 		} );
 		} );
 
 
-		it( 'should not revert default gravity when is overridden', () => {
+		it( 'should revert default gravity when is overridden', () => {
 			setData( model, '<$text bold="true" italic="true">foo[]</$text>' );
 			setData( model, '<$text bold="true" italic="true">foo[]</$text>' );
 
 
 			selection._overrideGravity();
 			selection._overrideGravity();
 
 
 			expect( Array.from( selection.getAttributeKeys() ) ).to.length( 0 );
 			expect( Array.from( selection.getAttributeKeys() ) ).to.length( 0 );
+			expect( selection.isGravityOverridden ).to.true;
 
 
 			selection._restoreGravity();
 			selection._restoreGravity();
 
 
 			expect( Array.from( selection.getAttributeKeys() ) ).to.have.members( [ 'bold', 'italic' ] );
 			expect( Array.from( selection.getAttributeKeys() ) ).to.have.members( [ 'bold', 'italic' ] );
+			expect( selection.isGravityOverridden ).to.false;
 		} );
 		} );
 
 
 		it( 'should do nothing when gravity is not overridden', () => {
 		it( 'should do nothing when gravity is not overridden', () => {
@@ -832,7 +842,24 @@ describe( 'DocumentSelection', () => {
 				selection._restoreGravity();
 				selection._restoreGravity();
 			} ).to.not.throw();
 			} ).to.not.throw();
 
 
-			expect( Array.from( selection.getAttributeKeys() ) ).to.have.members( [ 'bold', 'italic' ] );
+			expect( selection.isGravityOverridden ).to.false;
+		} );
+
+		it( 'should be called the same number of times as gravity is overridden to restore it', () => {
+			setData( model, '<$text bold="true" italic="true">foo[]</$text>' );
+
+			selection._overrideGravity();
+			selection._overrideGravity();
+
+			expect( selection.isGravityOverridden ).to.true;
+
+			selection._restoreGravity();
+
+			expect( selection.isGravityOverridden ).to.true;
+
+			selection._restoreGravity();
+
+			expect( selection.isGravityOverridden ).to.false;
 		} );
 		} );
 	} );
 	} );