Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#2156 closed Bug (fixed)

[IE] Can't remove styles after calling GetData()

Reported by: pmcelhaney Owned by: alfonsoml
Priority: Normal Milestone: FCKeditor 2.6.5
Component: Core : Styles Version: FCKeditor 2.5 Beta
Keywords: Confirmed IE Review+ Cc: pmcelhaney@…

Description

In IE7, open the first sample. (http://www.fckeditor.net/demo)

Call GetXML() by pasting this code into the address bar.

javascript:alert(FCKeditorAPI.GetInstance('FCKeditor1').GetData());

You should get an alert with the source currently in FCKEditor. Dismiss the alert.

Now try making the text that's bold not bold. It doesn't work. (I've tried selecting the bolded text and clicking the "B" button. I've also tried placing the cursor inside the bolded text an clicking the "B" button. These both work fine prior to calling GetData().)

Attachments (3)

2156_1.patch (2.9 KB) - added by pjhinton 7 years ago.
first candidate fix described in comment 7 of this ticket
2156_2.patch (2.3 KB) - added by pjhinton 7 years ago.
second candidate fix described in comment 7 of this ticket
2156_3.patch (5.8 KB) - added by alfonsoml 7 years ago.
reviewed and merged patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 8 years ago by w.olchawa

  • Keywords Confirmed IE added
  • Summary changed from Can't remove styles after calling GetData() in IE to [IE] Can't remove styles after calling GetData()

Confirmed both in IE6 and IE7. Works fine with FF. Tested on FCKeditor stable 2.6 and the latest SVN.

comment:2 Changed 8 years ago by pmcelhaney

This bug was introduced with the new style system (r774). It works fine in r772.

comment:3 Changed 8 years ago by pmcelhaney

Switching to source view and back clears up the problem.

comment:4 follow-up: Changed 8 years ago by pmcelhaney

I figured out what's happening. GetData() leaves _fckhtmljob attributes behind, which in turn triggers bug #2092.

comment:5 Changed 8 years ago by bmatheny

Any updates on this bug? It's been open for 9 months, it's still valid in 2.6.3, and no commits have yet been made to fix it. Is there a workaround?

comment:6 Changed 8 years ago by pmcelhaney

  • Version changed from FCKeditor 2.6 to FCKeditor 2.5 Beta

Changed the version to 2.5 Beta, since that's the first release in which it appeared. Still valid as of 2.6.4 Beta.

comment:7 in reply to: ↑ 4 Changed 7 years ago by pjhinton

Replying to pmcelhaney:

I figured out what's happening. GetData() leaves _fckhtmljob attributes behind, which in turn triggers bug #2092.

I have reproduced the issue described in the quoted comment. Here is a more detailed discussion of what is happening along with some proposed fixes.

When FCK.GetData() is called, FCKXHtml._AppendNode() gets invoked as part of an operation that copies the editor's internal HTML DOM document to an XML document that will be used to create the XHTML source string. FCKXHtml._AppendNode() adds a property, named _fckxhtmljob to each DOM node object that corresponds to an element. These properties are not removed after the copy is complete.

The presence of these properties has an adverse side effect in Internet Explorer. The function FCK.Style.RemoveFromRange() does not remove a styling element (e.g. strong) when a call to FCKDomTools.HasAttributes() returns true. A true result is obtained because Internet Explorer includes the _fckxhtmljob property in the styling element's attributes NamedNodeMap, and the Attr object corresponding to the _fckxhtmljob attribute has the specified property set to true.

The equivalence between an element object's property and an element's attributes is documented in the MSDN docs for HTML and CSS:

http://msdn.microsoft.com/en-us/library/ms533026(VS.85).aspx#Accessing_Element_Pr

I have attached two patch files that contain separate candidate fixes for this problem. The diffs are based on the current state of trunk sources as this comment was being written.

The first patch defines a new function in fckxhtml_ie.js called FCKXHtml._RemoveXHtmlJobProperties(). It recursively walks a DOM node object and its descendants to remove the _fckxhtmljob properties. It is invoked by FCKXHtml.GetXHTML() once the document copy operation is complete. It is called only when the browser is Internet Explorer.

The second patch modifies FCKDomTools.HasAttributes() in fckdomtools.js so that it skips over all attributes whose names start with _fck when the browser is Internet Explorer. This code builds upon an existing conditional that applies for IE. The use of StartsWith( '_fck') is based on its use in FCKXHtml._AppendAttributes().

To me, the first patch seems like the Right Thing to do because it forces FCKXHtml to clean up after itself. However, the second approach might be safer in situations where FCK.GetData() being called in the background. Theoretically, one could have a scenario when a user is attempting to perform a style change while an interval callback invokes GetData() to perform an autosave on the content.

If you have any comments or questions on these patches, please feel free to e-mail me. My employer, Compendium Blogware, uses FCKeditor in its application and counts itself among those who have been affected by this bug.

Thanks!

Changed 7 years ago by pjhinton

first candidate fix described in comment 7 of this ticket

Changed 7 years ago by pjhinton

second candidate fix described in comment 7 of this ticket

comment:8 Changed 7 years ago by pjhinton

  • Keywords Review? added

Adding 'Review?' keyword to this ticket as I failed to set this value after I uploaded the patches. I didn't realize this was necessary until reviewing the ticket instructions on the wiki.

comment:9 Changed 7 years ago by fredck

  • Milestone set to FCKeditor 2.6.5

This is something we are not being able to reproduce in V3, at least for now (some style features are still to be ported there).

I'm targeting it to the next V2 release, as the fix looks good.

comment:10 Changed 7 years ago by traq

I'm afraid I can't get either of these patches to work on V2. I have applied patch 2 here....

http://207.158.15.141/web/test_cases_modify_content.php?id=2 user: admin password: admin

But still when I use IE6, highlight bold text and try to remove it by clicking the bold button nothing happens.

Also tried patch 1 but that had didn't work either.

Any help with this would be very much appreciated.

Changed 7 years ago by alfonsoml

reviewed and merged patch

comment:11 Changed 7 years ago by alfonsoml

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

The previous patches did have some bugs: the first one wasn't really removing the fake attribute (it used another name and setting it to null doesn't clear it in IE) and the second one broke the else part of the loop, now IE never executed that.

I've merged both patches (so the code has all the protections) and now it's working correctly for me.

Ups, I forgot the thanks in the what's new to Compendium Blogware, I'll add it in the checkin when the patch is reviewed.

comment:12 Changed 7 years ago by alfonsoml

#3479 has been marked as dup

comment:13 Changed 7 years ago by martinkou

  • Keywords Review+ added; Review? removed

comment:14 Changed 7 years ago by alfonsoml

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

Fixed with [3882]

comment:15 Changed 7 years ago by wwalc

Unfortunately along with the new function FCKXHtml._RemoveXHtmlJobProperties() a new bug has been introduced, I have created a separate ticket:

#4642
Invalid HTML causes infinite loop in IE

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