Ticket #8248 (closed Bug: fixed)

Opened 3 years ago

Last modified 2 years ago

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

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

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

8248.patch (3.1 KB) - added by garry.yao 2 years ago.
8248_2.patch (3.1 KB) - added by garry.yao 2 years ago.
8248_3.patch (4.2 KB) - added by garry.yao 2 years ago.
8248_4.patch (9.5 KB) - added by garry.yao 2 years ago.
8248_5.patch (9.9 KB) - added by garry.yao 2 years ago.

Change History

comment:1 Changed 3 years ago by j.swiderski

  • Keywords IE8 added
  • Status changed from new to confirmed
  • Version set to 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 3 years ago by j.swiderski (previous) (diff)

comment:2 Changed 3 years ago by j.swiderski

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

Changed 2 years ago by garry.yao

comment:3 Changed 2 years ago by garry.yao

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

comment:4 Changed 2 years ago by garry.yao

  • Component changed from General to Core : Lists
  • Milestone set to CKEditor 3.6.3

comment:5 Changed 2 years ago by fredck

  • Status changed from review to review_failed

The ticket tc is still reproducible with the patch.

Changed 2 years ago by garry.yao

comment:6 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

comment:7 Changed 2 years ago by fredck

  • Status changed from review to assigned

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 2 years ago by garry.yao

comment:8 Changed 2 years ago by garry.yao

  • Status changed from assigned to review

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 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by garry.yao

comment:10 follow-up: ↓ 12 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

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 2 years ago by fredck

  • Status changed from review to review_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 2 years ago by fredck

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 2 years ago by garry.yao

comment:13 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

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 2 years ago by fredck

  • Status changed from review to review_passed

comment:15 Changed 2 years ago by garry.yao

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

Fixed with [7439].

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