Read-only: Bug feature for inline elements

[Frederico Caldeira Knabben]

Currently, the style system is made in a way that it will never touch read-only elements.

The above is good for block elements, but when we talk about inline elements, this is not true.

Here we have a simple TC:

1. Load the following:

<p>
    My name is <span contenteditable="false">%NAME%</span>,
 I&#39;m from <span contenteditable="false">%CITY%</span>.</p>

2. CTRL+A to select all.
3. Click the Bold button.

Current Results:

<p>
	<strong>My name is </strong><span contenteditable="false">%NAME%</span><strong>,
 I&#39;m from </strong><span contenteditable="false">%CITY%</span><strong>.</strong></p>

Expected Results:

<p>
	<strong>My name is <span contenteditable="false">%NAME%</span>,
 I&#39;m from <span contenteditable="false">%CITY%</span>.</strong></p>

So, the basic ideas is that, if a read-only inline element is fully included in the selection, then it get's included in the style range.

Is this assumption correct? Are there cases where this is not desired?

[Sa'ar Zac Elias]

I don't think so. Altough it does make some sense, I believe that readonly elements should never be changed. That's what they're all about.

[Charlie]

I think that both cases are valid. It should be up to the user to decide via a config option. "Allow readonly style changes" or something... Different people will use the contenteditable property for different reasons and I could see someone using it in the manner fred did (Or wanting to regardless of intended behavior)

Also, the term is contenteditable...perhaps they should also add a styleeditable attribute so we're all sure ;)

[Damian]

I would tend to agree with comp615, I can think of cases where authors might want to preserve styles within the non-editable spans.

[Damian]

Just to add to my previous comment. If a config parameter were added to alter the behaviour I think the default should be not to touch the read only elements.

[Frederico Caldeira Knabben]

My point is... we're not really touching the readonly element, but instead changing the context within which it's included, which instead (and appositely) is not readonly.

I would be much happier if someone could provide a real world tc for the no styling requirement.

[Charlie]

This is a weird and somewhat unrealistic example, but nevertheless I think it gets the idea across. Essentially any style you try to protect cannot be protected (Excuse my use of font tags):

<p>
    These are my thoughts that I wrote down while I was on the juice and trolling some forums. F you all, eat babies, etc. 
<span contenteditable="false"><font color="red"><b>NOTE: crazythoughts.com is not responsible for the content of these comments. They are solely those of the user<b></font></span>.</p>

Now highlight the disclaimer and apply font color white...no more disclaimer! It can be used to change the style which might be contained and desired within the protected span.

[Frederico Caldeira Knabben]

Replying to comp615:

Sorry but your tc is not correct. Consider that the applied color style will create a tab that will embrace the readonly element, and will not make changes inside of it, so the text will remain red.

[Frederico Caldeira Knabben]

• tab => tag (element)

[Alfonso Martínez de Lizarrondo]

Trying to define the behavior based just on personal ideas usually leads to an overcomplex system that tries to make everybody happy (so in the end everybody is unhappy because it doesn't work as expected or is too complex) instead of focusing on real usage as Fred has requested.

First of all, contentEditable doesn't prevent nor tries to avoid the removal of some content, it's a mechanism to make that content a single block that can't be modified by the user. The main usage that will exist for it is the placeholder plugin, where some markers can be inserted into the content and then the server can process them.

Those markers must follow a strict syntax, that's why they can't be modified by the user. But that doesn't mean that they must create a different context with regards to style and indeed they should be handled as inline text (a single letter to say so):

<p>Hello %NAME%:<br>
This month in the magazine you can find...

it should be possible to highlight the first line

<p><b>Hello %NAME%:</b><br>
This month in the magazine you can find...

That %NAME% marker will be replaced for editing with <span contentEditable="false">NAME</span> (or something like that) and so we have the TC described by Fred, if it's only allowed to set styles around it

<p><b>Hello </b>%NAME%<b>:</b><br>
This month in the magazine you can find...

people will say that it's a bug.

If someone has another proposal it would be better to have a real test case that uses correctly contentEditable.

The only part that I think that it's wrong in the original description is that a contentEditable element can't be partially included in the selection. The browser handles it as a single block, so it's either in or out of the selection.

[Frederico Caldeira Knabben]

Replying to alfonsoml:

The only part that I think that it's wrong in the original description is that a contentEditable element can't be partially included in the selection. The browser handles it as a single block, so it's either in or out of the selection.

That was include just because browsers are buggy, and may eventually make it possible to partially select readonly elements. So, the implementation must consider that.

[Damian]

I've been thinking about it a little more.

My initial concern has been with the ability to override the appearance of non editable content. We are considering new extensions to the editor that would insert "rich" inline non-editable content that is sensitive to presentation.

If we were to adopt this proposal then the solution here is to ensure that the author of the non-editable content is aware of this behaviour and styles the content appropriately, ensuring it displays as expected in any context. So this:

  <span contenteditable="false">
    <span style="color: rgb(255, 0, 0);">
      <strong>Do not change!</strong>
    </span>
  </span>

would have to become:

  <span contenteditable="false">
    <span style="color: rgb(255, 0, 0); background-color: rgb(255, 255, 255);"> 
      <strong>Do not change!</strong>
    </span>
  </span>

to be immune to changes in the background colour, otherwise a user could set the background color to red also.

The bottom line is that responsibility would then be on the content provider to ensure that the non-editable inline content is styled correctly(or as intended). I'm thinking that this might be a reasonable approach.

[Damian]

Replying to damo:

to be immune to changes in the background colour, otherwise a user could set the background color to red also.

I meant colour in both instances ;-)

[Charlie]

damo got what I meant ;) But I agree that it should act like fred described. My cautionary example was meant to have the effect damo described. But I think he's right that in the end styling should be up to the user. So I guess I see no reason not to go ahead with it.

