Opened 13 years ago

Closed 13 years ago

Last modified 13 years ago

#6975 closed Bug (fixed)

Edit definition lists crashes IE 7,8,9

Reported by: oyvihatl Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.1
Component: Core : DTD Version: 3.4
Keywords: IE Cc: mehrdad.kavousi@…

Description

Steps to reproduce:

Go to: http://nightly.ckeditor.com/6325/_samples/output_xhtml.html Or: http://ckeditor.com/demo

  1. Press "Source"
  1. Copy into editor:
    <dl>
      <dt>Coffee</dt>
      <dd>- black hot drink</dd>
      <dt>Milk</dt>
      <dd>- white cold drink</dd>
    </dl>
    
  2. Press "Source" to view
  1. Press "Source" again to edit

Notice:

It works with only <dt>'s inside <dl>:

<dl>
  <dt>Coffee</dt>
  <dt>Milk</dt>
</dl>

Browser name and OS:

IE 7.0.5730.11 - MS Windows Server 2003

Attachments (2)

6975.patch (1.7 KB) - added by Garry Yao 13 years ago.
6975_2.patch (1.2 KB) - added by Garry Yao 13 years ago.

Download all attachments as: .zip

Change History (19)

comment:1 Changed 13 years ago by oyvihatl

The title should say just "Edit definition lists crashes IE 7", as saving only occurs in our CMS.

comment:2 Changed 13 years ago by Krzysztof Studnik

Component: GeneralUI : Source View
Keywords: IE7 added
Status: newconfirmed
Summary: Edit and saving definition lists crashes IE 7Edit definition lists crashes IE 7
Version: 3.4

Error says "Stack overflow at line 27"
Behaviour confirmed for CKEditor 3.5 and IE7.
In previous versions of Editor, order of items is lost when user is switching back to Source mode, next, the <dt>Coffee</dt> part is gone when Source button is pressed third time.

comment:3 Changed 13 years ago by Alex (Shurf) Frenkel

Confirmed for 3.5 and IE8.

6997 is a dublicate please close

comment:4 Changed 13 years ago by Wiktor Walc

IE7 enters into an infinite loop here:

parser.onTagOpen.apply( this, arguments );

It looks like we have invalid code in fragment.js:

else if ( tagName in CKEDITOR.dtd.$listItem )
{
	parser.onTagOpen( 'ul', {} );
	addPoint = currentNode;
	reApply = true;
}

Note that $listItem is one of the following {dd:1,dt:1,li:1} and we're using ul for all of them.

comment:5 Changed 13 years ago by Jakub Ś

The same thing as on IE7 happens on IE6. I could reproduce it from version 3.4.2

comment:6 Changed 13 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.1

comment:7 Changed 13 years ago by Sa'ar Zac Elias

Keywords: IE added; IE7 removed

Also occurs in IE8:

fragment.js, line 127 character 4

if ( element.previous !== undefined )

comment:8 Changed 13 years ago by Garry Yao

As definition list is not *officially* supported before in the editor, this ticket would be a good start for it ;)

comment:9 Changed 13 years ago by Frederico Caldeira Knabben

Right now, let's simply not break because of it, maintaining the HTML structure.

Changed 13 years ago by Garry Yao

Attachment: 6975.patch added

comment:10 Changed 13 years ago by Garry Yao

Component: UI : Source ViewCore : DTD
Owner: set to Garry Yao
Status: confirmedreview

comment:11 Changed 13 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The fix is ok, but CKEDITOR.dtd is a kind of a "pure" thing in the API, which should be trustable enough and without hacks. Adding $optionalClose to it, implies this new list to have elements like "p", "li" and others.

As we need this check at the fragment code only, let's simply have this element list directly there.

Changed 13 years ago by Garry Yao

Attachment: 6975_2.patch added

comment:12 Changed 13 years ago by Garry Yao

Status: review_failedreview

comment:13 Changed 13 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

comment:14 Changed 13 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [6978].

comment:15 Changed 13 years ago by meka9233

Cc: mehrdad.kavousi@… added
Summary: Edit definition lists crashes IE 7Edit definition lists crashes IE 7,8,9

The fix did not workd for me, I send the content of a form to CKEDITOR that has tags like this:

<dl>
 <dt><dd>text<dd><dt>
<dt><dd>text<dd><dt>
<dt><dd>text<dd><dt>
</dl>

and as you see the <dt> tags are not closed as they should. The ckeditor cant show the content of the form mailed to this editor and make a stack overflow at line: 26. Is there any way to fix it?

Regards

Last edited 13 years ago by Jakub Ś (previous) (diff)

comment:16 Changed 13 years ago by Jennifer

I was directed to this page from bug #6151 http://dev.ckeditor.com/ticket/6151

I'm having the exact problem described in ticket #6151, (that dt dd flips position in IE7 and IE8 in compt. mode) but that problem is DIFFERENT than the problem being described here.

Unfortunatly bug #6151 is now closed and the resolution is set to "worksforme" (Works For Me) and the comment saya: Probably fixed at #6975.

Probably fixed here? Resolution set to worksforme?

I'm going to apply this fix and see if it "worksforme" too, but I just wanted to point out this odd method of bug closure on the other page. I wanted to add this comment now in case commenting gets turned off before I can check.

Last edited 13 years ago by Jennifer (previous) (diff)

comment:17 Changed 13 years ago by Jennifer

My current version of ckeditor, 3.3.2, is too far behind for me to apply this patch directly. I did some comparing of my fragment.js file to the new fragment.js in version 3.6.1 and although they are different and there are some new parameters being passed to some of the functions, I *think* I can replace the entire file without causing too much harm. (crazy I know, but so much has changed I think trying to hack in a solution would be worse)

This one will have to stay in the testing phase for a WHILE, but may be a workable solution until we are able to do a full upgrade.

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