Ckeditor5: Pasting data disallowed by schema should be not possible

Created on 10 Aug 2017  Â·  10Comments  Â·  Source: ckeditor/ckeditor5

aug-10-2017 09-32-48

Now it is possible to paste disallowed text to list items and headings but it is not possible for paragraphs 🤔

aug-10-2017 09-45-14

This probably will be an engine issue.

clipboard bug

Most helpful comment

I want to note that those algorithms and mechanisms are complicated in how they work. What may seem like a bug might have been a conscious decision to enable something else in a different part of code.

Copy+paste & autoparagraphing was originally written by @Reinmar, then I rewritten it a bit to fix some issues. There were some tricky cases to hop through, so don't be too hasty to call something a bug and fix it without having a bigger picture in mind. Cause something else may fail.

All 10 comments

There are two issues here:

  • That DataController#insertContent() doesn't check schema for text with attributes. It will reject elements (using schema), but not attributes. This should be fairly simple to fix.
  • That clipboard's content conversion takes place in a different container than to which this content will be pasted. Due to schema's limitations and general complications there's just one such container for the entire editor. We should try making this container dependent on the limit element (see Schema#getLimitElement()) to which pasting happens.

That DataController#insertContent() don't check schema for text with attributes. It will reject elements (using schema), but not attributes. This should be fairly simple to fix.

Looks like insertContent() does check schema for text with attributes but when dissalowed node is a text is inserted anyway by auto paragraphing.

I want to note that those algorithms and mechanisms are complicated in how they work. What may seem like a bug might have been a conscious decision to enable something else in a different part of code.

Copy+paste & autoparagraphing was originally written by @Reinmar, then I rewritten it a bit to fix some issues. There were some tricky cases to hop through, so don't be too hasty to call something a bug and fix it without having a bigger picture in mind. Cause something else may fail.

Note that is text with disallowed attributes is found, just the attributes are stripped out, but the text remains.

There are very few cases where text is to be removed... actually, I'm not even able to find an example for it.

@scofalik is right – this is awfully tricky. We need to be really careful here and it may require broader analysis.

However, if the issue is in insertContent() itself, then it's not about the "default block converter" which is implemented by ckeditor5-paragraph and which is supposed to handle block elements which have no converters loaded. This happens before insertContent().

The insertContent() method implements its general autoparagraphing mechanism but it's supposed to handle scenarios where a node cannot be inserted at a given position (despite splitting parents of that position).

I guess the place which causes the problem is this code:

https://github.com/ckeditor/ckeditor5-engine/blob/master/src/controller/insertcontent.js#L224-L233

It handles disallowed elements by unwrapping them and text nodes by autoparagraphing. The latter is incorrect... And correcting this for real may be really hard.

Initially, it seems that the _handleDisallowedNode() method should try to:

  1. check if a bare text (with no attributes) is allowed at this (original) position.

    1. if yes, then we know that we should insert this text here and try to insert it with as many of its original attributes as possible (all that are allowed),

    2. if no, then we need to do the autoparagraphing like we do now.

Unfortunately, the schema may define that in a certain context only text with specific attribute(s) is allowed. So "check if a bare text (with no attributes) is allowed" will give false in such cases and the text will not be pasted there at all. This is edge case, of course, and we don't need to consider it now (so we can implement the above algorithm). But the bug will be there.

Did https://github.com/ckeditor/ckeditor5-engine/issues/1088 fix this issue? Or is there something left?

There are two issues here:

  • That DataController#insertContent() doesn't check schema for text with attributes. It will reject elements (using schema), but not attributes. This should be fairly simple to fix.

Done.

  • That clipboard's content conversion takes place in a different container than to which this content will be pasted. Due to schema's limitations and general complications there's just one such container for the entire editor. We should try making this container dependent on the limit element (see Schema#getLimitElement()) to which pasting happens.

TODO. But it looks like a separate issue.

I'll extract the second part.

Was this page helpful?
0 / 5 - 0 ratings

Related issues

hybridpicker picture hybridpicker  Â·  3Comments

oleq picture oleq  Â·  3Comments

Reinmar picture Reinmar  Â·  3Comments

Reinmar picture Reinmar  Â·  3Comments

benjismith picture benjismith  Â·  3Comments