Ticket #8617 (closed Bug: fixed)

Opened 3 years ago

Last modified 2 years ago

Cursor jumps on Backspace in Chrome

Reported by: sheaujye2359 Owned by: garry.yao
Priority: Normal Milestone: CKEditor 3.6.3
Component: Core : Selection Version: 3.5.3
Keywords: Chrome Cc: fredck, camden.michael@…

Description

  1. Type ABC
  2. Select a Font Family
  3. Type DEF
  4. Select a Different Font Family
  5. Type GHI

By now the cursor should be after "I", pressing backspace should delete "I", but cursor quickly moved to after "F" and delete "F" weirdly.

This only happens on Chrome, NOT Safari, Nightly build is the same.

Attachments

8617.patch (1.2 KB) - added by garry.yao 3 years ago.
8617-2.patch (1.1 KB) - added by fdintino 3 years ago.
8617-3.diff (3.8 KB) - added by fdintino 3 years ago.
8617_1.patch (1.3 KB) - added by j.swiderski 2 years ago.
Character which caused editor not to load was removed.
8617_2.patch (1.7 KB) - added by garry.yao 2 years ago.
8617_3.patch (3.7 KB) - added by garry.yao 2 years ago.
8617_4.patch (2.3 KB) - added by fdintino 2 years ago.

Change History

comment:1 Changed 3 years ago by j.swiderski

  • Status changed from new to confirmed
  • Keywords Chrome added; Chrome, Backspace removed
  • Version changed from 3.6.2 to 3.5.3

I was not able to reproduc eyour issue but something very similar.

To reproduce:

  1. Type ABC
  2. Select with SHIFT+HOME and choose a Font Family.
  3. Put cursor behind "C", press ENTER and type DEF

By now the cursor should be after "F", pressing backspace should delete "F", but cursor quickly moved and joins ABC and DEF together.

