From 3316f07d872afa8e94f4c2e154e5a55915043f77 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 16 Jan 2020 16:42:06 +0100 Subject: [PATCH 1/3] Fix: DOM selection change will not be converted if the selection was placed outside of the editable element. --- src/view/observer/selectionobserver.js | 13 +++++--- tests/view/observer/selectionobserver.js | 40 +++++++----------------- 2 files changed, 21 insertions(+), 32 deletions(-) diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index f495bbc7d..a4b8e99b3 100644 --- a/src/view/observer/selectionobserver.js +++ b/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 if new view selection has no ranges in it. + // + // It means that the DOM selection was in some way incorrect. Ranges that were in the DOM selection could not be + // converted to the view selection. This happens when the selection is moved outside an editable. + if ( newViewSelection.rangeCount == 0 ) { + return; + } + if ( this.selection.isEqual( newViewSelection ) && this.domConverter.isDomSelectionCorrect( domSelection ) ) { return; } diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index f777c15e9..918747ceb 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -20,7 +20,7 @@ describe( 'SelectionObserver', () => { beforeEach( done => { domDocument = document; domRoot = domDocument.createElement( 'div' ); - domRoot.innerHTML = '
'; + domRoot.innerHTML = '

X

'; domMain = domRoot.childNodes[ 0 ]; domDocument.body.appendChild( domRoot ); @@ -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,27 @@ 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 domX = domRoot.childNodes[ 2 ].childNodes[ 0 ]; + + domSelection.collapse( domX, 0 ); } ); it( 'should not enter infinite loop', () => { From c2a84b6cccafa4f601785da2a0fe8434cbac60c9 Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Thu, 16 Jan 2020 16:43:56 +0100 Subject: [PATCH 2/3] Docs: Better comments. --- src/view/observer/selectionobserver.js | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/view/observer/selectionobserver.js b/src/view/observer/selectionobserver.js index a4b8e99b3..87ac9a6de 100644 --- a/src/view/observer/selectionobserver.js +++ b/src/view/observer/selectionobserver.js @@ -138,10 +138,10 @@ export default class SelectionObserver extends Observer { const domSelection = domDocument.defaultView.getSelection(); const newViewSelection = this.domConverter.domSelectionToView( domSelection ); - // Do not convert selection if new view selection has no ranges in it. + // Do not convert selection change if the new view selection has no ranges in it. // - // It means that the DOM selection was in some way incorrect. Ranges that were in the DOM selection could not be - // converted to the view selection. This happens when the selection is moved outside an editable. + // 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; } From a900f9557987069da40a7e4c067146c91f0f3e3a Mon Sep 17 00:00:00 2001 From: Szymon Cofalik Date: Tue, 21 Jan 2020 17:40:27 +0100 Subject: [PATCH 3/3] Tests: Fixed test failing on Firefox. --- tests/view/observer/selectionobserver.js | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/tests/view/observer/selectionobserver.js b/tests/view/observer/selectionobserver.js index 918747ceb..b9a7110ee 100644 --- a/tests/view/observer/selectionobserver.js +++ b/tests/view/observer/selectionobserver.js @@ -20,7 +20,7 @@ describe( 'SelectionObserver', () => { beforeEach( done => { domDocument = document; domRoot = domDocument.createElement( 'div' ); - domRoot.innerHTML = '

X

'; + domRoot.innerHTML = '
'; domMain = domRoot.childNodes[ 0 ]; domDocument.body.appendChild( domRoot ); @@ -138,9 +138,10 @@ describe( 'SelectionObserver', () => { }, 70 ); const domSelection = domDocument.getSelection(); - const domX = domRoot.childNodes[ 2 ].childNodes[ 0 ]; + const editable = domRoot.childNodes[ 1 ]; + editable.focus(); - domSelection.collapse( domX, 0 ); + domSelection.collapse( editable, 0 ); } ); it( 'should not enter infinite loop', () => {