Opened 6 years ago

Closed 5 years ago

#7645 closed Bug (fixed)

Lists not deleted properly using backspace

Reported by: jamescun Owned by: garry.yao
Priority: Normal Milestone:
Component: Core : Lists Version: 3.0
Keywords: IE WebKit IBM Cc: damo, satya, tmonahan

Description (last modified by Saare)

Steps to reproduce the defect:

  1. Open the Ajax sample.
  1. Paste the following code into WYSIWYG mode
<ol>
	<li>
		item 1</li>
	<li>
		item 2
		<ol>
			<li>
				item 3</li>
			<li>
				item 4</li>
			<li>
				item 5</li>
		</ol>
	</li>
</ol>

  1. Switch back to source mode.
  1. Select all of the list items with the mouse & click backspace.

Expected: Nothing remains in the editor.

Actual: The list is not fully deleted (see screenshot) & the following code still remains in the editor:

<ol>
	<li>
		<ol>
			<li>
				&nbsp;</li>
		</ol>
	</li>
</ol>

Attachments (7)

IE9 Delete List Defect.JPG (28.3 KB) - added by jamescun 6 years ago.
7645.patch (2.7 KB) - added by Saare 6 years ago.
7645_2.patch (4.7 KB) - added by garry.yao 6 years ago.
7645_3.patch (10.1 KB) - added by garry.yao 6 years ago.
7645_4.patch (9.6 KB) - added by garry.yao 5 years ago.
7645_5.patch (10.3 KB) - added by garry.yao 5 years ago.
7645_6.patch (8.5 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (25)

Changed 6 years ago by jamescun

comment:1 Changed 6 years ago by garry.yao

  • Keywords IE added; IE9 removed
  • Version changed from 3.5.4 (SVN - trunk) to 3.0

It's not IE specific and the backspace key behavior over range selection is quite browser dependent, we suggest not to rely on it.

comment:2 Changed 6 years ago by j.swiderski

  • Resolution set to invalid
  • Status changed from new to closed

comment:3 Changed 6 years ago by jamescun

Why has this ticket been closed as invalid? Highlighting a list with the mouse and using backspace to delete it is common practice for deleting a list.

comment:4 Changed 6 years ago by wwalc

  • Resolution invalid deleted
  • Status changed from closed to reopened

comment:5 Changed 6 years ago by j.swiderski

  • Status changed from reopened to confirmed

This has been true since CKEditor version 3.0. Behavior is browser dependable.

For IE6, IE7 & IE9:
Selecting list with a mouse and pressing del or backspace button always leaves code shown above by @jamescun.
Selecting list with CRTL+A and pressing del or backspace button removes everything.
Using the following combination: CRTL+A; DEL button; CRTL+Z; backspace button; leaves the code shown above by @jamescun.

IE8 behaves almost the same. The only difference is the combination CRTL+A; DEL button; CRTL+Z; backspace button; which leaves the following code:

<ol>
	<li>
		&nbsp;</li>
	<li>
		&nbsp;</li>
	<li>
		&nbsp;</li>
</ol>




Webkit and Opera are more consistent here. No matter whether you use mouse or CRTL+A, you are not able to select the whole list, thus after pressing backspace or del button you are always left with:

<ol>
	<li>
		&nbsp;</li>
</ol>

Pressing backspace or del one more time removes the leftovers.


In FireFox it works.

Changed 6 years ago by Saare

comment:6 Changed 6 years ago by Saare

  • Owner set to Saare
  • Status changed from confirmed to review

Patch introduces a new IE9-friendly version of fixListRange that was proposed in #6432.

Changed 6 years ago by garry.yao

comment:7 Changed 6 years ago by garry.yao

  • Status changed from review to review_failed

Nice try Saar, while the "fixListRange" method wasn't of good quality (my fault) ;/ it breaks on the following cases:

<ol>
	<li>
		<input name="name" type="checkbox" />[item1</li>
	<li>
		item2]</li>
</ol>
<ol>
	<li>
		[item1
		<ol>
			<li>
				item2]</li>
		</ol>
	</li>
</ol>

Also because it's violation on one principle that in all dom::range methods should have no impact on doc selection.

It looks like range::enlarge is the best place for introducing it, as it's all about expanding the range boundaries on a particular situation, should fit well in the semantics.

comment:8 Changed 6 years ago by garry.yao

  • Owner changed from Saare to garry.yao
  • Status changed from review_failed to review

comment:9 Changed 6 years ago by Saare

  • Status changed from review to review_failed

At least with IE8 (didn't test IE9 yet):

  • Add a table. Select it as a control element. Hit BACKSPACE. JS error is thrown.
  • The 2nd case you've specified failes (the first item should remain).

Changed 6 years ago by garry.yao

comment:10 Changed 6 years ago by garry.yao

  • Status changed from review_failed to review

The 2nd case you've specified failes (the first item should remain).

I vote instead the opposite, where the removes the entire list, as the selection is fully spanning the list, referencing the MS-Word behavior.

Instead of applying this fix to just IE, I'm now proposing it as a generic fix instead, which might give other browsers better result upon it.

comment:11 follow-up: Changed 6 years ago by Saare

  • Status changed from review to review_failed
  1. With the following:
    <ol>
    	<li>
    		<input name="name" type="checkbox" />[item1</li>
    	<li>
    		item2]</li>
    </ol>
    

Click on numbered list icon.
IE<9 : 2nd list item, though truncated, is not removed.
FF, Webkit : Cursor disappears. (FF: br is displayed in elements path).


  1. IE: Empty the editor. Add a table. Select it from the elements patch. Hit BACKSPACE. JS error is triggered:
    Message: 'is' is null or not an object
    Line: 773
    Char: 4
    Code: 0
    URI: /core/dom/range.js
    

  1. In Webkit (Chrome at least), following TC fails:
    <ol>
    	<li>
    		[item1
    		<ol>
    			<li>
    				item2]</li>
    		</ol>
    	</li>
    </ol>
    

Changed 5 years ago by garry.yao

comment:12 in reply to: ↑ 11 ; follow-up: Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

Replying to Saare:

  1. With the following:

I don't understand how this's related....

  1. IE: Empty the editor. Add a table. Select it from the elements patch. Hit BACKSPACE. JS error is triggered:

This one is addressed in the new patch.

  1. In Webkit (Chrome at least), following TC fails:

The patch has intended to address the issue for all browsers, while as we saw here it's not working in this case for webkit, while I dont think we should be blocked by it considering the ticket is of an IE issue and we don't brought a regression for webkit.

comment:13 Changed 5 years ago by fredck

  • Keywords NeedsTest added

comment:15 in reply to: ↑ 12 Changed 5 years ago by Saare

  • Description modified (diff)
  • Keywords WebKit added
  • Status changed from review to review_failed
  • Summary changed from IE9: Lists not deleted properly using backspace to Lists not deleted properly using backspace

Replying to garry.yao:

Replying to Saare: The patch has intended to address the issue for all browsers, while as we saw here it's not working in this case for webkit, while I dont think we should be blocked by it considering the ticket is of an IE issue and we don't brought a regression for webkit.

Since the same issue exsists in WebKit as well, it must be fixed there too.

Changed 5 years ago by garry.yao

comment:16 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

I've altered the implementation a bit to have webkit counted into the fix, note that changes to the other plugins (other than wysiwyg and selection) are necessarily caused by the update of CKEDITOR.tools.tryThese.

http://ckeditor.t/dt/plugins/styles/styles.html failures are not brought by the patch.

Changed 5 years ago by garry.yao

comment:17 Changed 5 years ago by Saare

  • Status changed from review to review_passed

R+ for 7645_6, to avoid redudant changes in 7645_5.

Last edited 5 years ago by Saare (previous) (diff)

comment:18 Changed 5 years ago by garry.yao

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

Fixed with [7243].

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