Removing Items from unordered list (WinXP/IE8)

[tom]

I've got a problem with unordered / numbered lists with IE8 on WindowsXP.

• Create an unordered list with some items.
• Now try to get one item to the upper line with the "delete"-key.
• This will not work.
• If you try the "backspace"-key on the line that should be added to the item above you got an inconsistent list, because the </li>-Tag of the upper line will not disappear.

This error is not in Firefox3 (WinXP), but IE8 (Windows7)

This is also the case in ​http://ckeditor.com/demo/

[tom]

Testlist in Word-Format

[tom]

This Bug is only with IE8 on WindowsXP!

IE8 on Windows7 works fine.

[Paweł Horzela]

Patch

[Garry Yao]

This patch is our first "delete" key handling logic, and looks really good. I'm not trying to make things complicated but while we should consider the following thing:

1. Apply it for other browsers as well? e.g. Firefox generally works well while stuck will the following case on "del":

   <ol>
   	<li>
   		<p>
   			item1^</p>
   	</li>
   	<li>
   		item2</li>
   </ol>
   

2. The merging result after "del" in the above list should be the following instead:

   <ol>
   	<li>
   		<p>
   			item1item2</p>
   	</li>
   </ol>
   

3. Nested list should be considered as well, generally "del" and "backspace" merge the first list item of sub list, then remove the list if empty;

[Paweł Horzela]

Patch

[Garry Yao]

Looks good, fails for the following cases on "Backspace":

<ul>
	<li><p>paragraph1</p>
		<p>paragraph2</p>
		<ul>
			<li>^inner item1</li>
		</ul>
	</li>
</ul>

[Garry Yao]

And the following points：

1. Cache intermediate element when possible (there's no releaser optimization there);
2. Undo/Redo should work as well;
3. innerPrevious/innerNext must be well checked to be valid for reception new children.

[Tobiasz Cudnik]

I would also add that those long lines could be splitted into multiline statements for a better readability.

[Paweł Horzela]

Patch

[Garry Yao]

Have the following content and selection press "Backspace":

<ol>
	<li>
		<p>
			item1</p>
		<ol>
			<li>
				<h1>
					^item21</h1>
				<h1>
					item22</h1>
			</li>
		</ol>
	</li>
</ol>

Actual Result:

<ol>
	<li>
		<p>
			item1</p>
		<h1>
			item22</h1>
	</li>
</ol>

Expected Result:

<ol>
	<li>
		<p>
			item1item21</p>
		<ol>
			<li>
				<h1>
					item22</h1>
			</li>
		</ol>
	</li>
</ol>

[Paweł Horzela]

Patch

[Garry Yao]

There's still lots of problems with the latest patch, I'm now taking over this ticket.

[Tobiasz Cudnik]

I have two points for the latest patch:

1. Following TC, already mentioned in comment 13 - IE8, use the content below, hit Backspace, nothing happens:

   <ol>
   	<li>
   		<p>
   			item1</p>
   		<ol>
   			<li>
   				<h1>
   					^item21</h1>
   				<h1>
   					item22</h1>
   			</li>
   		</ol>
   	</li>
   </ol>
   

2. I think all logic common to every instance of the editor shouldn't be placed inside an init function, as it's duplicated for every instance. This includes:

• mergeListItems
• doDelete
• keydown listener

[Garry Yao]

1. The TC WFM by having the nested list outdented, we could discuss though whether it's a good design.
2. Those are closures scoped to the editor by design.

[Tobiasz Cudnik]

It seems i've been having some cache issues, now i'm getting proper results with attached TC. I agree with the outdent approach for nested lists.

As it goes about closures, we don't need them there, the editor variable can be simply passed as a parameter, which would result in a reduced memory consumption in case of many editor instances.

[Frederico Caldeira Knabben]

Ok... I got a complete overview of the patch proposal. It's all interesting and I'm sure some good time has been spent on it. I'm even sorry to say that, but I think the approach to handle this situation was wrong. Let me expose some reasons:

1. The ticket is related to an issue limited to IE8+XP. Considering that it works well with Win7, we could even expect it to work better on XP in the future (is MS still supporting XP?... anyway). In any case, it's a specific tc on a specific browser. Considering the number of other issues we have to get fixed, this would have less priority (it was a mistake to target it initially) and in any case the amount of code would not justify the fix.

2. Garry came with a series of "edge cases". Of course, I would love to fix every single aspect of the editor behavior, but we must keep the editor size small. It's hard to justify the patch size because of the cases.

3. Then again... the cases didn't come from the "real world". I mean, people out there opening tickets for them. It makes it even harder to justify the fix and its size.

4. The solution proposed goes outside the ticket TC, being much broader. In fact it adds an important new feature into the editor, which should be handled in a dedicated ticket and even have an enlarged discussion to understand if we should introduce it now, and how to do that (avoiding many on the points in this list).

Other things to consider:

4. The editor is already being considered huge on size. We must be aware to not bloat its code.

5. We'll always have situations where to add a lot of code in the editor. We must be smart to decide when it's worth, and when it's not.

6. We didn't leave the option to the end developer to decide whether to use or not this fix.

To conclude, let's have this patch remembered, so it could be part of a the potential keyboard handler we could work on the future. But considering all the above, we should not proceed with it right now.