Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6141 closed Bug (fixed)

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

Reported by: Arne Owned by: Sa'ar Zac Elias
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 Sa'ar Zac Elias 7 years ago.
6141_2.patch (1.4 KB) - added by Sa'ar Zac Elias 7 years ago.
6141_3.patch (2.1 KB) - added by Sa'ar Zac Elias 7 years ago.
6141_4.patch (1.2 KB) - added by Garry Yao 7 years ago.
6141_5.patch (2.0 KB) - added by Sa'ar Zac Elias 7 years ago.

Download all attachments as: .zip

Change History (30)

comment:1 Changed 7 years ago by Arne

Summary: List plugin: outdenting list with indentOffset=0 doesn't workIndent plugin: outdenting list with indentOffset=0 doesn't work

comment:2 Changed 7 years ago by Frederico Caldeira Knabben

Component: GeneralCore : Lists
Milestone: CKEditor 3.4.1

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

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6141.patch added

comment:3 Changed 7 years ago by Sa'ar Zac Elias

Owner: set to Sa'ar Zac Elias
Status: newreview

comment:4 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_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 7 years ago by Arne

I believe this will work better :

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

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6141_2.patch added

comment:6 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

comment:7 Changed 7 years ago by Tobiasz Cudnik

Status: reviewreview_failed

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

Changed 7 years ago by Sa'ar Zac Elias

Attachment: 6141_3.patch added

comment:8 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

Changed 7 years ago by Garry Yao

Attachment: 6141_4.patch added

comment:9 Changed 7 years ago by Garry Yao

Status: reviewreview_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 7 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 7 years ago by Sa'ar Zac Elias

Keywords: Discussion added
Status: review_failedreview

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

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

comment:13 in reply to:  12 Changed 7 years ago by Sa'ar Zac Elias

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 7 years ago by Tobiasz Cudnik

Status: reviewreview_passed

Review+ for 6141_3.patch.

comment:15 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [5917].

comment:16 Changed 7 years ago by Sa'ar Zac Elias

Keywords: Discussion removed

comment:17 Changed 7 years ago by Garry Yao

Resolution: fixed
Status: closedreopened

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

[5917] reverted with [5921].

comment:19 Changed 7 years ago by Sa'ar Zac Elias

Status: reopenedreview

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

Status: reviewreview_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 7 years ago by Sa'ar Zac Elias

Attachment: 6141_5.patch added

comment:21 Changed 7 years ago by Sa'ar Zac Elias

Status: review_failedreview

comment:22 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:23 Changed 7 years ago by Sa'ar Zac Elias

Resolution: fixed
Status: review_passedclosed

Fixed with [2928].

comment:24 Changed 7 years ago by Sa'ar Zac Elias

Sorry, [5928].

comment:25 Changed 7 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 – 2017 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy