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

"TypeError: Cannot read property 'unselectable' of null" thrown when working with CKEditor in integrations #4390

Closed
Dumluregn opened this issue Nov 20, 2020 · 24 comments
Assignees
Labels
status:confirmed An issue confirmed by the development team. type:task Any other issue (refactoring, typo fix, etc).

Comments

@Dumluregn
Copy link
Contributor

Type of report

Bug/Task/Feature?

Provide detailed reproduction steps (if any)

Demo was provided by one of users in Angular: ckeditor/ckeditor4-angular#114 (comment).

Expected result

Errors are not thrown.

Actual result

TypeError: Cannot read property 'unselectable' of null error is thrown and CKEditor crashes.

Other details

There is an error that occurs in various frameworks where you try to integrate CKEditor. It was reported in Drupal, Laravel and Angular and now I encountered it while testing ckeditor/ckeditor4-angular#143.

The issue is most probably caused by this line:

topHtml && editor.ui.space( 'top' ).unselectable();
bottomHtml && editor.ui.space( 'bottom' ).unselectable();

Although the issue wasn't reproduced in the CKEditor itself so far, I think it could be worth to at least take a look at it as it makes integrating it a bit more difficult than it should be. It's hard to classify the character of this issue, so it may need to be relabeled.

@Dumluregn Dumluregn added type:task Any other issue (refactoring, typo fix, etc). status:confirmed An issue confirmed by the development team. size:M labels Nov 20, 2020
@sculpt0r sculpt0r self-assigned this Dec 16, 2020
@sculpt0r
Copy link
Contributor

sculpt0r commented Dec 16, 2020

Based on browser console - null is at line: https://github.com/ckeditor/ckeditor4/blob/major/core/creators/themedui.js#L458 (so, exactly as @Dumluregn said).
editor.ui is an object, but the result of space method is null.

My current status:

  • bug appears only for classic editor (so default one).
  • It happened only if the initial ckeditor element is hidden. If it's not - then both classic and inline editor types are fine.

@Dumluregn
Copy link
Contributor Author

It happened only if the initial ckeditor element is hidden

It makes sense and explains why it fails in the frameworks - they usually do some magic during the component initialisation which often happens before the component is actually rendered.

@sculpt0r
Copy link
Contributor

To be precise, in angular, the element is not hidden on not visible, but physically it's not in the document structure - that's why space( 'top' ) returns null. In the end it ends up with CKEDITOR.document.getById();

@sculpt0r
Copy link
Contributor

Alright!

Note: Below angular sample is targeting editorUrl as localhost. Remove it to use default CDN editor.

After analyzing the test case from angular: https://stackblitz.com/edit/angular-issue-4390

I was able to create the same environment in plain CKEditor4: https://stackblitz.com/edit/js-yndfve

DOM structure during editor creation in angular (with hidden editor element inside p-dialog) looks like this:

  • textarea element is created
  • textarea element is placed as children for <ckeditor> element
  • <ckeditor> is child for p-dialog which will be invisible
  • textarea element has parent => <ckeditor>
  • <ckeditor> has no parent => angular magic that hide content under p-dialog
  • textarea element is used as reference element when calling CKEDITOR.replace

Potentially I even have some solution idea which targets ui.space() implementation.

@sculpt0r
Copy link
Contributor

sculpt0r commented Dec 23, 2020

Fast fix approach in ui.space() method:

var id = '#' + this.spaceId( name );
var spaceElem = this.editor.container.findOne( id );

return spaceElem;

So we are looking for space elements inside the editor container - no in the entire document as before.

However...
I've got another problem with content iframe:

We have iframe element that contains editor data. It has full DOM tree structure, so e.g. html, body elements.
Our data is inside body.

Whenever unhide element with p-dialog, body element inside iframe is empty and contains only some angular markers:
<body data-new-gr-c-s-check-loaded="14.990.0" data-gr-ext-installed=""></body>

The same happens when the editor is initialized properly, then I hide it with p-dialog and show it again...

So there is no editor data...

Seems that p-dialog is a third party library for angular: https://primefaces.org/primeng/showcase/#/dialog

@Dumluregn
Copy link
Contributor Author

So there is no editor data...

Do you mean that the editor data is erased when p-dialog element is shown?

Since this issue was also reproducible without p-dialog element (during running editor component tests in Karma), I think it is worth to try to fix it even if the originally reported issue in ckeditor4-angular will still occur (since it will be a problem with p-dialog now and not ckeditor), but before creating new tickets let's try to do as good as we can with this one and we can create follow-ups during the reviews.

sculpt0r added a commit that referenced this issue Dec 23, 2020
Previously space elements were searched in the entire document.
During editor container is safer scope for this:
- we are expecting 'space' elements inside editor container anyway
- #4390 - there could be situations when document is not available
@sculpt0r
Copy link
Contributor

Do you mean that the editor data is erased when p-dialog element is shown?

Yes. Also, data bindings are gone.

I create two editors bound to data with https://ckeditor.com/docs/ckeditor4/latest/guide/dev_angular.html#integration-with-ngmodel

After hide & show the first one - I update data in the second one. The first editor shows no changes.

@sculpt0r
Copy link
Contributor

Whenever unhide element with p-dialog, body element inside iframe is empty and contains only some angular markers:

Iframe reloading is caused by reattaching element. Angular use it, but it's more like expected behaviour...

I tried set data manually and with onload event - but I'm crashing on

Uncaught TypeError: Cannot read property 'getSelection' of undefined while setData is invoked. So more references are broken inside.

