浏览代码

Changed: Markers model-to-view clearing conversion moved to a moment before an operation affecting marker is applied.

Szymon Cofalik 8 年之前
父节点
当前提交
68b53ab422

+ 74 - 5
packages/ckeditor5-engine/src/controller/editingcontroller.js

@@ -85,15 +85,12 @@ export default class EditingController {
 
 		const doc = this.model.document;
 
-		// When all changes are done, get the model diff containing all the changes and convert them to view and then render to DOM.
 		this.listenTo( doc, 'change', () => {
-			// Convert changes stored in `modelDiffer`.
 			this.modelToView.convertChanges( doc.differ );
+		}, { priority: 'low' } );
 
-			// After the view is ready, convert selection from model to view.
+		this.listenTo( model, '_change', () => {
 			this.modelToView.convertSelection( doc.selection );
-
-			// When everything is converted to the view, render it to DOM.
 			this.view.render();
 		}, { priority: 'low' } );
 
@@ -110,6 +107,58 @@ export default class EditingController {
 		this.modelToView.on( 'selection', convertRangeSelection(), { priority: 'low' } );
 		this.modelToView.on( 'selection', convertCollapsedSelection(), { priority: 'low' } );
 
+		// Convert markers removal.
+		//
+		// Markers should be removed from the view before changes to model are applied. This is because otherwise
+		// it would be impossible to map some markers to view (if, for example, marker boundary parent got removed).
+		//
+		// `removedMarkers` keeps information which markers already has been removed to prevent removing them twice.
+		// (The second remove would not be at "the soonest moment" and it could crash.)
+		const removedMarkers = new Set();
+
+		this.listenTo( model, 'applyOperation', ( evt, args ) => {
+			// Before operation is applied...
+			const operation = args[ 0 ];
+
+			for ( const marker of model.markers ) {
+				// Check all markers, that aren't already removed...
+				if ( removedMarkers.has( marker.name ) ) {
+					continue;
+				}
+
+				const markerRange = marker.getRange();
+
+				if ( _operationAffectsMarker( operation, marker ) ) {
+					// And if the operation in any way modifies the marker, remove the marker from the view.
+					removedMarkers.add( marker.name );
+					this.modelToView.convertMarkerRemove( marker.name, markerRange );
+
+					// TODO: This stinks but this is the safest place to have this code.
+					this.model.document.differ.bufferMarkerChange( marker.name, markerRange, markerRange );
+				}
+			}
+		}, { priority: 'high' } );
+
+		// If a marker with given name was added again, remove it from `removedMarkers`. This is to prevent a bug in
+		// a situation when marker is removed, then added, then removed again (would not got removed if it is saved in `removedMarkers`).
+		this.listenTo( model.markers, 'add', ( evt, marker ) => {
+			removedMarkers.delete( marker.name );
+		} );
+
+		// If a marker is removed through `model.Model#markers` directly (not through operation), just remove it (if
+		// it was not removed already).
+		this.listenTo( model.markers, 'remove', ( evt, marker ) => {
+			if ( !removedMarkers.has( marker.name ) ) {
+				removedMarkers.add( marker.name );
+				this.modelToView.convertMarkerRemove( marker.name, marker.getRange() );
+			}
+		} );
+
+		// When all changes are done, clear `removedMarkers` set.
+		this.listenTo( model, '_change', () => {
+			removedMarkers.clear();
+		}, { priority: 'low' } );
+
 		// Binds {@link module:engine/view/document~Document#roots view roots collection} to
 		// {@link module:engine/model/document~Document#roots model roots collection} so creating
 		// model root automatically creates corresponding view root.
@@ -140,3 +189,23 @@ export default class EditingController {
 }
 
 mix( EditingController, ObservableMixin );
+
+// Helper function which checks whether given operation will affect given marker after the operation is applied.
+function _operationAffectsMarker( operation, marker ) {
+	const range = marker.getRange();
+
+	if ( operation.type == 'insert' || operation.type == 'rename' ) {
+		return _positionAffectsRange( operation.position, range );
+	} else if ( operation.type == 'move' || operation.type == 'remove' || operation.type == 'reinsert' ) {
+		return _positionAffectsRange( operation.targetPosition, range ) || _positionAffectsRange( operation.sourcePosition, range );
+	} else if ( operation.type == 'marker' && operation.name == marker.name ) {
+		return true;
+	}
+
+	return false;
+}
+
+// Helper function which checks whether change at given position affects given range.
+function _positionAffectsRange( position, range ) {
+	return range.containsPosition( position ) || !range.start._getTransformedByInsertion( position, 1, true ).isEqual( range.start );
+}

+ 0 - 5
packages/ckeditor5-engine/src/conversion/modelconversiondispatcher.js

@@ -128,11 +128,6 @@ export default class ModelConversionDispatcher {
 	 * @param {module:engine/model/differ~Differ} differ Differ object with buffered changes.
 	 */
 	convertChanges( differ ) {
-		// First, before changing view structure, remove all markers that has changed.
-		for ( const change of differ.getMarkersToRemove() ) {
-			this.convertMarkerRemove( change.name, change.range );
-		}
-
 		// Convert changes that happened on model tree.
 		for ( const entry of differ.getChanges() ) {
 			if ( entry.type == 'insert' ) {

+ 188 - 0
packages/ckeditor5-engine/tests/controller/editingcontroller.js

@@ -339,6 +339,194 @@ describe( 'EditingController', () => {
 		} );
 	} );
 
+	describe( 'marker clearing', () => {
+		let model, modelRoot, editing, domRoot, mcd, p1;
+
+		beforeEach( () => {
+			model = new Model();
+			modelRoot = model.document.createRoot();
+
+			editing = new EditingController( model );
+
+			domRoot = document.createElement( 'div' );
+			domRoot.contentEditable = true;
+
+			document.body.appendChild( domRoot );
+			editing.createRoot( domRoot );
+
+			model.schema.register( 'paragraph', { inheritAllFrom: '$block' } );
+			model.schema.register( 'div', { inheritAllFrom: '$block' } );
+			buildModelConverter().for( editing.modelToView ).fromElement( 'paragraph' ).toElement( 'p' );
+			buildModelConverter().for( editing.modelToView ).fromElement( 'div' ).toElement( 'div' );
+			buildModelConverter().for( editing.modelToView ).fromMarker( 'marker' ).toHighlight( {} );
+
+			const modelData = new ModelDocumentFragment( parse(
+				'<paragraph>foo</paragraph>' +
+				'<paragraph>bar</paragraph>',
+				model.schema
+			)._children );
+
+			model.change( writer => {
+				writer.insert( modelData, modelRoot );
+				p1 = modelRoot.getChild( 0 );
+
+				model.document.selection.addRange( ModelRange.createFromParentsAndOffsets( p1, 0, p1, 0 ) );
+			} );
+
+			mcd = editing.modelToView;
+			sinon.spy( mcd, 'convertMarkerRemove' );
+		} );
+
+		afterEach( () => {
+			document.body.removeChild( domRoot );
+			editing.destroy();
+		} );
+
+		it( 'should remove marker from view if it will be affected by insert operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				writer.insertText( 'a', p1, 0 );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>af<span>o</span>o</p><p>bar</p>' );
+		} );
+
+		it( 'should remove marker from view if it will be affected by remove operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				writer.remove( ModelRange.createFromParentsAndOffsets( p1, 0, p1, 1 ) );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p><span>o</span>o</p><p>bar</p>' );
+		} );
+
+		it( 'should remove marker from view if it will be affected by move operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				const p2 = p1.nextSibling;
+
+				writer.move( ModelRange.createFromParentsAndOffsets( p2, 0, p2, 2 ), p1, 0 );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>baf<span>o</span>o</p><p>r</p>' );
+		} );
+
+		it( 'should remove marker from view if it will be affected by rename operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( modelRoot, 0, modelRoot, 1 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				writer.rename( p1, 'div' );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<div><span>foo</span></div><p>bar</p>' );
+		} );
+
+		it( 'should remove marker from view if it will be affected by marker operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				const p2 = p1.nextSibling;
+
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p2, 1, p2, 2 ) );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>b<span>a</span>r</p>' );
+		} );
+
+		it( 'should remove marker from view if it is removed through marker collection', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.markers.on( 'remove:marker', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.true;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+			}, { priority: 'low' } );
+
+			model.change( () => {
+				model.markers.remove( 'marker' );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>foo</p><p>bar</p>' );
+		} );
+
+		it( 'should not remove marker if applied operation is an attribute operation', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			// Adding with 'high' priority, because `applyOperation` is decorated - its default callback is fired with 'normal' priority.
+			model.on( 'applyOperation', () => {
+				expect( mcd.convertMarkerRemove.calledOnce ).to.be.false;
+				expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>f<span>o</span>o</p><p>bar</p>' );
+			}, { priority: 'high' } );
+
+			model.change( writer => {
+				writer.setAttribute( 'foo', 'bar', ModelRange.createFromParentsAndOffsets( p1, 0, p1, 2 ) );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>f<span>o</span>o</p><p>bar</p>' );
+		} );
+
+		it( 'should not crash if multiple operations affect a marker', () => {
+			model.change( writer => {
+				writer.setMarker( 'marker', ModelRange.createFromParentsAndOffsets( p1, 1, p1, 2 ) );
+			} );
+
+			model.change( writer => {
+				writer.insertText( 'a', p1, 0 );
+				writer.insertText( 'a', p1, 0 );
+				writer.insertText( 'a', p1, 0 );
+			} );
+
+			expect( getViewData( editing.view, { withoutSelection: true } ) ).to.equal( '<p>aaaf<span>o</span>o</p><p>bar</p>' );
+		} );
+	} );
+
 	describe( 'destroy()', () => {
 		it( 'should remove listenters', () => {
 			const model = new Model();