Skip to content

Commit

Permalink
Merge pull request #16267 from ckeditor/ck/14702
Browse files Browse the repository at this point in the history
Fix (engine): The reverse typing effect should not happen after the focus change. Closes #14702.
  • Loading branch information
niegowski committed Apr 25, 2024
2 parents bee09f6 + b3de933 commit 5e6c2b3
Show file tree
Hide file tree
Showing 2 changed files with 95 additions and 30 deletions.
85 changes: 55 additions & 30 deletions packages/ckeditor5-engine/src/view/observer/focusobserver.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
import DomEventObserver from './domeventobserver.js';
import type DomEventData from './domeventdata.js';
import type View from '../view.js';
import type { ViewDocumentInputEvent } from './inputobserver.js';

/**
* {@link module:engine/view/document~Document#event:focus Focus}
Expand Down Expand Up @@ -48,35 +49,14 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
this.useCapture = true;
const document = this.document;

document.on<ViewDocumentFocusEvent>( 'focus', () => {
this._isFocusChanging = true;

// Unfortunately native `selectionchange` event is fired asynchronously.
// We need to wait until `SelectionObserver` handle the event and then render. Otherwise rendering will
// overwrite new DOM selection with selection from the view.
// See https://github.com/ckeditor/ckeditor5-engine/issues/795 for more details.
// Long timeout is needed to solve #676 and https://github.com/ckeditor/ckeditor5-engine/issues/1157 issues.
//
// Using `view.change()` instead of `view.forceRender()` to prevent double rendering
// in a situation where `selectionchange` already caused selection change.
this._renderTimeoutId = setTimeout( () => {
this.flush();
view.change( () => {} );
}, 50 );
} );

document.on<ViewDocumentBlurEvent>( 'blur', ( evt, data ) => {
const selectedEditable = document.selection.editableElement;

if ( selectedEditable === null || selectedEditable === data.target ) {
document.isFocused = false;
this._isFocusChanging = false;

// Re-render the document to update view elements
// (changing document.isFocused already marked view as changed since last rendering).
view.change( () => {} );
document.on<ViewDocumentFocusEvent>( 'focus', () => this._handleFocus() );
document.on<ViewDocumentBlurEvent>( 'blur', ( evt, data ) => this._handleBlur( data ) );

document.on<ViewDocumentInputEvent>( 'beforeinput', () => {
if ( !document.isFocused ) {
this._handleFocus();
}
} );
}, { priority: 'highest' } );
}

/**
Expand All @@ -100,11 +80,56 @@ export default class FocusObserver extends DomEventObserver<'focus' | 'blur'> {
* @inheritDoc
*/
public override destroy(): void {
this._clearTimeout();
super.destroy();
}

/**
* The `focus` event handler.
*/
private _handleFocus(): void {
this._clearTimeout();
this._isFocusChanging = true;

// Unfortunately native `selectionchange` event is fired asynchronously.
// We need to wait until `SelectionObserver` handle the event and then render. Otherwise rendering will
// overwrite new DOM selection with selection from the view.
// See https://github.com/ckeditor/ckeditor5-engine/issues/795 for more details.
// Long timeout is needed to solve #676 and https://github.com/ckeditor/ckeditor5-engine/issues/1157 issues.
//
// Using `view.change()` instead of `view.forceRender()` to prevent double rendering
// in a situation where `selectionchange` already caused selection change.
this._renderTimeoutId = setTimeout( () => {
this._renderTimeoutId = 0;
this.flush();
this.view.change( () => {} );
}, 50 );
}

/**
* The `blur` event handler.
*/
private _handleBlur( data: DomEventData<FocusEvent> ): void {
const selectedEditable = this.document.selection.editableElement;

if ( selectedEditable === null || selectedEditable === data.target ) {
this.document.isFocused = false;
this._isFocusChanging = false;

// Re-render the document to update view elements
// (changing document.isFocused already marked view as changed since last rendering).
this.view.change( () => {} );
}
}

/**
* Clears timeout.
*/
private _clearTimeout(): void {
if ( this._renderTimeoutId ) {
clearTimeout( this._renderTimeoutId );
this._renderTimeoutId = 0;
}

super.destroy();
}
}

Expand Down
40 changes: 40 additions & 0 deletions packages/ckeditor5-engine/tests/view/observer/focusobserver.js
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,46 @@ describe( 'FocusObserver', () => {

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

it( 'should set isFocused to true on beforeinput after 50ms', () => {
expect( viewDocument.isFocused ).to.be.false;

observer.onDomEvent( { type: 'beforeinput', target: domMain } );
expect( viewDocument.isFocused ).to.be.false;

clock.tick( 50 );
expect( viewDocument.isFocused ).to.be.true;
} );

it( 'should set isFocused to true on beforeinput after flush', () => {
expect( viewDocument.isFocused ).to.be.false;

observer.onDomEvent( { type: 'beforeinput', target: domMain } );
expect( viewDocument.isFocused ).to.be.false;

observer.flush();
expect( viewDocument.isFocused ).to.be.true;
} );

it( 'should not set isFocused to true on beforeinput on other element after 50ms', () => {
expect( viewDocument.isFocused ).to.be.false;

observer.onDomEvent( { type: 'beforeinput', target: document } );
expect( viewDocument.isFocused ).to.be.false;

clock.tick( 50 );
expect( viewDocument.isFocused ).to.be.true;
} );

it( 'should not set isFocused to true on beforeinput on focused document after 50ms', () => {
viewDocument.isFocused = true;

observer.onDomEvent( { type: 'beforeinput', target: document } );
expect( viewDocument.isFocused ).to.be.true;

clock.tick( 50 );
expect( viewDocument.isFocused ).to.be.true;
} );
} );

describe( 'handle _isFocusChanging property of the document', () => {
Expand Down

0 comments on commit 5e6c2b3

Please sign in to comment.