sculpt0r added a commit that referenced this issue Dec 29, 2020
Previously space elements were searched in the entire document.
During editor container is safer scope for this:
- we are expecting 'space' elements inside editor container anyway
- #4390 - there could be situations when document is not available
@sculpt0r
Copy link
Contributor

sculpt0r commented Dec 31, 2020

After research and a talk with @f1ames we decide to split this issue into two separate features:

@sculpt0r
Copy link
Contributor

Summary:

  1. Original issue is caused by editor.ui.space method:

ckeditor4/core/ui.js

Lines 123 to 125 in 1aa2119

space: function( name ) {
return CKEDITOR.document.getById( this.spaceId( name ) );
},

then
getById: function( elementId ) {
var $ = this.$.getElementById( elementId );
return $ ? new CKEDITOR.dom.element( $ ) : null;
},

Since everything is detached - there is no proper element, so null is returned.

There is no verification whenever space was correctly found, this line gives an error:

topHtml && editor.ui.space( 'top' ).unselectable();

  1. Even after fix on editor.ui.space: there is another fix on selection to make:
    74eb1de

  2. Finally there is lot's of trouble with iframe reconstruction. Each time, editor is attached to DOM, browser tries to reload it. We have an empty src attribute, so it's impossible.

Manually recreation involves dealing with different setTimeouts while binding to load event and maintaining other events binding.

@sculpt0r
Copy link
Contributor

sculpt0r commented Jan 4, 2021

I found a related issue: ckeditor/ckeditor4-react#109

So it's no longer only a detached state issue...

@sculpt0r sculpt0r removed the status:blocked An issue which development is blocked by another issue (internal or external one). label May 26, 2021
@sculpt0r
Copy link
Contributor

Since we finished working on #4463 and #4601 - there is a time to verify our work with this root issue :)

@Dumluregn
Copy link
Contributor Author

Just a friendly reminder for anyone trying to test the features in integrations - easy way of using the latest CKE4 major version is nightly build: https://nightly.ckeditor.com/latest/standard/ckeditor/ckeditor.js.

@sculpt0r
Copy link
Contributor

I've pass editorUrl='https://nightly.ckeditor.com/latest/standard/ckeditor/ckeditor.js' to components in https://stackblitz.com/edit/angular-issue-4390 and delayIfDetached=true in config and there are no errors. Nor when the editor is hidden at the beginning or toggled. So... sounds good 🎉 👍

However... Inside #4390 (comment)

Yes. Also, data bindings are gone.

So I'll investigate it now.

@sculpt0r
Copy link
Contributor

sculpt0r commented May 27, 2021

When the editor is created with delay, ngModel seems to have no effect...
https://github.com/ckeditor/ckeditor4-angular/blob/a6be961ec82422925c998c39832968ee1615cbae/src/ckeditor/ckeditor.component.ts#L302-L304 assumes we get editor instance, but that doesn't have to be true with delayed creation.

I've updated ckeditor/ckeditor4-angular#190 description so that we should use delayIfDetached_callback to properly create and initialize the editor.

@sculpt0r sculpt0r added the status:blocked An issue which development is blocked by another issue (internal or external one). label May 27, 2021
@sculpt0r
Copy link
Contributor

Partially solvable with current ckeditor4 major, but for full fix: blocked by ckeditor/ckeditor4-angular#190

@Dumluregn
Copy link
Contributor Author

We have a root issue reported in Angular: ckeditor/ckeditor4-angular#114 and the CKE4 upstream is fixed. So I'd say this issue is solved, not blocked 🤔 WDYT @sculpt0r?

@sculpt0r
Copy link
Contributor

@Dumluregn sounds reasonable :)

I'm closing it according to: #4463 & #4601

@sculpt0r sculpt0r removed the status:blocked An issue which development is blocked by another issue (internal or external one). label May 28, 2021
@ezintz
Copy link

ezintz commented Jun 25, 2021

Is there a chance to get this released as 4.16.2? We experience this issue quite often using the official Angular module for our application, so it would be nice to get rid of it (:

@Dumluregn
Copy link
Contributor Author

Hello, unfortunately this is quite a big new feature and as such is going to be released as 4.17.0. If you want to test this feature earlier, you can use the nightly build temporarily: https://nightly.ckeditor.com/latest/standard/ckeditor/ckeditor.js (you can replace standard with e.g. full).

@ezintz
Copy link

ezintz commented Oct 11, 2021

Hello, thanks for your answer! I could see that the milestones for 4.17 are completed. Can we support somehow to get this new feature rather sooner than later?

@sculpt0r
Copy link
Contributor

Hi @ezintz
Changes are already merged to major branch, however, we also have a few other things there. You may try to use it, but please notice, that until the release it may be unstable. We will release 4.17.0 as soon as we will be sure, that everything is working fine with all changes. I can just ask you for the patient. We don't want to release fastly one fix and broke something else. Since CKE4 is a huge codebase - we always need to pay attention to all of our features.

Also, you can try to apply changes to your own CKE4 source code(see: #4390 (comment)). But it will be more convenient to wait for the official realease.

@ezintz
Copy link

ezintz commented Oct 17, 2021

Hi @sculpt0r, thanks for your answer! I am glad and happy that you want to have a stable and working version released - I really appreciate your work! 🙂 We just keep having this error in our logs and sometimes the users are blocked, so we are willing to help and try to contribute 😉, if there is a way. I may try the nightly builds with our application already, thank you.

Thanks again for all your work!

@Imaginativeone
Copy link

I'm running into this error while using Vue 2. Which details do you need from me to fully describe the problem?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status:confirmed An issue confirmed by the development team. type:task Any other issue (refactoring, typo fix, etc).
Projects
None yet
Development

No branches or pull requests

5 participants