Ticket #2368 (closed Bug: fixed)

Opened 6 years ago

Last modified 5 years ago

IE: Protected source for comments is broken

Reported by: fredck Owned by: martinkou
Priority: Normal Milestone: FCKeditor 2.6.3
Component: General Version: SVN (FCKeditor) - Retired
Keywords: Confirmed IE Review+ Cc: Scott

Description

The fix for #2263 is breaking source protection in IE SVN nightly.

To replicate:

  1. Click source.
  2. Add <!-- test --> to the source.
  3. Press Source again to revert to design mode.
  4. Press Source again, and you have a "null" cannot be null.

Attachments

2368.patch (1.5 KB) - added by martinkou 6 years ago.
2368_2.patch (1.6 KB) - added by martinkou 6 years ago.

Change History

comment:1 Changed 6 years ago by fredck

The following is a proposal fix for the part of the code that is throwing the error:

Index: D:/FCKeditor/SVN/FCKeditor/local/editor/_source/internals/fck.js
===================================================================
--- D:/FCKeditor/SVN/FCKeditor/local/editor/_source/internals/fck.js    (revision 2221)
+++ D:/FCKeditor/SVN/FCKeditor/local/editor/_source/internals/fck.js    (working copy)
 -1111,8 +1111,10 @@
                {
                        if ( typeof( this.Elements[i] ) == 'string' )
                        {
-                               node.innerHTML = this.Elements[i] ;
-                               this.Elements[i] = node.firstChild ;
+                               // For IE, we need to do this trick to avoid loosing comment
+                               // only HTML.
+                               node.innerHTML = '<div>&nbsp;</div>' + this.Elements[i] ;
+                               this.Elements[i] = node.removeChild( node.childNodes[1] ) ;
                        }
                }
        }

But then, there are still other things to get fixed to make it work.

comment:2 Changed 6 years ago by fredck

Sorry, Trac didn't like my inline diff... here you have it: http://pastebin.com/f637ed2bf

comment:3 Changed 6 years ago by martinkou

  • Owner set to martinkou
  • Status changed from new to assigned

comment:4 Changed 6 years ago by martinkou

Ok... apart from the weird IE bug in which you can't put a bare comment inside a node and expect it to appear in the DOM... there's also the issue that FCKeditor puts protected code as plain text into FCKTempBin, and those plain text should never be converted to DOM nodes at all.

Proposing a patch to fix both issues.

Changed 6 years ago by martinkou

comment:5 Changed 6 years ago by martinkou

  • Keywords Review? added

comment:6 Changed 6 years ago by fredck

  • Keywords Review- added; Review? removed

The proposed patch is ok. In my previously proposed patch I got also the chance with this fix to use removeChild to retrieve the final element so we release the reference to the temporary node. Couldn't it be used here too? (basically node.firstChild.removeChild( node.firstChild.lastChild ) at line 1118)

Changed 6 years ago by martinkou

comment:7 Changed 6 years ago by martinkou

  • Keywords Review? added; Review- removed

comment:8 Changed 6 years ago by fredck

  • Keywords Review+ added; Review? removed

comment:9 Changed 6 years ago by martinkou

  • Status changed from assigned to closed
  • Resolution set to fixed

Fixed with [2258].

Click here for more info about our SVN system.

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