Ticket #6975 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

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

6975.patch (1.7 KB) - added by garry.yao 3 years ago.
6975_2.patch (1.2 KB) - added by garry.yao 3 years ago.

Change History

comment:1 Changed 4 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 4 years ago by krst

  • Keywords IE7 added
  • Status changed from new to confirmed
  • Version set to 3.4
  • Component changed from General to UI : Source View
  • Summary changed from Edit and saving definition lists crashes IE 7 to Edit definition lists crashes IE 7

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 4 years ago by sirshurf

Confirmed for 3.5 and IE8.

6997 is a dublicate please close

comment:4 Changed 4 years ago by wwalc

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 3 years ago by j.swiderski

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

comment:6 Changed 3 years ago by fredck

  • Milestone set to CKEditor 3.6.1

comment:7 Changed 3 years ago by Saare

  • Keywords IE added; IE7 removed

Also occurs in IE8:

fragment.js, line 127 character 4

if ( element.previous !== undefined )

comment:8 Changed 3 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 3 years ago by fredck

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

Changed 3 years ago by garry.yao

comment:10 Changed 3 years ago by garry.yao

  • Status changed from confirmed to review
  • Owner set to garry.yao
  • Component changed from UI : Source View to Core : DTD

comment:11 Changed 3 years ago by fredck

  • Status changed from review to review_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 3 years ago by garry.yao

comment:12 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

comment:13 Changed 3 years ago by Saare

  • Status changed from review to review_passed

comment:14 Changed 3 years ago by garry.yao

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

Fixed with [6978].

comment:15 Changed 3 years ago by meka9233

  • Cc mehrdad.kavousi@… added
  • Summary changed from Edit definition lists crashes IE 7 to Edit 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 3 years ago by j.swiderski (previous) (diff)

comment:16 Changed 3 years ago by ladybug_3777

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 3 years ago by ladybug_3777 (previous) (diff)

comment:17 Changed 3 years ago by ladybug_3777

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 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy