Cannot highlight and delete a list item word

[MattMecham]

Steps to reproduce

Go to ckeditor.com/demo
Delete all default content
Create a list with two or more items, each item with two or more words
Exit the list as normal by hitting enter twice
Highlight one of the words on a single list item
Hit backspace

Result: Cursor jumps to the beginning of the word and does not delete the word

Expected Result: Word is deleted.

Original report: ​http://community.invisionpower.com/tracker/issue-34707-bulleted-list/ Video: ​http://screencast.com/t/6EP2XC0BP3M

[Jakub Ś]

To reproduce:

1. Paste the below code and switch to WYSIWYG

   <ul>
   	<li>
   		first word</li>
   	<li>
   		second word</li>
   	<li>
   		third word</li>
   </ul>
   

2. in second item select w^ord^ and click backspace

Results:

• In Webkit - cursor moves to the beginning of the selection. The second time you try that text gets deleted.
• In Opera - "w" gets deleted.

Bug has been reproducible from CKEditor 3.6.2 rev [7243] Recent regression

[fdintino]

This issue appears to happen within this call trace (leaving out all the initial listener and fire functions):

CKEDITOR.dom.selection.getSelectedElement()	plugins/selection/plugin.js:1143​
----→ CKEDITOR.tools.tryThese()	core/tools.js:741​
--------→ [anonymous] →	plugins/selection/plugin.js:1201​
------------→ CKEDITOR.dom.walker.checkBackward()	core/dom/walker.js:284​
----------------→ iterate()	core/dom/walker.js:29​
--------------------→ CKEDITOR.dom.range.trim()	core/dom/range.js:895​

CKEDITOR.dom.range.trim() can alter the DOM if the anchor or focus of the selection lies in the middle of a textNode. This issue arises because webkit browsers unselect active ranges when there are DOM changes on associated nodes (this behavior was part of the issue in ticket #8617). This is also the reason why the delete works the second time, since after the first unsuccessful delete the selected text range is split off from the containing textNode into its own node, so on the second attempt CKEDITOR.dom.range.trim() doesn't perform any DOM modifications.

I'm attaching a patch for review that sets a bookmark before the calls to walker.checkBackward()/walker.checkForward() on webkit browsers and restores the select after their execution. From the above description it sounds as though the issue in Opera might be distinct, though I can't say for certain not having tested it.

Here's an excerpt of the patch (preceding whitespace stripped for legibility):

_source/plugins/selection/plugin.js

@@ -1198 +1198 @@
 
         walker.guard = guard( walker );
 
+        // CKEDITOR.dom.walker.checkBackward()/checkForward() can
+        // make modifications to the DOM, and this has the effect
+        // of removing the selection in webkit. We bookmark the
+        // selection here for this contingency. (#8632)
+        var bm;
+        if ( CKEDITOR.env.webkit )
+                bm = self.createBookmarks()[ 0 ];
+
         if ( walker.checkBackward() && !walker.halted )
         {
                 walker = new CKEDITOR.dom.walker( testRange );
@@ -1207 +1215 @@
                 if ( walker.checkForward() && !walker.halted )
                 retval = root.$;
         }
+
+        // Restore the selection after running the walker methods. (#8632[2])
+        bm && self.selectBookmarks( [ bm ] );
         }
 
         if ( !retval )

[Jakub Ś]

Other tickets caused by rev [7243]: #8455, #8475, #8493

[Garry Yao]

I have a investigation on almost all walker usages and it turns out the text node splitting behavior (cursor panic) is not relied on at all, so it looks correct for me if we just change a bit how walker works, so that it will not anymore be dom deconstructive, the cost would be only a few dt fixes.

[Garry Yao]

Move to current milestone due to its severity.

[Frederico Caldeira Knabben]

It looks like this change is ok, considering our current use of it.

If any future development will require the previous behavior, it would be as simple as calling trim() before creating the walker. So we should be pretty safe.

[Frederico Caldeira Knabben]

Btw, please have the tests passing when committing.

[Garry Yao]

Fixed with [7395], dts fixed on master.

[Garry Yao]

Unfortunately, this commit is still found to break #7645, post fixed with [7397].