fix: add null check

[IstvanD]

Hello,

LPS-131699,

These changes will solve the issue on 7.3.x. On master, the issue is not reproducible, because, on that branch, Web Content uses ckeditor4-react instead of liferay-ckeditor.

Please review the changes, thanks

[IstvanD]

Dear Team,

I was wondering if you could find the time to review the pull request. Please let me know if you need any help or explanation.

Thank you and best regards, István

[NimrodPapp]

I'm not entirely sure who to mention, but based on previous PRs @antonio-ortega or @wincent, could you please review this PR for us?

Thank you, Nimród

[julien]

@IstvanD, thanks for submitting this.

I'm not sure this should be patched in CKEditor instead of being fixed in DXP:

I think that patching CKEditor's event emitter is taking a risk without really know what impact it could have. The patch isn't huge so you could think it's not a big deal, but my take on this is that if this was a CKEditor issue it would probably already have been fixed internally. Did you check the issues section on their GitHub to search for anything related?

On master, the issue is not reproducible, because, on that branch, Web Content uses ckeditor4-react instead of liferay-ckeditor.

If I remember correctly, even though Web Content uses ckeditor4-react, we are actually loading the ckeditor.js file from the liferay-ckeditorpackage, this is visible here and here.

Additionally, in master, we've had a very similar issue.

You'll see that in the event listener we use for the resize event, that the editor is being destroyed when the window is resized, maybe this would be a good place to start debugging.

Let me know what you think when you can.

[antonio-ortega]

Hi guys,

Let me try to bring some more information to this topic.

@julien We've already followed your steps debugging from that point and we saw what you can read in this comment.

@IstvanD When you submit a new pull request try to provide as much information as possible. Otherwise it is very difficult to understand for reviewers and prone to misunderstandings as you can see. Apart from that important point you should follow every time you submit a pull request, I'd like to ask you to clarify something else. In your comment in previous PTR you said:

On master, where the issue is not reproducible, ckeditor4-react is used instead of ckeditor4. There is a null check on prevHandler before removeListener is called.

I guessed you meant that null check is part of ckeditor4-react. Could you link that code, please?

And I understand, as well, you debugged it and check that null check in ckeditor4-react is called in Master opposite to what @julien said that we are using ckeditor.js from our internal liferay-ckeditor repo. @IstvanD Could you confirm you carried out those steps and you confirmed we are using ckeditor4-reactcode?

Thanks.

Regards.

[IstvanD]

Hi guys,

In that comment, I meant this null-check code, but I forgot to link it. https://github.com/ckeditor/ckeditor4-react/blob/master/src/ckeditor.jsx#L120

I debugged the issue in the following way on Liferay:

1. started creating a new web content
2. in the browser's JS debugger (F12), I searched for the ckeditor.js file
3. in ckeditor.js, I searched for the call of the removeListener() method. this is where my findings on 7.3.x and master differ, as removeListener() is not called the same way.

7.3.x master

the main point is, that on master, a null-check for prevHandler preceedes the call of removeListener().

Actually, I had no idea where this code resides

    _attachEventHandler( propName, prevHandler ) {
        const evtName = `${ propName[ 2 ].toLowerCase() }${ propName.substr( 3 ) }`;

        if ( prevHandler ) {
            this.editor.removeListener( evtName, prevHandler );
        }

        this.editor.on( evtName, this.props[ propName ] );
    }

as I have found it nowhere in the ckeditor project. What I did next will likely cause you to pull your hair out: I searched the code snippet on google. :) I found the ckeditor4-react repository, and there, I found ckeditor.jsx where I could locate the snippet.

I deduced, that on master, when creating a web content, ckeditor.jsx from ckeditor4-react get's activated instead of ckeditor.js from ckeditor, but I couldn't find the reason for this.

Best regards, István

[julien]

Hey @IstvanD,

Thanks for the information. I'll try to clear some doubts you might have, but my main point is that in this function that we are patching:

function removeListener() {
     me.removeListener( eventName, listenerFunction );
}

If me is undefined or null we probably have a way of figuring out why by looking at the call stack. CKEditor has tests to cover its event emitter and I'm still not convinced that we should patch CKEditor, but rather fix it on the DXP side.

