From 6b1cef80c22c95dbfe2a0f390108acceaf4ee436 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 29 Aug 2017 13:15:54 +0200 Subject: [PATCH 1/2] Fixed: Items inserted into an already highlighted element with custom highlight handling should not get additional highlight. --- src/conversion/modelconversiondispatcher.js | 42 +++++-- src/model/range.js | 11 ++ tests/conversion/modelconversiondispatcher.js | 114 ++++++++++++++++++ tests/model/range.js | 32 +++++ 4 files changed, 191 insertions(+), 8 deletions(-) diff --git a/src/conversion/modelconversiondispatcher.js b/src/conversion/modelconversiondispatcher.js index 2a42e2b4e..bef1750e9 100644 --- a/src/conversion/modelconversiondispatcher.js +++ b/src/conversion/modelconversiondispatcher.js @@ -217,15 +217,11 @@ export default class ModelConversionDispatcher { for ( const marker of this._modelDocument.markers ) { const markerRange = marker.getRange(); + const intersection = markerRange.getIntersection( range ); // Check if inserted content is inserted into a marker. - if ( markerRange.containsPosition( range.start ) ) { - this.convertMarker( 'addMarker', marker.name, markerRange.getIntersection( range ) ); - } - - // Check if inserted content contains a marker. - if ( range.containsRange( markerRange, true ) ) { - this.convertMarker( 'addMarker', marker.name, markerRange ); + if ( intersection && shouldMarkerChangeBeConverted( range.start, marker, this.conversionApi.mapper ) ) { + this.convertMarker( 'addMarker', marker.name, intersection ); } } } @@ -367,10 +363,16 @@ export default class ModelConversionDispatcher { this.fire( 'selection', { selection }, consumable, this.conversionApi ); for ( const marker of markers ) { + const markerRange = marker.getRange(); + + if ( !shouldMarkerChangeBeConverted( selection.getFirstPosition(), marker, this.conversionApi.mapper ) ) { + continue; + } + const data = { selection, markerName: marker.name, - markerRange: marker.getRange() + markerRange }; if ( consumable.test( selection, 'selectionMarker:' + marker.name ) ) { @@ -717,3 +719,27 @@ export default class ModelConversionDispatcher { } mix( ModelConversionDispatcher, EmitterMixin ); + +// Helper function, checks whether change of `marker` at `modelPosition` should be converted. Marker changes are not +// converted if they happen inside an element with custom conversion method. +// +// @param {module:engine/model/position~Position} modelPosition +// @param {module:engine/model/markercollection~Marker} marker +// @param {module:engine/conversion/mapper~Mapper} mapper +// @returns {Boolean} +function shouldMarkerChangeBeConverted( modelPosition, marker, mapper ) { + const range = marker.getRange(); + const ancestors = Array.from( modelPosition.getAncestors() ); + ancestors.shift(); // Remove root element. It cannot be passed to `model.Range#containsItem`. + ancestors.reverse(); + + const hasCustomHandling = ancestors.some( element => { + if ( range.containsItem( element ) ) { + const viewElement = mapper.toViewElement( element ); + + return !!viewElement.getCustomProperty( 'addHighlight' ); + } + } ); + + return !hasCustomHandling; +} diff --git a/src/model/range.js b/src/model/range.js index 138e1f4d2..e93272c05 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -117,6 +117,17 @@ export default class Range { return containsStart && containsEnd; } + /** + * Checks whether given {@link module:engine/model/item~Item} is inside this range. + * + * @param {module:engine/model/item~Item} item Model item to check. + */ + containsItem( item ) { + const pos = Position.createBefore( item ); + + return this.containsPosition( pos ) || this.start.isEqual( pos ); + } + /** * Two ranges are equal if their {@link #start} and {@link #end} positions are equal. * diff --git a/tests/conversion/modelconversiondispatcher.js b/tests/conversion/modelconversiondispatcher.js index 335d216cf..32c98c9af 100644 --- a/tests/conversion/modelconversiondispatcher.js +++ b/tests/conversion/modelconversiondispatcher.js @@ -15,6 +15,8 @@ import RenameOperation from '../../src/model/operation/renameoperation'; import AttributeOperation from '../../src/model/operation/attributeoperation'; import { wrapInDelta } from '../../tests/model/_utils/utils'; +import ViewContainerElement from '../../src/view/containerelement'; + describe( 'ModelConversionDispatcher', () => { let dispatcher, doc, root, gyPos; @@ -360,6 +362,78 @@ describe( 'ModelConversionDispatcher', () => { expect( callArgs[ 1 ] ).to.equal( 'marker' ); expect( callArgs[ 2 ].isEqual( markerRange ) ).to.be.true; } ); + + it( 'should not fire marker conversion if content is inserted into element with custom highlight handling', () => { + sinon.spy( dispatcher, 'convertMarker' ); + + const text = new ModelText( 'abc' ); + const caption = new ModelElement( 'caption', null, text ); + const image = new ModelElement( 'image', null, caption ); + root.appendChildren( [ image ] ); + + // Create view elements that will be "mapped" to model elements. + const viewCaption = new ViewContainerElement( 'caption' ); + const viewFigure = new ViewContainerElement( 'figure', null, viewCaption ); + + // Create custom highlight handler mock. + viewFigure.setCustomProperty( 'addHighlight', () => {} ); + viewFigure.setCustomProperty( 'removeHighlight', () => {} ); + + // Create mapper mock. + dispatcher.conversionApi.mapper = { + toViewElement( modelElement ) { + if ( modelElement == image ) { + return viewFigure; + } else if ( modelElement == caption ) { + return viewCaption; + } + } + }; + + const markerRange = ModelRange.createFromParentsAndOffsets( root, 0, root, 1 ); + doc.markers.set( 'marker', markerRange ); + + const insertionRange = ModelRange.createFromParentsAndOffsets( caption, 1, caption, 2 ); + dispatcher.convertInsertion( insertionRange ); + + expect( dispatcher.convertMarker.called ).to.be.false; + } ); + + it( 'should fire marker conversion if inserted into element with highlight handling but element is not in marker range', () => { + sinon.spy( dispatcher, 'convertMarker' ); + + const text = new ModelText( 'abc' ); + const caption = new ModelElement( 'caption', null, text ); + const image = new ModelElement( 'image', null, caption ); + root.appendChildren( [ image ] ); + + // Create view elements that will be "mapped" to model elements. + const viewCaption = new ViewContainerElement( 'caption' ); + const viewFigure = new ViewContainerElement( 'figure', null, viewCaption ); + + // Create custom highlight handler mock. + viewFigure.setCustomProperty( 'addHighlight', () => {} ); + viewFigure.setCustomProperty( 'removeHighlight', () => {} ); + + // Create mapper mock. + dispatcher.conversionApi.mapper = { + toViewElement( modelElement ) { + if ( modelElement == image ) { + return viewFigure; + } else if ( modelElement == caption ) { + return viewCaption; + } + } + }; + + const markerRange = ModelRange.createFromParentsAndOffsets( caption, 0, caption, 3 ); + doc.markers.set( 'marker', markerRange ); + + const insertionRange = ModelRange.createFromParentsAndOffsets( caption, 2, caption, 3 ); + dispatcher.convertInsertion( insertionRange ); + + expect( dispatcher.convertMarker.called ).to.be.true; + } ); } ); describe( 'convertMove', () => { @@ -600,6 +674,46 @@ describe( 'ModelConversionDispatcher', () => { expect( dispatcher.fire.calledWith( 'selectionMarker:name' ) ).to.be.true; } ); + it( 'should not fire event for marker if selection is in a element with custom highlight handling', () => { + // Clear after `beforeEach`. + root.removeChildren( 0, root.childCount ); + + const text = new ModelText( 'abc' ); + const caption = new ModelElement( 'caption', null, text ); + const image = new ModelElement( 'image', null, caption ); + root.appendChildren( [ image ] ); + + // Create view elements that will be "mapped" to model elements. + const viewCaption = new ViewContainerElement( 'caption' ); + const viewFigure = new ViewContainerElement( 'figure', null, viewCaption ); + + // Create custom highlight handler mock. + viewFigure.setCustomProperty( 'addHighlight', () => {} ); + viewFigure.setCustomProperty( 'removeHighlight', () => {} ); + + // Create mapper mock. + dispatcher.conversionApi.mapper = { + toViewElement( modelElement ) { + if ( modelElement == image ) { + return viewFigure; + } else if ( modelElement == caption ) { + return viewCaption; + } + } + }; + + doc.markers.set( 'name', ModelRange.createFromParentsAndOffsets( root, 0, root, 1 ) ); + doc.selection.setRanges( [ ModelRange.createFromParentsAndOffsets( caption, 1, caption, 1 ) ] ); + + sinon.spy( dispatcher, 'fire' ); + + const markers = Array.from( doc.markers.getMarkersAtPosition( doc.selection.getFirstPosition() ) ); + + dispatcher.convertSelection( doc.selection, markers ); + + expect( dispatcher.fire.calledWith( 'selectionMarker:name' ) ).to.be.false; + } ); + it( 'should not fire events if information about marker has been consumed', () => { doc.markers.set( 'foo', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); doc.markers.set( 'bar', ModelRange.createFromParentsAndOffsets( root, 0, root, 2 ) ); diff --git a/tests/model/range.js b/tests/model/range.js index 436e73bfd..578147eef 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -412,6 +412,12 @@ describe( 'Range', () => { expect( range.containsPosition( position ) ).to.be.true; } ); + } ); + + describe( 'containsRange()', () => { + beforeEach( () => { + range = new Range( new Position( root, [ 1 ] ), new Position( root, [ 3 ] ) ); + } ); it( 'should return true if ranges are equal and check is not strict', () => { const otherRange = Range.createFromRange( range ); @@ -441,6 +447,32 @@ describe( 'Range', () => { } ); } ); + describe( 'containsItem()', () => { + let a, b, c, d, xxx; + + beforeEach( () => { + a = new Element( 'a' ); + b = new Element( 'b' ); + c = new Element( 'c' ); + d = new Element( 'd' ); + + xxx = new Text( 'xxx' ); + b.appendChildren( xxx ); + + root.appendChildren( [ a, b, c, d ] ); + } ); + + it( 'should return true if element is inside range and false when it is not inside range', () => { + const range = Range.createFromParentsAndOffsets( root, 1, root, 3 ); // Range over `b` and `c`. + + expect( range.containsItem( a ) ).to.be.false; + expect( range.containsItem( b ) ).to.be.true; + expect( range.containsItem( xxx ) ).to.be.true; + expect( range.containsItem( c ) ).to.be.true; + expect( range.containsItem( d ) ).to.be.false; + } ); + } ); + describe( '_getTransformedByInsertion()', () => { it( 'should return an array of Range objects', () => { const range = new Range( new Position( root, [ 0 ] ), new Position( root, [ 1 ] ) ); From d67605a0b9bf3aea0cbfff00ed175bc42503dae1 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 29 Aug 2017 13:16:20 +0200 Subject: [PATCH 2/2] Fix: Fix for LiveRange transformation when element is split at the place where the live range starts. --- src/model/range.js | 2 +- tests/model/range.js | 13 +++++++++++++ 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/src/model/range.js b/src/model/range.js index e93272c05..57eb98d39 100644 --- a/src/model/range.js +++ b/src/model/range.js @@ -505,7 +505,7 @@ export default class Range { // ^

xx

{

a[b

}

c]d

-->

a[b

xx

c]d

// Note

xx

inclusion. // {

a[b

}^

c]d

-->

a[b

c]d

if ( - sourceRange.containsPosition( this.start ) && + ( sourceRange.containsPosition( this.start ) || sourceRange.start.isEqual( this.start ) ) && this.containsPosition( sourceRange.end ) && this.end.isAfter( targetPosition ) ) { diff --git a/tests/model/range.js b/tests/model/range.js index 578147eef..59ae001c3 100644 --- a/tests/model/range.js +++ b/tests/model/range.js @@ -986,6 +986,19 @@ describe( 'Range', () => { expect( transformed[ 0 ].end.path ).to.deep.equal( [ 1, 1 ] ); } ); + it( 'split at the beginning of multi-element range', () => { + range.start = new Position( root, [ 0, 4 ] ); + range.end = new Position( root, [ 1, 2 ] ); + + const delta = getSplitDelta( new Position( root, [ 0, 4 ] ), new Element( 'p' ), 3, 1 ); + + const transformed = range.getTransformedByDelta( delta ); + + expect( transformed.length ).to.equal( 1 ); + expect( transformed[ 0 ].start.path ).to.deep.equal( [ 1, 0 ] ); + expect( transformed[ 0 ].end.path ).to.deep.equal( [ 2, 2 ] ); + } ); + it( 'split inside range which starts at the beginning of split element', () => { range.start = new Position( root, [ 0, 0 ] ); range.end = new Position( root, [ 0, 4 ] );