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

[buren]

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:

• Webkit bug: ​https://bugs.webkit.org/show_bug.cgi?id=133371
• Blink bug: ​https://code.google.com/p/chromium/issues/detail?id=313082

---

Workaround:

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

[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

}

[Jakub Ś]

DUP of #9835.

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

Please see #9835 for more details.

[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.

[Piotrek Koszuliński]

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

[Jakub Ś]

@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.

[Jakub Ś]

#11789 was marked as duplicate.

Here are some links from that ticket:

• ​http://stackoverflow.com/questions/21202684/ckeditor-inline-editing-on-jquery-ui
• ​http://ckeditor.com/forums/CKEditor/Solved-Chrome-Toolbar-buttons-grayed-out-for-INLINE-editor

[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.

[Piotrek Koszuliński]

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.

[Piotrek Koszuliński]

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

[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.

[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.

[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 this is 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.

[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.

[Marek Lewandowski]

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.

[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?

[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.

[Olek Nowodziński]

Rebased branches on latest master.

[Olek Nowodziński]

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.

2. 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).

3. 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).

4. 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.

5. Missing tests for framed editor and divarea in dt/core/editor/readonly.html.

[Olek Nowodziński]

Playground sample

[Olek Nowodziński]

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

[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.

[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.

[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.

[Marek Lewandowski]

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

[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.

[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.

[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)

[Piotrek Koszuliński]

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.

[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.

[Piotrek Koszuliński]

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.

[Piotrek Koszuliński]

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.

[Piotrek Koszuliński]

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

[Piotrek Koszuliński]

DUP reported #13461.