Browse Source

Update selection data after each change that may affect the selection.

Oskar Wróbel 6 years ago
parent
commit
3d4771db63

+ 0 - 3
packages/ckeditor5-engine/src/model/document.js

@@ -305,9 +305,6 @@ export default class Document {
 	 */
 	_handleChangeBlock( writer ) {
 		if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
-			// Refresh the selection data as attributes or markers before the post-fixers are called.
-			this.selection._refresh( writer );
-
 			this._callPostFixers( writer );
 
 			if ( this.differ.hasDataChanges() ) {

+ 32 - 31
packages/ckeditor5-engine/src/model/documentselection.js

@@ -378,16 +378,6 @@ export default class DocumentSelection {
 	}
 
 	/**
-	 * Refreshes the selection data as attributes or markers.
-	 *
-	 * @protected
-	 * @param {module:engine/model/writer~Writer} writer
-	 */
-	_refresh( writer ) {
-		this._selection.refresh( writer );
-	}
-
-	/**
 	 * Moves {@link module:engine/model/documentselection~DocumentSelection#focus} to the specified location.
 	 * Should be used only within the {@link module:engine/model/writer~Writer#setSelectionFocus} method.
 	 *
@@ -608,7 +598,7 @@ class LiveSelection extends Selection {
 		// @type {Set}
 		this._overriddenGravityRegister = new Set();
 
-		// Add events that will ensure selection correctness.
+		// Ensure selection is correct and up to date after each range change.
 		this.on( 'change:range', () => {
 			for ( const range of this.getRanges() ) {
 				if ( !this._document._validateSelectionRange( range ) ) {
@@ -625,9 +615,22 @@ class LiveSelection extends Selection {
 					);
 				}
 			}
+
+			this._updateMarkers();
+			this._updateAttributes( false );
 		} );
 
-		this.listenTo( this._model, 'applyOperation', () => {
+		// Update markers data stored by the selection after each marker change.
+		this.listenTo( this._model.markers, 'update', () => this._updateMarkers() );
+
+		// Ensure selection is correct and up to date after each operation.
+		this.listenTo( this._model, 'applyOperation', ( evt, args ) => {
+			const operation = args[ 0 ];
+
+			if ( operation.type === 'marker' || operation.type === 'rename' || operation.type === 'noop' ) {
+				return;
+			}
+
 			while ( this._fixGraveyardRangesData.length ) {
 				const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();
 
@@ -636,10 +639,17 @@ class LiveSelection extends Selection {
 
 			if ( this._hasChangedRange ) {
 				this._hasChangedRange = false;
-
 				this.fire( 'change:range', { directChange: false } );
 			}
+
+			this._updateMarkers();
+			this._updateAttributes( false );
 		}, { priority: 'lowest' } );
+
+		// Clear selection attributes from element if no longer empty.
+		this.listenTo( this._document, 'change', ( evt, batch ) => {
+			clearAttributesStoredInElement( this._model, batch );
+		} );
 	}
 
 	get isCollapsed() {
@@ -729,17 +739,6 @@ class LiveSelection extends Selection {
 		}
 	}
 
-	refresh( writer ) {
-		// Update selection's markers.
-		this._updateMarkers();
-
-		// Update selection's attributes.
-		this._updateAttributes( false );
-
-		// Clear selection attributes from element if no longer empty.
-		clearAttributesStoredInElement( this._model, writer );
-	}
-
 	overrideGravity() {
 		const overrideUid = uid();
 
@@ -1146,8 +1145,8 @@ function getAttrsIfCharacter( node ) {
 //
 // @private
 // @param {module:engine/model/model~Model} model
-// @param {module:engine/model/writer~Writer} writer
-function clearAttributesStoredInElement( model, writer ) {
+// @param {module:engine/model/batch~Batch} batch
+function clearAttributesStoredInElement( model, batch ) {
 	const differ = model.document.differ;
 
 	for ( const entry of differ.getChanges() ) {
@@ -1159,12 +1158,14 @@ function clearAttributesStoredInElement( model, writer ) {
 		const isNoLongerEmpty = entry.length === changeParent.maxOffset;
 
 		if ( isNoLongerEmpty ) {
-			const storedAttributes = Array.from( changeParent.getAttributeKeys() )
-				.filter( key => key.startsWith( storePrefix ) );
+			model.enqueueChange( batch, writer => {
+				const storedAttributes = Array.from( changeParent.getAttributeKeys() )
+					.filter( key => key.startsWith( storePrefix ) );
 
-			for ( const key of storedAttributes ) {
-				writer.removeAttribute( key, changeParent );
-			}
+				for ( const key of storedAttributes ) {
+					writer.removeAttribute( key, changeParent );
+				}
+			} );
 		}
 	}
 }

+ 0 - 25
packages/ckeditor5-engine/tests/model/document.js

@@ -295,31 +295,6 @@ describe( 'Document', () => {
 			sinon.assert.calledTwice( callB );
 			sinon.assert.calledOnce( callC );
 		} );
-
-		// https://github.com/ckeditor/ckeditor5-engine/issues/1673
-		it( 'should be called when DocumentSelection data are up to date', done => {
-			doc.registerPostFixer( () => {
-				expect( model.document.selection.hasAttribute( 'foo' ) ).to.equal( true );
-				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker' ] );
-				done();
-			} );
-
-			model.schema.extend( '$text', { allowAttributes: 'foo' } );
-
-			model.change( writer => {
-				writer.insertText( 'abcdef', { foo: 'bar' }, doc.getRoot(), 0 );
-
-				writer.addMarker( 'marker', {
-					range: writer.createRange(
-						writer.createPositionFromPath( doc.getRoot(), [ 1 ] ),
-						writer.createPositionFromPath( doc.getRoot(), [ 5 ] )
-					),
-					usingOperation: false
-				} );
-
-				writer.setSelection( writer.createPositionFromPath( doc.getRoot(), [ 3 ] ) );
-			} );
-		} );
 	} );
 
 	describe( 'event change', () => {

+ 64 - 41
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -508,47 +508,6 @@ describe( 'DocumentSelection', () => {
 		} );
 	} );
 
-	describe( '_refresh()', () => {
-		it( 'should refresh selection markers', () => {
-			model.change( writer => {
-				writer.setSelection( writer.createRange(
-					writer.createPositionFromPath( root, [ 2, 3 ] )
-				) );
-			} );
-
-			const range = model.createRange(
-				model.createPositionFromPath( root, [ 2, 0 ] ),
-				model.createPositionFromPath( root, [ 2, 6 ] )
-			);
-
-			model.markers._set( 'marker', range, false, false );
-
-			expect( Array.from( selection.markers ) ).to.length( 0 );
-
-			selection._refresh();
-
-			expect( Array.from( selection.markers ) ).to.length( 1 );
-		} );
-
-		it( 'should refresh selection attributes', () => {
-			model.schema.extend( '$text', { allowAttributes: 'foo' } );
-
-			model.change( writer => {
-				writer.setSelection( writer.createRange(
-					writer.createPositionFromPath( root, [ 2, 3 ] )
-				) );
-			} );
-
-			root.getChild( 2 ).getChild( 0 )._setAttribute( 'foo', 'bar' );
-
-			expect( selection.hasAttribute( 'foo' ) ).to.equal( false );
-
-			selection._refresh();
-
-			expect( selection.hasAttribute( 'foo' ) ).to.equal( true );
-		} );
-	} );
-
 	describe( '_setTo() - set collapsed at', () => {
 		it( 'detaches all existing ranges', () => {
 			selection._setTo( [ range, liveRange ] );
@@ -1182,6 +1141,70 @@ describe( 'DocumentSelection', () => {
 		} );
 	} );
 
+	// https://github.com/ckeditor/ckeditor5-engine/issues/1673
+	describe( 'refreshing selection data', () => {
+		it( 'should be up to date when post fixers are called', done => {
+			model.schema.extend( '$text', { allowAttributes: 'foo' } );
+
+			const p = doc.getRoot().getChild( 0 );
+
+			doc.registerPostFixer( () => {
+				expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
+				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker' ] );
+				done();
+			} );
+
+			model.change( writer => {
+				writer.insertText( 'abcdef', { foo: 'bar' }, p );
+
+				writer.addMarker( 'marker', {
+					range: writer.createRange(
+						writer.createPositionFromPath( p, [ 1 ] ),
+						writer.createPositionFromPath( p, [ 5 ] )
+					),
+					usingOperation: false
+				} );
+
+				writer.setSelection( writer.createPositionFromPath( p, [ 3 ] ) );
+			} );
+		} );
+
+		it( 'should be up to date when post fixers have changed the data', () => {
+			model.schema.extend( '$text', { allowAttributes: 'foo' } );
+
+			const p = doc.getRoot().getChild( 0 );
+
+			doc.registerPostFixer( writer => {
+				writer.setAttribute( 'foo', 'biz', p.getChild( 0 ) );
+				writer.removeMarker( 'marker-1' );
+				writer.addMarker( 'marker-2', {
+					range: writer.createRange(
+						writer.createPositionFromPath( p, [ 1 ] ),
+						writer.createPositionFromPath( p, [ 5 ] )
+					),
+					usingOperation: false
+				} );
+			} );
+
+			model.change( writer => {
+				writer.insertText( 'abcdef', { foo: 'bar' }, p );
+
+				writer.addMarker( 'marker-1', {
+					range: writer.createRange(
+						writer.createPositionFromPath( p, [ 1 ] ),
+						writer.createPositionFromPath( p, [ 5 ] )
+					),
+					usingOperation: false
+				} );
+
+				writer.setSelection( writer.createPositionFromPath( p, [ 3 ] ) );
+			} );
+
+			expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
+			expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
+		} );
+	} );
+
 	// DocumentSelection uses LiveRanges so here are only simple test to see if integration is
 	// working well, without getting into complicated corner cases.
 	describe( 'after applying an operation should get updated and fire events', () => {