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

Commit

Permalink
Added mechanism that prevents from conflicts when selection gravity i…
Browse files Browse the repository at this point in the history
…s overridden more than once.
  • Loading branch information
oskarwrobel committed Feb 23, 2018
1 parent c56cb25 commit 28618b0
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 24 deletions.
60 changes: 39 additions & 21 deletions src/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -142,11 +142,13 @@ export default class DocumentSelection {
/**
* 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
* @return {boolean}
*/
get isGravityOverridden() {
return this._selection._isGravityOverriden;
return this._selection.isGravityOverridden;
}

/**
Expand Down Expand Up @@ -417,6 +419,8 @@ export default class DocumentSelection {
/**
* 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
* @protected
*/
Expand Down Expand Up @@ -501,12 +505,12 @@ class LiveSelection extends Selection {
// @member {Array} module:engine/model/liveselection~LiveSelection#_hasChangedRange
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.
this.on( 'change:range', () => {
Expand Down Expand Up @@ -593,6 +597,15 @@ class LiveSelection extends Selection {
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.
destroy() {
for ( let i = 0; i < this._ranges.length; i++ ) {
Expand Down Expand Up @@ -645,23 +658,28 @@ class LiveSelection extends Selection {
}

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() {
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.
Expand Down Expand Up @@ -915,7 +933,7 @@ class LiveSelection extends Selection {
const nodeAfter = position.textNode ? position.textNode : position.nodeAfter;

// 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.
attrs = getAttrsIfCharacter( nodeBefore );
}
Expand All @@ -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.
// When gravity is overridden then don't take node before into consideration.
if ( !this._isGravityOverriden && !attrs ) {
if ( !this.isGravityOverridden && !attrs ) {
let node = nodeBefore;

while ( node && !attrs ) {
Expand Down
8 changes: 7 additions & 1 deletion src/model/writer.js
Original file line number Diff line number Diff line change
Expand Up @@ -1033,6 +1033,10 @@ export default class Writer {
* move caret) but you can pass customRestore = true in which case you will have to call
* {@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
* {@link ~Writer#restoreSelectionGravity} will be called directly. When `false` then gravity is restored
* after selection is moved by user.
Expand All @@ -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() {
this.model.document.selection._restoreGravity();
Expand Down
31 changes: 29 additions & 2 deletions tests/model/documentselection.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
setData( model, '<$text bold="true" italic="true">foo[]</$text>' );

Expand Down Expand Up @@ -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>' );

selection._overrideGravity();

expect( Array.from( selection.getAttributeKeys() ) ).to.length( 0 );
expect( selection.isGravityOverridden ).to.true;

selection._restoreGravity();

expect( Array.from( selection.getAttributeKeys() ) ).to.have.members( [ 'bold', 'italic' ] );
expect( selection.isGravityOverridden ).to.false;
} );

it( 'should do nothing when gravity is not overridden', () => {
Expand All @@ -832,7 +842,24 @@ describe( 'DocumentSelection', () => {
selection._restoreGravity();
} ).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;
} );
} );

Expand Down

0 comments on commit 28618b0

Please sign in to comment.