Opened 10 years ago

Closed 10 years ago

#2765 closed Bug (fixed)

CKEditor is leaving cke_expando indices in the HTML source in IE

Reported by: Martin Kou Owned by: Garry Yao
Priority: Must have (possibly next milestone) Milestone: CKEditor 3.0
Component: Core : Output Data Version: SVN (FCKeditor) - Retired
Keywords: Confirmed IE Review? Cc:

Description

To reproduce:

  1. Open replacebyclass.html sample in IE6.
  2. Click anywhere in the paragraph within the editing area.
  3. Switch to source mode.
  4. You get something like this, note the _cke_expando attribute - it shouldn't be there:
    <p _cke_expando="125">
    	This is some <strong _cke_expando="124">sample text</strong>. You are using <a href="http://www.fckeditor.net/">FCKeditor</a>.</p>
    

Attachments (7)

element.js.diff (617 bytes) - added by Garry Yao 10 years ago.
2765.patch (617 bytes) - added by Garry Yao 10 years ago.
Name convention revised
2765.2.patch (6.2 KB) - added by Garry Yao 10 years ago.
code style revised
2765_3.patch (600 bytes) - added by Garry Yao 10 years ago.
2765_4.patch (551 bytes) - added by Garry Yao 10 years ago.
2765_5.patch (2.4 KB) - added by Garry Yao 10 years ago.
2765_6.patch (2.8 KB) - added by Garry Yao 10 years ago.
Fix fakeobject scope

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 years ago by Artur Formella

Keywords: Confirmed IE added

comment:2 Changed 10 years ago by Martin Kou

Priority: NormalHigh

Putting to high priority to get it fixed before the beta release.

comment:3 Changed 10 years ago by Garry Yao

Owner: set to Garry Yao
Status: newassigned

Changed 10 years ago by Garry Yao

Attachment: element.js.diff added

comment:4 Changed 10 years ago by Garry Yao

Keywords: Review? added

Changed 10 years ago by Garry Yao

Attachment: 2765.patch added

Name convention revised

Changed 10 years ago by Garry Yao

Attachment: 2765.2.patch added

code style revised

comment:5 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Same reasons as #2764, except the tabs are correct in this patch.

  1. The URF-8 BOM character is making the patch non-usable.
  2. Please don't reformat the length of comments.

Changed 10 years ago by Garry Yao

Attachment: 2765_3.patch added

comment:6 Changed 10 years ago by Garry Yao

Thank you Martin, Plz help me to check again.

Changed 10 years ago by Garry Yao

Attachment: 2765_4.patch added

comment:7 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:8 Changed 10 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:9 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [2951].

comment:10 Changed 10 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

Require to fix fakeobject html also.

Changed 10 years ago by Garry Yao

Attachment: 2765_5.patch added

comment:11 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review+ removed

Changed 10 years ago by Garry Yao

Attachment: 2765_6.patch added

Fix fakeobject scope

comment:12 Changed 10 years ago by Martin Kou

Keywords: Review- added; Review? removed

Ok... yes in the last review I missed out line 116 in _source/core/htmlparser/element.js. Thanks for the correction.

_source/plugins/fakeobjects/plugin.js - there's actually a reason for putting the things like copyParser outside of init(). Function object construction is a relatively slow operation in JavaScript, so the frequency of function construction should be minimized - functional programming techniques, while powerful, should only be used when needed.

Now the reason for putting copyParser(), makeTagOpenerHtml(), etc. outside of init() is because if we place them outside, those function would be constructed only once, when the plugin.js file is loaded. If those functions are placed inside init(), then every init() call would trigger function construction, which slows down editor instantiation by a bit.

But why are we putting a function on the outside and execute it immediately? Without the outer function, anything defined in the plugin.js file (e.g. copyParser) would end up in the global namespace. Even though we can add them as private variables via _ attributes, many of the things declared in the plugin.js file are not used anywhere outside of the plugin itself. So it would make sense to create a local function scope to put those things in. Thus an outer function is defined to avoid polluting the global namespace, and avoid adding unnecessary properties under other global objects.

Not sure about the changes in htmldataprocessor plugin, Fred didn't leave any documentations about why the plugin is designed like that.

comment:13 in reply to:  12 Changed 10 years ago by Garry Yao

Replying to martinkou:

_source/plugins/fakeobjects/plugin.js - there's actually a reason for putting the things like copyParser outside of init(). Function object construction is a relatively slow operation in JavaScript, so the frequency of function construction should be minimized - functional programming techniques, while powerful, should only be used when needed.

yes,Martin,closures is more expensive than OO-style of object construction,sometimes it even introduce obscure scope errors, we need to reserve closures which refer to the editor instance in our plug-ins' init function. The latest revised patch didn't use closure.

comment:14 Changed 10 years ago by Garry Yao

My reason for modified _source/plugins/fakeobjects/plugin.js and _source/plugins/htmldataprocessor/plugin.js is that fakeobjects transformation has been performed after the normal html processing:

                                                                // Get the HTML version of the data.
								if ( editor.dataProcessor )
									data = editor.dataProcessor.toHtml( data );

                                                                ...

								// Protect src or href attributes.
								data = protectUrls( data );

								// Replace tags with fake elements.
								if ( editor.fakeobjects )
									data = editor.fakeobjects.protectHtml( data );

So during the restore of fakeobject to real dom elements, some expando attrs were attached again, fakeobject is required to again use it's own editor's html-processor to clean it up, to make sure that even the fake objects's transformed output HTML were correctly formatted.

Are there other reasons for this patch's failure?

comment:15 Changed 10 years ago by Garry Yao

Keywords: Review? added; Review- removed

comment:16 Changed 10 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: reopenedclosed

This one has been fixed with [3043].

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