Fixed DOM selection rendering after missing selectionchange event (#1…

…2223)

* Upcasting DOM selection on mouseup event to make sure model selection is up-to-date at isSelecting flag set to false.

packages/ckeditor5-engine/src/view/observer/selectionobserver.ts

@@ -142,6 +142,14 @@ export default class SelectionObserver extends Observer {
 		};
 
 		const endDocumentIsSelecting = () => {
+			if ( !this.document.isSelecting ) {
+				return;
+			}
+
+			// Make sure that model selection is up-to-date at the end of selecting process.
+			// Sometimes `selectionchange` events could arrive after the `mouseup` event and that selection could be already outdated.
+			this._handleSelectionChange( null, domDocument );
+
 			this.document.isSelecting = false;
 
 			// The safety timeout can be canceled when the document leaves the "is selecting" state.
@@ -152,15 +160,19 @@ export default class SelectionObserver extends Observer {
 		// (e.g. by holding the mouse button and moving the cursor). The state resets when they either released
 		// the mouse button or interrupted the process by pressing or releasing any key.
 		this.listenTo( domElement, 'selectstart', startDocumentIsSelecting, { priority: 'highest' } );
-		this.listenTo( domElement, 'keydown', endDocumentIsSelecting, { priority: 'highest' } );
-		this.listenTo( domElement, 'keyup', endDocumentIsSelecting, { priority: 'highest' } );
+
+		this.listenTo( domElement, 'keydown', endDocumentIsSelecting, { priority: 'highest', useCapture: true } );
+		this.listenTo( domElement, 'keyup', endDocumentIsSelecting, { priority: 'highest', useCapture: true } );
 
 		// Add document-wide listeners only once. This method could be called for multiple editing roots.
 		if ( this._documents.has( domDocument ) ) {
 			return;
 		}
 
-		this.listenTo( domDocument, 'mouseup', endDocumentIsSelecting, { priority: 'highest' } );
+		// This listener is using capture mode to make sure that selection is upcasted before any other
+		// handler would like to check it and update (for example table multi cell selection).
+		this.listenTo( domDocument, 'mouseup', endDocumentIsSelecting, { priority: 'highest', useCapture: true } );
+
 		this.listenTo( domDocument, 'selectionchange', ( evt, domEvent ) => {
 			this._handleSelectionChange( domEvent, domDocument );
 

packages/ckeditor5-engine/tests/view/observer/selectionobserver.js

@@ -395,13 +395,70 @@ describe( 'SelectionObserver', () => {
 			// Note: The listener in SelectionObserver has the same priority but was attached first.
 			selectionObserver.listenTo( domDocument, 'mouseup', () => {
 				expect( viewDocument.isSelecting ).to.be.false;
+			}, { priority: 'highest', useCapture: true } );
+
+			domDocument.dispatchEvent( new Event( 'mouseup' ) );
+
+			expect( viewDocument.isSelecting ).to.be.false;
+		} );
+
+		it( 'should fire selectionChange event upon the "mouseup" event (if DOM selection differs from view selection', done => {
+			// Disable DOM selectionchange event to make sure that mouseup triggered view event.
+			selectionObserver.listenTo( domDocument, 'selectionchange', evt => {
+				evt.stop();
 			}, { priority: 'highest' } );
 
+			viewDocument.on( 'selectionChange', ( evt, data ) => {
+				expect( data ).to.have.property( 'domSelection' ).that.equals( domDocument.getSelection() );
+
+				expect( data ).to.have.property( 'oldSelection' ).that.is.instanceof( DocumentSelection );
+				expect( data.oldSelection.rangeCount ).to.equal( 0 );
+
+				expect( data ).to.have.property( 'newSelection' ).that.is.instanceof( ViewSelection );
+				expect( data.newSelection.rangeCount ).to.equal( 1 );
+
+				const newViewRange = data.newSelection.getFirstRange();
+				const viewFoo = viewDocument.getRoot().getChild( 1 ).getChild( 0 );
+
+				expect( newViewRange.start.parent ).to.equal( viewFoo );
+				expect( newViewRange.start.offset ).to.equal( 2 );
+				expect( newViewRange.end.parent ).to.equal( viewFoo );
+				expect( newViewRange.end.offset ).to.equal( 2 );
+
+				// Make sure that selectionChange event was triggered before the isSelecting flag reset
+				// so that model and view selection could get updated before isSelecting is reset
+				// and renderer updates the DOM selection.
+				expect( viewDocument.isSelecting ).to.be.true;
+
+				done();
+			} );
+
+			viewDocument.isSelecting = true;
+
+			changeDomSelection();
 			domDocument.dispatchEvent( new Event( 'mouseup' ) );
 
 			expect( viewDocument.isSelecting ).to.be.false;
 		} );
 
+		it( 'should not fire selectionChange event upon the "mouseup" event if it was not selecting', done => {
+			// Disable DOM selectionchange event to make sure that mouseup triggered view event.
+			selectionObserver.listenTo( domDocument, 'selectionchange', evt => {
+				evt.stop();
+			}, { priority: 'highest' } );
+
+			viewDocument.on( 'selectionChange', () => {
+				throw 'selectionChange fired';
+			} );
+
+			viewDocument.isSelecting = false;
+
+			changeDomSelection();
+			domDocument.dispatchEvent( new Event( 'mouseup' ) );
+
+			setTimeout( done, 100 );
+		} );
+
 		it( 'should set #isSelecting to false upon the "mouseup" event only once (editor with multiple roots)', () => {
 			const isSelectingSetSpy = sinon.spy();
 
@@ -439,7 +496,7 @@ describe( 'SelectionObserver', () => {
 			// Note: The listener in SelectionObserver has the same priority but was attached first.
 			selectionObserver.listenTo( domMain, 'keydown', () => {
 				expect( viewDocument.isSelecting ).to.be.false;
-			}, { priority: 'highest' } );
+			}, { priority: 'highest', useCapture: true } );
 
 			domMain.dispatchEvent( new Event( 'keydown' ) );
 
@@ -468,7 +525,7 @@ describe( 'SelectionObserver', () => {
 			// Note: The listener in SelectionObserver has the same priority but was attached first.
 			selectionObserver.listenTo( domMain, 'keyup', () => {
 				expect( viewDocument.isSelecting ).to.be.false;
-			}, { priority: 'highest' } );
+			}, { priority: 'highest', useCapture: true } );
 
 			domMain.dispatchEvent( new Event( 'keyup' ) );