Ckeditor5: Editor crashes trying to delete dom nodes that do not exist

[thedavidmeister]

🐞 Is this a bug report or feature request? (choose one)

• Bug report

💻 Version of CKEditor

v1.0.0-alpha.2 balloon editor

📋 Steps to reproduce

add and remove whitespace with nothing else in the editor

✅ Expected result

no errors

❎ Actual result

📃 Other details that might be useful

Here's the function that I added console logs to:

    /**
     * Checks if elements child list needs to be updated and possibly updates it.
     *
     * @private
     * @param {module:engine/view/element~Element} viewElement View element to update.
     * @param {Object} options
     * @param {module:engine/view/position~Position} options.inlineFillerPosition The position on which the inline
     * filler should be rendered.
     */
    _updateChildren( viewElement, options ) {
        const domConverter = this.domConverter;
        const domElement = domConverter.mapViewToDom( viewElement );

        if ( !domElement ) {
            // If there is no `domElement` it means that it was already removed from DOM.
            // There is no need to update it. It will be updated when re-inserted.
            return;
        }

    console.log('x', domElement);

        const domDocument = domElement.ownerDocument;
        const filler = options.inlineFillerPosition;
        const actualDomChildren = domElement.childNodes;
        const expectedDomChildren = Array.from( domConverter.viewChildrenToDom( viewElement, domDocument, { bind: true } ) );

        // Inline filler element has to be created during children update because we need it to diff actual dom
        // elements with expected dom elements. We need inline filler in expected dom elements so we won't re-render
        // text node if it is not necessary.
        if ( filler && filler.parent == viewElement ) {
            this._addInlineFiller( domDocument, expectedDomChildren, filler.offset );
        }

        const actions = diff_diff( actualDomChildren, expectedDomChildren, sameNodes );

        let i = 0;
        const nodesToUnbind = new Set();

    console.log('y', actualDomChildren, actions);

        for ( const action of actions ) {
            if ( action === 'insert' ) {
                insertat_insertAt( domElement, i, expectedDomChildren[ i ] );
                i++;
            } else if ( action === 'delete' ) {
        console.log('z', i, actualDomChildren [ i ]);
                nodesToUnbind.add( actualDomChildren[ i ] );
                dom_remove_remove( actualDomChildren[ i ] );
            } else { // 'equal'
                i++;
            }
        }

        // Unbind removed nodes. When node does not have a parent it means that it was removed from DOM tree during
        // comparision with the expected DOM. We don't need to check child nodes, because if child node was reinserted,
        // it was moved to DOM tree out of the removed node.
        for ( const node of nodesToUnbind ) {
            if ( !node.parentNode ) {
                this.domConverter.unbindDomElement( node );
            }
        }

        function sameNodes( actualDomChild, expectedDomChild ) {
            // Elements.
            if ( actualDomChild === expectedDomChild ) {
                return true;
            }
            // Texts.
            else if ( domConverter.isText( actualDomChild ) && domConverter.isText( expectedDomChild ) ) {
                return actualDomChild.data === expectedDomChild.data;
            }
            // Block fillers.
            else if ( isBlockFiller( actualDomChild, domConverter.blockFiller ) &&
                isBlockFiller( expectedDomChild, domConverter.blockFiller ) ) {
                return true;
            }

            // Not matching types.
            return false;
        }
    }

[thedavidmeister]

with this console.log('u', actualDomChildren, expectedDomChildren, actions); I get to this safely:

then i press delete and get:

[thedavidmeister]

i think the diff logic needs to be improved and/or functions that update the DOM should check the existence of nodes before trying to access their parents.

[Mgsy]

Hi, @thedavidmeister, thanks for your report. Unfortunately, I'm not able to reproduce your issue. Could you provide exact steps to reproduce and some details about your environment (your OS, browser, how do you implement the editor)?

Can I ask you to check whether this issue occurs also here?

[thedavidmeister]

@Mgsy i can reproduce here https://thedavidmeister.github.io/hoplon-ckeditor5-test/

implemented with a fairly barebones hoplon build.

i'm using mac + chrome.

http://g.recordit.co/SK1N8QHDm9.gif

[Reinmar]

