Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#6141 closed Bug (fixed)

Indent plugin: outdenting list with indentOffset=0 doesn't work

Reported by: arne Owned by: Saare
Priority: Normal Milestone: CKEditor 3.4.2
Component: Core : Lists Version:
Keywords: Cc:

Description

  1. Configure indentOffset in list plugin to 0
  2. Create a list with two items
  3. Indent the second item.
  4. Outdent the second item.

Suggested fix: Modify the following lines in the indentElement function in the list plugin:

if ( currentOffset < 0 )
   return false;

To :

if ( currentOffset <= 0 )
   return false;

Attachments (5)

6141.patch (500 bytes) - added by Saare 6 years ago.
6141_2.patch (1.4 KB) - added by Saare 6 years ago.
6141_3.patch (2.1 KB) - added by Saare 6 years ago.
6141_4.patch (1.2 KB) - added by garry.yao 6 years ago.
6141_5.patch (2.0 KB) - added by Saare 6 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 6 years ago by arne

  • Summary changed from List plugin: outdenting list with indentOffset=0 doesn't work to Indent plugin: outdenting list with indentOffset=0 doesn't work

comment:2 Changed 6 years ago by fredck

  • Component changed from General to Core : Lists
  • Milestone set to CKEditor 3.4.1

Sounds reasonable, considering that the fix can be that small.

Changed 6 years ago by Saare

comment:3 Changed 6 years ago by Saare

  • Owner set to Saare
  • Status changed from new to review

comment:4 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_failed

After using the patch following TC fails:

  • Use following content:
    <ul>
    	<li>
    		foo</li>
    	<li>
    		bar</li>
    </ul>
    
  • Indent "bar" twice
  • Oudent "bar" once
  • Result: "bar" is not indented
  • Expected: "bar" is indented with one level

comment:5 Changed 6 years ago by arne

I believe this will work better :

if ( currentOffset < 0 || (currentOffset == 0 && self.name == 'outdent')) {
   return false;
}

Changed 6 years ago by Saare

comment:6 Changed 6 years ago by Saare

  • Status changed from review_failed to review

comment:7 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_failed

With second patch, list outdent stopped working (both TCs from this ticket fail).

Changed 6 years ago by Saare

comment:8 Changed 6 years ago by Saare

  • Status changed from review_failed to review

Changed 6 years ago by garry.yao

comment:9 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

The purpose of having 'config.indentOffset' set to 0 is to disable normal paragraph indentation while keep the list transformation (am I right Arne?), the patch should simply allow this to happen with an explicit check.

comment:10 Changed 6 years ago by arne

Yes, that is the purpose. We don't want the user to be able to indent the list items beyond their list levels.

comment:11 Changed 6 years ago by Saare

  • Keywords Discussion added
  • Status changed from review_failed to review

The way I see it, if the list has indentation, it should be removed (just like every block) and only in the the second click its items should be transformed into paragraphs.

comment:12 follow-up: Changed 6 years ago by garry.yao

@Saar, are you asking for review on 6141_4?

comment:13 in reply to: ↑ 12 Changed 6 years ago by Saare

Replying to garry.yao:

@Saar, are you asking for review on 6141_4?

Asking for review on 6141_3 again actually, I explained the reason in comment:11 and in our conversation a few days ago.

comment:14 Changed 6 years ago by tobiasz.cudnik

  • Status changed from review to review_passed

Review+ for 6141_3.patch.

comment:15 Changed 6 years ago by Saare

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

Fixed with [5917].

comment:16 Changed 6 years ago by Saare

  • Keywords Discussion removed

comment:17 Changed 6 years ago by garry.yao

  • Resolution fixed deleted
  • Status changed from closed to reopened

This fix breaks indentation with custom class name instead of styling, as the following case:

  1. Use the 'indentClasses' config and update content styles;
    config.indentClasses = ['Indent1', 'Indent2', 'Indent3'];
    
    .Indent1{
    	margin-left:20px;
    }
    .Indent2{
    	margin-left:40px;
    }
    
    
  2. Indent at least two times on a single list item.
  1. Outdent the indented list item.
    • Actual Result: The first outdent transforms the list into paragraph.

comment:18 Changed 6 years ago by garry.yao

[5917] reverted with [5921].

comment:19 Changed 6 years ago by Saare

  • Status changed from reopened to review

It's not a regression of the fix, as I can still reproduce the bug after the fix was reverted. That's another bug, and we should make sure there is a ticket about it. Asking for another review of 6141_3.

comment:20 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

@Saar, it seems you're right on that the test case is not working originally, but by looking the patch the following logic is problematic by assuming that the indentation is always based on inline style, those lines would better keep unchanged IMO:

if ( !( indentWholeList &&
    ( self.name == 'indent' || parseInt( nearestListBlock.getStyle( getIndentCssProperty( nearestListBlock ) ), 10 ) ) &&
    indentElement( nearestListBlock ) ) )
        indentList( nearestListBlock );

#6447 opened for the above TC.

Changed 6 years ago by Saare

comment:21 Changed 6 years ago by Saare

  • Status changed from review_failed to review

comment:22 Changed 6 years ago by garry.yao

  • Status changed from review to review_passed

comment:23 Changed 6 years ago by Saare

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

Fixed with [2928].

comment:24 Changed 6 years ago by Saare

Sorry, [5928].

comment:25 Changed 6 years ago by arne

Setting indentOffset=0 results in an indentation of 40px because the test in the following code will evaluate the value of zero to false and return 40:

	var indentOffset = editor.config.indentOffset || 40;

in the indentElement function in the indent plugin.

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