From 7d785770a721ce11fb5c7077a85cfcbb1d3b2591 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Oct 2022 14:43:55 +0200 Subject: [PATCH 1/5] TooltipManager should prevent it's balloon panel from being destroyed until the last editor is destroyed. --- packages/ckeditor5-ui/src/tooltipmanager.ts | 8 +++++ .../tests/tooltip/tooltipmanager.js | 30 +++++++++++++++++++ 2 files changed, 38 insertions(+) diff --git a/packages/ckeditor5-ui/src/tooltipmanager.ts b/packages/ckeditor5-ui/src/tooltipmanager.ts index 19ba61bcbbe..993ad8baf81 100644 --- a/packages/ckeditor5-ui/src/tooltipmanager.ts +++ b/packages/ckeditor5-ui/src/tooltipmanager.ts @@ -214,9 +214,17 @@ export default class TooltipManager extends DomEmitter { * @param {module:core/editor/editor~Editor} editor The editor the manager was created for. */ public destroy( editor: Editor ): void { + const editorBodyViewCollection = editor.ui.view.body; + TooltipManager._editors.delete( editor ); this.stopListening( editor.ui ); + // Prevent the balloon panel from being destroyed in the EditorUI#destroy() cascade. It should be destroyed along + // with the last editor only (https://github.com/ckeditor/ckeditor5/issues/12602). + if ( editorBodyViewCollection.has( this.balloonPanelView ) ) { + editorBodyViewCollection.remove( this.balloonPanelView ); + } + if ( !TooltipManager._editors.size ) { this._unpinTooltip(); this.balloonPanelView.destroy(); diff --git a/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js b/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js index c8e7d8daded..c3eea315676 100644 --- a/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js +++ b/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js @@ -91,6 +91,36 @@ describe( 'TooltipManager', () => { sinon.assert.calledWithExactly( stopListeningSpy.secondCall, secondEditor.ui ); sinon.assert.calledWithExactly( stopListeningSpy.thirdCall ); } ); + + // https://github.com/ckeditor/ckeditor5/issues/12602 + it( 'should avoid destroying #balloonPanelView until the last editor gets destroyed', async () => { + const spy = testUtils.sinon.spy( tooltipManager.balloonPanelView, 'destroy' ); + const elements = getElementsWithTooltips( { + a: { + text: 'A' + } + } ); + const clock = sinon.useFakeTimers(); + + const secondEditor = await ClassicTestEditor.create( element, { + plugins: [ Paragraph, Bold, Italic ], + balloonToolbar: [ 'bold', 'italic' ] + } ); + + utils.dispatchMouseEnter( elements.a ); + utils.waitForTheTooltipToShow( clock ); + + await editor.destroy(); + + sinon.assert.notCalled( spy ); + + await secondEditor.destroy(); + + sinon.assert.calledOnce( spy ); + + destroyElements( elements ); + clock.restore(); + } ); } ); it( 'should unpin the #balloonPanelView', () => { From 1cea40b91ed9756698e77d90dae801a74dd5993b Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Oct 2022 15:25:38 +0200 Subject: [PATCH 2/5] TooltipManager should destroy properly if the editor.ui#view does not exist. --- packages/ckeditor5-ui/src/tooltipmanager.ts | 4 +-- .../tests/tooltip/tooltipmanager.js | 33 +++++++++++++++++++ 2 files changed, 35 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-ui/src/tooltipmanager.ts b/packages/ckeditor5-ui/src/tooltipmanager.ts index 993ad8baf81..ccd7546e2fc 100644 --- a/packages/ckeditor5-ui/src/tooltipmanager.ts +++ b/packages/ckeditor5-ui/src/tooltipmanager.ts @@ -214,14 +214,14 @@ export default class TooltipManager extends DomEmitter { * @param {module:core/editor/editor~Editor} editor The editor the manager was created for. */ public destroy( editor: Editor ): void { - const editorBodyViewCollection = editor.ui.view.body; + const editorBodyViewCollection = editor.ui.view && editor.ui.view.body; TooltipManager._editors.delete( editor ); this.stopListening( editor.ui ); // Prevent the balloon panel from being destroyed in the EditorUI#destroy() cascade. It should be destroyed along // with the last editor only (https://github.com/ckeditor/ckeditor5/issues/12602). - if ( editorBodyViewCollection.has( this.balloonPanelView ) ) { + if ( editorBodyViewCollection && editorBodyViewCollection.has( this.balloonPanelView ) ) { editorBodyViewCollection.remove( this.balloonPanelView ); } diff --git a/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js b/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js index c3eea315676..7dc32cae691 100644 --- a/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js +++ b/packages/ckeditor5-ui/tests/tooltip/tooltipmanager.js @@ -14,6 +14,7 @@ import Paragraph from '@ckeditor/ckeditor5-paragraph/src/paragraph'; import ClassicTestEditor from '@ckeditor/ckeditor5-core/tests/_utils/classictesteditor'; import testUtils from '@ckeditor/ckeditor5-core/tests/_utils/utils'; import TooltipManager from '../../src/tooltipmanager'; +import { Editor, EditorUI } from '@ckeditor/ckeditor5-core'; describe( 'TooltipManager', () => { let editor, element, tooltipManager; @@ -121,6 +122,38 @@ describe( 'TooltipManager', () => { destroyElements( elements ); clock.restore(); } ); + + it( 'should not throw if the editor has no ui#view', async () => { + class EditorWithoutUIView extends Editor { + static create( config ) { + return new Promise( resolve => { + const editor = new this( config ); + + resolve( + editor.initPlugins() + .then( () => { + editor.ui = new EditorUI( editor ); + editor.fire( 'ready' ); + } ) + .then( () => editor ) + ); + } ); + } + + destroy() { + this.ui.destroy(); + + return super.destroy(); + } + } + + const secondEditor = await EditorWithoutUIView.create(); + + await secondEditor.destroy(); + + // No error was thrown. + expect( secondEditor.state ).to.equal( 'destroyed' ); + } ); } ); it( 'should unpin the #balloonPanelView', () => { From c2c4713e6510bfffae33ca4b3b8e09c252625100 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Oct 2022 15:27:17 +0200 Subject: [PATCH 3/5] *EditorUI implementations should call parent EditorUI#destroy() first, then destroy their views. --- .../tests/_utils-tests/classictesteditor.js | 12 ++++++++++++ .../ckeditor5-core/tests/_utils/classictesteditor.js | 4 ++-- packages/ckeditor5-core/tests/editor/editorui.js | 4 ++-- .../ckeditor5-editor-balloon/src/ballooneditorui.js | 4 ++-- .../tests/ballooneditorui.js | 12 ++++++++++++ .../ckeditor5-editor-classic/src/classiceditorui.js | 4 ++-- .../tests/classiceditorui.js | 12 ++++++++++++ .../src/decouplededitorui.js | 4 ++-- .../tests/decouplededitorui.js | 12 ++++++++++++ .../ckeditor5-editor-inline/src/inlineeditorui.js | 4 ++-- .../ckeditor5-editor-inline/tests/inlineeditorui.js | 12 ++++++++++++ 11 files changed, 72 insertions(+), 12 deletions(-) diff --git a/packages/ckeditor5-core/tests/_utils-tests/classictesteditor.js b/packages/ckeditor5-core/tests/_utils-tests/classictesteditor.js index bdb5cb92459..5064a62ed11 100644 --- a/packages/ckeditor5-core/tests/_utils-tests/classictesteditor.js +++ b/packages/ckeditor5-core/tests/_utils-tests/classictesteditor.js @@ -263,5 +263,17 @@ describe( 'ClassicTestEditor', () => { } ); } ); } ); + + it( 'should call parent EditorUI#destroy() first before destroying the view', async () => { + const newEditor = await ClassicTestEditor.create( editorElement, { foo: 1 } ); + const parentEditorUIPrototype = Object.getPrototypeOf( newEditor.ui.constructor.prototype ); + + const parentDestroySpy = testUtils.sinon.spy( parentEditorUIPrototype, 'destroy' ); + const viewDestroySpy = testUtils.sinon.spy( newEditor.ui.view, 'destroy' ); + + await newEditor.destroy(); + + sinon.assert.callOrder( parentDestroySpy, viewDestroySpy ); + } ); } ); } ); diff --git a/packages/ckeditor5-core/tests/_utils/classictesteditor.js b/packages/ckeditor5-core/tests/_utils/classictesteditor.js index 1087137c59f..214287bc152 100644 --- a/packages/ckeditor5-core/tests/_utils/classictesteditor.js +++ b/packages/ckeditor5-core/tests/_utils/classictesteditor.js @@ -146,11 +146,11 @@ class ClassicTestEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + this._elementReplacer.restore(); this._view.destroy(); - - super.destroy(); } } diff --git a/packages/ckeditor5-core/tests/editor/editorui.js b/packages/ckeditor5-core/tests/editor/editorui.js index 9548139ad98..f107a0557b3 100644 --- a/packages/ckeditor5-core/tests/editor/editorui.js +++ b/packages/ckeditor5-core/tests/editor/editorui.js @@ -23,7 +23,7 @@ describe( 'EditorUI', () => { beforeEach( () => { editor = new Editor(); - ui = new EditorUI( editor ); + editor.ui = ui = new EditorUI( editor ); } ); afterEach( () => { @@ -461,7 +461,7 @@ describe( 'EditorUI', () => { it( 'should do nothing if no toolbars were registered', () => { const editor = new Editor(); - const ui = new EditorUI( editor ); + const ui = editor.ui = new EditorUI( editor ); const editingArea = document.createElement( 'div' ); document.body.appendChild( editingArea ); diff --git a/packages/ckeditor5-editor-balloon/src/ballooneditorui.js b/packages/ckeditor5-editor-balloon/src/ballooneditorui.js index 153ce71e358..309d740f073 100644 --- a/packages/ckeditor5-editor-balloon/src/ballooneditorui.js +++ b/packages/ckeditor5-editor-balloon/src/ballooneditorui.js @@ -86,13 +86,13 @@ export default class BalloonEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; editingView.detachDomRoot( view.editable.name ); view.destroy(); - - super.destroy(); } /** diff --git a/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js b/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js index 88287abc4e2..aef9cf2e79d 100644 --- a/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js +++ b/packages/ckeditor5-editor-balloon/tests/ballooneditorui.js @@ -186,6 +186,18 @@ describe( 'BalloonEditorUI', () => { } ); } ); } ); + + it( 'should call parent EditorUI#destroy() first before destroying the view', async () => { + const newEditor = await VirtualBalloonTestEditor.create( '' ); + const parentEditorUIPrototype = Object.getPrototypeOf( newEditor.ui.constructor.prototype ); + + const parentDestroySpy = testUtils.sinon.spy( parentEditorUIPrototype, 'destroy' ); + const viewDestroySpy = testUtils.sinon.spy( newEditor.ui.view, 'destroy' ); + + await newEditor.destroy(); + + sinon.assert.callOrder( parentDestroySpy, viewDestroySpy ); + } ); } ); describe( 'element()', () => { diff --git a/packages/ckeditor5-editor-classic/src/classiceditorui.js b/packages/ckeditor5-editor-classic/src/classiceditorui.js index 58430baae5c..6ebb06071e1 100644 --- a/packages/ckeditor5-editor-classic/src/classiceditorui.js +++ b/packages/ckeditor5-editor-classic/src/classiceditorui.js @@ -114,14 +114,14 @@ export default class ClassicEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; this._elementReplacer.restore(); editingView.detachDomRoot( view.editable.name ); view.destroy(); - - super.destroy(); } /** diff --git a/packages/ckeditor5-editor-classic/tests/classiceditorui.js b/packages/ckeditor5-editor-classic/tests/classiceditorui.js index 01fc64b6c47..f11d9d21ad1 100644 --- a/packages/ckeditor5-editor-classic/tests/classiceditorui.js +++ b/packages/ckeditor5-editor-classic/tests/classiceditorui.js @@ -297,6 +297,18 @@ describe( 'ClassicEditorUI', () => { } ); } ); } ); + + it( 'should call parent EditorUI#destroy() first before destroying the view', async () => { + const newEditor = await VirtualClassicTestEditor.create( '' ); + const parentEditorUIPrototype = Object.getPrototypeOf( newEditor.ui.constructor.prototype ); + + const parentDestroySpy = testUtils.sinon.spy( parentEditorUIPrototype, 'destroy' ); + const viewDestroySpy = testUtils.sinon.spy( newEditor.ui.view, 'destroy' ); + + await newEditor.destroy(); + + sinon.assert.callOrder( parentDestroySpy, viewDestroySpy ); + } ); } ); describe( 'view()', () => { diff --git a/packages/ckeditor5-editor-decoupled/src/decouplededitorui.js b/packages/ckeditor5-editor-decoupled/src/decouplededitorui.js index aafec9e14d4..64eb49ab28a 100644 --- a/packages/ckeditor5-editor-decoupled/src/decouplededitorui.js +++ b/packages/ckeditor5-editor-decoupled/src/decouplededitorui.js @@ -80,13 +80,13 @@ export default class DecoupledEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; editingView.detachDomRoot( view.editable.name ); view.destroy(); - - super.destroy(); } /** diff --git a/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js b/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js index 4c94753f1ce..59aa2f73ce3 100644 --- a/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js +++ b/packages/ckeditor5-editor-decoupled/tests/decouplededitorui.js @@ -238,6 +238,18 @@ describe( 'DecoupledEditorUI', () => { } ); } ); } ); + + it( 'should call parent EditorUI#destroy() first before destroying the view', async () => { + const newEditor = await VirtualDecoupledTestEditor.create( '' ); + const parentEditorUIPrototype = Object.getPrototypeOf( newEditor.ui.constructor.prototype ); + + const parentDestroySpy = testUtils.sinon.spy( parentEditorUIPrototype, 'destroy' ); + const viewDestroySpy = testUtils.sinon.spy( newEditor.ui.view, 'destroy' ); + + await newEditor.destroy(); + + sinon.assert.callOrder( parentDestroySpy, viewDestroySpy ); + } ); } ); describe( 'element()', () => { diff --git a/packages/ckeditor5-editor-inline/src/inlineeditorui.js b/packages/ckeditor5-editor-inline/src/inlineeditorui.js index fe41ba51422..2dfaa7ca57c 100644 --- a/packages/ckeditor5-editor-inline/src/inlineeditorui.js +++ b/packages/ckeditor5-editor-inline/src/inlineeditorui.js @@ -96,13 +96,13 @@ export default class InlineEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; editingView.detachDomRoot( view.editable.name ); view.destroy(); - - super.destroy(); } /** diff --git a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js index 23a4b92baf5..515864c94f6 100644 --- a/packages/ckeditor5-editor-inline/tests/inlineeditorui.js +++ b/packages/ckeditor5-editor-inline/tests/inlineeditorui.js @@ -323,6 +323,18 @@ describe( 'InlineEditorUI', () => { } ); } ); } ); + + it( 'should call parent EditorUI#destroy() first before destroying the view', async () => { + const newEditor = await VirtualInlineTestEditor.create( '' ); + const parentEditorUIPrototype = Object.getPrototypeOf( newEditor.ui.constructor.prototype ); + + const parentDestroySpy = testUtils.sinon.spy( parentEditorUIPrototype, 'destroy' ); + const viewDestroySpy = testUtils.sinon.spy( newEditor.ui.view, 'destroy' ); + + await newEditor.destroy(); + + sinon.assert.callOrder( parentDestroySpy, viewDestroySpy ); + } ); } ); describe( 'element()', () => { From eb5c645eb2b6ada17fe3d64aad1240d3dfc3bb56 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Oct 2022 15:27:52 +0200 Subject: [PATCH 4/5] Tests: Aligned the find and replace manual test to the latest *EditorUI#destroy() strategy. --- packages/ckeditor5-find-and-replace/tests/manual/multiroot.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/ckeditor5-find-and-replace/tests/manual/multiroot.js b/packages/ckeditor5-find-and-replace/tests/manual/multiroot.js index c06ba8e27f7..49cf4f0e668 100644 --- a/packages/ckeditor5-find-and-replace/tests/manual/multiroot.js +++ b/packages/ckeditor5-find-and-replace/tests/manual/multiroot.js @@ -138,6 +138,8 @@ class MultirootEditorUI extends EditorUI { } destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; @@ -146,8 +148,6 @@ class MultirootEditorUI extends EditorUI { } view.destroy(); - - super.destroy(); } _initToolbar() { From 8dc4ea4dd78eeb8ff6ce6f7364d00b0cea3dc6f8 Mon Sep 17 00:00:00 2001 From: Aleksander Nowodzinski Date: Wed, 12 Oct 2022 15:29:10 +0200 Subject: [PATCH 5/5] Docs: Updated editor implementations across documentation (snippets) to make sure they describe *EditorUI implementation that calls EditorUI#destroy() first before destroying the editor view. --- docs/_snippets/examples/multi-root-editor.js | 4 ++-- docs/examples/framework/multi-root-editor.md | 4 ++-- docs/framework/guides/custom-editor-creator.md | 4 ++-- .../docs/_snippets/examples/bootstrap-ui-inner.js | 4 ++-- packages/ckeditor5-ui/docs/examples/custom-ui.md | 4 ++-- packages/ckeditor5-ui/docs/framework/guides/external-ui.md | 4 ++-- 6 files changed, 12 insertions(+), 12 deletions(-) diff --git a/docs/_snippets/examples/multi-root-editor.js b/docs/_snippets/examples/multi-root-editor.js index aac83110c0d..f149d32a50e 100644 --- a/docs/_snippets/examples/multi-root-editor.js +++ b/docs/_snippets/examples/multi-root-editor.js @@ -249,6 +249,8 @@ class MultirootEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; @@ -257,8 +259,6 @@ class MultirootEditorUI extends EditorUI { } view.destroy(); - - super.destroy(); } /** diff --git a/docs/examples/framework/multi-root-editor.md b/docs/examples/framework/multi-root-editor.md index 7bebd8d4a15..627df46cfe1 100644 --- a/docs/examples/framework/multi-root-editor.md +++ b/docs/examples/framework/multi-root-editor.md @@ -264,6 +264,8 @@ class MultirootEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; @@ -272,8 +274,6 @@ class MultirootEditorUI extends EditorUI { } view.destroy(); - - super.destroy(); } /** diff --git a/docs/framework/guides/custom-editor-creator.md b/docs/framework/guides/custom-editor-creator.md index f1aa97e8937..a33a5054afa 100644 --- a/docs/framework/guides/custom-editor-creator.md +++ b/docs/framework/guides/custom-editor-creator.md @@ -238,6 +238,8 @@ class MultirootEditorUI extends EditorUI { * @inheritDoc */ destroy() { + super.destroy(); + const view = this.view; const editingView = this.editor.editing.view; @@ -246,8 +248,6 @@ class MultirootEditorUI extends EditorUI { } view.destroy(); - - super.destroy(); } /** diff --git a/packages/ckeditor5-ui/docs/_snippets/examples/bootstrap-ui-inner.js b/packages/ckeditor5-ui/docs/_snippets/examples/bootstrap-ui-inner.js index 45ccb50b008..480d26685ad 100644 --- a/packages/ckeditor5-ui/docs/_snippets/examples/bootstrap-ui-inner.js +++ b/packages/ckeditor5-ui/docs/_snippets/examples/bootstrap-ui-inner.js @@ -174,14 +174,14 @@ class BootstrapEditorUI extends EditorUI { } destroy() { + super.destroy(); + // Restore the original editor#element. this._elementReplacer.restore(); // Destroy the view. this._view.editable.destroy(); this._view.destroy(); - - super.destroy(); } // This method activates Bold, Italic, Underline, Undo and Redo buttons in the toolbar. diff --git a/packages/ckeditor5-ui/docs/examples/custom-ui.md b/packages/ckeditor5-ui/docs/examples/custom-ui.md index 55a207a2f8a..16587c65a5b 100644 --- a/packages/ckeditor5-ui/docs/examples/custom-ui.md +++ b/packages/ckeditor5-ui/docs/examples/custom-ui.md @@ -187,14 +187,14 @@ class BootstrapEditorUI extends EditorUI { } destroy() { + super.destroy(); + // Restore the original editor#element. this._elementReplacer.restore(); // Destroy the view. this._view.editable.destroy(); this._view.destroy(); - - super.destroy(); } // This method activates Bold, Italic, Underline, Undo and Redo buttons in the toolbar. diff --git a/packages/ckeditor5-ui/docs/framework/guides/external-ui.md b/packages/ckeditor5-ui/docs/framework/guides/external-ui.md index efbb3e3ab9f..e0c8e9e9981 100644 --- a/packages/ckeditor5-ui/docs/framework/guides/external-ui.md +++ b/packages/ckeditor5-ui/docs/framework/guides/external-ui.md @@ -309,14 +309,14 @@ class BootstrapEditorUI extends EditorUI { } destroy() { + super.destroy(); + // Restore the original editor#element. this._elementReplacer.restore(); // Destroy the view. this._view.editable.destroy(); this._view.destroy(); - - super.destroy(); } // This method activates Bold, Italic, Underline, Undo and Redo buttons in the toolbar.