Selaa lähdekoodia

Changed: Make `SelectionObserver` re-render view if selection has not changed instead of re-setting model selection if it has not changed.

Szymon Cofalik 8 vuotta sitten
vanhempi
commit
91779af348

+ 5 - 3
packages/ckeditor5-engine/src/conversion/view-selection-to-model-converters.js

@@ -39,8 +39,10 @@ export function convertSelectionChange( modelDocument, mapper ) {
 
 		modelSelection.setRanges( ranges, viewSelection.isBackward );
 
-		modelDocument.enqueueChanges( () => {
-			modelDocument.selection.setTo( modelSelection );
-		} );
+		if ( !modelSelection.isEqual( modelDocument.selection ) ) {
+			modelDocument.enqueueChanges( () => {
+				modelDocument.selection.setTo( modelSelection );
+			} );
+		}
 	};
 }

+ 20 - 14
packages/ckeditor5-engine/src/view/observer/selectionobserver.js

@@ -169,20 +169,26 @@ export default class SelectionObserver extends Observer {
 			return;
 		}
 
-		const data = {
-			oldSelection: this.selection,
-			newSelection: newViewSelection,
-			domSelection
-		};
-
-		// Should be fired only when selection change was the only document change.
-		this.document.fire( 'selectionChange', data );
-
-		// Call` #_fireSelectionChangeDoneDebounced` every time when `selectionChange` event is fired.
-		// This function is debounced what means that `selectionChangeDone` event will be fired only when
-		// defined int the function time will elapse since the last time the function was called.
-		// So `selectionChangeDone` will be fired when selection will stop changing.
-		this._fireSelectionChangeDoneDebounced( data );
+		if ( this.selection.isEqual( newViewSelection ) ) {
+			// If selection was equal and we are at this point of algorithm, it means that it was incorrect.
+			// Just re-render it, no need to fire any events, etc.
+			this.document.render();
+		} else {
+			const data = {
+				oldSelection: this.selection,
+				newSelection: newViewSelection,
+				domSelection
+			};
+
+			// Prepare data for new selection and fire appropriate events.
+			this.document.fire( 'selectionChange', data );
+
+			// Call` #_fireSelectionChangeDoneDebounced` every time when `selectionChange` event is fired.
+			// This function is debounced what means that `selectionChangeDone` event will be fired only when
+			// defined int the function time will elapse since the last time the function was called.
+			// So `selectionChangeDone` will be fired when selection will stop changing.
+			this._fireSelectionChangeDoneDebounced( data );
+		}
 	}
 
 	/**

+ 2 - 4
packages/ckeditor5-engine/tests/conversion/view-selection-to-model-converters.js

@@ -109,9 +109,7 @@ describe( 'convertSelectionChange', () => {
 		expect( model.selection.isBackward ).to.true;
 	} );
 
-	it( 'should re-convert selection even if it has not changed in model', () => {
-		// Selection might have not changed in the model but it needs to be reconverted because
-		// it ended up at an incorrect place in the DOM.
+	it( 'should not enqueue changes if selection has not changed', () => {
 		const viewSelection = new ViewSelection();
 		viewSelection.addRange( ViewRange.createFromParentsAndOffsets(
 			viewRoot.getChild( 0 ).getChild( 0 ), 1, viewRoot.getChild( 0 ).getChild( 0 ), 1 ) );
@@ -124,6 +122,6 @@ describe( 'convertSelectionChange', () => {
 
 		convertSelection( null, { newSelection: viewSelection } );
 
-		expect( spy.called ).to.be.true;
+		expect( spy.called ).to.be.false;
 	} );
 } );

+ 10 - 7
packages/ckeditor5-engine/tests/view/observer/selectionobserver.js

@@ -301,7 +301,7 @@ describe( 'SelectionObserver', () => {
 		}, 100 );
 	} );
 
-	it( 'should fire selectionChange event even if selections are similar if DOM selection is in incorrect place', done => {
+	it( 'should re-render view if selections are similar if DOM selection is in incorrect place', done => {
 		const sel = domDocument.getSelection();
 
 		// Add rendering on selectionChange event to check this feature.
@@ -320,18 +320,21 @@ describe( 'SelectionObserver', () => {
 		} );
 
 		viewDocument.once( 'selectionChange', () => {
-			viewDocument.once( 'selectionChange', ( evt, data ) => {
-				// 3. Selection change event was correctly fired.
-				// Check whether new and old view selection were in fact equal.
-				expect( data.oldSelection.isEqual( data.newSelection ) ).to.be.true;
+			// 2. Selection change has been handled and proper event has been fired.
+
+			// 3. Now add listener for `domDocument` because `selectionChange` event will not be fired.
+			selectionObserver.listenTo( domDocument, 'selectionchange', () => {
+				// 5. Check if view was re-rendered.
+				expect( viewDocument.render.called ).to.be.true;
 
 				done();
 			}, { priority: 'lowest' } );
 
-			// 2. Selection change has been handled and proper event has been fired.
-			// Now, collapse selection in similar position, but in UI element.
+			// 4. Now, collapse selection in similar position, but in UI element.
 			// Current and new selection position are same in view.
+			// Also add a spy to `viewDocument#render` to see if view will be re-rendered.
 			sel.collapse( domMain.childNodes[ 0 ].childNodes[ 1 ], 0 );
+			sinon.spy( viewDocument, 'render' );
 		}, { priority: 'lowest' } );
 
 		// 1. Collapse before ui element and wait for async selectionchange to fire selection change handling.