Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#8632 closed Bug (fixed)

Cannot highlight and delete a list item word

Reported by: MattMecham Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Lists Version: 3.6.2
Keywords: Webkit Opera Cc:

Description

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

Attachments (2)

8632.patch (1.0 KB) - added by fdintino 5 years ago.
8632_2.patch (3.6 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by j.swiderski

  • Keywords Webkit Opera added
  • Status changed from new to confirmed
  • Version set to 3.6.2

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

comment:2 Changed 5 years ago by 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

     
    11981198
    11991199        walker.guard = guard( walker );
    12001200
     1201        // CKEDITOR.dom.walker.checkBackward()/checkForward() can
     1202        // make modifications to the DOM, and this has the effect
     1203        // of removing the selection in webkit. We bookmark the
     1204        // selection here for this contingency. (#8632)
     1205        var bm;
     1206        if ( CKEDITOR.env.webkit )
     1207                bm = self.createBookmarks()[ 0 ];
     1208
    12011209        if ( walker.checkBackward() && !walker.halted )
    12021210        {
    12031211                walker = new CKEDITOR.dom.walker( testRange );
     
    12071215                if ( walker.checkForward() && !walker.halted )
    12081216                retval = root.$;
    12091217        }
     1218
     1219        // Restore the selection after running the walker methods. (#8632[2])
     1220        bm && self.selectBookmarks( [ bm ] );
    12101221        }
    12111222
    12121223        if ( !retval )

Changed 5 years ago by fdintino

comment:3 Changed 5 years ago by j.swiderski

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

Changed 5 years ago by garry.yao

comment:4 Changed 5 years ago by garry.yao

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

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.

comment:5 Changed 5 years ago by garry.yao

  • Milestone set to CKEditor 3.6.3

Move to current milestone due to its severity.

comment:6 Changed 5 years ago by fredck

  • Status changed from review to review_passed

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.

comment:7 Changed 5 years ago by fredck

Btw, please have the tests passing when committing.

comment:8 Changed 5 years ago by garry.yao

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

Fixed with [7395], dts fixed on master.

comment:9 Changed 5 years ago by garry.yao

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

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