Opened 8 years ago

Closed 8 years ago

#3618 closed Bug (fixed)

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 (3)

3618.patch (2.1 KB) - added by Garry Yao 8 years ago.
3618_2.patch (2.1 KB) - added by Garry Yao 8 years ago.
3618_3.patch (1.8 KB) - added by Garry Yao 8 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by Garry Yao

Attachment: 3618.patch added

comment:1 Changed 8 years ago by Garry Yao

Keywords: Review? added
Status: newassigned

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

comment:2 Changed 8 years ago by Martin Kou

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 8 years ago by Garry Yao

Attachment: 3618_2.patch added

comment:3 in reply to:  2 Changed 8 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 Changed 8 years ago by Martin Kou

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 8 years ago by Garry Yao

Attachment: 3618_3.patch added

comment:5 in reply to:  4 Changed 8 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 8 years ago by Martin Kou

Keywords: Review+ added; Review? removed

comment:7 Changed 8 years ago by Garry Yao

Resolution: fixed
Status: assignedclosed

Fixed with [3605].

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