Opened 8 years ago

Closed 8 years ago

#4552 closed Bug (fixed)

Float panel menu will not respond after the editor instance is re-created.

Reported by: markuspaek Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.1
Component: UI : Floating Panel Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Review+ Cc: matti.jarvinen@…, jitendra.kumar.jitu@…, jkavanag@…

Description (last modified by Frederico Caldeira Knabben)

check '_samples/ajax.html' (this is from your current distribution)

  1. Create an editor instance.
  2. Click the toolbar buttons such as 'text color' or 'font size' and select something in the float panel menu.
  3. Destroy the editor instance.
  4. Create a new editor instance.
  5. Then the toolbar buttons do not respond when clicked.

Firefox 3.5.3
Windows XP

Attachments (5)

4552.patch (790 bytes) - added by Garry Yao 8 years ago.
4552_2.patch (3.2 KB) - added by Garry Yao 8 years ago.
4552_3.patch (3.2 KB) - added by Garry Yao 8 years ago.
4552_4.patch (3.2 KB) - added by Garry Yao 8 years ago.
4552_5.patch (3.1 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 8 years ago by Matti Järvinen

Cc: matti.jarvinen@… added
Priority: NormalHigh

I noticed the same with IE8 and FF 3.5.3. With floatpanel buttons mentioned and format and style selections.

It seems to work the first time you use the button. On a second time with different instance floatpanel buttons that are unused work but after you have used them and destroy and create the instance again those buttons don't work anymore.

Is the floating selection created once (when shown) and instance of it saved in a variable that isn't cleared with editor.destroy() so it isn't recreated on a new instance?

Error I get is this.$ is undefined

Practically this is the showstoppper.

comment:2 Changed 8 years ago by Matti Järvinen

Component: GeneralUI : Floating Panel

related to #4241

panels get destroyed right but, panels[key] exists every time after the creation of the first editor with same parameters ( CKEDITOR.replace('editorField'); ) even though CKEDITOR.instances.editorField.destroy() is called.

plugins/floatpanel/plugin.js getPanel line 16-

var panel = panels[key];
...

return panel;

later on $ : function

...

// Register panels to editor for easy destroying ( #4241 ).
editor.panels ? editor.panels.push( element ) : editor.panels = [ element ];

themes/default/theme.js destroy line 156

var container = editor.container,
panels = editor.panels;

...
for( var i = 0 ; panels && i < panels.length ; i++ )
	panels[ i ].remove();

Changed 8 years ago by Garry Yao

Attachment: 4552.patch added

comment:3 Changed 8 years ago by Garry Yao

Keywords: Confirmed added
Milestone: CKEditor 3.1
Owner: set to Garry Yao
Priority: HighNormal
Status: newassigned
Version: 3.0.1SVN (CKEditor)

@matti: Thanks for the wonderful bug reporting and issue catching which helps us to locate the bug in a second.

comment:4 Changed 8 years ago by Garry Yao

Keywords: Review? added

comment:5 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

The proposed patch will not work if you have more than one instance of the editor in the page.

comment:6 Changed 8 years ago by Garry Yao

After discuss with Fred, the patch break the panel sharing mechanism between editors, so another solution should be found.

Changed 8 years ago by Garry Yao

Attachment: 4552_2.patch added

comment:7 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

It's actually a regression of [4167] from [4241].

comment:8 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed
  • Let's call the event "instanceDestroyed" instead of "instanceDestroy", just to be aligned with the "instanceCreated" name.
  • editor.panels is not needed anymore. We can use only the private panels for all needs (hide and destroy them in the same loop).
  • A "debugger" line remained in the code.
  • The coding style is bad is several places. It deserves attention and correction.

Changed 8 years ago by Garry Yao

Attachment: 4552_3.patch added

comment:9 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Corrected the above mentioned issue.

comment:10 Changed 8 years ago by Frederico Caldeira Knabben

Description: modified (diff)

comment:11 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

That's a very good fix.

My only note would be that we don't need the "editor.panel" trick at line 42. You can unconditionally hide the panels at line 346. It's a small simplification as I doubt we'll have editors being destroyed in the page while the use is attempting to use toolbar combos in other instances.

Changed 8 years ago by Garry Yao

Attachment: 4552_4.patch added

comment:12 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:13 Changed 8 years ago by Frederico Caldeira Knabben

#4624 has been marked as DUP.

comment:14 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review- added; Review? removed

There are still details that passed unobserved, all in the _source/plugins/floatpanel/plugin.js file:

  • At line 42, "panel.editor" is not needed and not used in the code.
  • The listener function at line 330 doesn't need the "evt" and "editor" variables anymore.
  • Instead of panels[ i ].destroy() we can have panel.destroy().
  • We don't need a delete panels[ i ] call for each panel; that variable can be reset at the end of the function with isLastInstance && ( panels = {} ).
  • The panel.element.hide() call can be inside an else instead of being called in all cases.

Also, I've just found out that the "instanceDestroyed" event is being fired improperly it must be like the following:

CKEDITOR.fire( 'instanceDestroyed', null, this ); 

Changed 8 years ago by Garry Yao

Attachment: 4552_5.patch added

comment:15 Changed 8 years ago by Garry Yao

Keywords: Review? added; Review- removed

Thanks for the point out, previous was quite a poor patch;)
Ticket Test added at :
http://ckeditor.t/tt/4552/1.html.

comment:16 Changed 8 years ago by Frederico Caldeira Knabben

Keywords: Review+ added; Review? removed

comment:17 Changed 8 years ago by Frederico Caldeira Knabben

#4641 has been marked as DUP.

comment:18 in reply to:  15 Changed 8 years ago by Jitendra Kumar

Cc: jitendra.kumar.jitu@… added

Ticket Test added at :
http://ckeditor.t/tt/4552/1.html.

This ticket test link is not working

comment:19 Changed 8 years ago by Joe Kavanagh

Cc: jkavanag@… added
Keywords: IBM added

comment:20 Changed 8 years ago by Damian

Priority: NormalHigh

comment:21 Changed 8 years ago by Garry Yao

Priority: HighNormal
Resolution: fixed
Status: assignedclosed

Fixed with [4440].

Note: See TracTickets for help on using tickets.
© 2003 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy