Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#9080 closed Bug (fixed)

List structure broken when deleting a list entry

Reported by: Teresa Monahan Owned by: Garry Yao
Priority: Normal Milestone: CKEditor 3.6.4
Component: Core : Lists Version: 3.6.3
Keywords: IBM Cc: Damian, Satya Minnekanti

Description (last modified by Frederico Caldeira Knabben)

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 (3)

9080.patch (3.7 KB) - added by Garry Yao 12 years ago.
9080_2.patch (8.0 KB) - added by Garry Yao 12 years ago.
9080_3.patch (9.7 KB) - added by Garry Yao 12 years ago.

Download all attachments as: .zip

Change History (27)

comment:1 Changed 12 years ago by Frederico Caldeira Knabben

Description: modified (diff)

comment:2 Changed 12 years ago by Frederico Caldeira Knabben

Milestone: CKEditor 3.6.4
Status: newconfirmed

comment:3 Changed 12 years ago by Frederico Caldeira Knabben

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 12 years ago by Garry Yao

Attachment: 9080.patch added

comment:4 Changed 12 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

comment:6 Changed 12 years ago by Teresa Monahan

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 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 12 years ago by Garry Yao

Status: review_failedreview

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 12 years ago by Garry Yao

Status: reviewassigned

Changed 12 years ago by Garry Yao

Attachment: 9080_2.patch added

comment:10 Changed 12 years ago by Garry Yao

Status: assignedreview

comment:12 Changed 12 years ago by Frederico Caldeira Knabben

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 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

comment:14 Changed 12 years ago by Garry Yao

Status: review_failedreview

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

Changed 12 years ago by Garry Yao

Attachment: 9080_3.patch added

comment:15 Changed 12 years ago by Frederico Caldeira Knabben

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

comment:16 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

comment:17 Changed 12 years ago by Garry Yao

Status: review_failedreview

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

comment:18 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:19 Changed 12 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7540].

comment:20 Changed 12 years ago by Robert

Resolution: fixed
Status: closedreopened

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 12 years ago by Robert (previous) (diff)

comment:21 Changed 12 years ago by Garry Yao

Component: GeneralCore : Lists
Resolution: fixed
Status: reopenedclosed

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 12 years ago by Teresa Monahan

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 12 years ago by Teresa Monahan

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 12 years 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 12 years ago by Garry Yao (previous) (diff)
Note: See TracTickets for help on using tickets.
© 2003 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy