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

CKEditor doesn't understand invalid nested lists #3007

Closed
zadam opened this issue Nov 4, 2018 · 10 comments · Fixed by ckeditor/ckeditor5-list#141
Closed

CKEditor doesn't understand invalid nested lists #3007

zadam opened this issue Nov 4, 2018 · 10 comments · Fixed by ckeditor/ckeditor5-list#141
Assignees
Labels
package:list type:bug This issue reports a buggy (incorrect) behavior.

Comments

@zadam
Copy link

zadam commented Nov 4, 2018

Evernote produces following HTML:

<ul>
    <li>item 1</li>
    <li>item 2</li>
    <ul>
        <li>sub-item 2.1</li>
        <li>sub-item 2.2</li>
        <ul>
            <li>sub-item 2.2.1</li>
        </ul>
    </ul>
</ul>

Obviously, this (ul -> ul instead of ul -> li -> ul) isn't valid, but browsers don't have any trouble rendering it as expected. But CKEditor does this:

image

So it just ignores the nested list altogether. I'm not sure what approach CKEditor takes regarding the invalid HTML.

@Reinmar
Copy link
Member

Reinmar commented Nov 5, 2018

cc @f1ames

@f1ames
Copy link
Contributor

f1ames commented Nov 5, 2018

As mentioned, this is an invalid HTML (ul > ul) which is the cause of this issue. The valid one ul > li > ul is handled correctly.

We are aware of some issues with lists conversions (for example ckeditor/ckeditor5-list#112 or #1318) so we will definately work on that in the future. However, I'm not sure to what extend we should support invalid markup. I can agree that this one may be common for lists - and by fixing it we provide additional support for pasting from Evernote. I'm for leaving it open as a valid issue and try to handle it while working on improvements in list conversions (WDYT @Reinmar?).


From more technical standpoint, the issue is caused by the list conversion mechanism and more precisely by cleanList() method which removes all ul/ol children which are not li elements during upcasting.

@Reinmar
Copy link
Member

Reinmar commented Nov 5, 2018

I think we should consider handling such lists if some popular apps generate such an invalid HTML. Or simply if it often happens that an invalid HTML is used on some websites and people try to copy-paste that into the editor (and report it as a bug to us). That'd be a reason to work.

But if this is a theoretical problem (no apps generate such content and no one stumbled upon this in real life), then it doesn't make sense to prioritise it.

Also, I'm curious if the browser will actually put such an invalid HTML to the clipboard on copy. Sometimes, they fix invalid markup internally and put a more valid one into the clipboard.

@Reinmar
Copy link
Member

Reinmar commented Nov 5, 2018

From more technical standpoint, the issue is caused by the list conversion mechanism and more precisely by cleanList() method which removes all ul/ol children which are not li elements during upcasting.

So would it be an easy fix to detect ul > ul and wrap that inner ul in an li?

@zadam
Copy link
Author

zadam commented Nov 5, 2018

Also, I'm curious if the browser will actually put such an invalid HTML to the clipboard on copy. Sometimes, they fix invalid markup internally and put a more valid one into the clipboard.

I encountered this issue not during copy & paste, but when I imported Evernote's ENEX file and then loaded it into the CKEditor.

It's difficult to test this issue by directly copy & pasting from Evernote, because it's complicated by ckeditor/ckeditor5-list#116. So it's broken in different way:

image

If instead I display the invalid list (without divs in li) in web browser and copy paste to CKEditor, then it behaves as the original issue (sub-list is skipped completely).

@f1ames
Copy link
Contributor

f1ames commented Nov 5, 2018

So would it be an easy fix to detect ul > ul and wrap that inner ul in an li?

I think it should work that way so looks like an easy fix. Shouldn't break anything during conversions IMHO.

I encountered this issue not during copy & paste, but when I imported Evernote's ENEX file and then loaded it into the CKEditor.

It's difficult to test this issue by directly copy & pasting from Evernote, because it's complicated by ckeditor/ckeditor5-list#116.

Ok, so we should check what type of content Evernote puts into clipboard. If it's something like

<ul>
    <li>
        <div>1</div>
    </li>
    <ul>
        <li>
            <div>1</div>
        </li>
    </ul>
</ul>

it will cause the same issue (but invisible now due to ckeditor/ckeditor5-list#116). If the content is valid ul > li > ul and this issue occurs only with the steps mentioned by @zadam:

when I imported Evernote's ENEX file and then loaded it into the CKEditor

maybe it will be good to see if there are other cases like @Reinmar mentioned:

if some popular apps generate such an invalid HTML. Or simply if it often happens that an invalid HTML is used on some websites and people try to copy-paste that into the editor (and report it as a bug to us).

Fixing it just because there is one specific, rare case (generating invalid HTML) may not makes sense ATM.

@f1ames
Copy link
Contributor

f1ames commented Nov 5, 2018

I have checked clipboard content when pasting simple list from Evernote (to Chrome):
image

Copied from Evernote browser (Chrome) client :

<ul style="-en-clipboard:true;">
    <li><div>item1</div></li>
    <ul>
        <li><div>item2</div></li>
    </ul>
</ul>
<div></div>

Copied from Evernote desktop (macOS) client:

<ul style="-en-clipboard:true;">
    <li><div>item1</div></li>
    <li style="list-style: none">
        <ul>
            <li><div>item2</div></li>
        </ul>
    </li>
</ul>

so browser client produces invalid HTML while desktop one valid 🤔 It means this issue will be visible while copy-pasting from Evernote browser client (when we fix ckeditor/ckeditor5-list#116).

@zadam
Copy link
Author

zadam commented Nov 5, 2018

I produced the invalid HTML content on Windows Evernote client. So it looks like Mac and Windows versions differ in handling this (which is a bit weird, but not impossible).

@f1ames
Copy link
Contributor

f1ames commented Nov 5, 2018

@zadam Now that you mentioned it I checked to ENEX export in macOS desktop client (the previous comment is the clipboard content when copy pasting from Evernote to the browser) and it produces:

<ul>
    <li><div>item1</div></li>
    <ul>
        <li><div>item2</div></li>
    </ul>
</ul>

so the same invalid content as on Windows.

@jodator jodator self-assigned this Aug 6, 2019
@jodator
Copy link
Contributor

jodator commented Aug 13, 2019

Reinmar referenced this issue in ckeditor/ckeditor5-list Oct 7, 2019
Fix: Improved conversion of invalid nested lists. Closes #115.
Reinmar referenced this issue in ckeditor/ckeditor5-paste-from-office Oct 7, 2019
Other: Remove the `fixListIndentation()` filter in favor of improved list converters fix. See ckeditor/ckeditor5-list#115.
@mlewand mlewand transferred this issue from ckeditor/ckeditor5-list Oct 9, 2019
@mlewand mlewand added this to the iteration 27 milestone Oct 9, 2019
@mlewand mlewand added status:confirmed type:bug This issue reports a buggy (incorrect) behavior. package:list labels Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package:list type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants