Opened 7 years ago

Closed 7 years ago

#3893 closed New Feature (fixed)

Unable to indent a list

Reported by: damo Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.3
Component: Core : Lists Version: SVN (CKEditor) - OLD
Keywords: IBM Confirmed Review+ Cc:

Description

The indent feature does not allow indenting whole lists.

Attachments (8)

3893.patch (6.6 KB) - added by garry.yao 7 years ago.
3893_2.patch (7.0 KB) - added by garry.yao 7 years ago.
3893_3.patch (6.8 KB) - added by garry.yao 7 years ago.
3893_4.patch (6.9 KB) - added by garry.yao 7 years ago.
3893_5.patch (6.4 KB) - added by garry.yao 7 years ago.
3893_6.patch (6.9 KB) - added by garry.yao 7 years ago.
3893_7.patch (8.1 KB) - added by garry.yao 7 years ago.
3893_8.patch (8.3 KB) - added by garry.yao 7 years ago.

Download all attachments as: .zip

Change History (32)

comment:1 Changed 7 years ago by damo

  • Type changed from Bug to New Feature

This is probably a new feature request?

We're getting more users ask about this feature. The current implementation of lists does not provide a way to indent the whole list an arbitrary number of times.

Is there a way we could support something like this? Perhaps we could wrap the list in a <div> or maybe a <blockquote>?

Any thoughts?

comment:2 follow-up: Changed 7 years ago by fredck

  • Keywords Confirmed added
  • Milestone changed from CKEditor 3.x to CKEditor 3.3

Considering that the indentation button is disabled if the selection is on the first item of the list, we could instead provide this feature for that case. So, it'll be enough to be in the first item to indent the entire list. If in other items, the current behavior is preserved.

Changed 7 years ago by garry.yao

comment:3 in reply to: ↑ 2 Changed 7 years ago by garry.yao

  • Component changed from General to Core : Lists
  • Keywords Review? added
  • Owner set to garry.yao
  • Status changed from new to assigned
  • Version set to SVN (CKEditor)

Replying to fredck:

So, it'll be enough to be in the first item to indent the entire list. If in other items, the current behavior is preserved.

That's exactly what featured by MS-Word in such case.

comment:4 follow-up: Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed
  • Setting "criteria" to { tagNames : 1 } in the contains function is wrong, as will not work. Please change the function and, to have a cleaner API, use the "arguments" object, instead of expect having an Array or String being passed (the apply function will have to be used to call it).
  • Load the following HTML and try to outdent the "B" item. It'll not work.
<ul>
	<li>A
		<ul>
			<li>B</li>
		</ul>
	</li>
	<li>C</li>
</ul>

Changed 7 years ago by garry.yao

comment:5 in reply to: ↑ 4 ; follow-up: Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

Replying to fredck:

Please change the function and, to have a cleaner API, use the "arguments" object, instead of expect having an Array or String being passed (the apply function will have to be used to call it).

I don't quite understand the idea behind this.

  • Load the following HTML and try to outdent the "B" item. It'll not work...

It should be considered as an edge-case, in fact, when it's impossible to 'outdent' the whole list anymore (in above case), we should also let it outdent the item instead.

comment:6 in reply to: ↑ 5 Changed 7 years ago by fredck

Replying to garry.yao:

I don't quite understand the idea behind this.

That's not important for now. Your last patch simplified it in a way that the feature I was mentioning can be implemented in the future backwards compatible.

It should be considered as an edge-case

No, that's not. This is a very common case when working on re-defining the structure of a complex list.

comment:7 follow-up: Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

The following TC is not working anymore:

  1. Load the following HTML and make the marked selection:
<ul>
	<li>Item 1</li>
	<li>Item [2</li>
	<li>Item] 3</li>
	<li>Item 4</li>
</ul>
  1. Hit the "Increase Indent" button.

After patch results: The entire list gets indented.

Current/Expected results: "Item 2" and "Item 3" forms a second level list under "Item 1".

Note that the proposed list indentation feature must work when the selection is totally inside the first element in the list, only.

