Opened 7 years ago

Closed 7 years ago

#6419 closed Bug (fixed)

IE: List creation by merging problem

Reported by: Tobiasz Cudnik Owned by: Tobiasz Cudnik
Priority: Normal Milestone: CKEditor 3.5
Component: General Version:
Keywords: IE IBM Cc: satya_minnekanti@…

Description (last modified by Tobiasz Cudnik)

IE: List creation by merging problem.

Steps to reproduce

  1. Type 3 lines
  2. Select all
  3. Apply the Bulleted List command
  4. Place a selection in the middle line
  5. Apply the Bulleted List command again, to remove this line from the list
  6. Select all

Result: A JS error is thrown.

Expected: Lists are merged into one containing 3 lines.

Reproduced in the IE8, strict. Most probably because the step 5 leaves bookmarks in the 1st and the 3rd line.

Attachments (4)

6419.patch (907 bytes) - added by Tobiasz Cudnik 7 years ago.
6419_2.patch (1.7 KB) - added by Tobiasz Cudnik 7 years ago.
6419_3.patch (2.0 KB) - added by Tobiasz Cudnik 7 years ago.
6419_4.patch (2.0 KB) - added by Tobiasz Cudnik 7 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 7 years ago by Tobiasz Cudnik

Status: newconfirmed

comment:2 Changed 7 years ago by Satya Minnekanti

Cc: satya_minnekanti@… added
Keywords: IBM added

comment:3 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

comment:4 Changed 7 years ago by Tobiasz Cudnik

Description: modified (diff)

comment:5 Changed 7 years ago by Tobiasz Cudnik

Description: modified (diff)

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6419.patch added

comment:6 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

The fault was inside the list splitting logic, which was cloning an UL element along with the _cke_expando ID. This expando was missleading next operations afterwards.

comment:7 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Problem is resolved, but now list ID always lost during transformation.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6419_2.patch added

comment:8 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

Although cloning IDs is not valid in the context of a document, i've reverted such behavior by extending the dom.node#clone method with ability to clone IDs without the cke_expando.

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

Are we able to fix this without touching the "clone" method? I mean is the custom data still needed inside arrayToList?

comment:10 Changed 7 years ago by Tobiasz Cudnik

We can fix it removing cke_expando manually, although there won't be any difference in the code size. Most important point here is that cloning expando is plain wrong.

There's no customData usage inside arrayToList, although it's needed in other places when used. If we would want to loose it, then we would need to reinvent whole logic of the list processing.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6419_3.patch added

comment:11 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

Third patch removes the _cke_expando always when cloning a node (right now it was cloned only in IE, thus this issue exists).

There can be more issues because of this, as nodes could became non-unique for such things as customData and event listeners.

comment:12 Changed 7 years ago by Garry Yao

Status: reviewreview_failed
  1. function 'removeIds' could be further optimized based on 'includeChildren';
  2. Just a guess, but as IE treated expando as attributes, copyAttributes should be fixed also;

comment:13 Changed 7 years ago by Tobiasz Cudnik

Regarding point 2, following TC confirms that copyAttributes doesn't copy the expando, which isn't actually weird, as copied are only children of an "attributes" property, which expando is not a child of.

var l = console.log;
var a = new CKEDITOR.dom.element( 'p' );
var b = new CKEDITOR.dom.element( 'p' );
a.setCustomData( 'foo', 'bar' );
b.copyAttributes( a );
l( a.$._cke_expando );
l( b.$._cke_expando );

comment:14 Changed 7 years ago by Tobiasz Cudnik

It's confirmed using IE8, IE8 in IE7 mode and IE6.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6419_4.patch added

comment:15 Changed 7 years ago by Tobiasz Cudnik

Status: review_failedreview

comment:16 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:17 Changed 7 years ago by Tobiasz Cudnik

Resolution: fixed
Status: review_passedclosed

Fixed with [6089].

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