Skip to content

Commit

Permalink
No longer crash during destroy() of MultiRootEditor or `InlineEdi…
Browse files Browse the repository at this point in the history
…tor` when the editable was detached manually before destroying. (#17684)

Fix (editor-inline, editor-multi-root): No longer crash the editor during destroy() when the editable was manually detached before destroying. Closes #16561
  • Loading branch information
Mati365 authored Jan 7, 2025
1 parent fe544f0 commit 0a7cf7f
Show file tree
Hide file tree
Showing 7 changed files with 72 additions and 3 deletions.
7 changes: 7 additions & 0 deletions packages/ckeditor5-editor-balloon/tests/ballooneditorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,13 @@ describe( 'BalloonEditorUI', () => {

sinon.assert.callOrder( parentDestroySpy, viewDestroySpy );
} );

it( 'should not crash if called twice', async () => {
const newEditor = await VirtualBalloonTestEditor.create( '' );

await newEditor.destroy();
await newEditor.destroy();
} );
} );

describe( 'element()', () => {
Expand Down
7 changes: 7 additions & 0 deletions packages/ckeditor5-editor-classic/tests/classiceditorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -694,6 +694,13 @@ describe( 'ClassicEditorUI', () => {

sinon.assert.callOrder( parentDestroySpy, viewDestroySpy );
} );

it( 'should not crash if called twice', async () => {
const newEditor = await VirtualClassicTestEditor.create( '' );

await newEditor.destroy();
await newEditor.destroy(); // Should not throw.
} );
} );

describe( 'view()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,13 @@ describe( 'DecoupledEditorUI', () => {

sinon.assert.callOrder( parentDestroySpy, viewDestroySpy );
} );

it( 'should not crash if called twice', async () => {
const newEditor = await VirtualDecoupledTestEditor.create( '' );

await newEditor.destroy();
await newEditor.destroy();
} );
} );

describe( 'element()', () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/ckeditor5-editor-inline/src/inlineeditorui.ts
Original file line number Diff line number Diff line change
Expand Up @@ -114,7 +114,10 @@ export default class InlineEditorUI extends EditorUI {
const view = this.view;
const editingView = this.editor.editing.view;

editingView.detachDomRoot( view.editable.name! );
if ( editingView.getDomRoot( view.editable.name! ) ) {
editingView.detachDomRoot( view.editable.name! );
}

view.destroy();
}

Expand Down
18 changes: 17 additions & 1 deletion packages/ckeditor5-editor-inline/tests/inlineeditorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,9 @@ describe( 'InlineEditorUI', () => {
} );

afterEach( async () => {
await editor.destroy();
if ( editor ) {
await editor.destroy();
}
} );

describe( 'constructor()', () => {
Expand Down Expand Up @@ -328,6 +330,20 @@ describe( 'InlineEditorUI', () => {

sinon.assert.callOrder( parentDestroySpy, viewDestroySpy );
} );

it( 'should not crash if the editable element is not present', async () => {
editor.editing.view.detachDomRoot( editor.ui.view.editable.name );

await editor.destroy();
editor = null;
} );

it( 'should not crash if called twice', async () => {
const editor = await VirtualInlineTestEditor.create( '' );

await editor.destroy();
await editor.destroy();
} );
} );

describe( 'element()', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -157,7 +157,12 @@ export default class MultiRootEditorUI extends EditorUI {
* @param editable Editable to remove from the editor UI.
*/
public removeEditable( editable: InlineEditableUIView ): void {
this.editor.editing.view.detachDomRoot( editable.name! );
const editingView = this.editor.editing.view;

if ( editingView.getDomRoot( editable.name! ) ) {
editingView.detachDomRoot( editable.name! );
}

editable.unbind( 'isFocused' );
this.removeEditableElement( editable.name! );
}
Expand Down
24 changes: 24 additions & 0 deletions packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js
Original file line number Diff line number Diff line change
Expand Up @@ -423,5 +423,29 @@ describe( 'MultiRootEditorUI', () => {

sinon.assert.callOrder( parentDestroySpy, viewDestroySpy );
} );

// Some of integrations might detach the DOM editing view *before* destroying the editor.
// It happens quite often in the strict mode of the React integration. In such case, the editor
// component is being unmounted after editable component is detached from the DOM. In such scenario,
// the root doesn't contain the DOM editable anymore. This test ensures that the editor does not throw.
// Issue: https://github.com/ckeditor/ckeditor5/issues/16561
it( 'should not throw when trying to detach a DOM root that was not attached to editing view', async () => {
const newEditor = await MultiRootEditor.create( { foo: '', bar: '' } );
const editingView = newEditor.editing.view;

// Simulate unmounting the editable child component before the editor component.
editingView.detachDomRoot( 'foo' );

// This should not throw
await newEditor.destroy();
} );

// Issue: https://github.com/ckeditor/ckeditor5/issues/16561
it( 'should not throw error when it was called twice', async () => {
const newEditor = await MultiRootEditor.create( { foo: '', bar: '' } );

await newEditor.destroy();
await newEditor.destroy(); // This should not throw
} );
} );
} );

0 comments on commit 0a7cf7f

Please sign in to comment.