fix: Maximize plugin breaks the UI in Firefox

[NemethNorbert]

This issue is we are putting contentEditable attribute to the body html, however it breaks the UI on Firefox. This change was introduce in https://dev.ckeditor.com/ticket/5560, reverting it does not reintroduce the issue.

Please review my changes, Thanks!

Fixes #186

[markocikos]

@NemethNorbert I couldn't reproduce the issue on DXP master. Looking at the JIRA ticket, it's not even reproducible on 7.3.x, but on 7.2.x-private. I don't think we should merge and maintain a patch that fixes a bug that was fixed a long time ago.

You mentioned in this JIRA comment that the reason might be React implementation, but ckeditor4-react in only a relatively small wrapper around base ckeditor, that enables it to be invoked as a React component. Nowhere in it is any mention of contentEditable. It is more likely that our update of base ckeditor fixed the issue. On 7.2.x it was 4.11.4, the latest version is 4.16.1. If a customer has this issue, they might try updating "liferay-ckeditor" to the latest "4.16.1-liferay.4", or some previous version that fixes the issue but has the least side-effects.

[NemethNorbert]

Hey @markocikos ,

You are right, I updated the base ckeditor version to 4.16.1 as well as used the lates liferay-ckeditor version on 7.2.x, however the issue still persisted. on 7.2.x one.document.getBody(); will return the body of the page instead of the ckeditor iframe's body. On 7.2.x, we are missing LPS-110011 LPS-118027 that turned the descriptionXML editor into a richtext editor by replacing the stripped down alloyeditor to ckeditor. Without those changes the summary field, does not contain the iframe and alloyeditor is used.

If we backport those tickets the issue is solved on 7.2.x as well, however it changes the summary field functionality and presentation. Are we good to make that change on 7.2.x ?

Thanks,

[markocikos]

Are we good to make that change on 7.2.x ?

LPS-110011 and LPS-118027 seem to be significant changes, new features. I am not sure if we should backport these. Normally we don't do backports of larger features, as it might introduce issues in the old environment and prove to be costly to fix. In contrast, the issue of this PR is a pretty minor one: it is a firefox-only bug, on a plugin we don't include by default. A known solution is simply to upgrade to 7.3 DXP. I would honestly recommend that we 'close / won't fix' this. Maybe we can offer this customer a hotfix with backported LPS-110011 and LPS-118027? @nhpatt what do you think?

[javiergamarra]

LPS-110011 is unbackporteable to 7.2.x, it changes the behaviour for clients way after than the official release/first GAs. I don't think they should be backported.

A known solution is simply to upgrade to 7.3 DXP. I would honestly recommend that we 'close / won't fix' this.

This puts all the burden in the client. I don't know how important is this for them and if we can just accept it and tell them that is already fixed in newer versions. If it's a stopper I would merge the patch instead of backporting for all the users.

Maybe we can offer this customer a hotfix with backported LPS-110011 and LPS-118027? @nhpatt what do you think?

That would be great but I don't think they do hotfixes based on things that are not integrated in 7.2.x, from branches, just for one client. If that possibility exists, patching that just for them I think it would be the best solution.

[markocikos]

Makes sense. I guess the way forward is to check with the customer:

1. How important is this fix?
2. Do they have plans to upgrade to 7.3?

If it's important, and they don't have plans to upgrade, then we merge this PR and publish.

[NemethNorbert]

Hey @markocikos,

The fix I proposed here just removes some code that is not even needed in our ckeditor, in my opinion that is a good trade for a quick fix, even if we will end up using this fix in all of our versions. Having said that we also inquired about the importance of this fix from the customer and if It is feasible I will offer them the option of customising the maximise plugin with the same fix I proposed here.

I will get back to you.

Thanks,

[markocikos]

@NemethNorbert It is very valuable for us to have the least possible amount of customizations of the base ckeditor. This enables us to easily upgrade base version, leveraging improvements from core ckeditor team. Every custom patch adds a possible future conflict or bug, and should be carefully considered before merging.

Having that in mind, it's not the end of the world to make a new patch, if it makes sense to do :slightly_smiling_face:

[NemethNorbert]

Hey @markocikos,

We asked about the future plans to upgrade to 7.3, however we also just received another ticket regarding the same issue on Firefox with the maximise plugin from another customer. Because of that, I would lean towards merging this fix.

Thanks,

[markocikos]

@NemethNorbert OK, let's merge this! Please rebase and rebuild, to fix conflict.

[NemethNorbert]

Hey @markocikos ,

I updated the PR and resolved the conflict.

Thanks,

[markocikos]

@NemethNorbert Please rebase master branch and rebuild with your patch on top. It should be the 11th patch, not replace the 10th.