Ticket #3618 (closed Bug: fixed)

Opened 5 years ago

Last modified 5 years ago

List is not merged

Reported by: garry.yao Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.0
Component: Core : Styles Version:
Keywords: Confirmed Review+ Cc:

Description

Reproducing Procedures

  1. Open the replace by class example page with 'enterMode = br' in FF;
  2. Make the content and selection same as below:
    <ul><li>item1</li></ul><ol><li>ite^m2</li></ol>
    
  3. Click on 'Bullet List' command to change the list type.
    • Expected Result:
      <ul><li>item1</li><li>item2</li></ul>
      
    • Actual Result: The previous list is not merged.
      <ul><li>item1</li></ul><ul><li>ite^m2</li></ol>
      

Attachments

3618.patch (2.1 KB) - added by garry.yao 5 years ago.
3618_2.patch (2.1 KB) - added by garry.yao 5 years ago.
3618_3.patch (1.8 KB) - added by garry.yao 5 years ago.

Change History

Changed 5 years ago by garry.yao

comment:1 Changed 5 years ago by garry.yao

  • Status changed from new to assigned
  • Keywords Review? added

The patch is including the fix at #3617 for easy reviewing.

comment:2 follow-up: ↓ 3 Changed 5 years ago by martinkou

  • Keywords Review- added; Review? removed

It's not working with the same test case you provided.

After the patch, item 2 is seen to jump on top of item 1:

<ul>
	<li>
		ite^m2</li>
	<li>
		item1</li>
</ul>

Changed 5 years ago by garry.yao

comment:3 in reply to: ↑ 2 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Replying to martinkou:

It's not working with the same test case you provided...

L496 of list/plugin.js is modified for passing Martin's catch.

comment:4 follow-up: ↓ 5 Changed 5 years ago by martinkou

  • Keywords Review- added; Review? removed

Ok, almost there, but the code can be simplified. :-)

  1. The stopFlag - this is actually my fault, you simply copied it over from my old code. But it's not necessary. It can be replaced by a break.
  2. mergeSibling is wrapped by CKEDITOR.tools.bind - but the only reference to "this" inside is just this.type. So the CKEDITOR.tools.bind() is actually quite wasteful. Instead, something like this would work with less complexity:
    var type = this.type;
    function mergeSibling( rtl )
    {
     ...
    }
    mergeSibling();
    mergeSibling( true );
    

Changed 5 years ago by garry.yao

comment:5 in reply to: ↑ 4 Changed 5 years ago by garry.yao

  • Keywords Review? added; Review- removed

Replying to martinkou:

Ok, almost there, but the code can be simplified...

Pretty high-quality review ;), I'm updating the empty spaces skipping with new param of CKEDITOR.dom.node::getNext/getPrevious.

comment:6 Changed 5 years ago by martinkou

  • Keywords Review+ added; Review? removed

comment:7 Changed 5 years ago by garry.yao

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

Fixed with [3605].

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