Opened 10 years ago

Closed 10 years ago

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

Download all attachments as: .zip

Change History (30)

comment:1 Changed 10 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 10 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 10 years ago by Sa'ar Zac Elias

Attachment: 6141.patch added

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

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

comment:4 Changed 10 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 10 years ago by Arne

I believe this will work better :

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

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 6141_2.patch added

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

Status: review_failedreview

comment:7 Changed 10 years ago by Tobiasz Cudnik

Status: reviewreview_failed

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

Changed 10 years ago by Sa'ar Zac Elias

Attachment: 6141_3.patch added

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

Status: review_failedreview

Changed 10 years ago by Garry Yao

Attachment: 6141_4.patch added

comment:9 Changed 10 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 10 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 10 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 10 years ago by Garry Yao

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

comment:13 in reply to:  12 Changed 10 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 10 years ago by Tobiasz Cudnik

Status: reviewreview_passed

Review+ for 6141_3.patch.

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

Resolution: fixed
Status: review_passedclosed

Fixed with [5917].

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

Keywords: Discussion removed

comment:17 Changed 10 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 10 years ago by Garry Yao

[5917] reverted with [5921].

comment:19 Changed 10 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 10 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 10 years ago by Sa'ar Zac Elias

Attachment: 6141_5.patch added

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

Status: review_failedreview

comment:22 Changed 10 years ago by Garry Yao

Status: reviewreview_passed

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

Resolution: fixed
Status: review_passedclosed

Fixed with [2928].

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

Sorry, [5928].

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