I agree with you that the fix might end up being very similar to this (the link you posted earlier)

if ( handler ) {
     editor.removeListener( evtName, handler );
}

The CKEditor.jsx (React) component just instantiates a CKEDITOR instance, just like we do in here. So maybe we need to do some checking on our side before calling the editor's removeListener function.

What I did next will likely cause you to pull your hair out: I searched the code snippet on google. :)

I actually think that being able to use google effectively is a skill, so no worries on my side :clap:

I deduced, that on master, when creating a web content, ckeditor.jsx from ckeditor4-react get's activated instead of ckeditor.js from ckeditor, but I couldn't find the reason for this.

I don't have all the details, but maybe a screenshot and some links might help:

This is Web Content in master, in my browser with React Developer Tools extension installed:

You'll see that I've highlighted a RichText component which in turn renders various children, among which is our (beloved) CKEditor component. When looking at this line I think that it should make even more sense.

To go a bit deeper down the rabbit hole, I checked what was going on in the journal-web module (Web Content), and this is what I see is that this RichText component is probably rendered by the <liferay-data-engine:data-layout-renderer /> tag here. Now I could go deeper and also check the source code of that tag but that still wouldn't solve our problem. On 7.3.x this is slightly different and I'm assuming that the <liferay-ddm:html /> tag renders ckeditor.jsp at some point.

I hope that helps, but let me know otherwise.

@antonio-ortega I read your comment (thanks for the extra info), and what I get from it is that:

we are not clearing all listeners as expected when changing locale

and that's what should focus on fixing. Is that correct?

If you guys need a hand with this, please let me know.

[IstvanD]

Hi @julien

Thank you very much for all the explanation. I think I understand much better why the differences occur on these different branches.

But I'm still having a hard time figuring out how to solve the issue on the DXP side. What is certain is that it occurs only after the backport of LPS-129456 and the backport version is identical to master. For some reason, adding the onInitEditor() callback causes me to be left undefined for some occasions, but I couldn't find the connection reason despite exhaustive debugging.

Best regards, István

[julien]

@IstvanD no problem, always happy to help. I'll take a look at this during the afternoon and tell you if I can spot anything.

[IstvanD]

Hi @julien I just wanted to catch up on this topic. Did you manage to find anything on the DXP side that may have led to the exception?

Thank you, and best regards, István

[julien]

Hi @IstvanD sorry for the late reply.

I finally managed to have a look into this and I don't think the issue is going to be trivial to fix.

The first "fishy" thing I'm seeing when we resize the browser's viewport is this message in the console.

I'm not saying it's related to our "duplicated paste" problem, but it's not a good sign, and should be fixed.

I also think that this issue has something to do with event listeners, in the sense that I have a feeling that one or more event listeners are not getting "removed" when we resize the browser and when we switch from one language to another. Then again, given all the different event handlers that manipulate ckeditor data, I haven't been able to spot which one was causing this bug (it's a bit like looking for a needle in a haystack).

The next thing I tried was fetching these changes, building liferay-ckeditor in DEBUG mode and linking it in dxp, in order to see if that fixed the issue, and it does.

So given that this is a severe bug, I have finally changed my mind about accepting this solution.

Unfortunately, there is a slight problem with this pull request because all the previous patches have been deleted, so could you please resend it (I don't really mind if you push force or send a new one).

Thanks!

[julien]

I also forgot about @antonio-ortega 's comment on the Jira ticket.

Taking that into account, I debugged that part of the code (and added some "log" points) and this is what I see, when I switch from one language to another

And here's the same thing with a bit more details

What's strange at first sight is that the listener doesn't really seem undefined (it just looks like an object literal) but Chrome's devtools show that it has a me property in it's "[[ Scopes ]]" (no idea what that actually is but I'll search :shrug:) that is undefined. So maybe some weird race condition going on?

It also seems that this is coming from the clipboard plugin, because that's where the initDragDataTransfer function is defined ... although other listeners also showed up :partying_face:

All that to say, that this patch doesn't seem wrong to me after all.

[julien]

Fixes #166