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

CKEditor5 rendering is stripping nested HTML content, providing data loss for user #16450

Closed
kevinlin98z opened this issue May 30, 2024 · 1 comment · Fixed by #16636
Closed
Labels
package:list support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@kevinlin98z
Copy link

kevinlin98z commented May 30, 2024

📝 Provide detailed reproduction steps (if any)

  1. Have CKEditor5 render the following payload:
<div>Dear Andy Coffey,</div><div>&nbsp;</div><div><span style=\"color:#000000; display:inline!important; float:none; font-family:&quot;Times New Roman&quot;; font-size:medium; font-style:normal; font-weight:400; letter-spacing:normal; text-align:start; text-decoration-color:initial; text-decoration-style:initial; text-indent:0px; text-transform:none; word-spacing:0px\">Thereference case number for this solution is:<span>&nbsp;</span></span><span style=\"font-size:11.0pt\"><span style=\"line-height:107%\"><span style=\"font-family:&quot;Calibri&quot;,sans-serif\"><span style=\"color:black\">3000373510</span></span></span></span></div><div><span style=\"color:#000000; display:inline!important; float:none; font-family:&quot;Times New Roman&quot;; font-size:medium; font-style:normal; font-weight:400; letter-spacing:normal; text-align:start; text-decoration-color:initial; text-decoration-style:initial; text-indent:0px; text-transform:none; word-spacing:0px\">Byanalyzing the system service logs uploaded by you we propose the following solution:</span></div><div>&nbsp;</div><div> <div><b>Action Plan:</b></div><br> We found 1 issue in FFDC file (7Z71CTO1WW_J30A09H2_xcc_mini-log_20240509-144330.zip):<br> <ul> <li> One event occurred on CMOS Battery at 2024-05-09T10:26:44Z.</li> </ul> We suggest following actions:<br><br> For event FQXSPPW0031J, \"Numeric sensor CMOS Battery going low (lower non-critical) has asserted.\", 2024-05-09T10:26:44Z<br> <ul> Complete the following steps until the problem is solved:<br>1. Remove the CMOS battery for 20 seconds and then install it back.<br>2. Replace the system CMOS battery<br>3. If the issue persists, please choose \"NO\" in the subsequent emails.</ul><br> </div><div>&nbsp;</div><div><strong style=\"color:#000000; font-family:&quot;Times New Roman&quot;; font-size:medium; font-style:normal; letter-spacing:normal; text-align:start; text-decoration-color:initial; text-decoration-style:initial; text-indent:0px; text-transform:none; word-spacing:0px\"><em>Wehope that by completing the above steps, the issue will be resolved. We will send you a follow-up email in 3 days from now to check the status. Please reply to the follow-up email and we will process your case based on the response selected.</em></strong></div>
  1. Notice above payload is invalid HTML here:
image 3. CKEditor5 renders the above payload like below: image

✔️ Expected result

What is the expected result of the above steps?

CKEditor5 rendering engine should not strip invalid <ul> content without <li> elements. If data absolutely has to be stripped, user should be notified.

❌ Actual result

What is the actual result of the above steps?

CKEditor5 rendering engine removes the entire <ul> content. This presents a data loss for the user, and is a regression compared to CKEditor4, which did not do this.

❓ Possible solution

Do not strip <ul> content that does not have <li> child elements (same applies to additional nesting elements like tables too). CKEditor4 did not have this behavior.

📃 Other details

  • Browser: …
  • OS: …
  • First affected CKEditor version: CKEditor5
  • Installed CKEditor plugins: …

If you'd like to see this fixed sooner, add a 👍 reaction to this post.

@kevinlin98z kevinlin98z added the type:bug This issue reports a buggy (incorrect) behavior. label May 30, 2024
@aldonace-wu aldonace-wu added the support:1 An issue reported by a commercially licensed client. label May 31, 2024
@Witoso
Copy link
Member

Witoso commented Jun 28, 2024

Notes:

  • our CKEditor 5 converters remove all text nodes that are not in li.
  • most likely we need to we need to tune the upcast to not clear the text nodes. 
  • it's possible that autoparagraphing will “just work”.
  • TBD if we will wrap in li or p, the goal is to not have data loss, but data change is acceptable.

niegowski added a commit that referenced this issue Jul 3, 2024
Fix (list): Text nodes directly inside `<ul>` or `<ol>` should not be removed while loading editor data. Closes #16450.
@CKEditorBot CKEditorBot added this to the iteration 77 milestone Jul 3, 2024
@pomek pomek modified the milestones: iteration 77, iteration 76 Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:list support:1 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants