Explorar el Código

Merge pull request #1814 from ckeditor/i/4199

Fix: DOM selection change will not be converted if the selection was placed outside of the editable element. Closes ckeditor/ckeditor5#4199.
Piotrek Koszuliński hace 5 años
padre
commit
f5f94b25ef

+ 9 - 4
packages/ckeditor5-engine/src/view/observer/selectionobserver.js

@@ -126,10 +126,7 @@ export default class SelectionObserver extends Observer {
 	 * @param {Document} domDocument DOM document.
 	 */
 	_handleSelectionChange( domDocument ) {
-		// Selection is handled when document is not focused but is read-only. This is because in read-only
-		// mode contenteditable is set as false and editor won't receive focus but we still need to know
-		// selection position.
-		if ( !this.isEnabled || ( !this.document.isFocused && !this.document.isReadOnly ) ) {
+		if ( !this.isEnabled ) {
 			return;
 		}
 
@@ -141,6 +138,14 @@ export default class SelectionObserver extends Observer {
 		const domSelection = domDocument.defaultView.getSelection();
 		const newViewSelection = this.domConverter.domSelectionToView( domSelection );
 
+		// Do not convert selection change if the new view selection has no ranges in it.
+		//
+		// It means that the DOM selection is in some way incorrect. Ranges that were in the DOM selection could not be
+		// converted to the view. This happens when the DOM selection was moved outside of the editable element.
+		if ( newViewSelection.rangeCount == 0 ) {
+			return;
+		}
+
 		if ( this.selection.isEqual( newViewSelection ) && this.domConverter.isDomSelectionCorrect( domSelection ) ) {
 			return;
 		}

+ 12 - 27
packages/ckeditor5-engine/tests/view/observer/selectionobserver.js

@@ -108,7 +108,7 @@ describe( 'SelectionObserver', () => {
 		} );
 	} );
 
-	it( 'should not fired if observer is disabled', done => {
+	it( 'should not fire if observer is disabled', done => {
 		view.getObserver( SelectionObserver ).disable();
 
 		viewDocument.on( 'selectionChange', () => {
@@ -120,43 +120,28 @@ describe( 'SelectionObserver', () => {
 		changeDomSelection();
 	} );
 
-	it( 'should not fired if there is no focus', done => {
-		viewDocument.isFocused = false;
-
-		// changeDomSelection() may focus the editable element (happens on Chrome)
-		// so cancel this because it sets the isFocused flag.
-		viewDocument.on( 'focus', evt => evt.stop(), { priority: 'highest' } );
-
-		viewDocument.on( 'selectionChange', () => {
-			// Validate the correctness of the test. May help tracking issue with this test.
-			expect( viewDocument.isFocused ).to.be.false;
+	it( 'should not fire if the DOM selection was set outside editable', done => {
+		const viewFoo = viewDocument.getRoot().getChild( 0 ).getChild( 0 );
 
-			throw 'selectionChange on render';
+		view.change( writer => {
+			writer.setSelection( viewFoo, 0 );
 		} );
 
-		setTimeout( done, 70 );
-
-		changeDomSelection();
-	} );
-
-	it( 'should fired if there is no focus but document is read-only', done => {
 		const spy = sinon.spy();
 
-		viewDocument.isFocused = false;
-		viewDocument.isReadOnly = true;
-
-		// changeDomSelection() may focus the editable element (happens on Chrome)
-		// so cancel this because it sets the isFocused flag.
-		viewDocument.on( 'focus', evt => evt.stop(), { priority: 'highest' } );
-
 		viewDocument.on( 'selectionChange', spy );
 
 		setTimeout( () => {
-			sinon.assert.called( spy );
+			expect( spy.called ).to.be.false;
+
 			done();
 		}, 70 );
 
-		changeDomSelection();
+		const domSelection = domDocument.getSelection();
+		const editable = domRoot.childNodes[ 1 ];
+		editable.focus();
+
+		domSelection.collapse( editable, 0 );
 	} );
 
 	it( 'should not enter infinite loop', () => {