Ticket #6103 (closed Bug: fixed)

Opened 4 years ago

Last modified 3 years ago

Read-only: Bug feature for inline elements

Reported by: fredck Owned by: fredck
Priority: Normal Milestone: CKEditor 3.5
Component: Core : Styles Version: 3.4 Beta
Keywords: Cc: lynne_kues, damo

Description (last modified by fredck) (diff)

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

6103.patch (4.0 KB) - added by tobiasz.cudnik 4 years ago.
6103_2.patch (2.4 KB) - added by garry.yao 3 years ago.
6103_3.patch (9.1 KB) - added by garry.yao 3 years ago.
6103_4.patch (5.7 KB) - added by fredck 3 years ago.
6103_5.patch (7.3 KB) - added by fredck 3 years ago.
6103_6.patch (7.5 KB) - added by fredck 3 years ago.

Change History

comment:1 Changed 4 years ago by fredck

  • Description modified (diff)

comment:2 Changed 4 years ago by Saare

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 4 years ago by comp615

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 follow-up: ↓ 5 Changed 4 years ago by damo

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 4 years ago by damo

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 4 years ago by fredck

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 follow-up: ↓ 8 Changed 4 years ago by comp615

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 4 years ago by fredck

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 4 years ago by fredck

  • tab => tag (element)

comment:10 follow-up: ↓ 11 Changed 4 years ago by alfonsoml

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 4 years ago by fredck

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 follow-up: ↓ 13 Changed 4 years ago by damo

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 4 years ago by damo

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 4 years ago by comp615

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 4 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 4 years ago by fredck

  • Keywords Discussion removed
  • Status changed from new to confirmed
  • Milestone changed from CKEditor 3.4 to CKEditor 3.4.1

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 4 years ago by tobiasz.cudnik

  • Owner set to tobiasz.cudnik
  • Status changed from confirmed to assigned

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 4 years ago by tobiasz.cudnik

comment:18 Changed 4 years ago by tobiasz.cudnik

  • Status changed from assigned to review

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 4 years ago by fredck

  • Status changed from review to review_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 4 years ago by fredck

  • Owner changed from tobiasz.cudnik to fredck
  • Status changed from review_failed to assigned

comment:21 Changed 3 years ago by garry.yao

  • Milestone changed from CKEditor 3.4.2 to CKEditor 3.5

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

Changed 3 years ago by garry.yao

comment:22 Changed 3 years ago by garry.yao

  • Status changed from assigned to review
  • Owner changed from fredck to 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 ); } };

comment:23 Changed 3 years ago by garry.yao

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

comment:24 Changed 3 years ago by fredck

  • Status changed from review to review_failed

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

Changed 3 years ago by garry.yao

comment:25 Changed 3 years ago by garry.yao

  • Status changed from review_failed to review

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 3 years ago by fredck

  • Status changed from review to review_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 3 years ago by fredck

  • Owner changed from garry.yao to fredck
  • Status changed from review_failed to assigned

comment:28 follow-up: ↓ 29 Changed 3 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 3 years ago by fredck

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 3 years ago by fredck

comment:30 Changed 3 years ago by fredck

  • Status changed from assigned to review

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 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by fredck

comment:32 Changed 3 years ago by fredck

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

Changed 3 years ago by fredck

comment:33 Changed 3 years ago by fredck

  • Status changed from review_failed to review

comment:34 follow-up: ↓ 35 Changed 3 years ago by garry.yao

  • Status changed from review to review_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 3 years ago by fredck

  • Status changed from review_failed to review

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 3 years ago by garry.yao

  • Status changed from review to review_passed

comment:37 Changed 3 years ago by fredck

  • Status changed from review_passed to closed
  • Resolution set to fixed

Fixed with [6128].

comment:38 Changed 3 years ago by satya

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

comment:39 Changed 3 years ago by satya

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 – 2012 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy