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

View renderer filler was lost #1210

Closed
FrciSmrci opened this issue Aug 19, 2018 · 17 comments
Closed

View renderer filler was lost #1210

FrciSmrci opened this issue Aug 19, 2018 · 17 comments

Comments

@FrciSmrci
Copy link

FrciSmrci commented Aug 19, 2018

🐞Bug report | Other

💻 CKEditor Build Inline v 11.0.1

📋 Steps to reproduce

  1. When using the Basic styling option as Bold a new strong element is created as a first child of the editing p node. The result turns out like this. <p><strong>&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;&#8203;</strong>Put your text here</p> I'm attaching a [screen recording] (https://drive.google.com/open?id=1oslX7Ni1Ds5bQqGiQpiOcku7UCzM4sZR) of the mentioned behaviour.

  2. When using the link option the results is an a element created as a first child of the editing p node with the same content as the link. <p><a class="ck-link_selected" href="https://google.com">https://google.com</a>Put your text here</p>

✅ Expected result

The edited content should be wrapped inside the appropriate tag.

❎ Actual result

The edited content is not wrapped, the tag gets inserted before the string.

📃

The bulleted and numbered list options work as expected.
When focusing out of the element I get the View renderer file was lost error.
PS: Till now I used CKEditor 4 and all of the mentioned scenarios worked as expected.

Do you have any idea why something like this would be happening or could you at least point me into some the right direction what to research further?

Thank you in advance!

@f1ames
Copy link
Contributor

f1ames commented Aug 20, 2018

