Opened 6 years ago

Closed 6 years ago

#7645 closed Bug (fixed)

Lists not deleted properly using backspace

Reported by: James Cunningham Owned by: Garry Yao
Priority: Normal Milestone:
Component: Core : Lists Version: 3.0
Keywords: IE WebKit IBM Cc: Damian, Satya Minnekanti, Teresa Monahan

Description (last modified by Sa'ar Zac Elias)

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 James Cunningham 6 years ago.
7645.patch (2.7 KB) - added by Sa'ar Zac Elias 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 6 years ago.
7645_5.patch (10.3 KB) - added by Garry Yao 6 years ago.
7645_6.patch (8.5 KB) - added by Garry Yao 6 years ago.

Download all attachments as: .zip

Change History (25)

Changed 6 years ago by James Cunningham

Attachment: IE9 Delete List Defect.JPG added

comment:1 Changed 6 years ago by Garry Yao

Keywords: IE added; IE9 removed
Version: 3.5.4 (SVN - trunk)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 Jakub Ś

Resolution: invalid
Status: newclosed

comment:3 Changed 6 years ago by James Cunningham

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 Wiktor Walc

Resolution: invalid
Status: closedreopened

comment:5 Changed 6 years ago by Jakub Ś

Status: reopenedconfirmed

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 Sa'ar Zac Elias

Attachment: 7645.patch added

comment:6 Changed 6 years ago by Sa'ar Zac Elias

Owner: set to Sa'ar Zac Elias
Status: confirmedreview

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

Changed 6 years ago by Garry Yao

Attachment: 7645_2.patch added

comment:7 Changed 6 years ago by Garry Yao

Status: reviewreview_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 Sa'ar Zac Elias to Garry Yao
Status: review_failedreview

comment:9 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_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

Attachment: 7645_3.patch added

comment:10 Changed 6 years ago by Garry Yao

Status: review_failedreview

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 Changed 6 years ago by Sa'ar Zac Elias

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

Attachment: 7645_4.patch added

comment:12 in reply to:  11 ; Changed 6 years ago by Garry Yao

Status: review_failedreview

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

Keywords: NeedsTest added

comment:14 Changed 6 years ago by Frederico Caldeira Knabben

Keywords: NeedsTest removed
Last edited 6 years ago by Frederico Caldeira Knabben (previous) (diff)

comment:15 in reply to:  12 Changed 6 years ago by Sa'ar Zac Elias

Description: modified (diff)
Keywords: WebKit added
Status: reviewreview_failed
Summary: IE9: Lists not deleted properly using backspaceLists 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 6 years ago by Garry Yao

Attachment: 7645_5.patch added

comment:16 Changed 6 years ago by Garry Yao

Status: review_failedreview

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

Attachment: 7645_6.patch added

comment:17 Changed 6 years ago by Sa'ar Zac Elias

Status: reviewreview_passed

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

Last edited 6 years ago by Sa'ar Zac Elias (previous) (diff)

comment:18 Changed 6 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7243].

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