Opened 14 years ago
Closed 13 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 )
Steps to reproduce the defect:
- Open the Ajax sample.
- 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>
- Switch back to source mode.
- 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> </li> </ol> </li> </ol>
Attachments (7)
Change History (25)
Changed 14 years ago by
Attachment: | IE9 Delete List Defect.JPG added |
---|
comment:1 Changed 14 years ago by
Keywords: | IE added; IE9 removed |
---|---|
Version: | 3.5.4 (SVN - trunk) → 3.0 |
comment:2 Changed 14 years ago by
Resolution: | → invalid |
---|---|
Status: | new → closed |
comment:3 Changed 14 years ago by
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 14 years ago by
Resolution: | invalid |
---|---|
Status: | closed → reopened |
comment:5 Changed 14 years ago by
Status: | reopened → 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> </li> <li> </li> <li> </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> </li> </ol>
Pressing backspace or del one more time removes the leftovers.
In FireFox it works.
Changed 14 years ago by
Attachment: | 7645.patch added |
---|
comment:6 Changed 14 years ago by
Owner: | set to Sa'ar Zac Elias |
---|---|
Status: | confirmed → review |
Patch introduces a new IE9-friendly version of fixListRange that was proposed in #6432.
Changed 14 years ago by
Attachment: | 7645_2.patch added |
---|
comment:7 Changed 14 years ago by
Status: | review → 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 14 years ago by
Owner: | changed from Sa'ar Zac Elias to Garry Yao |
---|---|
Status: | review_failed → review |
comment:9 Changed 14 years ago by
Status: | review → 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 14 years ago by
Attachment: | 7645_3.patch added |
---|
comment:10 Changed 14 years ago by
Status: | review_failed → 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: 12 Changed 14 years ago by
Status: | review → review_failed |
---|
- 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).
- 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
- In Webkit (Chrome at least), following TC fails:
<ol> <li> [item1 <ol> <li> item2]</li> </ol> </li> </ol>
Changed 13 years ago by
Attachment: | 7645_4.patch added |
---|
comment:12 follow-up: 15 Changed 13 years ago by
Status: | review_failed → review |
---|
Replying to Saare:
- With the following:
I don't understand how this's related....
- 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.
- 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 13 years ago by
Keywords: | NeedsTest added |
---|
comment:14 Changed 13 years ago by
Keywords: | NeedsTest removed |
---|
comment:15 Changed 13 years ago by
Description: | modified (diff) |
---|---|
Keywords: | WebKit added |
Status: | review → review_failed |
Summary: | IE9: Lists not deleted properly using backspace → 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.
- Ther're some debugger leftovers in the patch.
- http://ckeditor.t/dt/plugins/styles/styles.html fails in WebKit.
Changed 13 years ago by
Attachment: | 7645_5.patch added |
---|
comment:16 Changed 13 years ago by
Status: | review_failed → 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 13 years ago by
Attachment: | 7645_6.patch added |
---|
comment:17 Changed 13 years ago by
Status: | review → review_passed |
---|
R+ for 7645_6, to avoid redudant changes in 7645_5.
comment:18 Changed 13 years ago by
Resolution: | → fixed |
---|---|
Status: | review_passed → closed |
Fixed with [7243].
It's not IE specific and the backspace key behavior over range selection is quite browser dependent, we suggest not to rely on it.