This issue has been reproducible from CKEditor 3.5.3 rev [6461] (it's rev for CKE 3.5.2 it was probably included in 3.5.3.)

Tested on Chrome 16.0.912.75

Last edited 3 years ago by j.swiderski (previous) (diff)

comment:2 Changed 3 years ago by j.swiderski

@sheaujye2359 - if this is not what you are getting and TC to reproduce your issue please leave a comment.

comment:3 Changed 3 years ago by j.swiderski

#8627 was merked as duplicate.

Another TC:
When entering a sentence with emphasis - italic or bold - after completing the emphasized text, then some more - if you hit backspace the cursor is positioned at the beginning of the emphasis and backspaces occur there.

comment:4 Changed 3 years ago by j.swiderski

The one more use case (provided by @karena):

  1. Type any text and select it with ctrl-shift-left arrow
  2. Apply any font name
  3. Unselect text with right arrow key
  4. Hit Enter and type text, select it with ctrl-shift-left arrow and apply another font name

Note that range is collapsed, cursor jumped to start of the paragraph and font name is not applied.

I noticed that setting new text to filling char node (fillingChar.setText()) collapses the range.

Last edited 3 years ago by j.swiderski (previous) (diff)

comment:5 Changed 3 years ago by j.swiderski

#8697 was marked as duplicate.

comment:6 Changed 3 years ago by j.swiderski

TC from #8697 has helped me to find more simple way to reproduce the issue:

  1. In empty editor choose font style (bold for example)
  2. type word like “ckeditor” (don't click behind text) and press right arrow key

Result:
Cursor will move behind second letter c^keditor. It looks that CKEditor sees cursor in different place comparing it to what you see on screen.

comment:7 Changed 3 years ago by alissa

Hi We debugged this issue and realized that the issue is in the selection plugin in the 'removeFillingChar' function In Chrome this sets the all native selection offsets to 0, meaning the beginning of the span. We reproduced this bug with Spans and Enter-key

A patch would be appreciated ASAP as this is very urgent for us. Thank you

comment:8 Changed 3 years ago by garry.yao

Changed 3 years ago by garry.yao

comment:9 Changed 3 years ago by garry.yao

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

comment:10 Changed 3 years ago by sheaujye2359

Thanks garry for the patch, it seems to be fixed with the patch

comment:11 Changed 3 years ago by alissa

few changes to the patch:

  • if the offset is before the no space char then we don't need to change the offset
  • I don't see any reason to change the focusOffset only if the anchorOffset has changed
  • don't change offset 0

here is the altered code (I don't have it checked out from SVN)

function removeFillingChar( doc )
    {
        var fillingChar = doc && doc.removeCustomData( 'cke-fillingChar' );

        if ( fillingChar )
        {
            var sel =  doc.getSelection();
            // Text selection position might get mangled by
            // subsequent dom modification. #8617[1]
            var bm = getFixedBookmark(doc, sel, fillingChar);
            // We can't simply remove the filling node because the user
            // will actually enlarge it when typing, so we just remove the
            // invisible char from it.
            fillingChar.setText( fillingChar.getText().replace( /\u200B/g, '' ) );
            fillingChar = 0;
            bm && sel.selectBookmarks( [ bm ] );
        }
    }

    function getFixedBookmark(doc, sel, fillingChar){
        var nativeSel = sel.getNative();

        if(nativeSel.type != 'None'){
            var bm = sel.createBookmarks2()[ 0 ];
            var anchorFillingCharIndex = (nativeSel.anchorNode && nativeSel.anchorNode.data) ? nativeSel.anchorNode.data.indexOf('\u200B') : -1;
            var focusFillingCharIndex = (nativeSel.focusNode && nativeSel.focusNode.data) ? nativeSel.focusNode.data.indexOf('\u200B') : -1;
            if ( anchorFillingCharIndex > -1 && nativeSel.anchorOffset >  anchorFillingCharIndex){
                bm.startOffset--;
            }
            if(!nativeSel.isCollapsed && focusFillingCharIndex > -1 && nativeSel.focusOffset >  focusFillingCharIndex){
                bm.endOffset--;
            }
        }
        return bm;
    }

comment:12 Changed 3 years ago by fdintino

We created a patch for this issue at the Atlantic that is a bit more concise than the ones posted — I'm curious whether we're missing something. I'm attaching it for review. Below is an overview of the diff (leading whitespace stripped for readability):

Index: _source/plugins/selection/plugin.js
===================================================================
--- _source/plugins/selection/plugin.js (revision 7354)
+++ _source/plugins/selection/plugin.js (revision )
@@ -182,6 +182,14 @@
editor.on( 'beforeSetMode', function() { removeFillingChar( editor.document ); } );
editor.on( 'key', function( e )
    {
+       // Create a bookmark for the cursor so we can restore its position
+       // after removing the filling character.
+       //
+       // See #8617 Cursor jumps on Backspace in Chrome
+       // http://dev.ckeditor.com/ticket/8617
+       var selection = editor.getSelection();
+       var bookmark = ( selection ) ? selection.createBookmarks() : null;
+
        // Remove the filling char before some keys get
        // executed, so they'll not get blocked by it.
        switch ( e.data.keyCode )
@@ -193,6 +201,9 @@
            case 8 :    // BACKSPACE
                removeFillingChar( editor.document );
        }
+
+       if ( bookmark ) 
+           selection.selectBookmarks( bookmark );
    }, null, null, 10 );
 
var fillingCharBefore,

Changed 3 years ago by fdintino

comment:13 Changed 3 years ago by j.swiderski

#8696 was marked as duplicate.

comment:14 Changed 3 years ago by fdintino

Turns out I was missing something — createBookmarks/selectBookmarks wasn't being called for every instance of removeFillingChar, which caused some unexpected behavior. I'm attaching an amended patch.

Two notes on the patch posted by garry.yao: There appears to be a strange double quote on line 176, and line 177 has the typo

Error: Failed to load processor text/javascript
No macro or processor named 'text/javascript' found

Changed 3 years ago by fdintino

comment:15 Changed 3 years ago by alissa

there is a side effect.. try selecting some color writing something pressing enter twice -> you lose the color

or just choose color and press enter once

it happens because when the span is empty on selectBookmars the selection moves out of the span

Changed 2 years ago by j.swiderski

Character which caused editor not to load was removed.

comment:16 Changed 2 years ago by j.swiderski

#8756 was marked as duplicate.

Another TC from that ticket:

  1. Paste<p><span>just some text</span></p>
  2. Press enter and type something
  3. press enter

Second line will jump to third.

comment:17 Changed 2 years ago by j.swiderski

#8753 was marked as duplicate.

comment:18 Changed 2 years ago by fredck

  • Cc fredck added

Changed 2 years ago by garry.yao

comment:19 Changed 2 years ago by garry.yao

New patch addresses the following comments from @alissa:

  1. enter key to continue inline style;
  2. Keystroke immediately after opening style;

Manual tc added: http://ckeditor.t/tt/8617/1.html

comment:20 Changed 2 years ago by fredck

  • Status changed from review to review_failed

:/ it looks like the patch has been committed by mistake: [7376]. Can you check it, eventually putting things in order?

comment:21 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

Oops, trunk is now fixed for review.

comment:22 Changed 2 years ago by mrfr0g

  • Cc camden.michael@… added

comment:23 Changed 2 years ago by fredck

  • Status changed from review to review_failed
  • Component changed from General to Core : Selection

The patch seems to solve several of the issues.

But I'm still able to reproduce comment:4 and the following similar TC:

  1. CTRL+B to start bold.
  2. ENTER
  3. Type ABC.
  4. SHIFT+HOME or CTRL+SHIFT+LEFT to select ABC.
  5. ENTER

comment:24 Changed 2 years ago by fredck

  • Milestone set to CKEditor 3.6.3

Changed 2 years ago by garry.yao

comment:25 Changed 2 years ago by garry.yao

  • Status changed from review_failed to review

New patch change to intercept char code instead of keystroke, covering more keystrokes combination like shift-enter, shift-home, ctrl-arrow, alt-arrow...while we also need to understand it's impossible to exhaust all possible control-key combinations, e.g. Ctrl-A.

comment:26 follow-ups: ↓ 27 ↓ 28 Changed 2 years ago by mrfr0g

I'm not sure if this is related to this patch, but I have found the following behavior after applying the patch (8617_3);

  1. Write out a line of text, "Hello world"
  2. Use SHIFT+HOME to select the text, and then apply bold (CTRL+B)
  3. Press 'END' to get the end of the line, and press enter
  4. Write out another line of text, "Hello world" (line should be bold)
  5. Press enter twice

Result: A single space will be created after the second line

Expected result: Two empty lines should be created after the second line because enter was pressed twice

comment:27 in reply to: ↑ 26 Changed 2 years ago by garry.yao

Replying to mrfr0g:

I'm not sure if this is related to this patch, but I have found the following behavior after applying the patch (8617_3);

  1. Write out a line of text, "Hello world"
  2. Use SHIFT+HOME to select the text, and then apply bold (CTRL+B)
  3. Press 'END' to get the end of the line, and press enter
  4. Write out another line of text, "Hello world" (line should be bold)
  5. Press enter twice

Result: A single space will be created after the second line

Expected result: Two empty lines should be created after the second line because enter was pressed twice

WFM, can u reduce a bit the tc? thx.

comment:28 in reply to: ↑ 26 Changed 2 years ago by fredck

Replying to mrfr0g:

Well... the TC is not so complex, but it works for me as well.

comment:29 follow-ups: ↓ 30 ↓ 33 Changed 2 years ago by fdintino

I've attached a patch that I believe covers the test cases discussed so far. It's a slight modification of the patch in comment:11.

Changed 2 years ago by fdintino

comment:30 in reply to: ↑ 29 Changed 2 years ago by fredck

Replying to fdintino:

I've attached a patch that I believe covers the test cases discussed so far. It's a slight modification of the patch in comment:11.

Thanks for the new patch, but it fails with at least the comment:23 TC. I didn't test others.

comment:31 Changed 2 years ago by fredck

  • Status changed from review to review_passed

R+ on attachment:8617_3.patch for now.

@fdintino and others, if we feel it can be done in a different way, just let us know.

comment:32 Changed 2 years ago by fdintino

Hm, it looks like you're right about that test case; concur on the decision re: 8617_3.patch.

comment:33 in reply to: ↑ 29 Changed 2 years ago by mrfr0g

Replying to fdintino:

I've attached a patch that I believe covers the test cases discussed so far. It's a slight modification of the patch in comment:11.

You patch does allow the cursor to move out of the line, but it takes an additional enter key press to move out of the bold style and create a new line. Please see the following screencast, http://screencast-o-matic.com/watch/cleo2ABDz

comment:34 Changed 2 years ago by garry.yao

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

Thanks guys for participation, if possible please help us on insisting a browser fix here.

Fixed with [7393].

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