Let's make sure we destroy all keystroke handlers

[oleq]

Provide a description of the task

A follow-up of #6087. Precisely ckeditor/ckeditor5-ui#552 (comment).

See how many instances are created:

14 results - 14 files

font • src/ui/colortableview.js:
  77: 		this.keystrokes = new KeystrokeHandler();

🖼 image • src/imagetextalternative/ui/textalternativeformview.js:
  55: 		this.keystrokes = new KeystrokeHandler();

🔗 link • src/ui/linkactionsview.js:
  54: 		this.keystrokes = new KeystrokeHandler();

🔗 link • src/ui/linkformview.js:
  64: 		this.keystrokes = new KeystrokeHandler();

media-embed • src/ui/mediaformview.js:
  58: 		this.keystrokes = new KeystrokeHandler();

table • src/tablecellproperties/ui/tablecellpropertiesview.js:
  186: 		this.keystrokes = new KeystrokeHandler();

table • src/tableproperties/ui/tablepropertiesview.js:
  161: 		this.keystrokes = new KeystrokeHandler();

🕹 ui • src/focuscycler.js:
  42:  *		const keystrokeHandler = new KeystrokeHandler();

🕹 ui • src/colorgrid/colorgridview.js:
  72: 		this.keystrokes = new KeystrokeHandler();

🕹 ui • src/dropdown/dropdownview.js:
  176: 		this.keystrokes = new KeystrokeHandler();

🕹 ui • src/dropdown/button/splitbuttonview.js:
  97: 		this.keystrokes = new KeystrokeHandler();

🕹 ui • src/list/listview.js:
  52: 		this.keystrokes = new KeystrokeHandler();

🕹 ui • src/toolbar/toolbarview.js:
  100: 		this.keystrokes = new KeystrokeHandler();

utils • src/keystrokehandler.js:
  29:  *				this.keystrokes = new KeystrokeHandler();

and so few are destroyed 😛

1 result - 1 file

core • src/editor/editor.js:
  269: 				this.keystrokes.destroy();

To stay on the safe side, let's balance that.

[Reinmar]

I'm not sure about this. Is it really necessary? In which cases we are forced to do that?

Once a UI component is destroyed it's also detached from the DOM (or should be). If the listener was added to the element that was removed from the DOM, then removing this listener is not necessary.

How would a case in which we really need to destroy a keystroke handler look? Do we have such cases?

[jodator]

If the listener was added to the element that was removed from the DOM, then removing this listener is not necessary.

I don't recall if it did cause problems in #1341. However, we still haven't addressed new issues (#5954) and such DOM references might cause problems. Any listener attached to DOM might hold reference to the editor (by scope).

But, in most cases, removing it from DOM should be safe - so proper UI destroy() should cover that.