Opened 6 years ago

Last modified 3 years ago

#9814 confirmed Bug

Inline editor created in "display:none" element results in editor with disabled buttons

Reported by: buren Owned by:
Priority: Normal Milestone:
Component: General Version: 4.0
Keywords: Webkit Blink Cc:

Description (last modified by Piotrek Koszuliński)

If rendered as below contenteditable will be set to false automatically (I guess by ckeditor). However if I set the div #my-id as visible with javascript and at the same time set contenteditable back to true the editor will still be in readonly mode. (Sometimes I can for some reason use copy-paste to enter text to the editor but I can't write regularly with the keyboard.)

Sent by the server: <div id="my-id" style="display:none;">

<h3 contenteditable="true">Header</h3>

</div>

After page render by the browser (it changed contenteditable to false): <div id="my-id" style="display:none;">

<h3 contenteditable="false" class="ck-editable cke_editable cke_editable_inline cke_contents_ltr cke_focus"" spellcheck="false">Header</h3>

</div>

After my custom javascript (removes display:none & sets h3 tag to contenteditable="true"): <div id="my-id" style="display:none;">

<h3 contenteditable="true" class="ck-editable cke_editable cke_editable_inline cke_contents_ltr cke_focus"" spellcheck="false">Header</h3>

</div>


Issue is caused by:


Workaround:

var ck = CKEDITOR.inline(element);
ck.on( 'instanceReady', function( ev ) {
     var editor = ev.editor;
     editor.setReadOnly( false );
});

Attachments (2)

display_none.html (9.9 KB) - added by cheesypoof 5 years ago.
9814.html (1.3 KB) - added by Olek Nowodziński 4 years ago.
Playground sample

Download all attachments as: .zip

Change History (38)

comment:1 Changed 6 years ago by buren

I saw this in the source code.. so I guess its somewhat a feature.... but still wondering how to get around this, to be able to edit content that is displayed in collapsable boxes..

isEditable: function (a) {

var b = this.getName(); if (this.isReadOnly() | | this.getComputedStyle("display") == "none" | | this.getComputedStyle("visibility") == "hidden" | | CKEDITOR.dtd.$nonEditable[b] | | CKEDITOR.dtd.$empty[b] | | this.is("a") && (this.data("cke-saved-name") | | this.hasAttribute("name")) && !this.getChildCount()) return false; if (a !== false) {

a = CKEDITOR.dtd[b] | | CKEDITOR.dtd.span; return !(!a | | !a#)

} return true

}

comment:2 Changed 6 years ago by Jakub Ś

Keywords: inline removed
Resolution: duplicate
Status: newclosed

DUP of #9835.

Automatic initialization works only for elements which exist on page when it's loaded.

Please see #9835 for more details.

Changed 5 years ago by cheesypoof

Attachment: display_none.html added

comment:3 Changed 5 years ago by cheesypoof

I don't believe this ticket was handled properly. The op did not in any way suggest that he inserted element(s) into the DOM with JavaScript. He merely attempted some scripting remedies after CKEditor did not behave as he expected.

With that said, the issue still remains as of 4.3.2... Please see my attached example - drop it in your samples folder.

In Chrome (and maybe other WebKit browsers), a hidden editor instance is automatically set to readOnly mode. This is not the case in Firefox and IE.

Thanks.

comment:4 Changed 5 years ago by Piotrek Koszuliński

Resolution: duplicate
Status: closedreopened

I agree. This ticket was incorrectly closed as a DUP of #9835. There's a difference between nonexisting element and hidden element.

comment:5 Changed 5 years ago by Jakub Ś

Keywords: Webkit Blink added
Status: reopenedconfirmed

@cheesypoof thanks for pointing this out.

Problem can be reproduced from CKEditor 4.0 in Webkit and Blink browsers only.

In CKEditor 4.0 beta editor wasn't initialized at all.

comment:6 Changed 5 years ago by Jakub Ś

Summary: Inline editor with css property "display:none"Inline editor created in "display:none" element results in editor with disabled buttons

comment:7 Changed 5 years ago by Bram

The description of #11789 explains the problem much better and provides a working example. This issue has already been closed falsely, probably because the original description is not clear enough. Would it be possible to update the description with the one from the duplicate issue?

After more then a year and a half it would be nice to get the balling rolling on this.

Last edited 5 years ago by Bram (previous) (diff)

comment:8 Changed 5 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.2

I remember that there were some problems with initializing editor in hidden elements. Problems which could not be solved. But if I remember correctly, they were related to the classic editor (using iframe). It indeed makes sense to investigate the case described in this ticket, because it's related to inline editor, so it may be a different problem.

However, we should check if all kinds (even classic editor) of editors can be initialized in hidden elements and if they are usable after the elements have been shown. This TC covers only setting display property. Not detaching editor's DOM which is a separate topic (and in case of classic editor it won't be fixed because of browsers nature and complexity of the hack).