I tried to duplicate this behaviour on clean CKEditor instance without any success (you can check here https://codepen.io/f1ames/pen/yxBdPw).

Also looking at the stack trace in the video it shows some Angular package (I think) - zone.js which might have something to do with this error:

image

Few questions to clarify:

  • What browser version do you test with?
  • Are you able to reproduce this error without 3rd-party js libraries (clean CKEditor)?
  • If you're using Angular, do you integrated CKEditor via ckeditor5-angular integration?

@f1ames f1ames changed the title View renderer file was lost View renderer filler was lost Aug 20, 2018
@FrciSmrci
Copy link
Author

Hey @f1ames ,

thank you for the fast reply.

I'm using Google Chrome version 68.0.3440.106 (Official Build) (64-bit).
I tried and couldn't reproduce the error without 3rd-party JS libraries, therefore I think that the problem is not in the CKEditor itself.

The project I'm trying to integrate the CKEditor into is built with Angular, but the library that uses it isn't (GrapesJS).

I did a rewrite of a plugin which extends the default GrapesJS RichTextEditor with the CKEditor 5 instead of 4.

The library itself creates a canvas iframe, could this be a problem in the new version of CkEditor?

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2018

I wonder whether it may be related to the @dimaip's PR: ckeditor/ckeditor5-engine#1502. I know that Neos uses iframes too.

@dimaip, I know you had the same error. Did your PR was supposed to fix that (we should've asked that before merging it ;P)?

@dimaip
Copy link

dimaip commented Aug 20, 2018

Yes. It fixed it for me and I'm almost sure it would fix it for @FrciSmrci as well. You could do this to give it a try: yarn add ckeditor/ckeditor5-engine#16b0280258fb30a9e504136410f57914f2888a96

@FrciSmrci
Copy link
Author

Thank you @Reinmar. @dimaip Sadly, the error still persists with this version of the ckeditor engine.

@dimaip
Copy link

dimaip commented Aug 20, 2018

Wow, that's super strange. I'd then try to set a breakpoint here https://github.com/ckeditor/ckeditor5-engine/blob/d710524a780084833fa676f355b5e6f55ac64e05/src/view/renderer.js#L385
and step into startsWithFiller to see if it's working the way expected.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2018

I'd risk saying that you may be using the old engine still. It's a bit tricky to force npm to install just one version of a package. I think that with yarn it may be tricky too. If any package depends on that old version (and they do) it will be installed anyway and webpack will use it too. Which may lead to even more errors (duplicated code on runtime).

@dimaip
Copy link

dimaip commented Aug 20, 2018

yeah when developing locally you could just

cd node_modules/@ckeditor
rm -rf ckeditor5-engine
git clone https://github.com/ckeditor/ckeditor5-engine

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2018

I'm sorry but that's not going to work either ;/ We've made huge changes in OT implementation and they are on master in ckeditor5-engine already. That makes ckeditor5-engine incompatible with other packages (at least, it is expected that they break at some point).

There are more advanced way of doing it, but in this simple case (the patch is really short) I'd do this:

  1. Check whether ckeditor5-engine is installed only once in your node_modules. That it's only under node_modules/@ckeditor/ckeditor5-engine and not duplicated in node_modules/@ckeditor/ckeditor5-*/node_modules/@ckeditor/ckeditor5-engine. If it is, then you're actually using it so you're fine.
  2. If the above isn't true, then reinstall your node_modules (so nothing's duplicated) and edit your node_modules/@ckeditor/ckeditor5-engine/srv/view/filler.js so in:

https://github.com/ckeditor/ckeditor5-engine/pull/1502/files#diff-919cfd118a2ad8986b37703deda561d3L87

the ( domNode instanceof Text ) is replaced with:

( Object.prototype.toString.call( domNode ) == '[object Text]' )

Then, you can check it again.

@Reinmar
Copy link
Member

Reinmar commented Aug 20, 2018

For more flexible, maintainable and usable setup (in the future), mgit2 and Lerna allow you to freely use git based versions of packages. We use them in our development setup which is described in https://ckeditor.com/docs/ckeditor5/latest/framework/guides/contributing/development-environment.html. If you'd like to use them in your project, I'd need to write a tutorial how to add them to an existing project. This may be helpful for more guys.

PS. The other way is using e.g. mgit2 plus webpack's aliases to force webpack to use pkgs cloned by mgit2.

@FrciSmrci
Copy link
Author

FrciSmrci commented Aug 20, 2018

@Reinmar you are right. I searched for the error occurrences through the code and realised that the logic is bundled in @ckeditor/ckeditor5-build-inline/build/ckeditor.js I confirmed it was being fired by setting a debugging point inside the _removeInlineFiller method.

Could I clone the ckeditor5 repo and build an ckeditor5-build-inline package with the specified ckeditor-engine version? What would you propose?

@FrciSmrci
Copy link
Author

@Reinmar Not sure if this question slipped by. Could I clone the ckeditor5 repo and build an ckeditor5-build-inline package with the specified ckeditor-engine version? What would you propose?

@Reinmar
Copy link
Member

Reinmar commented Aug 22, 2018

I'm sorry, I missed your comment.

So, you want to rebuild an existing build. That's covered in https://ckeditor.com/docs/ckeditor5/latest/builds/guides/development/custom-builds.html.

However, the problem is backporting the fix that we made in https://github.com/ckeditor/ckeditor5-engine/pull/1502/files to the last stable release of ckeditor5-engine (which is here: https://github.com/ckeditor/ckeditor5-engine/tree/stable). You cannot use the master branch of `ckeditor5-engine because it's heavily changed.

To do that, @dimaip planned to fork ckeditor5-engine and cherry pick that patch there. IDK if he did that, but if not, you can do it easily too.

With that done, you can you do this as follows:

git clone https://github.com/ckeditor/ckeditor5-build-inline.git
cd ckeditor5-build-inline
npm i

That installed you the latest stable (published) version of ckeditor5-engine, so let's replace it with your fork:

cd node_modules/\@ckeditor
rm -rf ckeditor5-engine
git clone git@github.com:FrciSmrci/ckeditor5-engine.git
cd ../..
npm run build

I hope I didn't miss anything...

@Reinmar
Copy link
Member

Reinmar commented Aug 22, 2018

tl;dr is that to rebuild ckeditor5-build-inline, the shortest way is to:

  1. clone that specific repo
  2. install its dependencies (which includes the non-patched engine)
  3. replace the non-patched engine with a patched engine (which you can do by cloning your fork or patching it manually in your file system)
  4. rebuild the build

The same can be done by cloning ckeditor5 and using mgit2 and lerna, but it's a bit longer scenario:

  1. clone ckeditor5
  2. git co stable (again, you must use the latest stable version)
  3. mgit boostrap (automatically uses stable branches of all sub packages)
  4. replace packages/ckeditor5-engine with a patched version
  5. lerna bootstrap (symlinks everything)
  6. cd packages/ckeditor5-build-inline
  7. npm run build

@dimaip
Copy link

dimaip commented Aug 22, 2018

Btw, to save a few steps you could use my fork: https://github.com/dimaip/ckeditor5-engine/tree/iframe
This branch adds the fix on top of latest stable release. Works for us at Neos CMS.

@FrciSmrci
Copy link
Author

@dimaip and @Reinmar Thank you for all the help! :) I managed to build the version and the mentioned error isn't occurring anymore, although the actions work only on newly added text, not on the already inserted ones. @dimaip Did you encounter this issue as well?

@dimaip
Copy link

dimaip commented Aug 23, 2018

Nope I did not, CKE5 works perfectly fine for us now...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants