Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DataController#set() throws when adding content with an existing marker #4505

Closed
scofalik opened this issue Apr 5, 2019 · 0 comments · Fixed by ckeditor/ckeditor5-engine#1745
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Milestone

Comments

@scofalik
Copy link
Contributor

scofalik commented Apr 5, 2019

Assume editor data with a marker, for example:

<p>
  A
  <comment id="e64dce95ad23165bb6aca6c5efb543636" type="start"></comment>
  bcd
  <comment id="e64dce95ad23165bb6aca6c5efb543636" type="end"></comment>
  ef
</p>

Calling editor.setData( editor.getData() ) with throw an error. Why?

The whole data is removed, but the marker isn't removed it is actually moved to the graveyard. Then .setData() adds the marker with the same name second time which throws an error.

It feels incorrect that under any circumstances editor.setData( editor.getData() ) fails. Also, I think that it feels natural, that if the specified marker already exists in the editor, it should be moved to a new place. OTOH, we rather discourage using editor.setData() as it is rarely correct to do so.

What exactly happens is that DataController#set() uses model.Writer#insert() and the writer handles the markers insertions. We need to think about two things:

  1. Maybe a marker should be automatically removed when it is moved to the graveyard. I already thought about it earlier and now @Reinmar also suggested that. As I think of it, it makes sense for all marker usages I can think of. We already need to write post-fixers which removes those markers after they end up in the graveyard! Mind, that auto-removing will not break undo.

  2. If not, we should think whether this issue should be fixed in model.Writer#insert() or in DataController#set(). We need to think if the writer, on inserting, should handle existing markers or if it is strictly DataController#set() scenario and responsibility.

@scofalik scofalik changed the title DataController#set() throws when adding content with existing marker DataController#set() throws when adding content with an existing marker Apr 5, 2019
pjasiun referenced this issue in ckeditor/ckeditor5-engine Jun 11, 2019
Fix: `Mode.writer#insert` will no longer crash when the data to set contains markers that are already in the editor content. Closes #1721.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-engine Oct 9, 2019
@mlewand mlewand added this to the iteration 25 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:engine labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
2 participants