Opened 16 years ago

Closed 15 years ago

Last modified 14 years ago

#2156 closed Bug (fixed)

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

Reported by: Patrick McElhaney Owned by: Alfonso Martínez de Lizarrondo
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 P.J. Hinton 15 years ago.
first candidate fix described in comment 7 of this ticket
2156_2.patch (2.3 KB) - added by P.J. Hinton 15 years ago.
second candidate fix described in comment 7 of this ticket
2156_3.patch (5.8 KB) - added by Alfonso Martínez de Lizarrondo 15 years ago.
reviewed and merged patch

Download all attachments as: .zip

Change History (18)

comment:1 Changed 16 years ago by Wojciech Olchawa

Keywords: Confirmed IE added
Summary: Can't remove styles after calling GetData() in IE[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 16 years ago by Patrick McElhaney

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

comment:3 Changed 16 years ago by Patrick McElhaney

Switching to source view and back clears up the problem.

comment:4 Changed 16 years ago by Patrick McElhaney

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

comment:5 Changed 15 years ago by Blake Matheny

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 15 years ago by Patrick McElhaney

Version: FCKeditor 2.6FCKeditor 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 15 years ago by P.J. Hinton

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 15 years ago by P.J. Hinton

Attachment: 2156_1.patch added

first candidate fix described in comment 7 of this ticket

Changed 15 years ago by P.J. Hinton

Attachment: 2156_2.patch added

second candidate fix described in comment 7 of this ticket

comment:8 Changed 15 years ago by P.J. Hinton

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 15 years ago by Frederico Caldeira Knabben

Milestone: 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 15 years ago by William Echlin

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 15 years ago by Alfonso Martínez de Lizarrondo

Attachment: 2156_3.patch added

reviewed and merged patch

comment:11 Changed 15 years ago by Alfonso Martínez de Lizarrondo

Owner: set to Alfonso Martínez de Lizarrondo
Status: newassigned

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 15 years ago by Alfonso Martínez de Lizarrondo

#3479 has been marked as dup

comment:13 Changed 15 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:14 Changed 15 years ago by Alfonso Martínez de Lizarrondo

Resolution: fixed
Status: assignedclosed

Fixed with [3882]

comment:15 Changed 14 years ago by Wiktor Walc

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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy