Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#14279 closed Bug (invalid)

pressing delete key inside empty td[contenteditable="true"]

Reported by: Sergiy Kuzmenko Owned by:
Priority: Normal Milestone:
Component: Core : Editable Version:
Keywords: Cc:

Description (last modified by Jakub Ś)

Steps to reproduce

  1. Set CKEDITOR.dtd.$editable.td = 1 to enable editing inside td.
  2. Create the following HTML layout:
    <table border="0" cellpadding="0" cellspacing="0" width="600">
        <tbody>
	    <tr>
                <td width="100%" contenteditable="true"></td>
	    <tr>
        </tbody>
    <table>
    <table border="0" cellpadding="0" cellspacing="0" width="600">
        <tbody>
	    <tr>
                <td width="100%" contenteditable="true"></td>
	    <tr>
        </tbody>
    <table>
  1. Click on the last (empty) td and press DELETE key

Expected result

Nothing to happen.

Actual result

  1. Zero Width Space is inserted between tables.
  2. Cursor disappears.
  3. Pressing DELETE again will throw Uncaught TypeError: Cannot read property 'startPath' of undefined here. This will cause onKeyDown event propagation which in Chrome and Opera will navigate back in history.

Other details (browser, OS, CKEditor version, installed plugins)

All WebKit browsers. OSX (but I'm fairly sure this affects WebKit browsers on any OS).

Solution

Removing range.select(); solves the problem.

It is hard to tell for the uninitiated what's going on here but it seems CKEditor erroneously treats <td contenteditable="true"></td> as if it were a child of content editable block, rather than content editable container. This is further aggravated by handling WebKit quirks (if range.select(); is called).

Attachments (1)

inlineall2.html (1.0 KB) - added by Jakub Ś 3 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 Changed 3 years ago by Sergiy Kuzmenko

There are actually two separate issues here. Both are triggered on adjacent tables with content editable td when pressing DELETE inside empty cell:

  1. Insertion of ZWS between the tables (i.e. outside content editable) is Webkit specific error.
  2. Cursor disappearance and subsequent JS error. This one happens in all browsers (in Webkit it's only aggravated by event propagation which moves us back in history).

For my project, I solved this issue by simply capturing and stoping CKEditor's key event. It appears to work well without any side effects. This makes me think that special handling of deletion is a workaround for historical quirks which are no longer present in modern browsers.

comment:2 Changed 3 years ago by Jakub Ś

Description: modified (diff)
Status: newpending
Version: 4.5.4

I have tried reproducing this issue in Safari 9.0 on Mac 10.11 and in Chrome on Windows. In both cases nothing happened as expected.

I have tried version 4.5.4-4.5.6 and full package. I could not reproduce it. I have used below code. Did I do something wrong?

CKEDITOR.dtd.$editable.td = 1;
var editor = CKEDITOR.replace( 'editor1', {
	allowedContent : true
});

Could you provide more details perhaps?


By the way, why are you modifying dtd? Are you perhaps trying to create a widget? If so why couldn't you use this option for defining table column as editable: http://docs.ckeditor.com/#!/api/CKEDITOR.plugins.widget.definition-property-editables

comment:3 Changed 3 years ago by Sergiy Kuzmenko

The problem is with inline editing. Here's JS Fiddle: https://jsfiddle.net/de7xcz0x/. As per comment, cursor disappearance and JS error happen in all browsers. To replicate: press Backspace Delete key at the beginning of empty cell more than once. In addition to that, webkit places ZWS outside content editable.

Why modifying dtd? Because of inline editing inside TD and by default CKEditor does not load inside td[contenteditable="true"]. Why TD and not DIV? Still need TD in "email friendly" templates for Outlook compatibility.

Changed 3 years ago by Jakub Ś

Attachment: inlineall2.html added

comment:4 Changed 3 years ago by Jakub Ś

Please see attached file inlineall2.html.

Are you able to use divs inside the table? Will outlook accept this?

At the moment you are changing editor default behaviour (I believe there was a reason for not allowing td's to be editables. Will verify it in Monday) while there might be simple workaround which allows using Div, Paragraph or perhaps span inside TD element.

comment:5 Changed 3 years ago by Sergiy Kuzmenko

Either changing <td></td> to <td><div></div></td> or <table>...</table> to <div><table>...</table></div> solves my problem. Outlook will accept both but I wanted to keep HTML cruft at minimal level. So I ended up adding keyboard event handler which simply stops CKEDITOR default behaviour:

    if(event.data.keyCode == 8 || event.data.keyCode == 46) {
        event.stop();
    }

This makes me think that CKEditor attempts to solve a problem which is no longer there and by doing so it breaks certain edge cases. If you believe otherwise please feel free to close this ticket as won't fix.

comment:6 Changed 3 years ago by Jakub Ś

This is your decision but please note that much better solution is using div inside table.

With your current solution you modify CKEditor internals, this is causing error and then you workaround this error. What if your users will want to use Del button to delete text?

comment:7 Changed 3 years ago by Jakub Ś

Resolution: invalid
Status: pendingclosed

comment:8 Changed 3 years ago by Sergiy Kuzmenko

It deletes text. :) I'm cancelling CKEditor's fix for deletion, not browser event. I have a bunch of Selenium tests which confirm deletion is performed correctly in all browsers.

IMO, no matter where inline editable occurs the editor should not modify any content outside it.

comment:9 Changed 3 years ago by Sergiy Kuzmenko

BTW, CKEDITOR.dtd.$editable is "special" public API: http://docs.ckeditor.com/#!/api/CKEDITOR.dtd-property-S-editable. So I don't think what I'm doing qualifies as modifying CKEditor's internals.

comment:10 Changed 3 years ago by Piotrek Koszuliński

It is public, but it does not say that whatever you'll change there, it'll work ;). DTD is only a configuration, there must be also a support for certain settings. And there's definitely no support for making table cells editable.

This ticket could be transformed into new feature request. However, I rather recommend using divs inside table cells to workaround this limitation.

comment:11 Changed 3 years ago by Jakub Ś

It deletes text. :) I'm cancelling CKEditor's fix for deletion,

Sorry for that. I just had a quick glimpse at the 'if' and assumed there is event.cancel() and not event.stop()

BTW, CKEDITOR.dtd.$editable is "special" public API:

Yes it is but as @Reinmar has explained it. Not everything will work after you change DTD. There is no fixed set of allowed things you can set/change (like with other config optioons) so there some features may get broken.

So I don't think what I'm doing qualifies as modifying CKEditor's internals.

All I wanted to say is that I believe it is better to use extra HTML node than making a change in editor and then making a workaround for that change. This is just how I see it.

comment:12 Changed 3 years ago by Sergiy Kuzmenko

I wouldn't do a feature request to make tables editable. Divs are better. And in few years Outlook will go away. Even today its market share is pretty low. Thanks for your time.

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