Skip to content

Commit

Permalink
Merge pull request #10803 from ckeditor/ck/5954-memory-leaks
Browse files Browse the repository at this point in the history
Internal: Made sure `KeystrokeHandler` and `FocusTracker` instances are destroyed across the UI in the project to avoid memory leaks. Closes #10749. Closes #6561.

Tests (ckeditor5): Added a semi-automated manual test to investigate memory leaks in the editor (see #10749).
  • Loading branch information
Reinmar authored Nov 5, 2021
2 parents 9dc7bd8 + e8b0d2d commit c661165
Show file tree
Hide file tree
Showing 33 changed files with 940 additions and 0 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -353,6 +353,16 @@ export default class FindAndReplaceFormView extends View {
this._initKeystrokeHandling();
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this._focusTracker.destroy();
this._keystrokes.destroy();
}

/**
* Focuses the fist {@link #_focusables} in the form.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -673,6 +673,24 @@ describe( 'FindAndReplaceFormView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view._focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view._keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'focus()', () => {
it( 'should focus the #findInputView', () => {
const spy = sinon.spy( view._findInputView, 'focus' );
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-font/src/ui/colortableview.js
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,16 @@ export default class ColorTableView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Appends {@link #staticColorsGrid} and {@link #documentColorsGrid} views.
*/
Expand Down
18 changes: 18 additions & 0 deletions packages/ckeditor5-font/tests/ui/colortableview.js
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,24 @@ describe( 'ColorTableView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( colorTableView.focusTracker, 'destroy' );

colorTableView.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( colorTableView.keystrokes, 'destroy' );

colorTableView.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'focus tracker', () => {
it( 'should focus first child of colorTableView in DOM', () => {
const spy = sinon.spy( colorTableView._focusCycler, 'focusFirst' );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -203,6 +203,16 @@ export default class ImageInsertPanelView extends View {
}, { priority: 'high' } );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Returns a view of the integration.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,16 @@ export default class TextAlternativeFormView extends View {
} );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Creates the button view.
*
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,24 @@ describe( 'ImageUploadPanelView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'focus()', () => {
it( 'should focus on the first integration', () => {
const spy = sinon.spy( view.getIntegration( 'insertImageViaUrl' ), 'focus' );
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,24 @@ describe( 'TextAlternativeFormView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'DOM bindings', () => {
describe( 'submit event', () => {
it( 'should trigger submit event', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-link/src/ui/linkactionsview.js
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,16 @@ export default class LinkActionsView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Focuses the fist {@link #_focusables} in the actions.
*/
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-link/src/ui/linkformview.js
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,16 @@ export default class LinkFormView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Focuses the fist {@link #_focusables} in the form.
*/
Expand Down
18 changes: 18 additions & 0 deletions packages/ckeditor5-link/tests/ui/linkactionsview.js
Original file line number Diff line number Diff line change
Expand Up @@ -199,6 +199,24 @@ describe( 'LinkActionsView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'focus()', () => {
it( 'focuses the #previewButtonView', () => {
const spy = sinon.spy( view.previewButtonView, 'focus' );
Expand Down
18 changes: 18 additions & 0 deletions packages/ckeditor5-link/tests/ui/linkformview.js
Original file line number Diff line number Diff line change
Expand Up @@ -171,6 +171,24 @@ describe( 'LinkFormView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'DOM bindings', () => {
describe( 'submit event', () => {
it( 'should trigger submit event', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-media-embed/src/ui/mediaformview.js
Original file line number Diff line number Diff line change
Expand Up @@ -210,6 +210,16 @@ export default class MediaFormView extends View {
}, { priority: 'high' } );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Focuses the fist {@link #_focusables} in the form.
*/
Expand Down
18 changes: 18 additions & 0 deletions packages/ckeditor5-media-embed/tests/ui/mediaformview.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,24 @@ describe( 'MediaFormView', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'DOM bindings', () => {
describe( 'submit event', () => {
it( 'should trigger submit event', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -430,6 +430,16 @@ export default class TableCellPropertiesView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Focuses the fist focusable field in the form.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -379,6 +379,16 @@ export default class TablePropertiesView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Focuses the fist focusable field in the form.
*/
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -760,6 +760,24 @@ describe( 'table cell properties', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'DOM bindings', () => {
describe( 'submit event', () => {
it( 'should trigger submit event', () => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -693,6 +693,24 @@ describe( 'table properties', () => {
} );
} );

describe( 'destroy()', () => {
it( 'should destroy the FocusTracker instance', () => {
const destroySpy = sinon.spy( view.focusTracker, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );

it( 'should destroy the KeystrokeHandler instance', () => {
const destroySpy = sinon.spy( view.keystrokes, 'destroy' );

view.destroy();

sinon.assert.calledOnce( destroySpy );
} );
} );

describe( 'DOM bindings', () => {
describe( 'submit event', () => {
it( 'should trigger submit event', () => {
Expand Down
10 changes: 10 additions & 0 deletions packages/ckeditor5-ui/src/colorgrid/colorgridview.js
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,16 @@ export default class ColorGridView extends View {
this.keystrokes.listenTo( this.element );
}

/**
* @inheritDoc
*/
destroy() {
super.destroy();

this.focusTracker.destroy();
this.keystrokes.destroy();
}

/**
* Fired when the `ColorTileView` for the picked item is executed.
*
Expand Down
Loading

0 comments on commit c661165

Please sign in to comment.