Balloon toolbar on destroy doesn't destroy its content #1482

engineering-this · 2018-01-22T13:28:44Z

What is the purpose of this pull request?

Bugfix

Does your PR contain necessary tests?

All patches which change the editor code must include tests. You can always read more
on PR testing,
how to set the testing environment and
how to create tests
in the official CKEditor documentation.

This PR contains

Unit tests
Manual tests

What changes did you make?

I've made balloonToolbar destroy to call also destroy on each of its _items. And added destroy method to richCombo, which removes all listeners from it.

Closes #1477

Comandeer · 2018-08-24T12:24:51Z

CHANGES.md

@@ -24,6 +24,7 @@ Fixed Issues:
 * [#1356](https://github.com/ckeditor/ckeditor-dev/issues/1356): Fixed: [Border parse function](https://docs.ckeditor.com/ckeditor4/docs/#!/api/CKEDITOR.tools.style.parse-method-border) should allow spaces in the color value.
 * [#1426](https://github.com/ckeditor/ckeditor-dev/issues/1426): [IE8-9] Fixed: Missing balloon toolbar background in Kama skin. Thanks to [Christian Elmer](https://github.com/keinkurt)!
 * [#1010](https://github.com/ckeditor/ckeditor-dev/issues/1010): Fixed: CSS `border` shorthand property was incorrectly expanded ignoring `border-color` style.
+* [#1477](https://github.com/ckeditor/ckeditor-dev/issues/1477): Fixed: Balloon toolbar on destroy doesn't destroy its content.


Please link plugin.

Comandeer · 2018-08-24T12:25:14Z

plugins/balloontoolbar/plugin.js

@@ -131,6 +131,7 @@
 	 * @param {CKEDITOR.ui.button/CKEDITOR.ui.richCombo} element Instance of ui element.
 	 */
 	CKEDITOR.ui.balloonToolbar.prototype.addItem = function( name, element ) {
+		// console.log( this._items[ name ] );


What's this? 😛

Comandeer · 2018-08-24T12:25:36Z

plugins/balloontoolbar/plugin.js

-	 * Hides the toolbar and removes it from the DOM.
+	 * Cleares Balloon Toolbar from each menu items.
+	 *
+	 * @since 4.9.0


Please update version tag.

Comandeer · 2018-08-24T12:26:13Z

plugins/richcombo/plugin.js

+			/**
+			 * Removes all listeners from richCombo element.
+			 *
+			 * @since 4.9.0


Please update version tag.

Comandeer · 2018-08-24T12:27:33Z

tests/plugins/balloontoolbar/destroy.js

@@ -0,0 +1,49 @@
+/* bender-tags: balloontoolbar, 1477, 4.9.0 */


Please update version tag.

Comandeer · 2018-08-24T12:33:55Z

tests/plugins/balloontoolbar/manual/destroy.md

@@ -0,0 +1,15 @@
+@bender-ui: collapsed
+@bender-tags: 4.9.0 bug, 1477, balloontoolbar


Please update version tag.

Comandeer · 2018-08-24T12:38:51Z

plugins/balloontoolbar/plugin.js

@@ -169,9 +170,24 @@
 	};

 	/**
-	 * Hides the toolbar and removes it from the DOM.
+	 * Cleares Balloon Toolbar from each menu items.


Some better API docs would be nice, e.g. note that it invokes destroy method of every item.

Comandeer · 2018-08-24T14:08:01Z

plugins/balloontoolbar/plugin.js

+	 *
+	 * @since 4.9.0
+	 */
+	CKEDITOR.ui.balloonToolbar.prototype.clearItems = function() {


I'm wondering if this method should be public. I don't see any real use-cases for it except of destroying the toolbar.

Comandeer · 2018-08-24T14:08:45Z

plugins/richcombo/plugin.js

+			 * @since 4.9.0
+			 */
+			destroy: function() {
+				if ( this._listeners.length ) {


IMO this check is totally unnecessary. forEach loop would work totally fine with empty array.

Comandeer · 2018-08-24T14:10:33Z

tests/plugins/balloontoolbar/manual/destroy.md

+
+## Unexpected:
+
+1. Console throws an error.


If we use console here, it's nice to include opening console inside the procedure. Also it makes this test mobile unfriendly, so it should be ignored in mobile environments.

Comandeer · 2018-09-04T15:51:21Z

tests/plugins/balloontoolbar/destroy.js

+			}
+			panel.destroy();
+
+			items.forEach( function( item ) {


IMO such condition could be nicer expressed using CKEDITOR.tools.array.every:

var listenersDeleted = CKEDITOR.tools.array.every( items, function( item ) { return !( item instanceof CKEDITOR.ui.richCombo ) || item._listeners.length === 0; }; assert.isTrue( listenersDeleted, 'All listeners are deleted' );

Please also note the move of the assertion itself. In fact we are interested in deleting all listeners, not individual ones.

Comandeer · 2018-09-04T15:56:12Z

plugins/balloontoolbar/plugin.js

 	 */
 	CKEDITOR.ui.balloonToolbar.prototype.destroy = function() {
+		for ( var key in this._items ) {


Maybe I'm already a bit biased, but CKEDITOR.tools.forEach + CKEDITOR.tools.objectKeys looks better to me.

Actually I adapted your suggestion in other place. However I think for in loop is fine here.

Comandeer · 2018-09-04T15:56:34Z

plugins/richcombo/plugin.js

@@ -96,6 +96,8 @@ CKEDITOR.plugins.add( 'richcombo', {
 				panelDefinition: panelDefinition,
 				items: {}
 			};
+
+			this._listeners = [];


IMO it'd be nice to add some API docs for this private property.

Comandeer · 2018-09-04T15:57:44Z

tests/plugins/balloontoolbar/manual/destroy.md

@@ -0,0 +1,16 @@
+@bender-ui: collapsed
+@bender-tags: 4.11.0 bug, 1477, balloontoolbar


Missing comma after version tag.

Comandeer

LGTM, there is only one thing I miss: unit test for richcombo. At the moment it's tested only inside balloon toolbar.

Comandeer · 2018-09-06T13:34:07Z

plugins/richcombo/plugin.js

+			 * Array containing richCombos registered listeners.
+			 *
+			 * @private
+			 */


There is missing type of this property. It'd be ideal to use dedicated type (see #2383), but some more generic approach would be also enough.

Comandeer · 2018-09-12T10:59:15Z

plugins/richcombo/plugin.js

+			 * @type {Array}
+			 * @private
+			 */
+			this._listeners = [];


I didn't notice it earlier 😐 but we don't need new _-variable as we already have _ namespace here to put private stuff inside. In such case we could even omit the docs. Please update the code from _listeners to _.listeners. And sorry for not noticing it earlier 😞

Comandeer · 2018-09-14T10:28:52Z

tests/plugins/balloontoolbar/destroy.js

+	};
+
+	var tests = {
+		'test destroy toolbar with richcombo': function() {


This test does not test anything. Array passed to CKEDITOR.tools.array.every is empty, therefore test always passes.

Comandeer · 2018-09-14T10:29:27Z

tests/plugins/balloontoolbar/destroy.js

+			panel.destroy();
+
+			var listenersDeleted = CKEDITOR.tools.array.every( items, function( item ) {
+				return !( item instanceof CKEDITOR.ui.richCombo ) || item._listeners.length === 0;


I discovered it by this typo: ~~item._listeners~~ instead of item._.listeners.

Comandeer · 2018-09-14T10:32:33Z

tests/plugins/richcombo/richcombo.js

@@ -38,5 +38,21 @@ bender.test( {
 			anchorEl = CKEDITOR.document.getById( 'cke_' + combo.id ).findOne( 'a' );

 		assert.areEqual( anchorEl.getAttribute( 'aria-haspopup' ), 'listbox' );
+	},
+	'test destroy removes combo listeners': function() {


Please add issue reference.

Comandeer · 2018-09-14T10:33:57Z

plugins/richcombo/plugin.js

+			 * @type {Array}
+			 * @private
+			 */
+			this._.listeners = [];


If we add it to _ namespace, API docs are not needed. It would be even weird to document only one property of it. Let's just add listeners array to the object literal above.

I guess every piece of extra information might be helpful in future especially if it's already written, but I guess this property is so obvious we can drop docs, and move it to the object literal.

Comandeer

LGTM!

mlewand added the review:easy Pull requests that can be reviewed by a Junior Developer before being reviewed by the Reviewer. label Aug 16, 2018

Comandeer self-requested a review August 23, 2018 15:29

Comandeer self-assigned this Aug 23, 2018

CKEditorBot unassigned Comandeer Aug 23, 2018

mlewand changed the title ~~Bugfix: Balloon toolbar on destroy doesn't destroy its content~~ Balloon toolbar on destroy doesn't destroy its content Aug 24, 2018

Comandeer self-assigned this Aug 24, 2018

Comandeer requested changes Aug 24, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Aug 24, 2018

engineering-this self-assigned this Aug 31, 2018

engineering-this force-pushed the t/1477 branch from 9996246 to 19d88d2 Compare August 31, 2018 14:02

engineering-this requested a review from Comandeer August 31, 2018 14:09

CKEditorBot unassigned engineering-this Aug 31, 2018

Comandeer self-assigned this Sep 4, 2018

Comandeer requested changes Sep 4, 2018

View reviewed changes

engineering-this assigned engineering-this and unassigned Comandeer Sep 5, 2018

engineering-this force-pushed the t/1477 branch from 19d88d2 to 2903d65 Compare September 6, 2018 10:01

engineering-this requested a review from Comandeer September 6, 2018 10:35

engineering-this removed their assignment Sep 6, 2018

Comandeer self-assigned this Sep 6, 2018

Comandeer requested changes Sep 6, 2018

View reviewed changes

Comandeer removed their assignment Sep 6, 2018

engineering-this self-assigned this Sep 6, 2018

engineering-this force-pushed the t/1477 branch 2 times, most recently from 52d55c9 to d18afa9 Compare September 6, 2018 14:51

engineering-this removed their assignment Sep 6, 2018

engineering-this requested a review from Comandeer September 6, 2018 15:35

Comandeer self-assigned this Sep 11, 2018

Comandeer requested changes Sep 12, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 12, 2018

engineering-this added 7 commits September 13, 2018 18:04

Tests: dropping 'for in' loop for 'array.map'.

67090a7

Added docs entry.

536da83

Tests: typo in @bender-tags.

188edb8

Added type to docs entry.

554495b

Tests: added tc.

ec165d1

Moved '_listeners' to '_.listeners'.

ded24d8

Test: changed after api changes.

11a1214

engineering-this force-pushed the t/1477 branch from 7c01738 to 11a1214 Compare September 13, 2018 16:04

engineering-this requested review from Comandeer and removed request for Comandeer September 13, 2018 16:05

engineering-this self-assigned this Sep 13, 2018

engineering-this requested a review from Comandeer September 13, 2018 16:25

CKEditorBot unassigned engineering-this Sep 13, 2018

Comandeer self-assigned this Sep 14, 2018

Comandeer requested changes Sep 14, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 14, 2018

engineering-this self-assigned this Sep 14, 2018

engineering-this added 3 commits September 14, 2018 17:29

Tests: corrected typo resulting in allways passing test.

f7e7f93

Tests: added issue reference.

cd452df

Moved '_.listeners' to '_' object literal.

a5a5b5a

engineering-this requested a review from Comandeer September 14, 2018 15:38

CKEditorBot unassigned engineering-this Sep 14, 2018

Comandeer self-assigned this Sep 15, 2018

Comandeer added 2 commits September 15, 2018 13:50

Fix code style issues.

3ab4299

Update changelog entry.

6caa707

Comandeer approved these changes Sep 15, 2018

View reviewed changes

CKEditorBot unassigned Comandeer Sep 15, 2018

Comandeer merged commit f7ee77b into major Sep 15, 2018

CKEditorBot deleted the t/1477 branch September 15, 2018 11:53

Comandeer mentioned this pull request Oct 9, 2018

Balloon toolbar should destroy its content properly #1477

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Balloon toolbar on destroy doesn't destroy its content #1482

Balloon toolbar on destroy doesn't destroy its content #1482

engineering-this commented Jan 22, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Aug 24, 2018

Comandeer Sep 4, 2018

Comandeer Sep 4, 2018

engineering-this Sep 6, 2018

Comandeer Sep 4, 2018

Comandeer Sep 4, 2018

Comandeer left a comment

Comandeer Sep 6, 2018

Comandeer Sep 12, 2018

Comandeer Sep 14, 2018

Comandeer Sep 14, 2018

Comandeer Sep 14, 2018

Comandeer Sep 14, 2018

engineering-this Sep 14, 2018

Comandeer left a comment

		@@ -0,0 +1,49 @@
		/* bender-tags: balloontoolbar, 1477, 4.9.0 */

		@@ -0,0 +1,15 @@
		@bender-ui: collapsed
		@bender-tags: 4.9.0 bug, 1477, balloontoolbar

		@@ -0,0 +1,16 @@
		@bender-ui: collapsed
		@bender-tags: 4.11.0 bug, 1477, balloontoolbar

Balloon toolbar on destroy doesn't destroy its content #1482

Balloon toolbar on destroy doesn't destroy its content #1482

Conversation

engineering-this commented Jan 22, 2018

What is the purpose of this pull request?

Does your PR contain necessary tests?

This PR contains

What changes did you make?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comandeer left a comment

Choose a reason for hiding this comment