diff --git a/src/highlightstack.js b/src/highlightstack.js index 3a7adaae..f5d9f9df 100644 --- a/src/highlightstack.js +++ b/src/highlightstack.js @@ -42,7 +42,7 @@ export default class HighlightStack { const newTop = stack[ 0 ]; // When new object is at the top and stores different information. - if ( oldTop !== newTop && !cmp( oldTop, newTop ) ) { + if ( oldTop !== newTop && !compareDescriptors( oldTop, newTop ) ) { this.fire( 'change:top', { oldDescriptor: oldTop, newDescriptor: newTop @@ -58,51 +58,43 @@ export default class HighlightStack { */ remove( descriptor ) { const stack = this._stack; - const length = stack.length; - if ( length === 0 ) { - return; - } - - let i = 0; - - while ( stack[ i ] && !compareDescriptors( descriptor, stack[ i ] ) ) { - i++; - - // Descriptor not found. - if ( i >= stack.length ) { - return; - } - } - - stack.splice( i, 1 ); - - // Element from stack top was removed - fire `change:top` event with new first element. It might be `undefined` - // which informs that no descriptor is currently at the top. - if ( i === 0 ) { - const data = { - oldDescriptor: descriptor - }; - - if ( stack[ 0 ] ) { - const newDescriptor = stack[ 0 ]; - data.newDescriptor = newDescriptor; - } + const oldTop = stack[ 0 ]; + this._removeDescriptor( descriptor ); + const newTop = stack[ 0 ]; - this.fire( 'change:top', data ); + // When new object is at the top and stores different information. + if ( oldTop !== newTop && !compareDescriptors( oldTop, newTop ) ) { + this.fire( 'change:top', { + oldDescriptor: oldTop, + newDescriptor: newTop + } ); } } + /** + * Inserts given descriptor in correct place in the stack. It also takes care about updating information when + * descriptor with same id is already present. + * + * @private + * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptor + */ _insertDescriptor( descriptor ) { const stack = this._stack; const index = stack.findIndex( item => item.id === descriptor.id ); - // If descriptor with same id is on the list - remove it. + // Inserting exact same descriptor - do nothing. + if ( compareDescriptors( descriptor, stack[ index ] ) ) { + return; + } + + // If descriptor with same id but with different information is on the stack - remove it. if ( index > -1 ) { stack.splice( index, 1 ); } // Find correct place to insert descriptor in the stack. + // It have different information (for example priority) so it must be re-inserted in correct place. let i = 0; while ( stack[ i ] && shouldABeBeforeB( stack[ i ], descriptor ) ) { @@ -112,25 +104,34 @@ export default class HighlightStack { stack.splice( i, 0, descriptor ); } + /** + * Removes descriptor with given id from the stack. + * + * @private + * @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptor + */ + _removeDescriptor( descriptor ) { + const stack = this._stack; + const index = stack.findIndex( item => item.id === descriptor.id ); + + // If descriptor with same id is on the list - remove it. + if ( index > -1 ) { + stack.splice( index, 1 ); + } + } } mix( HighlightStack, EmitterMixin ); -function cmp( a, b ) { - return a && b && a.id == b.id && classesToString( a.class ) == classesToString( b.class ) && a.priority == b.priority; -} - -// Compares two highlight descriptors by priority and CSS class names. Returns `true` when both descriptors are -// considered equal. +// Compares two descriptors by checking their priority and class list. // -// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptorA -// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} descriptorB -// @returns {Boolean} -function compareDescriptors( descriptorA, descriptorB ) { - return descriptorA.id == descriptorB.id; +// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} a +// @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} b +// @returns {Boolean} Returns true if both descriptors are defined and have same priority and classes. +function compareDescriptors( a, b ) { + return a && b && a.priority == b.priority && classesToString( a.class ) == classesToString( b.class ); } - // Checks whenever first descriptor should be placed in the stack before second one. // // @param {module:engine/conversion/buildmodelconverter~HighlightDescriptor} a diff --git a/tests/highlightstack.js b/tests/highlightstack.js index d74d2232..765026dc 100644 --- a/tests/highlightstack.js +++ b/tests/highlightstack.js @@ -38,6 +38,23 @@ describe( 'HighlightStack', () => { expect( spy.secondCall.args[ 1 ].oldDescriptor ).to.equal( descriptor ); } ); + it( 'should fire event when top element has updated', () => { + const spy = sinon.spy(); + const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' }; + const secondDescriptor = { priority: 11, class: 'css-class', id: 'descriptor-1' }; + + stack.on( 'change:top', spy ); + stack.add( descriptor ); + stack.add( secondDescriptor ); + + sinon.assert.calledTwice( spy ); + expect( spy.firstCall.args[ 1 ].newDescriptor ).to.equal( descriptor ); + expect( spy.firstCall.args[ 1 ].oldDescriptor ).to.be.undefined; + + expect( spy.secondCall.args[ 1 ].newDescriptor ).to.equal( secondDescriptor ); + expect( spy.secondCall.args[ 1 ].oldDescriptor ).to.equal( descriptor ); + } ); + it( 'should not fire event when element with lower priority was added', () => { const spy = sinon.spy(); const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' }; @@ -133,10 +150,10 @@ describe( 'HighlightStack', () => { expect( spy.firstCall.args[ 1 ].oldDescriptor ).to.equal( descriptor ); } ); - it.only( 'should not fire event when new top descriptor is same as previous', () => { + it( 'should not fire event when new top descriptor is same as previous', () => { const spy = sinon.spy(); const descriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' }; - const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-1' }; + const secondDescriptor = { priority: 10, class: 'css-class', id: 'descriptor-2' }; stack.add( descriptor ); stack.add( secondDescriptor ); @@ -148,9 +165,9 @@ describe( 'HighlightStack', () => { it( 'should sort by class when priorities are the same', () => { const spy = sinon.spy(); - const descriptorA = { priority: 10, class: 'css-a' }; - const descriptorB = { priority: 10, class: 'css-b' }; - const descriptorC = { priority: 10, class: 'css-c' }; + const descriptorA = { priority: 10, class: 'css-a', id: 'descriptor-1' }; + const descriptorB = { priority: 10, class: 'css-b', id: 'descriptor-2' }; + const descriptorC = { priority: 10, class: 'css-c', id: 'descriptor-3' }; stack.on( 'change:top', spy ); stack.add( descriptorB ); @@ -167,9 +184,9 @@ describe( 'HighlightStack', () => { it( 'should sort by class when priorities are the same - array of CSS classes', () => { const spy = sinon.spy(); - const descriptorA = { priority: 10, class: [ 'css-a', 'css-z' ] }; - const descriptorB = { priority: 10, class: [ 'css-a', 'css-y' ] }; - const descriptorC = { priority: 10, class: 'css-c' }; + const descriptorA = { priority: 10, class: [ 'css-a', 'css-z' ], id: 'descriptor-1' }; + const descriptorB = { priority: 10, class: [ 'css-a', 'css-y' ], id: 'descriptor-2' }; + const descriptorC = { priority: 10, class: 'css-c', id: 'descriptor-3' }; stack.on( 'change:top', spy ); stack.add( descriptorB ); diff --git a/tests/utils.js b/tests/utils.js index b1d26408..d43770eb 100644 --- a/tests/utils.js +++ b/tests/utils.js @@ -59,10 +59,10 @@ describe( 'widget utils', () => { expect( typeof set ).to.equal( 'function' ); expect( typeof remove ).to.equal( 'function' ); - set( element, { priority: 1, class: 'highlight' } ); + set( element, { priority: 1, class: 'highlight', id: 'highlight' } ); expect( element.hasClass( 'highlight' ) ).to.be.true; - remove( element, { priority: 1, class: 'highlight' } ); + remove( element, { priority: 1, class: 'highlight', id: 'highlight' } ); expect( element.hasClass( 'highlight' ) ).to.be.false; } ); @@ -75,11 +75,11 @@ describe( 'widget utils', () => { expect( typeof set ).to.equal( 'function' ); expect( typeof remove ).to.equal( 'function' ); - set( element, { priority: 1, class: [ 'highlight', 'foo' ] } ); + set( element, { priority: 1, class: [ 'highlight', 'foo' ], id: 'highlight' } ); expect( element.hasClass( 'highlight' ) ).to.be.true; expect( element.hasClass( 'foo' ) ).to.be.true; - remove( element, { priority: 1, class: [ 'foo', 'highlight' ] } ); + remove( element, { priority: 1, class: [ 'foo', 'highlight' ], id: 'highlight' } ); expect( element.hasClass( 'highlight' ) ).to.be.false; expect( element.hasClass( 'foo' ) ).to.be.false; } ); @@ -174,7 +174,7 @@ describe( 'widget utils', () => { } ); it( 'should call highlight methods when descriptor is added and removed', () => { - const descriptor = { priority: 10, class: 'highlight' }; + const descriptor = { priority: 10, class: 'highlight', id: 'highlight' }; set( element, descriptor ); remove( element, descriptor ); @@ -187,8 +187,8 @@ describe( 'widget utils', () => { } ); it( 'should call highlight methods when next descriptor is added', () => { - const descriptor = { priority: 10, class: 'highlight' }; - const secondDescriptor = { priority: 11, class: 'highlight' }; + const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' }; + const secondDescriptor = { priority: 11, class: 'highlight', id: 'highlight-2' }; set( element, descriptor ); set( element, secondDescriptor ); @@ -199,8 +199,8 @@ describe( 'widget utils', () => { } ); it( 'should not call highlight methods when descriptor with lower priority is added', () => { - const descriptor = { priority: 10, class: 'highlight' }; - const secondDescriptor = { priority: 9, class: 'highlight' }; + const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' }; + const secondDescriptor = { priority: 9, class: 'highlight', id: 'highlight-2' }; set( element, descriptor ); set( element, secondDescriptor ); @@ -210,8 +210,8 @@ describe( 'widget utils', () => { } ); it( 'should call highlight methods when descriptor is removed changing active descriptor', () => { - const descriptor = { priority: 10, class: 'highlight' }; - const secondDescriptor = { priority: 11, class: 'highlight' }; + const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' }; + const secondDescriptor = { priority: 11, class: 'highlight', id: 'highlight-2' }; set( element, descriptor ); set( element, secondDescriptor ); @@ -228,8 +228,8 @@ describe( 'widget utils', () => { } ); it( 'should call highlight methods when descriptor is removed not changing active descriptor', () => { - const descriptor = { priority: 10, class: 'highlight' }; - const secondDescriptor = { priority: 9, class: 'highlight' }; + const descriptor = { priority: 10, class: 'highlight', id: 'highlight-1' }; + const secondDescriptor = { priority: 9, class: 'highlight', id: 'highlight-2' }; set( element, descriptor ); set( element, secondDescriptor ); @@ -242,8 +242,8 @@ describe( 'widget utils', () => { } ); it( 'should call highlight methods - CSS class array', () => { - const descriptor = { priority: 10, class: [ 'highlight', 'a' ] }; - const secondDescriptor = { priority: 10, class: [ 'highlight', 'b' ] }; + const descriptor = { priority: 10, class: [ 'highlight', 'a' ], id: 'highlight-1' }; + const secondDescriptor = { priority: 10, class: [ 'highlight', 'b' ], id: 'highlight-2' }; set( element, descriptor ); set( element, secondDescriptor );