Ticket #9080 (closed Bug: fixed)

Opened 22 months ago

Last modified 21 months ago

List structure broken when deleting a list entry

Reported by: tmonahan Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.4
Component: Core : Lists Version: 3.6.3
Keywords: IBM Cc: damo, satya

Description (last modified by fredck) (diff)

To Reproduce:

  • Copy the following into source view:
<ol>
	<li>A</li>
	<li>B</li>
	<li>C</li>
</ol>
  • Return to wysiwyg and place the cursor after 'B'
  • Press backspace twice to delete the 2nd entry entirely.

Expected Result: The cursor should be placed after 'A'.

Actual result: A new paragraph is created and the list is broken:

<ol>
	<li>A</li>
</ol>
<p>
	&nbsp;</p>
<ol>
	<li>C</li>
</ol>

Even if the user hits backspace again to try to re-create the original list structure, 'A' and 'C' are now treated as separate lists e.g. the numbering is restarted, if the user tries to indent the list, only 'A' is indented.

This was introduced with rev [7392] and also occurs for bulleted lists.

Attachments

9080.patch (3.7 KB) - added by garry.yao 22 months ago.
9080_2.patch (8.0 KB) - added by garry.yao 22 months ago.
9080_3.patch (9.7 KB) - added by garry.yao 22 months ago.

Change History

comment:1 Changed 22 months ago by fredck

  • Description modified (diff)

comment:2 Changed 22 months ago by fredck

  • Status changed from new to confirmed
  • Milestone set to CKEditor 3.6.4

comment:3 Changed 22 months ago by fredck

Please note that, as decided earlier, we'll follow the MS Word behavior on this.

This ticket will exclusively fix the merging behavior of BACKSPACE.

Changed 22 months ago by garry.yao

comment:4 Changed 22 months ago by garry.yao

  • Owner set to garry.yao
  • Status changed from confirmed to review

comment:6 follow-up: ↓ 7 Changed 22 months ago by tmonahan

MS Word does the outdent, however it also maintains the integrity of the list. This is where the behavior in CKEditor differs.

Another important usecase to satisfy after this fix is to maintain the list structure for sub-lists e.g. Copy the following into source mode:

<ol>
	<li>AAA
		<ol>
			<li>BBB</li>
			<li>CCC</li>
			<li>DDD
				<ol>
					<li>EEE</li>
					<li>FFF</li>
				</ol>
			</li>
		</ol>
	</li>
</ol>

  • Place the cursor after CCC and hit backspace enough times to delete the entry altogether.

Expected Result: BBB and DDD should still be present within the same sub-list.

This example works as expected in MS Word.

comment:7 in reply to: ↑ 6 Changed 22 months ago by fredck

  • Status changed from review to review_failed

Replying to tmonahan:

Well... this is a very good justification for it.

In fact, we'll not always be able to mimic the MS Word behavior, because HTML has different requirements and limitations.

In this specific case, Word can handle lists starting at deeper levels without problems. We're not able to do so, because HTML lists must always start at level 1.

This means that we made a bad decision in the past and we need to change our position now, providing a simpler solution for it, which simple merge list items on backspace, as expected by the reporter.

comment:8 Changed 22 months ago by garry.yao

  • Status changed from review_failed to review

The patch contains 3 parts:

  1. Reverted the main change of [7392] to not have indentation affected by backspace key;
  2. Re-fixed #8249 by following the behavior of having backspace join the current list item with previous;
  3. Last side change related to undo - introduced editor::selectionChange to check immediately the selection, to avoid resulting in wrong undo snapshots.

comment:9 Changed 22 months ago by garry.yao

  • Status changed from review to assigned

Changed 22 months ago by garry.yao

comment:10 Changed 22 months ago by garry.yao

  • Status changed from assigned to review

comment:11 Changed 22 months ago by garry.yao

comment:12 Changed 22 months ago by fredck

This test is failing: http://ckeditor.t/tt/8249/1.html

