Skip to content

Commit

Permalink
Merge pull request #10701 from ckeditor/ck/10700
Browse files Browse the repository at this point in the history 
Internal (engine): Made the `SelectionObserver` listeners managing `Document#isSelecting` respond to DOM events from the DOM root only. Closes #10700.
  • Loading branch information
@niegowski
niegowski authored Oct 14, 2021
2 parents 8caffcf + 8df8282 commit 5189087
Show file tree
Hide file tree
Showing 2 changed files with 83 additions and 22 deletions.
26 changes: 13 additions & 13 deletions packages/ckeditor5-engine/src/view/observer/selectionobserver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -115,11 +115,6 @@ export default class SelectionObserver extends Observer {
observe( domElement ) {
const domDocument = domElement.ownerDocument;

// Add listener once per each document.
if ( this._documents.has( domDocument ) ) {
return;
}

const startDocumentIsSelecting = () => {
this.document.isSelecting = true;

Expand All @@ -134,6 +129,19 @@ export default class SelectionObserver extends Observer {
this._documentIsSelectingInactivityTimeoutDebounced.cancel();
};

// The document has the "is selecting" state while the user keeps making (extending) the selection
// (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' } );

// 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.listenTo( domDocument, 'selectionchange', ( evt, domEvent ) => {
this._handleSelectionChange( domEvent, domDocument );

Expand All @@ -142,14 +150,6 @@ export default class SelectionObserver extends Observer {
this._documentIsSelectingInactivityTimeoutDebounced();
} );

// The document has the "is selecting" state while the user keeps making (extending) the selection
// (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( domDocument, 'selectstart', startDocumentIsSelecting, { priority: 'highest' } );
this.listenTo( domDocument, 'mouseup', endDocumentIsSelecting, { priority: 'highest' } );
this.listenTo( domDocument, 'keydown', endDocumentIsSelecting, { priority: 'highest' } );
this.listenTo( domDocument, 'keyup', endDocumentIsSelecting, { priority: 'highest' } );

this._documents.add( domDocument );
}

Expand Down
79 changes: 70 additions & 9 deletions packages/ckeditor5-engine/tests/view/observer/selectionobserver.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ describe( 'SelectionObserver', () => {
changeDomSelection();
} );

it( 'should add only one listener to one document', done => {
it( 'should add only one #selectionChange listener to one document', done => {
// Add second roots to ensure that listener is added once.
createViewRoot( viewDocument, 'div', 'additional' );
view.attachDomRoot( domDocument.getElementById( 'additional' ), 'additional' );
Expand Down Expand Up @@ -358,16 +358,32 @@ describe( 'SelectionObserver', () => {
} );

describe( 'Management of view Document#isSelecting', () => {
it( 'should not set #isSelecting to true upon the "selectstart" event outside the DOM root', () => {
const selectStartChangedSpy = sinon.spy();

expect( viewDocument.isSelecting ).to.be.false;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domMain, 'selectstart', selectStartChangedSpy, { priority: 'highest' } );

// The event was fired somewhere else in DOM.
domDocument.dispatchEvent( new Event( 'selectstart' ) );

expect( viewDocument.isSelecting ).to.be.false;
sinon.assert.notCalled( selectStartChangedSpy );
} );

it( 'should set #isSelecting to true upon the "selectstart" event', () => {
expect( viewDocument.isSelecting ).to.be.false;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domDocument, 'selectstart', () => {
selectionObserver.listenTo( domMain, 'selectstart', () => {
expect( viewDocument.isSelecting ).to.be.true;
}, { priority: 'highest' } );

domDocument.dispatchEvent( new Event( 'selectstart' ) );
domMain.dispatchEvent( new Event( 'selectstart' ) );

expect( viewDocument.isSelecting ).to.be.true;
} );
Expand All @@ -386,30 +402,75 @@ describe( 'SelectionObserver', () => {
expect( viewDocument.isSelecting ).to.be.false;
} );

it( 'should set #isSelecting to false upon the "mouseup" event only once (editor with multiple roots)', () => {
const isSelectingSetSpy = sinon.spy();

createViewRoot( viewDocument, 'div', 'additional' );
view.attachDomRoot( domDocument.getElementById( 'additional' ), 'additional' );

viewDocument.isSelecting = true;

viewDocument.on( 'set:isSelecting', isSelectingSetSpy );

domDocument.dispatchEvent( new Event( 'mouseup' ) );
expect( viewDocument.isSelecting ).to.be.false;
sinon.assert.calledOnce( isSelectingSetSpy );
} );

it( 'should not set #isSelecting to false upon the "keydown" event outside the DOM root', () => {
const keydownSpy = sinon.spy();

viewDocument.isSelecting = true;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domMain, 'keydown', () => keydownSpy, { priority: 'highest' } );

domMain.dispatchEvent( new Event( 'keydown' ) );

expect( viewDocument.isSelecting ).to.be.false;
sinon.assert.notCalled( keydownSpy );
} );

it( 'should set #isSelecting to false upon the "keydown" event', () => {
viewDocument.isSelecting = true;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domDocument, 'keydown', () => {
selectionObserver.listenTo( domMain, 'keydown', () => {
expect( viewDocument.isSelecting ).to.be.false;
}, { priority: 'highest' } );

domDocument.dispatchEvent( new Event( 'keydown' ) );
domMain.dispatchEvent( new Event( 'keydown' ) );

expect( viewDocument.isSelecting ).to.be.false;
} );

it( 'should not set #isSelecting to false upon the "keyup" event outside the DOM root', () => {
const keyupSpy = sinon.spy();

viewDocument.isSelecting = true;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domMain, 'keyup', () => keyupSpy, { priority: 'highest' } );

domMain.dispatchEvent( new Event( 'keyup' ) );

expect( viewDocument.isSelecting ).to.be.false;
sinon.assert.notCalled( keyupSpy );
} );

it( 'should set #isSelecting to false upon the "keyup" event', () => {
viewDocument.isSelecting = true;

// Make sure isSelecting was already updated by the listener with the highest priority.
// Note: The listener in SelectionObserver has the same priority but was attached first.
selectionObserver.listenTo( domDocument, 'keyup', () => {
selectionObserver.listenTo( domMain, 'keyup', () => {
expect( viewDocument.isSelecting ).to.be.false;
}, { priority: 'highest' } );

domDocument.dispatchEvent( new Event( 'keyup' ) );
domMain.dispatchEvent( new Event( 'keyup' ) );

expect( viewDocument.isSelecting ).to.be.false;
} );
Expand All @@ -434,7 +495,7 @@ describe( 'SelectionObserver', () => {
it( 'should set #isSelecting to false after 5000ms since the selectstart event', done => {
expect( viewDocument.isSelecting ).to.be.false;

domDocument.dispatchEvent( new Event( 'selectstart' ) );
domMain.dispatchEvent( new Event( 'selectstart' ) );

expect( viewDocument.isSelecting ).to.be.true;

Expand All @@ -453,7 +514,7 @@ describe( 'SelectionObserver', () => {
it( 'should postpone setting #isSelecting to false after 5000ms if "selectionchange" fired in the meantime', done => {
expect( viewDocument.isSelecting ).to.be.false;

domDocument.dispatchEvent( new Event( 'selectstart' ) );
domMain.dispatchEvent( new Event( 'selectstart' ) );

expect( viewDocument.isSelecting ).to.be.true;

Expand Down

0 comments on commit 5189087

Please sign in to comment.