Wrong checkReadOnly implementation

[Dinu]

	function checkReadOnly( selection )
	{
		if ( selection.getType() == CKEDITOR.SELECTION_ELEMENT )
			return selection.getSelectedElement().isReadOnly();
		else
			return selection.getCommonAncestor().isReadOnly();
	}

Problems:
1) If selection is type element, the selection is read-only if the parent (rather than element) is read-only. Consider:

<p>
    <span contenteditable="false">abcd</span>
</p>

If the selected element is <span>, it's editable (as in, it can be deleted, replaced, etc.
2) If selection is a range, the logic is more complex; it can so happen that the common ancestor is editable but the range spans an uneditable fragment; consider:

<p>
    <span contenteditable="false">
        ab[cd
    </span>
    ef]gh

Marked with [...] is the selection; in this case, the common ancestor is <p> which is editable, but the range should be read-only because the [cd] portion is uneditable. For non-nested contenteditable, the right logic is: if for all ranges, the start, end, or common ancestor is read-only, the selection is read-only; for nested contenteditable, the logic is gruesome.

[Frederico Caldeira Knabben]

1) Confirmed. The element itself should not defined its "editability" as it's selected as a whole.

2) Confirmed, partially. We should respect the rule we have currently for it: the start boundary of the range must be used as the reference point for checking, not the entire range.

[Alfonso Martínez de Lizarrondo]

I'm not sure about what's the issue in 1.

contentEditable means that the content itself isn't editable, it doesn't matter if it's selected as a whole or with some extra space, but in any case, contentEditable doesn't mean that the element can't be deleted or replaced by other content. That would require a different property and a whole new logic when inserting/deleting contents to check if there are non-deletable elements inside.

[Dinu]

Replying to alfonsoml: No, the issue comes later:

	function onInsertHtml( evt )
...
	    if ( checkReadOnly( selection ) )
		return;
...
	{

It should validate insertion for the first example and not validate for the second. As I understand it, the read-only property (of a selection) should read "deleting, replacing or moving the selection will not affect any non-editable element's contents (but it may discard/insert non-editable elements in editable space)". To this end, I'm cooking the fix, the logic should be that the start and end of the range should not break any non-editable element below the range root (I think this is what fredck second point translates to).

[Dinu]

Attached my fix. Done:

• Moved checkIsEditable to CKEDITOR.dom.selection since it's actually a property of the selection and it might (and should) be used to toggle plugin commands that rely on rewriting the selection (such as link)
  
• Implemented following logic: selection is read-only if the start or the end break a non-editable tag below the selection root
  
• Code is obfuscated for size by habit, don't know if this is consistent with CKE coding style
  

Side fixes:

• in dom/node.js : refactored isReadOnly to remove the assumption that all _cke_notReadOnly elements are editable (should be so, but relies on too many other assumptions to take for granted), and the constraint that body is always editable (maybe somewhere in the future one would like to make a non-editable body). It does not affect current functioning.
  

Needs postfixing:

• #6738

[Garry Yao]

Looks the correct patch, but please use element::equal instead of strictly equal to compare elements.

But when taking nested editing host into consideration, it's wrong by saying the following selection is editable, as considering when inserting a paragraph could easily have the read-only paragraph spitted.

<p contenteditable="false">some <span contenteditable="true">[sample]</span> text</p>

[Alfonso Martínez de Lizarrondo]

About this last comment, there's something interesting to note here: if the functions that walk through the DOM can be stopped when the are in a non-editable element, then it could be possible to provide a feature that people have been requesting, and that's the ability to wrap the editable zone in a div (or several divs) to better mimic the page css, and going forward, it could be also a step into performing an iframe-less editing when just some part of the page wants to be edited.

The main point is: instead of checking where it ends by comparing it with "body" it should check if its parent is not editable (and whatever additional check might be needed)

[Dinu]

Actually to say that selection is editable is quite right (as it's fully contained in one editable container), the logic that will split the editable element is wrong, it should treat the editable container as a text root (like a local 'body', like alfonsoml suggests). I got iframe-less editing working in IE (others just die), save these refactoring gimmicks.

[Garry Yao]

Ticket Test added:
​run OR ​view source.

[Garry Yao]

Replying to garry.yao: Link correction:

​run OR ​view source.

[Sa'ar Zac Elias]

Please update the patch (consider changes of [6289]).

[Sa'ar Zac Elias]

Using the following:

[Text <span contenteditable="false">Readonly] text</span> Text

Try to put inline style, color or bg color, result is a JS error.

[Sa'ar Zac Elias]

Augh, sorry, not related to the patch.

[Garry Yao]

Fixed with [6329].