Opened 6 years ago

Closed 5 years ago

#6735 closed Bug (fixed)

Wrong checkReadOnly implementation

Reported by: dinu Owned by: Saare
Priority: Normal Milestone: CKEditor 3.5.1
Component: Core : Read-only Version: 3.4.2
Keywords: Cc:


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

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

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

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:

    <span contenteditable="false">

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.

Attachments (4)

6735.patch (3.0 KB) - added by dinu 6 years ago.
6735_2.patch (3.0 KB) - added by dinu 5 years ago.
6375_3.patch (6.8 KB) - added by garry.yao 5 years ago.
6735_4.patch (8.4 KB) - added by garry.yao 5 years ago.

Download all attachments as: .zip

Change History (21)

comment:1 Changed 6 years ago by fredck

  • Status changed from new to confirmed

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.

comment:2 follow-up: Changed 6 years ago by alfonsoml

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.

comment:3 in reply to: ↑ 2 Changed 6 years ago by dinu

Replying to alfonsoml: No, the issue comes later:

	function onInsertHtml( evt )
	    if ( checkReadOnly( selection ) )

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

Changed 6 years ago by dinu

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

comment:5 Changed 5 years ago by garry.yao

  • Component changed from General to Core: Read-only
  • Milestone set to CKEditor 3.5.1

comment:6 Changed 5 years ago by garry.yao

  • Keywords HasPatch added

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

comment:8 Changed 5 years ago by alfonsoml

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)

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

Changed 5 years ago by dinu

Changed 5 years ago by garry.yao

comment:10 follow-up: Changed 5 years ago by garry.yao

  • Keywords HasPatch removed
  • Owner set to garry.yao
  • Status changed from confirmed to review

Ticket Test added:
run OR view source.

comment:11 in reply to: ↑ 10 Changed 5 years ago by garry.yao

Replying to garry.yao: Link correction:

run OR view source.

comment:12 Changed 5 years ago by Saare

  • Status changed from review to review_failed

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

Changed 5 years ago by garry.yao

comment:13 Changed 5 years ago by garry.yao

  • Status changed from review_failed to review

comment:14 Changed 5 years ago by Saare

  • Status changed from review to review_failed

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.

comment:15 Changed 5 years ago by Saare

  • Owner changed from garry.yao to Saare
  • Status changed from review_failed to review

Augh, sorry, not related to the patch.

comment:16 Changed 5 years ago by Saare

  • Status changed from review to review_passed

comment:17 Changed 5 years ago by garry.yao

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

Fixed with [6329].

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