I'm setting milestone to 4.4.2. It does not mean that we'll fix all issues. First we need to find what is really broken.

Last edited 5 years ago by Piotrek Koszuliński (previous) (diff)

comment:9 Changed 5 years ago by Piotrek Koszuliński

I rewrote comment:8, because I found it unclear after a while :D

comment:10 Changed 5 years ago by Bram

I found this Chromium issue that might be relevant: https://code.google.com/p/chromium/issues/detail?id=313082. Chromium consider all hidden elements un-editable even if they have the attribute content-editable set to true.

I'm not familiar enough with the CKEditor code base to just dive in and check if the editor uses isContentEditable. If that is the case I think we have found the origin of this issue.

Update: after some googling if found the following line of code that confirms my suspicion: https://github.com/ckeditor/ckeditor-dev/blob/master/core/dom/node.js#L727. The isReadOnly function will return false for hidden contenteditable=true elements in chrome. I'm not sure when or where this read only check is called but it guess it's not being called when the visibility of the element changes, causing the element to be stuck in read only mode.

Last edited 5 years ago by Bram (previous) (diff)

comment:11 in reply to:  10 ; Changed 5 years ago by Piotrek Koszuliński

Replying to bramcordie:

I found this Chromium issue that might be relevant: https://code.google.com/p/chromium/issues/detail?id=313082. Chromium consider all hidden elements un-editable even if they have the attribute content-editable set to true.

I'm not familiar enough with the CKEditor code base to just dive in and check if the editor uses isContentEditable. If that is the case I think we have found the origin of this issue.

Thanks! It may be a reason or one of reasons. Editor starts in read only mode and that mode is not refreshed (because how could it be) when container's visibility is changed.

comment:12 in reply to:  11 Changed 5 years ago by Bram

Replying to Reinmar:

Editor starts in read only mode and that mode is not refreshed (because how could it be) when container's visibility is changed.

Having to watch the visibility of each element would indeed be a hassle (is it even possible without polling?). As far as I know the only way to make the editor's control bar to appear is when you focus the content-editable element. If this is the case, the focus event could be used to refresh the isReadOnly check and render the control bar correctly based on the element's current visibility.

What I ended up doing is forcing the editor out of read-only mode when it gets focused. No the cleanest solution but it works for my use case. For anyone else interested, an example below:

var editor = CKEDITOR.inline("editorID", config);

editor.on('focus', function () {
    editor.setReadOnly(false);
});

A better solution would be to use CKEditor's build in isReadOnly function whenever an inline editor gets focused and set read only mode accordingly in the background. If you can point me in the right direction to where I can implement this, I would be happy to hack on it and send you a pull request on Github.

Last edited 5 years ago by Bram (previous) (diff)

comment:13 Changed 5 years ago by Marek Lewandowski

Assumptions given by bramcordie are correct.

Updating CKEDITOR.dom.node#isReadOnly would indeed fix this particular issue, but we're concerned about its performance cost.

We're going to investigate more UCs coming from the same source.

comment:14 Changed 5 years ago by Marek Lewandowski

Owner: set to Marek Lewandowski
Status: confirmedassigned

comment:15 Changed 5 years ago by Marek Lewandowski

Status: assignedreview

