Opened 6 years ago

Closed 6 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 6 years ago.
6419_2.patch (1.7 KB) - added by tobiasz.cudnik 6 years ago.
6419_3.patch (2.0 KB) - added by tobiasz.cudnik 6 years ago.
6419_4.patch (2.0 KB) - added by tobiasz.cudnik 6 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by tobiasz.cudnik

  • Status changed from new to confirmed

comment:2 Changed 6 years ago by satya

  • Cc satya_minnekanti@… added
  • Keywords IBM added

comment:3 Changed 6 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Description modified (diff)

comment:5 Changed 6 years ago by tobiasz.cudnik

  • Description modified (diff)

Changed 6 years ago by tobiasz.cudnik

comment:6 Changed 6 years ago by tobiasz.cudnik

  • Status changed from assigned to review

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 6 years ago by garry.yao

  • Status changed from review to review_failed

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

Changed 6 years ago by tobiasz.cudnik

comment:8 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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 6 years ago by garry.yao

  • Status changed from review to review_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 6 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 6 years ago by tobiasz.cudnik

comment:11 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

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 6 years ago by garry.yao

  • Status changed from review to review_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 6 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 6 years ago by tobiasz.cudnik

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

Changed 6 years ago by tobiasz.cudnik

comment:15 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review_failed to review

comment:16 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:17 Changed 6 years ago by tobiasz.cudnik

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

Fixed with [6089].

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