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

model.insertContent() does not insert markers (added in a document fragment) #12467

Closed
scofalik opened this issue Sep 14, 2022 · 0 comments · Fixed by #12491
Closed

model.insertContent() does not insert markers (added in a document fragment) #12467

scofalik opened this issue Sep 14, 2022 · 0 comments · Fixed by #12491
Assignees
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. type:improvement This issue reports a possible enhancement of an existing feature.

Comments

@scofalik
Copy link
Contributor

📝 Provide a description of the improvement

When model.DocumentFragment is inserted to the model using model.insertContent(), its children are inserted but markers saved in model.DocumentFragment#markers are skipped. They should be inserted as well.

Most probably here: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-engine/src/model/utils/insertcontent.ts#L90

The difficulty in this task is that we need to re-calculate the markers ranges. Additional difficulty is introduced by the fact that model.insertContent() may heavily affect the model, so re-calculation would be different depending on how some elements are split, merged, etc. I don't think that just "using math" will be enough.

One idea to simplify this calculations that I have is to add temporary, fake elements to the document fragment at the positions were marker boundaries are. Then, those elements would be inserted to the model together with regular children. After this is done, fake elements positions would be saved, the elements would be removed and markers would be added instead. I am not sure this method would be 100% accurate in all cases (e.g. when markers are at the beginning/end of the document fragment, or when the insertion causes massive splitting and merging in the document model).

The other idea is to handle markers in insertion.handleNodes() (maybe change the method name). Markers could be passed as second parameter and then processed in Insertion class as the nodes are inserted to the model. This way we could very precisely calculate new positions for markers boundaries.

I think the second idea would be more precise but it is probably tougher to implement. We can do it in two steps, first use fake nodes and then implement better solution if needed.

Another thing is that I wonder whether we should put it behind some kind of a flag for backward compatibility but I am leaning towards "no". At the moment, the markers are "silently dropped", so, yeah, we will introduce backward incompatibility. But will it be problematic?

  1. In most cases you don't have any markers in the document fragment (or you don't even insert a document fragment). We noticed this problem only now, after long time and it was probably never raised by our users.
  2. In cut/copy+paste, markers are removed earlier (probably during copy) and they are not available inside insertContent(). So this won't be a problem.
  3. Internally, we use insertContent() in a couple of places. We can check if there's a possibility that in those places document fragment with marker might be inserted.
  4. If the user has problems, the fix is very easy (just remove markers from document fragment before inserting it).
  5. The fact that markers are skipped can be treated as a bug, actually. We can treat this as fixing incorrect behavior of insertContent(). If your document fragment has markers and you don't want them, remove them.

So, I propose no flag and marking this as minor breaking change + adding simple migration guide.

@scofalik scofalik added type:improvement This issue reports a possible enhancement of an existing feature. package:engine labels Sep 14, 2022
@scofalik scofalik changed the title Inserting document fragment using model.insertContent() does not insert markers Markers are not inserted by model.insertContent() Sep 14, 2022
@scofalik scofalik changed the title Markers are not inserted by model.insertContent() model.insertContent() does not insert markers (passed in document fragment) Sep 14, 2022
@scofalik scofalik changed the title model.insertContent() does not insert markers (passed in document fragment) model.insertContent() does not insert markers (added in a document fragment) Sep 14, 2022
@scofalik scofalik added the squad:collaboration Issue to be handled by the Collaboration team. label Sep 15, 2022
@DawidKossowski DawidKossowski self-assigned this Sep 16, 2022
@CKEditorBot CKEditorBot added the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 16, 2022
scofalik added a commit that referenced this issue Sep 28, 2022
…t-insert-markers

Other (engine): `Model#insertContent()` will now insert markers added to `DocumentFragment#markers`. Closes #12467.
@CKEditorBot CKEditorBot removed the status:in-progress Set automatically when an issue lands in the "In progress" column. We are working on it. label Sep 28, 2022
@CKEditorBot CKEditorBot added this to the iteration 58 milestone Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:engine squad:collaboration Issue to be handled by the Collaboration team. type:improvement This issue reports a possible enhancement of an existing feature.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants