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

[Arne]

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;

[Frederico Caldeira Knabben]

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

[Tobiasz Cudnik]

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

[Arne]

I believe this will work better :

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

[Tobiasz Cudnik]

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

[Garry Yao]

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.

[Arne]

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

[Sa'ar Zac Elias]

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.

[Garry Yao]

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

[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.

[Tobiasz Cudnik]

Review+ for 6141_3.patch.

[Sa'ar Zac Elias]

Fixed with [5917].

[Garry Yao]

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.

3. Outdent the indented list item.
   • Actual Result: The first outdent transforms the list into paragraph.

[Garry Yao]

[5917] reverted with [5921].

[Sa'ar Zac Elias]

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.

[Garry Yao]

@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.

[Sa'ar Zac Elias]

Fixed with [2928].

[Sa'ar Zac Elias]

Sorry, [5928].

[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.