From ed194fe4a4c0359dc81232e0f0a8727b9d95b320 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 23 Dec 2024 07:53:22 +0100 Subject: [PATCH 1/3] No longer crash during `destroy()` of `MultiRootEditor` or `InlineEditor` when the editable was detached manually before destroying. --- .../src/inlineeditorui.ts | 5 ++++- .../tests/inlineeditorui.js | 11 ++++++++++- .../src/multirooteditorui.ts | 7 ++++++- .../tests/multirooteditorui.js | 16 ++++++++++++++++ 4 files changed, 36 insertions(+), 3 deletions(-) diff --git a/packages/ckeditor5-editor-inline/src/inlineeditorui.ts b/packages/ckeditor5-editor-inline/src/inlineeditorui.ts index d11f04e7efe..3e21c216761 100644 --- a/packages/ckeditor5-editor-inline/src/inlineeditorui.ts +++ b/packages/ckeditor5-editor-inline/src/inlineeditorui.ts @@ -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(); } diff --git a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js index 3842d42ff08..4fe4d04b341 100644 --- a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js +++ b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js @@ -41,7 +41,9 @@ describe( 'InlineEditorUI', () => { } ); afterEach( async () => { - await editor.destroy(); + if ( editor ) { + await editor.destroy(); + } } ); describe( 'constructor()', () => { @@ -328,6 +330,13 @@ 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; + } ); } ); describe( 'element()', () => { diff --git a/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts b/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts index 6d0c2edbae1..9948298c9de 100644 --- a/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts +++ b/packages/ckeditor5-editor-multi-root/src/multirooteditorui.ts @@ -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! ); } diff --git a/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js b/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js index cc8df837cdb..27787834a7c 100644 --- a/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js +++ b/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js @@ -423,5 +423,21 @@ 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(); + } ); } ); } ); From 69533b20a2626549cd571d95ba0c0659a66752d2 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 23 Dec 2024 09:07:49 +0100 Subject: [PATCH 2/3] Add tests for checking if two `destroy()` editor calls cause crash. --- .../ckeditor5-editor-balloon/tests/ballooneditorui.js | 7 +++++++ .../ckeditor5-editor-classic/tests/classiceditorui.js | 7 +++++++ .../ckeditor5-editor-decoupled/tests/decouplededitorui.js | 7 +++++++ packages/ckeditor5-editor-inline/tests/inlineeditorui.js | 7 +++++++ .../tests/multirooteditorui.js | 8 ++++++++ 5 files changed, 36 insertions(+) diff --git a/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js b/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js index 28d9488779e..ee8f0cec686 100644 --- a/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js +++ b/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js @@ -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()', () => { diff --git a/packages/ckeditor5-editor-classic/tests/classiceditorui.js b/packages/ckeditor5-editor-classic/tests/classiceditorui.js index 210626a8a1e..2fbfa63404c 100644 --- a/packages/ckeditor5-editor-classic/tests/classiceditorui.js +++ b/packages/ckeditor5-editor-classic/tests/classiceditorui.js @@ -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()', () => { diff --git a/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js b/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js index 5563d9dc512..85cef70964b 100644 --- a/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js +++ b/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js @@ -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()', () => { diff --git a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js index 4fe4d04b341..f63032d6939 100644 --- a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js +++ b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js @@ -337,6 +337,13 @@ describe( 'InlineEditorUI', () => { await editor.destroy(); editor = null; } ); + + it( 'should not crash if called twice', async () => { + const editor = VirtualInlineTestEditor.create( '' ); + + await editor.destroy(); + await editor.destroy(); + } ); } ); describe( 'element()', () => { diff --git a/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js b/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js index 27787834a7c..22efb56fbac 100644 --- a/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js +++ b/packages/ckeditor5-editor-multi-root/tests/multirooteditorui.js @@ -439,5 +439,13 @@ describe( 'MultiRootEditorUI', () => { // 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 + } ); } ); } ); From dc2d3bc5cf35b815100084d673708c7d868c29c2 Mon Sep 17 00:00:00 2001 From: Mateusz Baginski Date: Mon, 23 Dec 2024 09:20:14 +0100 Subject: [PATCH 3/3] Fix typo in inline test destroy #[69533b20a2626549cd571d95ba0c0659a66752d2]. --- packages/ckeditor5-editor-inline/tests/inlineeditorui.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js index f63032d6939..96db3b25bdc 100644 --- a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js +++ b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js @@ -339,7 +339,7 @@ describe( 'InlineEditorUI', () => { } ); it( 'should not crash if called twice', async () => { - const editor = VirtualInlineTestEditor.create( '' ); + const editor = await VirtualInlineTestEditor.create( '' ); await editor.destroy(); await editor.destroy();