Opened 6 years ago

Closed 6 years ago

#8248 closed Bug (fixed)

[[WinXP & IE8]] Removing last list item in outer list creating a new list item/ breaking the list

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

Description

To reproduce the defect:

Copy the following code and paste it in source view.

<ol>
	<li> FirstListItem  </li>
	<li> SecondListItem
			<ol> 
				<li> ThirdListItem  </li>
				<li>  FourthListItem  </li>
				<li>  FifthListItem  </li>
			</ol>
	</li>
	<li> SixthListItem  </li>
</ol>

Scenario 1:

Go back to rich text, keep the cursor at end of last list item(Sixth list item) and press backspace until you delete list item

Expected Result: last list item is deleted and cursor goes to end of last list item(Fifth list item) in inner list.

Actual Result: last list item is deleted but a new empty list item is created after last list item(Fifth list item) in inner list.

Scenario 2:

Go back to rich text, keep the cursor at end of last but one list item(Fifth list item) and press delete

Expected Result: last list item is deleted and text in last list item gets appended to the text of last list item(Fifth list item) in inner list.

Actual Result: last list item is deleted but a new empty list item is created after last list item(Fifth list item) in inner list and a new list starts with the text in last list item

keep cursor at the new empty created list item and press Enter.

You will get a java script error

This is only happening on IE8 in Windows XP

Attachments (5)

8248.patch (3.1 KB) - added by Garry Yao 6 years ago.
8248_2.patch (3.1 KB) - added by Garry Yao 6 years ago.
8248_3.patch (4.2 KB) - added by Garry Yao 6 years ago.
8248_4.patch (9.5 KB) - added by Garry Yao 6 years ago.
8248_5.patch (9.9 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by Jakub Ś

Keywords: IE8 added
Status: newconfirmed
Version: 3.0

This issue has been reproducible form CKE 3.0

Pasting the error just in case it helps:
Message: 'null' is empty or not an object
Line: 544
URI: 3.6.1/ckeditor/_source/core/dom/node.js

Last edited 6 years ago by Jakub Ś (previous) (diff)

comment:2 Changed 6 years ago by Jakub Ś

When reproducing this issue I have found strange behavior in Firefox. I have reported it here: #8249.

Changed 6 years ago by Garry Yao

Attachment: 8248.patch added

comment:3 Changed 6 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

comment:4 Changed 6 years ago by Garry Yao

Component: GeneralCore : Lists
Milestone: CKEditor 3.6.3

comment:5 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

The ticket tc is still reproducible with the patch.

Changed 6 years ago by Garry Yao

Attachment: 8248_2.patch added

comment:6 Changed 6 years ago by Garry Yao

Status: review_failedreview

comment:7 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewassigned

The proposed idea is even ok, but it means a lot of hackery and potential instability. Other than that, it introduces a good amount of "code that should not be there" in the editor. If possible, we should avoid it.

A final motivation is that this doesn't happen with IE9 and IE7.

Considering that users are still able to fix the situation in these cases, let's proceed with a simple fix first, just avoiding the JavaScript error.

After that we may consider a more drastic solution that fixes the entire tc behavior, if we feel it is really needed.

Changed 6 years ago by Garry Yao

Attachment: 8248_3.patch added

comment:8 Changed 6 years ago by Garry Yao

Status: assignedreview

It looks impossible to eliminate the error in tc2 without managing the keystroke behavior here, since the error is pruned by a invalid DOM structure as a consequence of the wrong list item merging.

Provide a new patch to address the "del" key behavior from TC2.

comment:9 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

With IE8:

  1. Load the original tc html:
<ol>
	<li> FirstListItem  </li>
	<li> SecondListItem
			<ol> 
				<li> ThirdListItem  </li>
				<li>  FourthListItem  </li>
				<li>  FifthListItem  </li>
			</ol>
	</li>
	<li> SixthListItem  </li>
</ol>
  1. Click after "SecondListItem".
  2. Hit DEL. (strange effect... the nested list outdents and a strange numeration is kept).
  3. Hit DEL. (nothing happens... just the caret starts blinking at the start of the next item)
  4. Hit BACKSPACE. (BOOM!)

Note that this is not caused by the patch, but it is a pretty basic tc, very related to the patch, so it could be considered.

Changed 6 years ago by Garry Yao

Attachment: 8248_4.patch added

comment:10 Changed 6 years ago by Garry Yao

Status: review_failedreview

New patch considers the above TC, while it's a pity that we're unable to automate with test here since the selection ranges doesn't work for having cursor before a sub list, browser will always normalize it to be at the beginning of the sub list item:

<ol>
	<li>1^
		<ol>
			<li>1.1</li>
		</ol>
	</li>
</ol>

Will always be transformed into the following:

<ol>
	<li>1
		<ol>
			<li>^1.1</li>
		</ol>
	</li>
</ol>

So manual testing is required for it.

comment:11 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

Unfortunately we still have this tc:

  1. Load the original ticket tc html:
<ol>
	<li> FirstListItem  </li>
	<li> SecondListItem
			<ol> 
				<li> ThirdListItem  </li>
				<li>  FourthListItem  </li>
				<li>  FifthListItem  </li>
			</ol>
	</li>
	<li> SixthListItem  </li>
</ol>
  1. Put the caret at the end of "FirstListItem".
  2. Hit DEL.

The nested list will be outdented and the list items order will be mangled.

comment:12 in reply to:  10 Changed 6 years ago by Frederico Caldeira Knabben

Replying to garry.yao:

browser will always normalize it to be at the beginning of the sub list item

I was wondering... what it we put the caret one character before the end (<li>^1) and then use the native browser selection to move one character ahead (textRange.move( 'character', 1 )).

Changed 6 years ago by Garry Yao

Attachment: 8248_5.patch added

comment:13 Changed 6 years ago by Garry Yao

Status: review_failedreview

Unfortunately, textRange::move doesn't help here, luckily, I've found some other workaround to make the tests runs, so the above mentioned cases are now under test coverage.

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

Status: reviewreview_passed

comment:15 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7439].

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