I can reproduce it. But to debug it we'd need to see it locally and locally I'm not able to reproduce it.

When did you build and how this editor? Could you provide the exact steps?

[thedavidmeister]

@Reinmar it is on github pages, the build is https://github.com/thedavidmeister/hoplon-ckeditor5-test/tree/master/gh-pages

those compiled files are perhaps not super helpful as they are minified.

the build is in this file:

https://github.com/thedavidmeister/hoplon-ckeditor5-test/blob/master/build.boot

(deftask build
 []
 (comp
  (hoplon)
  (cljs
   :optimizations :advanced
   :compiler-options compiler-options)))

(deftask deploy
 []
 (comp
  (build)
  (target
   :dir #{"gh-pages"})
  (github-pages)))

this uses a basic boot setup for clojurescript: https://github.com/boot-clj/boot

you can do local dev by cloning the repo, installing boot then running boot front-dev and visiting localhost:8000

the editor is included through this https://github.com/thedavidmeister/hoplon-ckeditor5-test/blob/master/src/pages/index.cljs

it just references "https://cdn.ckeditor.com/ckeditor5/1.0.0-alpha.2/balloon/ckeditor.js" directly.

Once the global BalloonEditor exists:

(def ready?
 (j/with-let [c (j/cell nil)]
  (h/with-interval
   1000
   (when (exists? js/BalloonEditor)
    (reset! c true)))))

it calls create on a div:

   (j/with-let [el (h/div "foo edit me!")]
    (j/cell=
     (when ready?
      (js/BalloonEditor.create el)))))))

[Reinmar]

I'm sorry, but I'll need to ask you again to prepare a JavaScript version of it. In order to properly debug an issue like this one, we need to make sure that we understand every bit of the problem and Clojure script is not something we know. It's unlikely but I'd like to be sure that the issue wasn't caused by a compiler and/or some additional libraries added in that setup. After all, for some reason, this issue can't be reproduced on official builds.

[thedavidmeister]

That's fair, i'll need to set aside some time to reproduce - i unfortunately have the reverse problem!

[thedavidmeister]

i'm getting different but similar errors at https://jsfiddle.net/bozsjmxj/ when adding/deleting white space.

[Reinmar]

OK, I can reproduce it... I thought that it may be Grammarly or something, but it throws even without it. Thanks. We'll check it.

[Reinmar]

Since I can't reproduce it in the build's sample my guess would be that this issue is triggered in an iframe. Which makes a lot of sense because the stack trace points to the scroll util.

At the same time, it's a completely different issue than what you reported initially @thedavidmeister... I'm afraid you'll still need to try to reproduce the previous one.

[thedavidmeister]

ok, i don't have steps to reproduce yet, but i did find a potential clue.

actualDomChildren is a br in hoplon right when it breaks upon pressing delete but text " " in the working raw HTML version.

@Reinmar does that help at all?

[Reinmar]

Unfortunately not much. There are issues like #692 but they are really hard to analyse even if you can reproduce them. Things happen simultaneously and we touch the "quirks" here which should not even make to the view (they should be filtered out by the Renderer and DomConverter – see the diagram https://docs.ckeditor.com/ckeditor5/latest/framework/guides/architecture/intro.html#Overview).

BTW, do you get CKEditor 5's source from git or npm?

[thedavidmeister]

to debug i built from npm then manually add console logs to the produced file

[thedavidmeister]

@Reinmar here is the stack trace, if not the view, where should i be debug logging do you think?

[Reinmar]

I've looked into your initial logs and it seems that in this loop:

        for ( const action of actions ) {
            if ( action === 'insert' ) {
                insertat_insertAt( domElement, i, expectedDomChildren[ i ] );
                i++;
            } else if ( action === 'delete' ) {
                nodesToUnbind.add( actualDomChildren[ i ] );
                dom_remove_remove( actualDomChildren[ i ] );
            } else { // 'equal'
                i++;
            }
        }

We should first have the insertion handled, i increased, and then on the next step the deletion happen at index 1. Your logs show that this is exactly what happens. But somehow, insertion at position 0 doesn't move the previously existing text node to index 1 in actualDomChildren (so you get undefined when accessing actualDomChildren[ 1 ]).

