Merge pull request #1674 from ckeditor/t/1673

Fix: Update model selection attributes and markers after each change that affects the selection. Closes #1673.

Author: Piotr Jasiun

src/model/documentselection.js

@@ -598,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 ) ) {
@@ -615,20 +615,22 @@ class LiveSelection extends Selection {
 					);
 				}
 			}
-		} );
 
-		this.listenTo( this._document, 'change', ( evt, batch ) => {
-			// 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, batch );
 		} );
 
-		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.isDocumentOperation || operation.type == 'marker' || operation.type == 'rename' || operation.type == 'noop' ) {
+				return;
+			}
+
 			while ( this._fixGraveyardRangesData.length ) {
 				const { liveRange, sourcePosition } = this._fixGraveyardRangesData.shift();
 
@@ -637,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() {

tests/model/documentselection.js

@@ -1141,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', () => {