Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#9929 closed Bug (fixed)

[Blink Webkit]   is created when deleting character and typing common space

Reported by: sir qwerty Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 4.2.3
Component: Core : Keystrokes Version: 4.0
Keywords: WebKit Blink Cc:

Description (last modified by Jakub Ś)

This is new erroneous behavior in CKEditor 4 using Chrome 23, Win7 x64. Steps to reproduce:

  1. go to official demo page of CKEditor 4, wysiwyg mode
  2. delete any character using backspace or delete key
  3. immediately type a space just by plain spacebar
  4. see the source - nbsp is created instead of common space char

On my configuration SCAYT is disabled, no difference.


Another TC:

  1. Insert below code into editor and switch to WYSIWYG:
    <p>This is a simple sentence.</p>
    
  2. Delete all spaces and create them again

Result:

<p>This&nbsp;is&nbsp;a&nbsp;simple&nbsp;sentence.</p>

Problem can be reproduced in Blink and Webkit.

Change History (25)

comment:1 Changed 6 years ago by Jakub Ś

Keywords: nbsp space removed
Version: 4.0.13.0

This behaviour can actually be reproduced on Opera, Safari and Chrome from CKEditor 3.0 and you don’t need to press delete to get this result. Single space is enough. I'm not sure if this is a bug as this is how these browsers behave. It can be checked with page holding div with contenteditable="true" attribute.

comment:2 Changed 6 years ago by Piotrek Koszuliński

Status: newconfirmed

Kuba, you're not right here. This backspace changes behaviour.

  1. Type 'foooo' and place caret in the middle of this word.
  2. Press delete.
  3. Press space.
  4. Result: 'fo&nbsp;oo'.
  1. Type 'foooo' and place caret in the middle of this word.
  2. Press space.
  3. Result: 'foo oo'.

I can't find a reason explaining first behaviour and I cannot reproduce it on native contentediable, so it's somehow caused by editor.

comment:3 Changed 6 years ago by Jakub Ś

Keywords: Webkit added
Version: 3.04.0

@Reinmar you are right. Problem can be reproduced in Webkit browsers only from CKEditor 4.0 (3.x and 4 beta are free of this bug).

comment:4 Changed 6 years ago by Niklas

Is there anything happening with this bug?

comment:5 Changed 6 years ago by Niklas

I started to look into this. With Chrome's developer tools I found out a part of the reason for this:

I demonstrate DT output here:

Step 1: The beginning. I added new row with enter and typed "foobar". Everything seems to be ok.

<p>
  "foobar"
</p>

Quotes are extra, the actual data is <p>foobar</p>, but Chrome seems to break it into more peaces.

Step 2: Press backspace for "b". DT output becomes

<p>
  "foo"
  "ar"
</p>

The editor now shows it as "fooar", so there is no space between "foo" and "ar".

Step 3: Add space between "o" and "a". DT shows this:

<p>
  "foo&nbsp;"
  "ar"
</p>

It seems as on backspace/delete string is exploded into substrings for some reason and adding space to an end of a substring is converted into &nbsp;. If I type a "b" after "foo&nbsp;" it becomes:

<p>
  "foo b"
  "ar"
</p>

comment:6 Changed 6 years ago by Niklas

I think this issue can be fixed with very easy solution:

Replace