You can check whether insertAt() does its job right by logging the outerHTML of domElement (I think).

I can think of two things which may go wrong in such a DOM-oriented scenario:

• Browser (or something else) does something bizarre after insertion happens – e.g. it merges two subsequent text nodes into one.
• Something breaks the liveness of NodeList. actualDomChildren should be a reference to a live node list so after a node is inserted at 0 the previous content should be shifted right.

Since only you can reproduce this error so far, I'm still suspicious regarding ClosureScript. Maybe the compilation breaks some refs?

BTW, you need to be careful when using console.log() on objects. In your console logs here:

You can first see an array with a text node inside to later see a <br> after you inspected this array. This actually shows that the node list is live because according to the steps you described (type space, delete space, error), <br> is inserted right before the error but after console.log(y). That <br> should also replace a space, so the screenshot of console.log(y) shows that exactly this happens.

tl;dr: You need to go step by step in that code and see if insertion works fine and refs are ok. There should be two items in actualDomChildren after the first iteration.

Happy debugging :D Welcome to our world ;)

[Reinmar]

PS. that <br> is called bogus br (or a block filler) and it's a way to fill an empty block element so it has some height. Otherwise empty block elements would collapse.

Bogus brs are added by the renderer automatically. They should not be present in the view which should be "quirk free".

Another such a lovely addition are inline fillers (7 zero-width-space characters) which are used to fill empty inline nodes. This allows retaining the selection there which is crucial for correctly handling typing in certain languages. If we wouldn't render them, some browsers would move the selection out of those empty inline elements (which you might've just created by pressing Ctrl+B) and the first character typed by the user would be inserted outside this element too, which we'd need to post-fix (by rerendering the DOM) which would, in turn, break text composition (e.g. IME).

[thedavidmeister]

i will try to reproduce with a basic clojurescript setup - raw JS interop, no hoplon

[thedavidmeister]

update, bug is not present in a raw CLJS build using leiningen to compile:

https://github.com/thedavidmeister/ckeditor-cljs-test

i didn't setup a github pages for this one, but tested locally with this code:

(ns core.main)

(let [el (.createElement js/document "div")
      body (.-body js/document)]
 (.appendChild body el)
 (.create js/BalloonEditor el))

steps to use locally:

1. install leiningen (https://leiningen.org/) brew install leiningen if you are on mac
2. checkout the test repo git clone https://github.com/thedavidmeister/ckeditor-cljs-test
3. in the repo run lein cljsbuild once to compile the cljs to js
4. open resources/public/index.html in a browser

so the bug is either:

• with boot-cljs, the alternative way to compile cljs
• with hoplon

i will try boot-cljs next.

[thedavidmeister]

ok, i'm pretty sure now that there is something in hoplon conflicting with something in ckeditor.

i took the existing hoplon repo and compiled with all hoplon functionality commented out and it worked, re-introducing hoplon also reintroduced the error.

[thedavidmeister]

here is the outerHTML before and after with the bug:

<p>&nbsp; &nbsp;</p> before
"&nbsp;&nbsp;" "expected before"
<p><br data-cke-filler="true"></p> after

and without:

<p>&nbsp; &nbsp;</p> before
"&nbsp;&nbsp;" "expected before"
<p>&nbsp;&nbsp;&nbsp; &nbsp;</p> after

so it seems _something_ is different in the insertAt call

[thedavidmeister]

this looks suspicious https://github.com/hoplon/hoplon/blob/master/src/hoplon/core.cljs#L211 - like some of the DOM manipulation functions are being modified. i'll chase this up.

[thedavidmeister]

i have a WIP fix in progress at https://github.com/hoplon/hoplon/pull/205/files

confirmed that it fixes this bug in ckeditor, now working on not breaking hoplon ;)

[thedavidmeister]

this has been fixed upstream with hoplon, merged into master in v7.1

thanks for the debugging help! @Reinmar

[tiaguinho]

I'm having this problem.
We need to isolate the style where the ckeditor lives, so the application runs with this structure:

- iframe
      - iframe
            - iframe
               ckeditor

When I press ENTER, ckeditor pass two times for the line @ckeditor-utils/dom/scroll.js:264.
The problem is that the second time, elementOrRange is null.