Changed 7 years ago by garry.yao

comment:8 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:9 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed
  1. Load the following HTML and make the marked selection:
<ul>
	<li>[A</li>
	<li>B]</li>
	<li>C</li>
</ul>
  1. Hit the "Increase Indent" button.

Expected results: The entire list gets indented.

After patch results: The "C" item disappears and no indentation happens.

comment:10 Changed 7 years ago by fredck

Another TC:

  1. Load the following HTML:
<p>Para 1</p>
<ul>
	<li>A</li>
	<li>B</li>
	<li>C</li>
</ul>
<p>Para 2</p>
  1. Hit CTRL+A to select all.
  2. Click the "Increase Indent" button.

Expected results: the paragraphs and the list get indented.

After patch results: the paragraphs get indented, but the list stays in-place, having its items contents indented.

comment:11 in reply to: ↑ 7 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

...when the selection is totally inside the first element in the list, only.

...when the selection start from the first element in the list.

Another TC: ... After patch results: the paragraphs get indented, but the list stays in-place, having its items contents indented.

This's is yet another feature and requires a drastic change (domIterator), which we should void at the current moment.

Changed 7 years ago by garry.yao

comment:12 follow-up: Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed
  1. Load the following HTML:
<ul style="margin-left: 40px;">
	<li>A</li>
	<li>B</li>
	<li>C</li>
</ul>
  1. Put the caret in the "B" item.
  2. Click the "Increase Indent" button.

Expected results: the "B" item becomes e sub-list of "A".

After patch results: the "B" item becomes a sub-list of "A", but it also takes it's margin-left style, which should happen only in a second click.

comment:13 in reply to: ↑ 12 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

The above bug is already existed in trunk, I've opened #5372 for it.

comment:14 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed
  1. In FF, load the following HTML:
<ul>
	<li>
		A</li>
	<li>
		B</li>
</ul>
  1. Hit CTRL+A to select all.
  2. Click the "Increase Indent" button.

A js error is thrown and bookmark nodes are outputted in the source view.

Changed 7 years ago by garry.yao

comment:15 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:16 Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

This patch breaks the indentation features... the "Decrease Indentation" never gets enabled, even on normal paragraphs.

Changed 7 years ago by garry.yao

comment:17 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:18 follow-up: Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

Unfortunately the TCs of comments 12 and 14 are still broken.

comment:19 in reply to: ↑ 18 Changed 7 years ago by fredck

Replying to fredck:

Unfortunately the TCs of comments 12 and 14 are still broken.

Ops, my fault... comment 12 has already been addressed with a new ticket, as explained by Garry. But I can still reproduce the comment:14 issue.

Changed 7 years ago by garry.yao

comment:20 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

comment:21 follow-up: Changed 7 years ago by fredck

  • Keywords Review- added; Review? removed

It's nice to see the final patch here!

My only concern is regarding the changes in contents.css. We should not make changes in that file, because that's a "user file". Users should point to custom files, which may not have it. Other than that, the patch has the 40px value hardcoded, but it depends on the settings instead. So, those CSS rules should go inside the plugin file, sensitive to the editor settings.

Changed 7 years ago by garry.yao

comment:22 in reply to: ↑ 21 Changed 7 years ago by garry.yao

  • Keywords Review? added; Review- removed

Replying to fredck:

My only concern is regarding the changes in contents.css. We should not make changes in that file

Fortunately the list problem affects only wysiwyg mode instead of the output page, so that's a good idea to keep those reset styles inside our internal style sheet.

Other than that, the patch has the 40px value hardcoded, but it depends on the settings instead.

Not true, it's only a reset for raw HTML list, doesn't impact our indent feature.

comment:23 Changed 7 years ago by fredck

  • Keywords Review+ added; Review? removed

When committing, please remove the listStyleText variable as it's not needed, having the string passed to the addCss function directly. Btw, there is extra space in "editor .addCss", which can also be corrected.

Please commit this on in the 3.3.x branch.

comment:24 Changed 7 years ago by garry.yao

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

Fixed with [5307] on 3.3.x branch.

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