Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

DOM selection change will not be converted if the selection was placed outside of the editable element #1814

Merged
merged 4 commits into from
Feb 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/view/observer/selectionobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}

Expand All @@ -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;
}
Expand Down
39 changes: 12 additions & 27 deletions tests/view/observer/selectionobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand All @@ -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', () => {
Expand Down