Răsfoiți Sursa

Merge pull request #1730 from ckeditor/t/ckeditor5-typing/188

Fix: Prevented from losing selection attributes between operations - IME. Closes https://github.com/ckeditor/ckeditor5-typing/issues/188.
Piotr Jasiun 6 ani în urmă
părinte
comite
6701df5df6

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

@@ -307,12 +307,19 @@ export default class Document {
 		if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
 		if ( this._hasDocumentChangedFromTheLastChangeBlock() ) {
 			this._callPostFixers( writer );
 			this._callPostFixers( writer );
 
 
+			// Refresh selection attributes according to the final position in the model after the change.
+			this.selection.refresh();
+
 			if ( this.differ.hasDataChanges() ) {
 			if ( this.differ.hasDataChanges() ) {
 				this.fire( 'change:data', writer.batch );
 				this.fire( 'change:data', writer.batch );
 			} else {
 			} else {
 				this.fire( 'change', writer.batch );
 				this.fire( 'change', writer.batch );
 			}
 			}
 
 
+			// Theoretically, it is not necessary to refresh selection after change event because
+			// post-fixers are the last who should change the model, but just in case...
+			this.selection.refresh();
+
 			this.differ.reset();
 			this.differ.reset();
 		}
 		}
 
 
@@ -391,6 +398,14 @@ export default class Document {
 
 
 		do {
 		do {
 			for ( const callback of this._postFixers ) {
 			for ( const callback of this._postFixers ) {
+				// Ensure selection attributes are up to date before each post-fixer.
+				// https://github.com/ckeditor/ckeditor5-engine/issues/1673.
+				//
+				// It might be good to refresh the selection after each operation but at the moment it leads
+				// to losing attributes for composition or and spell checking
+				// https://github.com/ckeditor/ckeditor5-typing/issues/188
+				this.selection.refresh();
+
 				wasFixed = callback( writer );
 				wasFixed = callback( writer );
 
 
 				if ( wasFixed ) {
 				if ( wasFixed ) {

+ 33 - 36
packages/ckeditor5-engine/src/model/documentselection.js

@@ -359,6 +359,14 @@ export default class DocumentSelection {
 		return this._selection.hasAttribute( key );
 		return this._selection.hasAttribute( key );
 	}
 	}
 
 
+	/**
+	 * Refreshes selection attributes and markers according to the current position in the model.
+	 */
+	refresh() {
+		this._selection._updateMarkers();
+		this._selection._updateAttributes( false );
+	}
+
 	/**
 	/**
 	 * Checks whether object is of given type following the convention set by
 	 * Checks whether object is of given type following the convention set by
 	 * {@link module:engine/model/node~Node#is `Node#is()`}.
 	 * {@link module:engine/model/node~Node#is `Node#is()`}.
@@ -598,6 +606,26 @@ class LiveSelection extends Selection {
 		// @type {Set}
 		// @type {Set}
 		this._overriddenGravityRegister = new Set();
 		this._overriddenGravityRegister = new Set();
 
 
+		// Ensure selection is correct 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();
+
+				this._fixGraveyardSelection( liveRange, sourcePosition );
+			}
+
+			if ( this._hasChangedRange ) {
+				this._hasChangedRange = false;
+				this.fire( 'change:range', { directChange: false } );
+			}
+		}, { priority: 'lowest' } );
+
 		// Ensure selection is correct and up to date after each range change.
 		// Ensure selection is correct and up to date after each range change.
 		this.on( 'change:range', () => {
 		this.on( 'change:range', () => {
 			for ( const range of this.getRanges() ) {
 			for ( const range of this.getRanges() ) {
@@ -615,38 +643,12 @@ class LiveSelection extends Selection {
 					);
 					);
 				}
 				}
 			}
 			}
-
-			this._updateMarkers();
-			this._updateAttributes( false );
 		} );
 		} );
 
 
 		// Update markers data stored by the selection after each marker change.
 		// Update markers data stored by the selection after each marker change.
 		this.listenTo( this._model.markers, 'update', () => this._updateMarkers() );
 		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();
-
-				this._fixGraveyardSelection( liveRange, sourcePosition );
-			}
-
-			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.
+		// Ensure selection is up to date after each change block.
 		this.listenTo( this._document, 'change', ( evt, batch ) => {
 		this.listenTo( this._document, 'change', ( evt, batch ) => {
 			clearAttributesStoredInElement( this._model, batch );
 			clearAttributesStoredInElement( this._model, batch );
 		} );
 		} );
@@ -715,12 +717,12 @@ class LiveSelection extends Selection {
 
 
 	setTo( selectable, optionsOrPlaceOrOffset, options ) {
 	setTo( selectable, optionsOrPlaceOrOffset, options ) {
 		super.setTo( selectable, optionsOrPlaceOrOffset, options );
 		super.setTo( selectable, optionsOrPlaceOrOffset, options );
-		this._refreshAttributes();
+		this._updateAttributes( true );
 	}
 	}
 
 
 	setFocus( itemOrPosition, offset ) {
 	setFocus( itemOrPosition, offset ) {
 		super.setFocus( itemOrPosition, offset );
 		super.setFocus( itemOrPosition, offset );
-		this._refreshAttributes();
+		this._updateAttributes( true );
 	}
 	}
 
 
 	setAttribute( key, value ) {
 	setAttribute( key, value ) {
@@ -747,7 +749,7 @@ class LiveSelection extends Selection {
 		this._overriddenGravityRegister.add( overrideUid );
 		this._overriddenGravityRegister.add( overrideUid );
 
 
 		if ( this._overriddenGravityRegister.size === 1 ) {
 		if ( this._overriddenGravityRegister.size === 1 ) {
-			this._refreshAttributes();
+			this._updateAttributes( true );
 		}
 		}
 
 
 		return overrideUid;
 		return overrideUid;
@@ -773,15 +775,10 @@ class LiveSelection extends Selection {
 
 
 		// Restore gravity only when all overriding have been restored.
 		// Restore gravity only when all overriding have been restored.
 		if ( !this.isGravityOverridden ) {
 		if ( !this.isGravityOverridden ) {
-			this._refreshAttributes();
+			this._updateAttributes( true );
 		}
 		}
 	}
 	}
 
 
-	// Removes all attributes from the selection and sets attributes according to the surrounding nodes.
-	_refreshAttributes() {
-		this._updateAttributes( true );
-	}
-
 	_popRange() {
 	_popRange() {
 		this._ranges.pop().detach();
 		this._ranges.pop().detach();
 	}
 	}

+ 44 - 5
packages/ckeditor5-engine/tests/model/documentselection.js

@@ -1143,7 +1143,7 @@ describe( 'DocumentSelection', () => {
 
 
 	// https://github.com/ckeditor/ckeditor5-engine/issues/1673
 	// https://github.com/ckeditor/ckeditor5-engine/issues/1673
 	describe( 'refreshing selection data', () => {
 	describe( 'refreshing selection data', () => {
-		it( 'should be up to date when post fixers are called', done => {
+		it( 'should be up to date before post fixers', () => {
 			model.schema.extend( '$text', { allowAttributes: 'foo' } );
 			model.schema.extend( '$text', { allowAttributes: 'foo' } );
 
 
 			const p = doc.getRoot().getChild( 0 );
 			const p = doc.getRoot().getChild( 0 );
@@ -1151,7 +1151,6 @@ describe( 'DocumentSelection', () => {
 			doc.registerPostFixer( () => {
 			doc.registerPostFixer( () => {
 				expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'bar' );
 				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker' ] );
 				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker' ] );
-				done();
 			} );
 			} );
 
 
 			model.change( writer => {
 			model.change( writer => {
@@ -1169,7 +1168,7 @@ describe( 'DocumentSelection', () => {
 			} );
 			} );
 		} );
 		} );
 
 
-		it( 'should be up to date when post fixers have changed the data', () => {
+		it( 'should be up to date between post fixers', () => {
 			model.schema.extend( '$text', { allowAttributes: 'foo' } );
 			model.schema.extend( '$text', { allowAttributes: 'foo' } );
 
 
 			const p = doc.getRoot().getChild( 0 );
 			const p = doc.getRoot().getChild( 0 );
@@ -1186,6 +1185,11 @@ describe( 'DocumentSelection', () => {
 				} );
 				} );
 			} );
 			} );
 
 
+			doc.registerPostFixer( () => {
+				expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
+				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
+			} );
+
 			model.change( writer => {
 			model.change( writer => {
 				writer.insertText( 'abcdef', { foo: 'bar' }, p );
 				writer.insertText( 'abcdef', { foo: 'bar' }, p );
 
 
@@ -1199,9 +1203,44 @@ describe( 'DocumentSelection', () => {
 
 
 				writer.setSelection( writer.createPositionFromPath( p, [ 3 ] ) );
 				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' ] );
+		it( 'should be up to date after post fixers (on `change` event)', done => {
+			model.schema.extend( '$text', { allowAttributes: 'foo' } );
+
+			const p = doc.getRoot().getChild( 0 );
+
+			doc.on( 'change', () => {
+				expect( model.document.selection.getAttribute( 'foo' ) ).to.equal( 'biz' );
+				expect( Array.from( model.document.selection.markers, m => m.name ) ).to.deep.equal( [ 'marker-2' ] );
+				done();
+			} );
+
+			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 ] ) );
+			} );
 		} );
 		} );
 	} );
 	} );