In addition i've enhanced this code a little bit to better work with contenteditable values like empty string, or TRUE uppercased.

Pushed to t/9814 at dev and t/9814 at tests.

comment:16 Changed 5 years ago by Piotrek Koszuliński

As for the value normalization - CKEditor requires it to be exactly true in dozen of places and don't think we should bloat code in order to handle all variants.

Very interesting approach with using the deoptimised version. Have you checked already if editor really works when browser says that the editable element isn't editable but editor is initialized as not in read only mode?

comment:17 Changed 5 years ago by Marek Lewandowski

I've tested it in case when parent div was not visible (so case when Webkit returns false for isContentEditable) mainly for things like selection, focus, set/getData, ACF.

It turned out not to be diffrent from inline editor in typical case. I've found one issue with selection not being inited, but it's a subject for separate ticket: #11948.

Last edited 5 years ago by Marek Lewandowski (previous) (diff)

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

Rebased branches on latest master.

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

Status: reviewreview_failed
  1. Our problem has nothing to do with webkit or any other specific browser engine. At least in my opinion supported by the research. So this line is wrong https://github.com/cksource/ckeditor-dev/blob/52c242025902e3e308b95f442aa5903bd211ec39/core/dom/node.js#L729
    1. And dt/core/dom/node.html#test isReadOnly allowDeopt is also wrong.
  1. We do not support contenteditable="TRUE" or contenteditable or contenteditable="tRuE" in general. Considering those cases in node.isReadOnly makes no sense since there are billions of other methods/conditions which simply ignore it (https://github.com/cksource/ckeditor-dev/blob/52c242025902e3e308b95f442aa5903bd211ec39/core/dom/node.js#L739-L745).
  1. Most of additional assertions in dt/core/dom/node.html#test_isReadOnly makes no sense because
    1. 2.
    2. They do not mirror preceding assertions (those without an argument in isReadOnly).
  1. In dt/core/editor/readonly.html you specified the following editor
    +            // This editor is inited in hidden div, then after its initialization, its parent
    +            // element display should be switched to visible.
    +            inline_hidden: {
    +                name: 'inline_hidden',
    +                creator: 'inline',
    +                readOnly: false,
    +                config: {
    +                    on: {
    +                        instanceReady: function() {
    +                            // We need to show parent block after initialization.
    +                            CKEDITOR.document.getById( 'inline_hidden_wrapper' ).setStyle( 'display', 'block' );
    +                        }
    +                    }
    +                }
    
    which defined an obsolete instanceReady listener. It is obsolete because automated tests in this file check editor.readOnly once – when editor is ready. Whatever you do next there does not change anything.
  1. Missing tests for framed editor and divarea in dt/core/editor/readonly.html.

Changed 4 years ago by Olek Nowodziński

Attachment: 9814.html added

Playground sample

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

Owner: changed from Marek Lewandowski to Olek Nowodziński
Status: review_failedassigned

comment:21 Changed 4 years ago by Olek Nowodziński

Status: assignedreview

Pushed a simplified solution to branch:t/9814b (+tests).

comment:22 in reply to:  19 Changed 4 years ago by Marek Lewandowski

Replying to a.nowodzinski:

  1. Issue is present only in webkit browsers, other determine value correctly, so there is no reason to decrease performance for other browsers.
  2. contenteditable attribute is case-insensitive, all UA treats that this way
  3.  
    1. 2
    2. These initial tests might be in fact duplicated for deoptimized version, and vice-versa
  4. That's didn't know that, good
  5. Correct, should be added.

At the end solution in t/9814b is essentially the same as in t/9814 but:

  • not respecting uppercased values
  • uses this alternative computing for all browsers, even these which does not need it

I think we should add tests to t/9814 and proceed with it, since it's not really needed to use that for all browsers.

Last edited 4 years ago by Marek Lewandowski (previous) (diff)

comment:23 Changed 4 years ago by Piotrek Koszuliński

not respecting uppercased values

We do not respect uppercased values in dozens of other places too. Hence, we can say that we require the lowercase value.

uses this alternative computing for all browsers, even these which does not need it

We should avoid per browser code branching if the situation does not force us to do so. And AFAIR the useDeopt was overriden inside isReadOnly(), taking the free will away from developer using that method.

comment:24 Changed 4 years ago by Piotrek Koszuliński

uses this alternative computing for all browsers, even these which does not need it

We should avoid per browser code branching if the situation does not force us to do so. And AFAIR the useDeopt was overriden inside isReadOnly(), taking the free will away from developer using that method.

Sorry for making my last comment unclear. The two points - one about code branching and one about making decision inside isReadOnly() are independent.

BTW. More related reason for avoiding code branching - what if FF or IE will change their behaviour to conform to Webkit/Blink's? The Webkit/Blink one makes sense, so this is not impossible. Sometimes it's impossible, but this time we can handle what may come in advance.

Last edited 4 years ago by Piotrek Koszuliński (previous) (diff)

comment:25 Changed 4 years ago by Marek Lewandowski

Comment 23 was clear imo, alright in such situation t/9814b seems to be the way to go.

comment:26 Changed 4 years ago by Olek Nowodziński

Hum. That's funny: yesterday it seemed to fail on all browsers but today it's indeed just Webkit. So yes, we can restrict that fix to Webkit-based browsers.

As for the support of different cases of contenteditable attribute – no, we don't want to do that. Right now, we don't support other cases in our codebase (~25 places) so isReadOnly should do neither.

Last edited 4 years ago by Olek Nowodziński (previous) (diff)

comment:27 Changed 4 years ago by Piotrek Koszuliński

Hum. That's funny: yesterday it seemed to fail on all browsers but today it's indeed just Webkit. So yes, we can restrict that fix to Webkit-based browsers.

Why would we do that? Please see comment:23 and comment:24.

comment:28 in reply to:  27 Changed 4 years ago by Olek Nowodziński

Replying to Reinmar:

Hum. That's funny: yesterday it seemed to fail on all browsers but today it's indeed just Webkit. So yes, we can restrict that fix to Webkit-based browsers.

Why would we do that? Please see comment:23 and comment:24.

OK. As we agreed, we don't want that code branching:

  1. isReadOnly( 1 ) will be called once per instance, thus it's a negligible performance loss
  2. If other vendors decide to make their browsers act like Webkit/Blink, future, yet old CKEditor installations would be exposed to this issue.

branch:t/9814b (+tests)

comment:29 Changed 4 years ago by Piotrek Koszuliński

Status: reviewassigned

It turned out that current behaviour of Blink/Webkit is inconsistent with the spec. I thought that it's not included in any spec, but it is: https://www.w3.org/Bugs/Public/show_bug.cgi?id=25908

I'll try to convince Blink/Webkit teams to fix this behaviour. For now, I'm leaving this ticket opened. We can make decision right before closing 4.4.2.

comment:30 Changed 4 years ago by Piotrek Koszuliński

I reported also a ticket for Webkit https://bugs.webkit.org/show_bug.cgi?id=133371 because the one for Blink was reopened https://code.google.com/p/chromium/issues/detail?id=313082

So, let's wait now for engine authors reaction. If it will look promising, we'll avoid making unnecessary changes in our code.

comment:31 Changed 4 years ago by Piotrek Koszuliński

Description: modified (diff)

comment:32 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.2CKEditor 4.4.3
Owner: Olek Nowodziński deleted
Status: assignedconfirmed

According to Webkit developers this bug is not anymore reproducible on Webkit. Blink developers need some time to fix it too, so let's wait few more weeks.

comment:33 Changed 4 years ago by Piotrek Koszuliński

Milestone: CKEditor 4.4.3

Both (Webkit and Blink) bugs (see comment:30) will be fixed soon, so there's no need to change anything on our side. I'm removing milestone.

comment:34 Changed 4 years ago by Piotrek Koszuliński

This bug is no longer reproducible on Safari 7.1. So only Chrome and Opera left.

comment:35 Changed 4 years ago by Piotrek Koszuliński

Description: modified (diff)

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

DUP reported #13461.

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