Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

List plugin will merge lists that are not the same #8829

Closed
mlewand opened this issue Jan 13, 2021 · 4 comments
Closed

List plugin will merge lists that are not the same #8829

mlewand opened this issue Jan 13, 2021 · 4 comments
Assignees
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. package:list resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.

Comments

@mlewand
Copy link
Contributor

mlewand commented Jan 13, 2021

📝 Provide detailed reproduction steps (if any)

Currently list plugin's converters (during downcasting) will consider lists the same as long as they have: same tag name and classes.

See:

if ( !firstList || !secondList || ( firstList.name != 'ul' && firstList.name != 'ol' ) ) {
return null;
}
// Both parameters are list elements, so compare types now.
if ( firstList.name != secondList.name || firstList.getAttribute( 'class' ) !== secondList.getAttribute( 'class' ) ) {
return null;
}

And combine both lists.

Lists could be as well differentiated by different attributes. For instance, per spec, ordered list can contain ol type (values like a, I or 1 denote numbering type, though it can be done by CSS too). With this logic two different lists will be merged.

Also developer might use custom data- attribute to semantically differentiate some lists type.

For instance I saw one CKEditor 5 implementation where there were 3 list types (numbered, bulleted and third). Third type was merged with bulleted as it was downcasted to ol[type] and it was considered to be same as plain ol which caused bug.

📃 Other details

  • Browser: Any
  • OS: Any
  • First affected CKEditor version: 24.0.0 (didin't check earlier but surely it's here for a longer time)

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

@mlewand mlewand added type:bug This issue reports a buggy (incorrect) behavior. package:list domain:dx This issue reports a developer experience problem or possible improvement. domain:v4-compatibilty support:2 An issue reported by a commercially licensed client. labels Jan 13, 2021
@Reinmar Reinmar added this to the nice-to-have milestone Jan 18, 2021
@mlewand
Copy link
Contributor Author

mlewand commented Jan 25, 2021

I pushed a i/8829 branch where I verified that adding attribute check solves the problem.

However, there are 11 failing unit test in list style so there's probably a need to accommodate for the fix in list style plugin too.

@Reinmar Reinmar added domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. and removed domain:v4-compatibilty labels Mar 9, 2021
@jacekbogdanski jacekbogdanski self-assigned this Jul 20, 2021
@jacekbogdanski jacekbogdanski modified the milestones: nice-to-have, iteration 45 Jul 22, 2021
@Mgsy Mgsy modified the milestones: iteration 45, nice-to-have Aug 2, 2021
@Reinmar Reinmar added squad:core Issue to be handled by the Core team. and removed squad:compat labels Sep 27, 2021
@pomek pomek removed this from the nice-to-have milestone Feb 21, 2022
@dunx
Copy link

dunx commented Feb 21, 2024

This remains an issue 2 years on from the last update. Is there another issue anybody is aware of?
We have a client that has CKE4 markup with two very different styled lists next to each other and they are arbitrarily being merged by CKE5.

@star-szr
Copy link

Is there another issue anybody is aware of?

See #14478, more recently the AdjacentListsSupport plugin has been added that somewhat handles this use case. See #14944 for a known issue with regard to nested lists.

@Witoso
Copy link
Member

Witoso commented Feb 27, 2024

Yes, thank you @star-szr, let's close it as a duplicate, this is resolved viaAdjacentListsSupport, and we need to fix #14944.

@Witoso Witoso closed this as not planned Won't fix, can't repro, duplicate, stale Feb 27, 2024
@Witoso Witoso added the resolution:resolved This issue was already resolved (e.g. by another ticket). label Feb 27, 2024
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
domain:dx This issue reports a developer experience problem or possible improvement. domain:v4-compatibility This issue reports a CKEditor 4 feature/option that's missing in CKEditor 5. package:list resolution:resolved This issue was already resolved (e.g. by another ticket). squad:core Issue to be handled by the Core team. support:2 An issue reported by a commercially licensed client. type:bug This issue reports a buggy (incorrect) behavior.
Projects
None yet
Development

No branches or pull requests

8 participants