[Lynne Kues]

We would want styling to be disallowed for our usage of read-only regions. We will use the feature for embedded text content. The embedded content will be represented as a url in our document (i.e., pointer to embedded content), but when the user is viewing the document, we will present the actual embedded content to the user.

Ideally, when the option is configured to disallow styling, if the selection includes a read-only region, I would expect the appropriate toolbar/menu items to be disabled.

[Frederico Caldeira Knabben]

Ok. It looks like we could have a flexible full feature solution, without bringing so much code overhead:

• A configuration option named "disableReadonlyStyling", which is "false" by default.

• Support for a special attribute named "cke_nostyle" that, if enabled in an element, tells the styling system to not style the element (being it readonly or not).

I'm posting it, because this may involve some important changes to the styling system, which we want to avoid during the beta stabilization.

[Tobiasz Cudnik]

In my option contenteditable doesn't have anything to do with styling, as i see content as text, not format. And even if we consider format as content, it's also not modified by this case - it only inherits more properties and format definition inside contenteditable=false remains the same.

[Tobiasz Cudnik]

Patch seems to meet the requirements, although i would be careful with it. Here's another TC for cke_nostyle

<p>
	foo <span contenteditable="false">bar <span cke_nostyle="true">xoo</span></span> baz</p>

[Frederico Caldeira Knabben]

I think this patch overcomplicated the code, and there should be a simpler way for it, based on the current styling code. I'll take over the ticket.

[Garry Yao]

It's more of a new feature than a bug, which should be handled by 3.5.

[Garry Yao]

1. "cke_nostyle" is not an option, we simply cannot require users to put anything that's not (X)HTML compatible into their content;
2. "disableReadonlyStyling" will be be flexible enough, as user will need to make one or more particular styles inapplicable.

With the proposed patch, say if user don't want italic style to affect read-only, he just need:

CKEDITOR.config.coreStyles_italic = { element : 'em', overrides : 'i', childRule : function( element ){ return !element.isReadOnly( element ); } };

[Garry Yao]

typo "be be" -> "not be".

[Frederico Caldeira Knabben]

We're not anymore opening a discussion here. The patch is not providing the features defined in at comment:16.

[Garry Yao]

We'll have to think of how to document "cke_nostyle", but I'm sure that's not part of the patch.

[Frederico Caldeira Knabben]

• There are changes to API methods (apply, remove and getRanges) which are not acceptable at this stage because of backwards compatibility.

• I have the impression that the removal from line 468 will break things.

• Considering it's a boolean, there is no need to have a default definition for disableReadonlyStyling. It can be documentation only.

[Garry Yao]

I have the impression that the removal from line 468 will break things.

Um...maybe you can tell how does that work originally, but I see it instead a bug.

[Frederico Caldeira Knabben]

Replying to garry.yao:

Um...maybe you can tell how does that work originally, but I see it instead a bug.

It simply says that, if an element that doesn't accept styling has been found, anything that has been included in the style range so far needs to be styled.

Other than that, I don't see how that change is related to the ticket.

[Frederico Caldeira Knabben]

Just to be aligned with recent talks, the patch uses the "data-cke-nostyle" attribute name, instead of "cke_nostyle".

The following HTML ca nbe used to help testing (not extensively though):

<p>
	My name is <span data-cke-nostyle="1">%NAME%</span>,
I&#39;m from <span contenteditable="false">%CITY%</span>.</p>
<p contenteditable="false">
	A non-editable paragraph.</p>
<p>
	Some more text.</p>

[Garry Yao]

6103_3.patch works for the following cases:

1. Using just the TC, selecting all, open Styles combo and apply "marker:yellow" style, result in %CITY% unstyled.
2. Using the follow content, select all and apply the any bgcolor style, result in "CKEditor" unstyled.

   This's <a contenteditable="false" href="http://ckeditor.com">CKEditor</a> from CKSource.
   

[Frederico Caldeira Knabben]

Please disregard the last attached patch. I'll come with yet another one.

[Garry Yao]

1. It's a hack, but well seems the only to go for now;
2. We must not taking care of this individual by each plugins, it should be done just once:

   // Read-only element must have precedence over child rules on inline styles.
   if ( this.type == CKEDITOR.STYLE_INLINE )
   {
   	styleDefinition.childRule = CKEDITOR.tools.override( styleDefinition.childRule,
   	function( org )
   	{
   		return function( element ) { return org && org( element ) || element.isReadOnly(); }
   	});
   }
   

[Frederico Caldeira Knabben]

Replying to garry.yao:

1. It's a hack, but well seems the only to go for now;

Yup! But elegant, isn't it? ;)

2. We must not taking care of this individual by each plugins, it should be done just once:

Wrong! Suppose we have a child-rule that has been explicitly designed to NOT style <a> elements. If we do that, it's enough to put contenteditable="false" in the element and the rule will be ignored.

Child-rule must have precedence on that, so it must also control the "stylability" of the element. Let's have it as a general rule.

[Frederico Caldeira Knabben]

Fixed with [6128].

[Satya Minnekanti]

This is not fixed and i could reproduce this on nightly build.

[Satya Minnekanti]

Please ignore my previous comment i have not looked at all the comments and discussions that were mentioned in this ticket. This feature is working as designed