fix: display formatted text in Firefox Preview Source dialog

[mateomustapic]

This PR is a fix for LPS-120763 - "CKEditor - Formatted text doesn't display in Preview Source dialog on Firefox" bug in codemirror plugin.

As described in the ticket, the issue happened when editing text and opening Source dialog in Firefox. The added text would't appear in the code preview area. This cause of this issue is that in Firefox, the frame's content is not recognised if there is no initial content set. To solve this we need to trigger a page load and then set innerHtml to iFrame preview.

---

To test this:

1. Build CKEditor with this change
2. Access to portal on Firefox browser.
3. Go to Web Content.
4. Add a content.
5. In content field, click Source button in ckeditor toolbar to go to source view.
6. Type "test" or

   test

7. Click the Expand icon next to the Source button to preview source.

Old behaviour

New behaviour

[carloslancha]

Hey @mateomustapic !

This fix feels weird to me. document.open() is meant to be used to prepare the document to write on it (document.write), and causes some side effects like remove all nodes and listeners attached to it.

Doing a quick test looks like if, with the original code, you add a breakpoint after the iframe creation and before setting the innerHTML value, everything is working as expected, so maybe we're facing some kind of career condition? Something related with the iframe load event?

[mateomustapic]

Hey @mateomustapic !

This fix feels weird to me. document.open() is meant to be used to prepare the document to write on it (document.write), and causes some side effects like remove all nodes and listeners attached to it.

Doing a quick test looks like if, with the original code, you add a breakpoint after the iframe creation and before setting the innerHTML value, everything is working as expected, so maybe we're facing some kind of career condition? Something related with the iframe load event?

Hey @mateomustapic !

This fix feels weird to me. document.open() is meant to be used to prepare the document to write on it (document.write), and causes some side effects like remove all nodes and listeners attached to it.

Doing a quick test looks like if, with the original code, you add a breakpoint after the iframe creation and before setting the innerHTML value, everything is working as expected, so maybe we're facing some kind of career condition? Something related with the iframe load event?

Hey @carloslancha I am aware that this fix was a bit weird, but it was the only solution I could at the time. I am resending now.

[julien]

Hey @mateomustapic,

This works in Firefox

But now it's not working in Chrome

It also seems that the styles in Firefox are not loaded properly, so if it doesn't seem to hard we could fix that as well, otherwise, we'll have to create an issue

[mateomustapic]

Hey @mateomustapic,

This works in Firefox

But now it's not working in Chrome

It also seems that the styles in Firefox are not loaded properly, so if it doesn't seem to hard we could fix that as well, otherwise, we'll have to create an issue

hey @julien If you are referring to styles described in this issue https://issues.liferay.com/browse/LPS-120758, I have prepared a fix for it. I am now going to working on fixing the issue of not working in Chrome.

[julien]

@mateomustapic I was thinking about the fonts in the preview, but we can see that later.

[mateomustapic]

Hey @julien

I have pushed the change where this fix is working both in Chrome and in Firefox.

[julien]

@mateomustapic this fixes the issue in Firefox but it looks like you forgot to update everything. You're setting innerHTML here and here. Are you sure both are needed?

Given that the styles aren't present in Firefox, I think we could have taken care of this in the same PR, but that's just my opinion - I'll wait for @carloslancha to share his thoughts on that

[mateomustapic]

@mateomustapic this fixes the issue in Firefox but it looks like you forgot to update everything. You're setting innerHTML here and here. Are you sure both are needed?

Given that the styles aren't present in Firefox, I think we could have taken care of this in the same PR, but that's just my opinion - I'll wait for @carloslancha to share his thoughts on that

yes, you are right. I will now push the code without setting the "first" innerHTML.

[mateomustapic]

I pushed the changes containing previously discussed fix for Firefox and moved styles for Firefox in load callback. This was done because in Firefox stylesheets are not added to the iframe's head, so styles for Preview dialog were not loaded as in Chrome.

[julien]

@mateomustapic I checked and it is fixed for Firefox.

I was wondering, if we shouldn't but all that code in it's own "named" function instead of an anonymous one, it can always be done later but since this is the type of thing we're likely to forget or postpone, I'd suggest doing it now.

Another thing I noticed is that when we do a build (without setting DEBUG=1) there is a warning concerning the class keyword that we could probably get rid of by surrounding it in quotes.

[mateomustapic]

I agree with you @julien on this. It is better to have a named function for the readability. Regarding second comment, I haven’t noticed it since I build editor with ‘DEBUG=1’, but I can do that too in the next iteration.

[carloslancha]

Regarding second comment, I haven’t noticed it since I build editor with ‘DEBUG=1’, but I can do that too in the next iteration.

As a good practice I recommend to test the build not in debug mode before sending / updating a pr. This way we'll be sure we're not introducing things CKBuilder will be complaining about that the one who will prepare the release will have to fix.

[julien]

I also got rid of the warning that was emitted when building - due to the use of the class keyword Waiting for CI to go green and I'll merge