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

Support HTML comment nodes #8822

Closed
mlewand opened this issue Jan 13, 2021 · 2 comments · Fixed by #10237
Closed

Support HTML comment nodes #8822

mlewand opened this issue Jan 13, 2021 · 2 comments · Fixed by #10237
Assignees
Labels
domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. Epic package:html-support support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).

Comments

@mlewand
Copy link
Contributor

mlewand commented Jan 13, 2021

📝 Provide a description of the new feature

Currently, if your editor data will include an HTML comment, it will get removed.

Steps to reproduce:

  1. Open ckeditor.com/docs/ckeditor5/latest/examples/builds/classic-editor.html
  2. Open JavaScript dev console.
  3. Paste and execute following code: editor.setData( '<p>foo bar<!-- aa --></p><!-- bb -->' ); console.log( editor.getData() );

Expected: <p>foo bar<!-- aa --></p><!-- bb --> should be logged.

Actual: <p>foo bar</p>

We got a use case where CKEditor 4 integrators used HTML comments to include some metadata, like page number etc.


If you'd like to see this feature implemented, add a 👍 reaction to this post.

@mlewand mlewand added type:feature This issue reports a feature request (an idea for a new functionality or a missing option). domain:v4-compatibilty support:2 An issue reported by a commercially licensed client. labels Jan 13, 2021
@Reinmar Reinmar added this to the nice-to-have milestone Jan 14, 2021
@Mgsy
Copy link
Member

Mgsy commented Feb 11, 2021

Handling HTML comments won't be trivial, so we need to split this task. Cases to be handled:

  • We need to introduce changes in DOMConverter, which should be able to handle HTML comments. We should also introduce a new view node type. Probably we'll need to review the renderer. These changes should be introduced together. 
  • We should handle the conversion. 
    • HTML comments shouldn't touch the editing layer, so the easiest option will be to use markers.
    • Alternative approach 1: Representing HTML comments in the view as elements.
    • Alternative approach 2: introducing HTML comments as widgets.
      • Potential dangerous cases, e.g. inserting a comment between two <td>.

@Reinmar Reinmar added domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. and removed domain:v4-compatibilty labels Mar 9, 2021
@Reinmar
Copy link
Member

Reinmar commented Apr 3, 2021

Meeting notes.


HTML comments kickoff (01.04.2021)

  • Use cases:
    • Some backend's pre-processor rules inside comments. Special case: Conditional for IEs.
    • Or, human-targeted information (like - what is this piece of HTML). In this case, the positioning of a comment is less important. If we move it a bit in the tree, it should still be fine.
  • HTML comments may exist only between nodes in the input/output HTML (i.e. not inside tags, attrs etc.).
  • However, that's enough for them to exist between list items, inside table structures, etc. which makes this complicated.
    • Edge case: Comments inside e.g. <figure class=image>.
    • If we can make this somehow work, let's do that, but it may e.g. ignore the order of nodes.
  • It'll be possible in the future to implement a feature that loads HTML comments into the editor and display them as inline widgets (e.g. to make them editable). But for now, in the scope of this feature, it is only to make them survive the editing.

How to represent comments in the model?

  • Option A: separate element. Highly problematic because now all other features would have to take it into consideration.
    • No: because Backspace/Arrow keys, etc. would have to all know about this.
  • Option B: markers. Transparent for all the features. The only problem: performance.
    • Where to store comments content?
      • Option B1: marker name (question: is there a limit for marker name length?)
        • What about two subsequent comments with the same name?
      • Option B2: root attributes
        • Idea for the future: Plugin for the editor to store document's metadata.
        • Keep every comment's data as a separate root attribute (for OT).
    • Cons: Performance. Not critical as it would not show up until there are 100+ comments. Also, we want to improve their performance.
    • Pros: No problems regarding where they are used. No risk of duplication. Should survive the most type of operations.
    • Conclusion: Let's go with markers + root attributes for comment data.
  • Option C: model node attributes.
    • What'd be the behavior on node delete/move/etc?
    • Would it stick to the right or left?
    • Would they get duplicated when you press enter or start typing, apply a text attr, etc?
    • Multiple comments one after another.
    • Pros: Performance.
    • Question: Would conversion get complicated?
    • Tip: Kinda similar to how we output markers to the data.