else if ( range.collapsed )
{
	// Handle the following special cases: (#6217)
	// 1. Del/Backspace key before/after table;
	// 2. Backspace Key after start of table.
	if ( ( block = path.block ) &&
		 range[ rtl ? 'checkStartOfBlock' : 'checkEndOfBlock' ]() &&
		 ( next = block[ rtl ? 'getPrevious' : 'getNext' ]( isNotWhitespace ) ) &&
		 next.is( 'table' ) )

with

else if ( range.collapsed )
{
	// Handle the following special cases: (#6217)
	// 1. Del/Backspace key before/after table;
	// 2. Backspace Key after start of table.
	if ( ( block = path.block ) &&
		 ( next = block[ rtl ? 'getPrevious' : 'getNext' ]( isNotWhitespace ) ) &&
		 next.is( 'table' ) &&
		 range[ rtl ? 'checkStartOfBlock' : 'checkEndOfBlock' ]() )

This way dom does not get changed by these checks if del/backspace is not table related.

comment:7 Changed 6 years ago by Niklas

I made a pull request for this: https://github.com/ckeditor/ckeditor-dev/pull/46

comment:8 Changed 6 years ago by sir qwerty

Almost a year and still producing tons and tons of unwanted &nbsp; entities during daily usage of CKeditor in Chrome for Windows. Just sweet.

comment:9 Changed 6 years ago by Jakub Ś

Description: modified (diff)
Keywords: Blink added

comment:10 Changed 6 years ago by Jakub Ś

Summary: [Chrome 23] &nbsp; is created when deleting character and typing common space[Blink Webkit] &nbsp; is created when deleting character and typing common space

#10917 was marked as duplicate

comment:11 Changed 6 years ago by fdintino

Is there something wrong with the pull request in comment:7, or something else holding it up? This bug is the number one complaint from our writers about CKEDITOR.

comment:12 Changed 6 years ago by Piotrek Koszuliński

We haven't got enough time to review it properly. We remember about it though and will check it as soon as possible.

Fortunately CKEditor's source is open, so if you find it ok, you can include it in your build.

comment:13 Changed 6 years ago by fdintino

I ran the patch and it appears to fix the issue for me. Note that Niklaus reposted the pull request to remove unrelated commits that had gotten attached. It is now pull request #60

Also, I don't want to insult anyone, but the line of code his patch is for… it speaks for itself:

if ( ( block = path.block ) &&
   range[ rtl ? 'checkStartOfBlock' : 'checkEndOfBlock' ]() &&
   ( next = block[ rtl ? 'getPrevious' : 'getNext' ]( isNotWhitespace ) ) &&
   next.is( 'table' ) )
{

Two assignments and one function call with DOM side-effects, all in the same conditional! I think Niklaus's patch is correct at the very least because this if block is only meant to apply to ranges which precede <table> elements in the DOM. It is also fine for the checkStartOfBlock / checkEndOfBlock call to follow the assignment of next, since the value returned from block.getPrevious() and block.getNext() won't be affected by the DOM changes that occur in range.checkStartOfBlock() or range.checkEndOfBlock().

comment:14 Changed 6 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.2.3

Thanks for the feedback. We'll test it and hopefully include it in 4.2.3 :).

comment:15 Changed 6 years ago by Frederico Caldeira Knabben

Keywords: WebKit added; Webkit removed

So, I've been checking this issue in depth. To start off, before others keep blaming on us, this is a browser bug (yet another).

I checked the patch that Niklaus brilliantly propose to us. It is not a required change, but it fixes this ticket tc. But the fact is that it is simply saying that calling checkStartOfBlock/checkEndOfBlock causes the problem.

The root of the problem is that checkStartOfBlock/checkEndOfBlock break text nodes (probably for simplification) . Due to a WebKit/Blink bug, a &nbsp; is inserted in the dom on SPACE, instead of a plain space.

What this all means is that Niklaus's fix may fix just one problematic case, but whenever WebKit/Blink will find two successive text nodes, a &nbsp; will be inserted.

For example, here we have an additional TC:

  1. Type "AB"
  2. Put the caret in between A and B
  3. CTRL+B to enable Bold
  4. CTRL+B to disable Bold
  5. SPACE

We gonna have A&nbsp;B.

So this is a general problem and we'll never be able to fix every single use case. Actually we shouldn't do so as this is a browser bug, so we should try to have things fixed on their side.

To this end, I have just opened issues on both WebKit and Blink:
https://bugs.webkit.org/show_bug.cgi?id=123163
https://code.google.com/p/chromium/issues/detail?id=310149

Please show your interest on the above issues, so eventually they'll have more attention.

comment:16 Changed 6 years ago by Frederico Caldeira Knabben

In any case, Niklaus patch should be ok and it would help reducing the impact of the browser bug for a quite common tc. As it doesn't change the code for real, it's worth proceeding with it and having it fixed at our side. So I confirm it on 4.2.3... unless browsers will be faster than us :)

comment:17 Changed 6 years ago by Frederico Caldeira Knabben

Owner: set to Frederico Caldeira Knabben
Status: confirmedreview

I've updated Niklaus's branch with the current master and pushed it into t/9929@cksource.

comment:18 Changed 5 years ago by Olek Nowodziński

Rebased branch on latest master.

comment:19 Changed 5 years ago by Olek Nowodziński

Status: reviewreview_passed

comment:20 Changed 5 years ago by Olek Nowodziński

Resolution: fixed
Status: review_passedclosed

Merged branch into master git:cd791a6 as a part of 4.2.3.

comment:21 Changed 5 years ago by Jakub Ś

Continuation of this ticket was created in #11415.

comment:22 Changed 5 years ago by Frank Farm

This problem appears to still exist in the standard editor currently at http://ckeditor.com/demo (which I presume is version 4) in Google Chrome 36.0.1985.143 on Mac 10.9.4. However, the problem does not appear with Firefox 30.0 and 31.0 on Mac 10.9.4.

How should I proceed to see this resolved in Google Chrome 36.0.1985.143 on Mac 10.9.4? Is this ticket reopened or should I create a new one? Or, is http://dev.ckeditor.com/ticket/11415 sufficient?

Last edited 5 years ago by Frank Farm (previous) (diff)

comment:23 Changed 5 years ago by Jakub Ś

Or, is http://dev.ckeditor.com/ticket/11415 sufficient?

Is the problem you are getting the same as in #11415? If not you can report new ticket (you can also add note that ticket might be related to #11415). If yes you can leave comment under #11415 with your steps to reproduce (if they differ from what was written in original test case for #11415)

In both cases we will verify what you have written and act accordingly (confirm, close ticket or open new ticket based on comment).

Please remember to provide step by step scenario that causes problems for you. Don't forget to mention Browser and operating system like you have done here.

comment:24 Changed 5 years ago by Frank Farm

#9929 and #11415 seem to be the same bug to me. #9929 is classified as version 4.0 but I can reproduce it in 4.4.3 currently at http://ckeditor.com/demo . #11415 is classified as version 3.0 and I can reproduce it in 4.4.3 currently at http://ckeditor.com/demo . in both cases the problem happens in chrome but not firefox.

comment:25 Changed 5 years ago by Jakub Ś

is classified as version 4.0 but I can reproduce it in 4.4.3

Version is used to indicate when problem has started.

Anyway this ticket will be continued in #11415.

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