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

BUGFIX: Fix ckeditor inline mode autoparagraph: false #3532

Merged
merged 6 commits into from
Jun 29, 2023

Conversation

mhsdesign
Copy link
Member

@mhsdesign mhsdesign commented Jun 21, 2023

Bug from #2018
Resolves: #2918

Since there is still no native support for a inline ckeditor mode. ckeditor/ckeditor5#762 we will continue our hacky road ;)

This fixes several issues:

i also added a test for some inline editing behavior. (Like that there are no outer span or p tags)
But writing a tests for #2918 is currently impossible, as our tests currently dont use https which means we cant access the navigator.clipboard and test that ckeditor works correctly when content is pasted. For that reasons you can use the new manual testing environment to check everything is working correctly.


We remove the outer tags in the first place, so they are not included in the final html.

image

Without processing:

image

What I did

How I did it

How to verify it

@Sebobo
Copy link
Member

Sebobo commented Jun 21, 2023

We now have to decide to still use this hack or adjust the CK plugin to fix this in the model....

@Sebobo Sebobo self-requested a review June 21, 2023 11:12
Since there is still no native support for a inline ckeditor mode. ckeditor/ckeditor5#762 we will continue our hacky road ;)

This fixes several issues:
- Inserting text with newlines will now correctly insert soft breaks (br)
- We use our own pseodo `<neos-inline-wrapper>` tag to avoid the issue of having to parse html with regex and doing it wrong #2918

i also added a test for some inline editing behavior. (Like that there are no outer span or p tags)
But writing a tests for #2918 is currently impossible, as our tests currently dont use `https` which means we cant access the `navigator.clipboard` and test that ckeditor works correctly when content is pasted.
@mhsdesign mhsdesign marked this pull request as ready for review June 28, 2023 14:07
@mhsdesign mhsdesign changed the title WIP: BUGFIX: Dont remove every span tags around content in inline mode BUGFIX: Dont remove every span tags around content in inline mode Jun 28, 2023
@github-actions github-actions bot added the Bug Label to mark the change as bugfix label Jun 28, 2023
@mhsdesign mhsdesign changed the title BUGFIX: Dont remove every span tags around content in inline mode BUGFIX: Fix ckeditor inline mode autoparagraph: false Jun 28, 2023
Copy link
Member

@crydotsnake crydotsnake left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested and works as expected

@mhsdesign
Copy link
Member Author

We must also test this carefully with other ck plugins ^^

@mhsdesign
Copy link
Member Author

mhsdesign commented Jun 28, 2023

I just found a regression in 7.3 which doesn't appear in 8.3:
The heading select box doesnt work once you selected "paragraph" - you can never go back to a h1 h2 heading.

This made no sense in my head, as the node type definition were exactly the same and i though the code base should have changed much and that we should both have ckeditor v16 installed.

Turns out wrong. This 2020 update (Neos 5.2) https://github.com/neos/neos-ui/pull/2624/files#diff-7ae45ad102eab3b6d7e7896acd08c427a9b25b346470d7bc6507b6481575d519R97-R106 - which supposedly updated ckeditor from 11/12 to 16 actually only changed the double declared dependencies in the root package json, but the "real" dependencies on the ckeditor-bindings package

"@ckeditor/ckeditor5-alignment": "^11.1.1",
"@ckeditor/ckeditor5-basic-styles": "^11.1.1",
"@ckeditor/ckeditor5-editor-decoupled": "^12.1.1",
"@ckeditor/ckeditor5-essentials": "^11.0.2",
"@ckeditor/ckeditor5-heading": "^11.0.2",
"@ckeditor/ckeditor5-highlight": "^11.0.2",
"@ckeditor/ckeditor5-link": "^11.0.2",
"@ckeditor/ckeditor5-list": "^12.0.2",
"@ckeditor/ckeditor5-paragraph": "^11.0.2",
"@ckeditor/ckeditor5-remove-format": "^10.0.0",
"@ckeditor/ckeditor5-table": "^13.0.0",
"@ckeditor/ckeditor5-widget": "^11.0.2",
were forgotten. That means the update dindt happen at this suspected time. Only lately while cleaning up the codebase in Neos 8.2 we dropped the duplicated imports declaring "12" and were left with the "16" version. That means we accidentally did the real ckeditor update in #3116

I will revert the "optional" isLimit: true part as it doesnt seem to work well in ckeditor v 12 and will target 8.2 for this special fix.

Edit 2

the issue isLimit caused could also be reproduced in Neos 8.3. I tracked the "issue" down to that the heading command is not enabled, because getSelectedBlocks will stop searching if it encounters a limit element: https://github.com/ckeditor/ckeditor5-engine/blob/28c6f90d62ad33fd1b1199f7a45821ddc546c2d3/src/model/selection.js#L856

I believe that this is not an issue, but we simply use "isLimit" wrong ^^.

isLimit – It can be understood as whether this element should not be split by Enter. Examples of limit elements: $root, table cell, image caption, etc. In other words, all actions that happen inside a limit element are limited to its content. Note: All objects (isObject) are treated as limit elements, too.

So while it makes sense to not be able to set the heading like h1 - as we are indeed in the INLINE mode, this would be to breaky in a bugfix.

Also isLimit will only prevent from multiple paragraphs being manually entered -> eg by pasting or entering, but when the contents are already two p tags <p>1</p><p>2</p> or a heading <h1>1</h1> the editor will not forbid this.

@@ -0,0 +1,21 @@
<html lang="en">
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i added some manual tests, which can be used to test our ckeditor integration without a fully fledged neos

Fix bug in the case you had multiple paragraphs and a headline and switched to autoparagrahp: false
@mhsdesign
Copy link
Member Author

We actually dont need isLimit: true to solve that newlines wont be preserved on paste

To enable inserting br when content with newlines is pasted, it seems enough to remove this upcast editor.conversion.for('upcast').elementToElement({model: 'paragraph', view: 'span', converterPriority: 'high'}); - i did this accidentally (by only specifying "editingDowncast" and "dataDowncast")

Copy link
Member

@Sebobo Sebobo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❤️

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

Successfully merging this pull request may close these issues.

3 participants