Additionally, the following case has a weird effect:

  1. Load this:
<ol>
	<li>
		&nbsp;</li>
	<li>
		B</li>
	<li>
		C</li>
</ol>
  1. Put the caret at the first item.
  2. Hit BACKSPACE.

The list item is deleted and a new paragraph will be quickly created before the list, with the selection on it. This paragraph almost immediately disappears, having the selection moved back the to the new top of the list ("B").

In this case, nothing should happen, just like when we have that list item filled with text.

comment:13 Changed 22 months ago by fredck

  • Status changed from review to review_failed

comment:14 Changed 22 months ago by garry.yao

  • Status changed from review_failed to review

The issue in comment:12 can be reproduced without the patch, but it can be handled by the patch as well.

Changed 22 months ago by garry.yao

comment:15 Changed 22 months ago by fredck

Ok, that tc is now ok, but this test is still failing :(
http://ckeditor.t/tt/8249/1.html

comment:16 Changed 22 months ago by fredck

  • Status changed from review to review_failed

comment:17 Changed 22 months ago by garry.yao

  • Status changed from review_failed to review

My fault...I've failed to push some TCs.

comment:18 Changed 22 months ago by fredck

  • Status changed from review to review_passed

comment:19 Changed 22 months ago by garry.yao

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

Fixed with [7540].

comment:20 Changed 21 months ago by r.mikolajuk

  • Status changed from closed to reopened
  • Resolution fixed deleted

The issue in comment:12 is still present in Opera 12 - after hitting BACKSPACE whole list is deleted.

In IE8 something different happens:

  1. After BACKSPACE hit - first list item is deleted, the rest of the list gets proper numbers, new paragraph is created with content from first list item:
    <p>
    	&nbsp;</p>
    <ol>
    	<li>
    		B</li>
    	<li>
    		C</li>
    </ol>
    
  2. Now, when caret is in the paragraph hit DELETE. In visual the list is lost, we can see B and C letters one under another (with empty line before them):
    
    B
    C
    
  3. Go back to source mode. The code is almost the same as in point 1 (after BACKSPACE hit) but we get <ul> elements instead of<ol>. Switching back to visual mode recreates the list with bullets - not numbers.

Steps 2-3 can be repeated indefinitely.

Last edited 21 months ago by r.mikolajuk (previous) (diff)

comment:21 follow-up: ↓ 22 Changed 21 months ago by garry.yao

  • Status changed from reopened to closed
  • Component changed from General to Core : Lists
  • Resolution set to fixed

For the Opera issue it's not a regression, I've opened #9129 for it.

I cannot confirm the above issue on IE8 though.

comment:22 in reply to: ↑ 21 Changed 21 months ago by tmonahan

Replying to garry.yao:

For the Opera issue it's not a regression, I've opened #9129 for it.

I cannot confirm the above issue on IE8 though.

I can also reproduce this issue on IE8 using the steps and markup specified in comment:12 - the problem does not occur in IE9.

comment:23 Changed 21 months ago by tmonahan

The steps to reproduce the problem I am seeing are:

  • Load this:
    <ol>
    	<li>
    		&nbsp;</li>
    	<li>
    		B</li>
    	<li>
    		C</li>
    </ol>
    
  • Place the cursor at the first list item.
  • Press BACKSPACE. The first list item is removed and a paragraph is created before the list.
  • Press DELETE. Problem: The paragraph is deleted and the list items no longer appear as a list (even though li is still displayed on the elements path bar).
  • Go to Source view and then back to wysiwyg mode. Problem: The list now appears but it has bullets instead of numbers.

I can reproduce this on standalone IE8 on WinXP. It does not happen on an IE9 install running in IE8 mode.

comment:24 Changed 21 months ago by garry.yao

The above TC is valid though not related to the ticket, it concerns delete key behavior outside of a list structure, which is not handled by our list keystrokes logic, plus it's limited affecting range, opened a new ticket instead #9152.

Last edited 21 months ago by garry.yao (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy