Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#6103 closed Bug (fixed)

Read-only: Bug feature for inline elements

Reported by: Frederico Caldeira Knabben Owned by: Frederico Caldeira Knabben
Priority: Normal Milestone: CKEditor 3.5
Component: Core : Styles Version: 3.4 Beta
Keywords: Cc: Lynne Kues, Damian

Description (last modified by 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>
  1. CTRL+A to select all.
  2. 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?

Attachments (6)

6103.patch (4.0 KB) - added by Tobiasz Cudnik 7 years ago.
6103_2.patch (2.4 KB) - added by Garry Yao 7 years ago.
6103_3.patch (9.1 KB) - added by Garry Yao 7 years ago.
6103_4.patch (5.7 KB) - added by Frederico Caldeira Knabben 7 years ago.
6103_5.patch (7.3 KB) - added by Frederico Caldeira Knabben 7 years ago.
6103_6.patch (7.5 KB) - added by Frederico Caldeira Knabben 7 years ago.

Download all attachments as: .zip

Change History (45)

comment:1 Changed 7 years ago by Frederico Caldeira Knabben

Description: modified (diff)

comment:2 Changed 7 years ago by 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.

comment:3 Changed 7 years ago by 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 ;)

comment:4 Changed 7 years ago by 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.

comment:5 in reply to:  4 Changed 7 years ago by 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.

comment:6 Changed 7 years ago by 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.

comment:7 Changed 7 years ago by 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.

comment:8 in reply to:  7 Changed 7 years ago by 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.

comment:9 Changed 7 years ago by Frederico Caldeira Knabben

  • tab => tag (element)

comment:10 Changed 7 years ago by 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.

comment:11 in reply to:  10 Changed 7 years ago by 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.

comment:12 Changed 7 years ago by 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.

comment:13 in reply to:  12 Changed 7 years ago by 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 ;-)

comment:14 Changed 7 years ago by 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.

comment:15 Changed 7 years ago by 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.

comment:16 Changed 7 years ago by Frederico Caldeira Knabben

Keywords: Discussion removed
Milestone: CKEditor 3.4CKEditor 3.4.1
Status: newconfirmed

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.

comment:17 Changed 7 years ago by Tobiasz Cudnik

Owner: set to Tobiasz Cudnik
Status: confirmedassigned

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.

Changed 7 years ago by Tobiasz Cudnik

Attachment: 6103.patch added

comment:18 Changed 7 years ago by Tobiasz Cudnik

Status: assignedreview

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>

comment:19 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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.

comment:20 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Tobiasz Cudnik to Frederico Caldeira Knabben
Status: review_failedassigned

comment:21 Changed 7 years ago by Garry Yao

Milestone: CKEditor 3.4.2CKEditor 3.5

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

Changed 7 years ago by Garry Yao

Attachment: 6103_2.patch added

comment:22 Changed 7 years ago by Garry Yao

Owner: changed from Frederico Caldeira Knabben to Garry Yao
Status: assignedreview
  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 ); } };

comment:23 Changed 7 years ago by Garry Yao

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

comment:24 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed

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

Changed 7 years ago by Garry Yao

Attachment: 6103_3.patch added

comment:25 Changed 7 years ago by Garry Yao

Status: review_failedreview

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

comment:26 Changed 7 years ago by Frederico Caldeira Knabben

Status: reviewreview_failed
  • 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.

comment:27 Changed 7 years ago by Frederico Caldeira Knabben

Owner: changed from Garry Yao to Frederico Caldeira Knabben
Status: review_failedassigned

comment:28 Changed 7 years ago by 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.

comment:29 in reply to:  28 Changed 7 years ago by 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.

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 6103_4.patch added

comment:30 Changed 7 years ago by Frederico Caldeira Knabben

Status: assignedreview

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>

comment:31 Changed 7 years ago by Garry Yao

Status: reviewreview_failed

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.
    

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 6103_5.patch added

comment:32 Changed 7 years ago by Frederico Caldeira Knabben

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

Changed 7 years ago by Frederico Caldeira Knabben

Attachment: 6103_6.patch added

comment:33 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

comment:34 Changed 7 years ago by Garry Yao

Status: reviewreview_failed
  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(); }
    	});
    }
    

comment:35 in reply to:  34 Changed 7 years ago by Frederico Caldeira Knabben

Status: review_failedreview

Replying to garry.yao:

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

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

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

comment:36 Changed 7 years ago by Garry Yao

Status: reviewreview_passed

comment:37 Changed 7 years ago by Frederico Caldeira Knabben

Resolution: fixed
Status: review_passedclosed

Fixed with [6128].

comment:38 Changed 7 years ago by Satya Minnekanti

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

comment:39 Changed 7 years ago by 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

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