Ticket #7494 (closed Bug: fixed)

Opened 3 years ago

Last modified 6 months ago

Getting Unresponsive Script Error when we try to paste Numbered/Bulleted lists with one more than one level

Reported by: satya Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.5.3
Component: Plugin : Paste from Word Version: 3.5.3
Keywords: IBM Cc: damo, jamescun, tmonahan

Description

To reproduce the defect:

  1. Open Ajax sample, Copy Numbered/Bulleted list from the attached doc and open Paste from Word dialog.
  1. Paste the content in to the dialog and press OK button.

Expected Result:

Numbered/Bulleted list is pasted properly.

Actual Result:

we are getting an Unresponsive Script Error

Attachments

Numbers & Bulltes lists.doc (20.5 KB) - added by satya 3 years ago.
7494.patch (2.5 KB) - added by Saare 3 years ago.
7494_2.patch (3.1 KB) - added by garry.yao 3 years ago.
7494_3.patch (3.1 KB) - added by garry.yao 3 years ago.

Change History

Changed 3 years ago by satya

comment:1 Changed 3 years ago by wwalc

  • Status changed from new to confirmed

Caused by [6629]

comment:2 Changed 3 years ago by wwalc

  • Milestone set to CKEditor 3.5.3

comment:3 Changed 3 years ago by j.swiderski

It causes all bowsers to hang, except Opera - everything looks fine there.

comment:4 Changed 3 years ago by wwalc

Note: I managed to trigger this bug in IE8 when working with lists, without pasting the attached (or any other) document from Word. I cannot find a way now to trigger this bug again, but it definitely happened.

comment:5 Changed 3 years ago by fredck

I've found a reduced tc for this issue. Just load the following:

<ol>
	<li>1</li>
	<ol>
		<li>2</li>
	</ol>
	<li>3</li>
</ol>

It hangs the editor.

comment:6 Changed 3 years ago by Saare

  • Owner set to Saare
  • Status changed from confirmed to assigned

comment:7 Changed 3 years ago by fredck

The above case is not covered here ([6643]):
http://ckeditor.t/dt/core/htmlparser/htmlparser.html

comment:8 Changed 3 years ago by fredck

  • not => now

Changed 3 years ago by Saare

comment:9 Changed 3 years ago by Saare

  • Status changed from assigned to review

Patch removes the changes that are not required to fix either bugs.

comment:10 Changed 3 years ago by fredck

  • Status changed from review to review_failed

Some of tests are not passing after patch:
http://ckeditor.t/dt/core/htmlparser/fragment.html

In fact I found it strange that that code has been introduced but it's not needed.

Changed 3 years ago by garry.yao

comment:11 Changed 3 years ago by garry.yao

  • Owner changed from Saare to garry.yao
  • Status changed from review_failed to review

Patch targets also #7497

comment:12 Changed 3 years ago by fredck

  • Status changed from review to review_failed

Please, let's start to avoid enlarging parameters lists on functions calls just to hack the code. A simple list check on a new entry on CKEDITOR.dtd will probably make it work in the same way, having much cleaner code.

Additionally, please review the changes when creating patches. There is some minor unnecessary code formatting changes there.

Changed 3 years ago by garry.yao

comment:13 follow-up: ↓ 14 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

Please, let's start to avoid enlarging parameters lists on functions calls just to hack the code

I'm not sure as:

  1. We're talking about an internally used function;
  2. The optional close is not a dtd semantics but used when we're opening a new (fixing) tag dynamically.

comment:14 in reply to: ↑ 13 Changed 3 years ago by fredck

  • Status changed from review to review_passed

Replying to garry.yao:

  1. We're talking about an internally used function;

Sorry, this is not a good excuse to mess up the code.

  1. The optional close is not a dtd semantics but used when we're opening a new (fixing) tag dynamically.

I got your point now.

---

Go ahead committing the change on fragment.js, but do not change element.js. Other than having duplicated code there on the last patch, there is no need at all to initialize those properties there, especially considering that this is an internal flag exclusive to the fragment.js code.

comment:15 Changed 3 years ago by Saare

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

Fixed with [6646].

comment:16 Changed 6 months ago by fredck

  • Cc changed from damo,jamescun,tmonahan to damo, jamescun, tmonahan
  • Component changed from General to Plugin : Paste from Word
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy