Opened 12 years ago

Closed 12 years ago

Last modified 12 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 12 years ago.
8632_2.patch (3.6 KB) - added by Garry Yao 12 years ago.

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by Jakub Ś

Keywords: Webkit Opera added
Status: newconfirmed
Version: 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 12 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 12 years ago by fdintino

Attachment: 8632.patch added

comment:3 Changed 12 years ago by Jakub Ś

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

Changed 12 years ago by Garry Yao

Attachment: 8632_2.patch added

comment:4 Changed 12 years ago by Garry Yao

Owner: set to Garry Yao
Status: confirmedreview

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

Milestone: CKEditor 3.6.3

Move to current milestone due to its severity.

comment:6 Changed 12 years ago by Frederico Caldeira Knabben

Status: reviewreview_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 12 years ago by Frederico Caldeira Knabben

Btw, please have the tests passing when committing.

comment:8 Changed 12 years ago by Garry Yao

Resolution: fixed
Status: review_passedclosed

Fixed with [7395], dts fixed on master.

comment:9 Changed 12 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 – 2022, CKSource sp. z o.o. sp.k. All rights reserved. | Terms of use | Privacy policy