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

Balloon editor crashes on init when there is a comment in source html #5734

Closed
pogromistik opened this issue Nov 14, 2019 · 11 comments · Fixed by #7403
Closed

Balloon editor crashes on init when there is a comment in source html #5734

pogromistik opened this issue Nov 14, 2019 · 11 comments · Fixed by #7403
Assignees
Labels
intro Good first ticket. support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@pogromistik
Copy link

Hello!

📝 Provide detailed reproduction steps (if any)

  1. Create editor div with comment in content
<div id="editor">
    <!---->
    <p>text</p>
</div>
  1. Init editor
BalloonEditor
    .create( document.querySelector( '#editor' ) )
    .catch( error => {
        console.error( error );
    } );

✔️ Expected result

Editor works

❌ Actual result

Exception

index.html:15 CKEditorError: unexpected-error {"originalError":{"message":"Cannot read property 'toLowerCase' of undefined","stack":"TypeError: Cannot read property 'toLowerCase' of undefined\n    at Xo (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:159540)\n    at u (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:151031)\n    at $o (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:151116)\n    at Jo._findReplaceActions (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:156788)\n    at Jo._updateChildrenMappings (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:153310)\n    at Jo.render (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:152166)\n    at Ls._render (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:188896)\n    at Ls.<anonymous> (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:185429)\n    at Ls.fire (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:96400)\n    at Ls.change (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:188191)","name":"TypeError"}}
    at Function.rethrowUnexpectedError (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:3355)
    at Ls.fire (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:96637)
    at Ls.change (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:188191)
    at Ls._disableRendering (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:188802)
    at Jc.Aa.listenTo.priority (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:244481)
    at Jc.fire (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:96400)
    at Jc._runPendingChanges (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:327297)
    at Jc.enqueueChange (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:324716)
    at $a.init (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:265927)
    at $a.<anonymous> (https://cdn.ckeditor.com/ckeditor5/15.0.0/balloon/ckeditor.js:5:110855)

📃 Other details

  • Browser: Chrome 77.0.3865.120
  • OS: Windows 10
  • CKEditor version: 15.0.0

Here is repo with reproduction - https://github.com/pogromistik/CKEditor-comment-bug. And you can view result here - https://pogromistik.github.io/CKEditor-comment-bug/.

We are trying to integrate ckeditor into Blazor app and face this problem, cause Blazor inserts comments into html for elements tracking.

It can be related to this change - #3860

@pogromistik pogromistik added the type:bug This issue reports a buggy (incorrect) behavior. label Nov 14, 2019
@Mgsy
Copy link
Member

Mgsy commented Nov 14, 2019

Hi, thanks for the report, I can confirm this issue. It occurs in the balloon and inline editors.

cc @Reinmar @mlewand

@Reinmar
Copy link
Member

Reinmar commented Nov 15, 2019

Interesting. It does not blow up setData(). It should be something fairly simple.

@Reinmar Reinmar added this to the next milestone Nov 15, 2019
@Reinmar Reinmar added the intro Good first ticket. label Nov 15, 2019
@Reinmar
Copy link
Member

Reinmar commented Nov 15, 2019

By a coincident, there may be a PR which fixes this issue: ckeditor/ckeditor5-engine#1806. But it's unfinished and its author didn't explain what bug it fixes.

@snowchenlei
Copy link

If you integrate ckeditor into Blazor app successfully, can you open source?
Thanks!

@pogromistik
Copy link
Author

@snowchenlei

If you integrate ckeditor into Blazor app successfully, can you open source?
Thanks!

  1. install ckeditor5 from npm
  2. index.ts
const BalloonEditor = require('@ckeditor/ckeditor5-build-balloon');

window["BlazorCKE"] = {
    init: function (params) {
        BalloonEditor
            .create(document.querySelector('#' + params.selector))
            .then(editor => {
                editor.setData(params.content ? params.content : '');
                editor.model.document.on('change:data', () => {
                    params.instance.invokeMethodAsync('updateText', editor.getData());
                });
            })
            .catch(error => {
                console.error(error);
            });
    }
};
  1. CKEComponent.cs
public partial class CKEComponent
{
     private string EditorId { get; } = $"cke_{Guid.NewGuid()}";

     protected override bool TryParseValueFromString(string value, out string result,
         out string validationErrorMessage)
     {
         result = value;
         validationErrorMessage = "";
         return true;
     }

     protected override async Task OnAfterRenderAsync(bool firstRender)
     {
         await base.OnAfterRenderAsync(firstRender);
         if (firstRender)
         {
             await initialiseEditor();
         }
     }

     public ValueTask<string> initialiseEditor()
     {
         var arg = new {selector = $"{EditorId}", instance = DotNetObjectReference.Create(this), content = Value};
         return JsRuntime.InvokeAsync<string>(
             "BlazorCKE.init",
             arg);
     }

     [JSInvokable]
     public async Task<bool> updateText(string editorText)
     {
         Value = editorText;
         await ValueChanged.InvokeAsync(Value);
         EditContext?.NotifyFieldChanged(FieldIdentifier);
         return true;
     }
 }
  1. CKEComponent.razor
@using Microsoft.JSInterop
@inject IJSRuntime JsRuntime
@inherits Microsoft.AspNetCore.Components.Forms.InputBase<string>

<div id="@EditorId"></div>
  1. Call component:
<CKEComponent @bind-Value="Value"/>

@tikudev
Copy link

tikudev commented Jan 29, 2020

For people who face this issue while it is not fixed yet, there is the workaround of just removing all Comment Nodes before the CKEditor is mounted (of course, this does not work if something relies on those comments in the future)

function removeCommentNodes(node) {
	if (node.nodeType === 8) {
		//is comment node
		node.remove();
	}
	node.childNodes.forEach(removeCommentNodes);
}

removeCommentNodes(selector);
InlineEditor.create(selector, config);

@mlewand
Copy link
Contributor

mlewand commented Feb 6, 2020

I just checked this issue, and I can no longer reproduce it so it looks like we fixed this somewhere down the line.
@Mgsy can you confirm that it is no longer reproducible?

@mlewand mlewand added pending:feedback This issue is blocked by necessary feedback. and removed status:confirmed labels Feb 6, 2020
@Mgsy
Copy link
Member

Mgsy commented Feb 6, 2020

@mlewand, unfortunately, I can still reproduce this issue.

@mlewand mlewand added status:confirmed and removed pending:feedback This issue is blocked by necessary feedback. labels Feb 11, 2020
@jswiderski jswiderski added the support:1 An issue reported by a commercially licensed client. label Mar 31, 2020
@FilipTokarski
Copy link
Member

FilipTokarski commented Apr 1, 2020

I checked it with frameworks integrations and I had the following results:

  • Vue: all ok
  • Angular:
    • Classic: ok
    • Inline: TypeError: Cannot read property 'toLowerCase' of undefined
    • Balloon: TypeError: Cannot read property 'toLowerCase' of undefined
    • Balloon Block: TypeError: Cannot read property 'toLowerCase' of undefined
    • Decoupled: TypeError: Cannot read property 'toLowerCase' of undefined
  • React
    • Classic: ok
    • Inline: TypeError: Cannot read property 'toLowerCase' of undefined
    • Balloon: TypeError: Cannot read property 'toLowerCase' of undefined
    • Balloon Block: TypeError: Cannot read property 'toLowerCase' of undefined
    • Decoupled: TypeError: Cannot read property 'toLowerCase' of undefined

@Reinmar
Copy link
Member

Reinmar commented May 26, 2020

Issue:

  • We load the content to the model (that works fine).
  • We then try to render that back to the DOM and now:
    • If that's a classic editor, there's no issue becuse we render to a newly created element
    • If that's an inline editor, we reuse the passed element (that had a comment initially) and that's when the renderer tries to compare node lists and the areSimilar function fails

Solution:

As proposed in the PR, the areSimilar() function must be secured to correctly compare comment nodes. Since there is no support for comments in the view, it's enough to just make it treat comments as non-equal to anything else.

@tomalec
Copy link
Contributor

tomalec commented Jun 8, 2020

new PR is available at #7403

tomalec added a commit that referenced this issue Jun 12, 2020
oleq added a commit that referenced this issue Jun 15, 2020
Fix (engine): The editor should not crash when the initial data includes HTML comments. Closes #5734.
@lslowikowska lslowikowska added support:3 An issue reported by a commercially licensed client. and removed support:1 An issue reported by a commercially licensed client. labels Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
intro Good first ticket. support:3 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants