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

BUG: CKEditor writes "unexpected" markup after "clicking around" - not even typing #3839

Open
mhsdesign opened this issue Aug 19, 2024 · 6 comments
Labels
8.3 Bug Label to mark the change as bugfix CKEditor

Comments

@mhsdesign
Copy link
Member

The logic introduced with #3833 has a downside ... if a ckeditor plugin is ill-programmed the logic - comparing the transformed html after defocus - rather than if a user really changed stuff will lead to unexpected markup being written even-though nothing was might have changed in the eyes of the editor if they were just clicking around.

A more prominent example will probably be that if you uninstall a ckeditor plugin or change their config.
For example if you adjust https://www.neos.io/download-and-extend/packages/techdivision/techdivision-ckstyles.html to use text-green instead of text-red as css class on spans, ckeditor will cease to understand the previous without an migration and thus just removes the markup.

This is not a new problem, but previously editors could influence when something was saved and that was only when typing.

NeosUi: 8.3.9

@mhsdesign
Copy link
Member Author

Another unexpected usecase is the following:

Imagine you have an inline editable property which - dont ask me - is also shown in the inspector.

Now if you insert text in the inspector like "Me & Myself" (containing special chars) after reloading and clicking into the text via the ckeditor it will save a converted version "Me & Myself" on defocusing the text.
Any editor will not understand that subtle new change now which appears in the publishing dropdown. And also the workspace-diff will show that looks the same.

@mhsdesign
Copy link
Member Author

mhsdesign commented Aug 20, 2024

Another case is when the autoparagraph setting was changed for the property. It might be unexpected that the conversation is triggered here implicitly as well?

Or basically anything like disallowing lists or links afterwards.

@mhsdesign
Copy link
Member Author

So after talking to @dlubitz and @kitsunet about this we might need to consider making silent changes like this transparent.
One way would be to show an dialog. A mini poc can be seen here:

Bildschirmaufnahme.2024-08-20.um.23.40.11.mov

Following can be seen or not seen in the clip:

  • The headline is inline editable directly because its not broken
  • The paragraph was changed to autoparagraph: false and thus we have a potential problem here
  • The editor can decide to actually take the new ckeditor version, or leave everything untouched which will leave the editor in an readonly state
  • If confirmed the new markup is saved immediately and the editor may proceed with their actual changes

It could be great to also show in the readonly state the actual content of the live site instead of the upcasted data but for that we would have to lazily initialise ckeditor everywhere.
Regarding implementation we must be careful that calling editor.getData on initialisation of every inline element is not too expensive...

Do you think its worth as well going down that route? The only downside is that potentially a project might face now many of these warnings but they might not care because its just a autoparagraph change and its fine for them.

@mhsdesign
Copy link
Member Author

@grebaldi and me just exchanged ideas regarding this matter.

At first we discussed to reintroduce the old Neos Ui 8.3.8 behaviour by specifically listening if the user did any "real" intentional changes like typing, making something bold, pasting etc.
While this would get us back to a better state - as in better known - we would not fix the underlying problem that surfaces here.

To conclude again, CKEditor does an aggressive upcasting of the markup throwing if necessary parts away it was now configured to understand. Say removing the listing ability in the formatting will lead to the content in the backend being shown without listing and a resave would also remove the listing live.

The above proposed idea to show a markup when attempting to edit such text with a warning like "By editing the text certain formatting might get lost. Further technical information regarding the Markup: ..." doesnt suffice alone.

The editor must be warned beforehand of clicking into an element to edit it to avoid a negative experience - basically playing minesweeper.

Additionally we think that the Neos ui should show the actual live formatted text in case the ckeditor conversion was not a full succeed. Doing so in addition with the warning overlay and indication plus dialog should ensure the most fluid experience, as the user can see the state of the text beforehand directly in the backend and after confirming to edit watch out for any unintentional changes.

This can possibly be implemented by cloning the property node and initialising CKeditor there, on a successful upcasting we can replace the dom node directly, if we seek for confirmation we do it later.

@mhsdesign
Copy link
Member Author

Ckeditor can also not handle markup it created previously under certain circumstances. Like there was the bug with autoparagraph which would create invalid html (#3532):

Some Text</span><span>&nbsp;

again this is not visible to the eye but probably a good thing that it will be noticed by the neos ui?

@grebaldi grebaldi changed the title CKEditor writes "unexpected" markup after "clicking around" - not even typing BUG: CKEditor writes "unexpected" markup after "clicking around" - not even typing Aug 28, 2024
@grebaldi grebaldi added Bug Label to mark the change as bugfix CKEditor 8.3 labels Aug 28, 2024
@mhsdesign
Copy link
Member Author

while working on the dialog idea #3842 i noticed that it wont be enough because there might be still changes on defocusing if there is a discrepancy with decoded html entities. Thats why we definitely need to use a proper is dirty handling which comes with #3846. The dialog change is now more of a nice to have on top.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
8.3 Bug Label to mark the change as bugfix CKEditor
Projects
None yet
Development

No branches or pull requests

2 participants