How to represent comments in the view (downcast)?

  • In the editing view: We don't need them at all. Cool for performance and simplicity.
  • What about data view?
  • Option A: UIElement.
    • It's probably possible to make render() return different node types than elements.
      • Or move that to the Renderer or HtmlDataProcessor (e.g. generate a <cke-comment> element and replace it with a real comment later on in the process).
      • Idea: Make a prototype of this early.
    • UIElements are nearly transparent.
    • Problem: Cases where they aren't transparent. Fortunately, in the data view this is simpler as we rarely do any getChild() or some modifications on an existing structure that could be affected.
    • Let's have a couple of integration tests with more complex features such as lists, tables, images to check whether it at least doesn't crash.
    • Problem: Ok, this only applies to the downcast... but what about upcast? There are no UIElements there. There may be some code that does not expect to see UIElements in the data pipeline.
      • Also, UIElements are to represent UI. UI in the data pipeline? That's wrong.
      • Haha, markerToData() already uses UIElements.
  • Option B: Special type of a text node.
    • Problem: Position mapping.
    • Problem: Not transparent for features like "image looks for the <img> element inside its <figure>".
    • Cons: Also prone to getChild(0)
  • Option C: Attributes of view nodes.
    • At some point they have to turned into comments anyway. When would that happen?
    • Problem: Between text nodes - we'd need to store them on view elements with the information regarding offsets.
  • Option D: New node type (Comment extends Node)
    • Cons: Tones of changes everywhere.
    • Cons: Also prone to getChild(0).
  • Option E: New element type (CommentElement extends Element)
    • Cons: In the tree created by DomConverter.domToView() so far we only have Elements.
    • Cons: Also prone to getChild(0).
  • Conclusion:
    • getChild(0) is wrong and should be replaced with a more bullet-proof getChild(matcher) or getDescendant(matcher).
  • Option F: Make it symmetrical to the upcast structure so <$comment/> with a custom property.
    • Taken that getChild(0) is always a problem, then anything we'll choose will create issues.
    • So, making it symmetrical may be the easiest choice.

How to represent comments in the view (upcast)?

  • An element <$comment/> with a custom property $rawContent in a similar way to how _rawContentElementMatcher works.
  • This element will be created by DomConverter#domToView()

Position mapping

<paragraph><$text bold=true>Foo</$text></paragraph>

<p><$comment />Foo</p>

How will mapping the model's Foo position to the view's Foo text node work? The $comment element changes the indexes in the view.

But the same situation happens on markerToData(). How does it work there? Probably, we just need to copy the same solution.

Tada: markerToData() uses UIElement. So, we already use UIElements in the data view.

Delivery

  • Separate package or one of the existing ones?
    • Let's not create a separate package. Not worth it.
    • GHS may be a good starting point. GHS is about HTML support and comments are part of HTML.

@Mgsy Mgsy added the Epic label Apr 14, 2021
@Mgsy Mgsy modified the milestones: nice-to-have, iteration 43 Apr 15, 2021
@Mgsy Mgsy modified the milestones: iteration 44, Iteration 45 Jul 1, 2021
ma2ciek added a commit that referenced this issue Jul 28, 2021
Feature (html-support): Introduced the HTML comment plugin. Closes #8822.

Other (engine): Undeprecated the `elementToMarker` upcast helper. 

Feature (engine): Introduced new option `skipComments` in `DomConverter#domToView()` (`false` by default) to make it possible to decide whether HTML comments should be removed from the data.

Internal (paste-from-office): Removed all HTML comments from pasted data.

Fix (table): Fixed model mappings in table cell if a paragraph is bound to its parent.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. Epic package:html-support support:2 An issue reported by a commercially licensed client. type:feature This issue reports a feature request (an idea for a new functionality or a missing option).
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants