[upstream] Use CKEditor 5's native <ol type> and <ul type> UX

Hello, I spend some time working here but I couldn't finish because I found some issues that I couldn't fix.

The form was updated adding the new style option.

The list plugin also was updated to return the new attributes when the style is selected.

Also, the property style is already returning the use attribute as true.

The result of these changes we can see in the gif attached.

The UI with the type list is working and the user can choose the type of list.

The elements ul and ol is working with the type attribute, so when the style is checked on the admin area the elements do not use list-style-type and use the type attribute.

The issue that I couldn't fix is on the CKEditor UI that didn't change when we select another type of list, if we save or click on the source code is correct, the type attribute was correctly added, but in the UI of CKEditor, nothing changes. Also if we save the value when the node is rendered the ul or ol is displayed correctly with the attribute type.

So, if anyone wants to start from my patch is attached here, and will try to fix it in the next few days

So I applied the above patch on D10.1.x-dev to figure out the what might be causing the problem stated by @joaopauloc.dev,

What I found:

For OL:

1. Looks default in the CKEditor UI
2. Looks as per the style selected on node view page

For UL:

1. Shows up with an empty circle in the CKEditor UI instead of a filled black circle as per the toolbar logo
2. Shows up as a filled black circle on node view page irrespective of the style selected

For Styling:

• While inspecting the element and styles applied to it, the li had styles coming from list-style-type property from 2 classes which were set on ol (its parent)
• The 2 classes were .ck-content ol which was applying through the style tag
• and ol which was coming through a css file from /sites/default/files/css
• Turning off the list-style-type properties on both the classes worked and the UI as well as the node view page showed the selected style
• the same was the case with ul > li as well

I am attaching a screen recording of everything I have found and tried out, hopefully it helps!

thanks @@joaopauloc.dev,
We are decoupled system, so your patch helped us! However, it still displays the incorrect style for the Drupal view and edit view.

I'll check that.

I also checked the patch from #13 recently and did so again today with the latest commit of 10.1.x . Unfortunately the problems that metaism outlined in #14 persist and some styles make the application of a list type via the type attribute on <ol> and <ul> fail as they override that style set by the attribute.

As metaism describes, one of these styles is in a <style> tag which is actually shipped with ckeditor, see here: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-list/theme/list.css . The other mentioned style is located in core/themes/claro/css/base/elements.css (vanilla Drupal install).

The former is particularly interesting as it suggests that ckeditor5 isn't really fully supporting the type attribute due to these styles shipped and applied unconditionally by ckeditor5's list plugin. Looking at the demo page for lists, the type attribute is not used there either: https://ckeditor.com/docs/ckeditor5/latest/features/lists/document-lists.html . I could be missing something here, but it seems like that's a fix that needs to be done in ckeditor5, at least partially. The style in elements.css can be done here of course.

I investigated some more on this and tried if I can replicate the issue in a plain html page initializing a CKEditor5 instance to verify that this is a ckeditor5 problem and it turns out that the feature is indeed broken inside ckeditor itself. I filed an issue on github outlining the details: https://github.com/ckeditor/ckeditor5/issues/14613

Adding a separate patch that will remove some styles from core themes which are actually unnecessary as they are just the same as browser defaults but are preventing the styling using the list's type attributes in this case here.

It's probably worth to mention that there's a working implementation of this inside a contrib module patch (MR) over at https://www.drupal.org/project/ckeditor_liststyle/issues/3326957 . That MR's patch + the patch in the previous comment here together will make those list style types available in Drupal until the ckeditor issues are sorted out and core updated to the release that sorted them out.

Hi, I'm the owner/maintainer of previously mentioned CKEditor List Style module. Sorry for joining this party at a later stage.

I am very much looking forward to move this functionality to the core. And it is almost there. :)

Please find the attached re-roll of the patch with some CSS additions, and try it on a Firefox browser.

Here is why: https://caniuse.com/mdn-css_selectors_attribute_case_sensitive_modifier

And at this point I also would like to ask why we insist using the type attribute for the OL and UL elements?

Currently at least, the CKEditor uses following type attribute values for all available list styles:

ul[type="disc"]     // Disc
ul[type="circle"]   // Circle
ul[type="square"]   // Square

ol[type="1"]        // Decimal
ol[type="i"]        // Lower-Roman
ol[type="I"]        // Upper-Roman  
ol[type="a"]        // Lower-Latin
ol[type="A"]        // Upper-Latin

However, the ordered-lists are having a problem with these assignments, at least in my experience.

The values are the same, just lower and upper cases, however, by default, the CSS attribute selectors are case-insensitive.
See https://developer.mozilla.org/en-US/docs/Web/CSS/Attribute_selectors

I didn't find any discussions around this in drupal.org, i hope I'm not spamming this thread with this : )

Sorry, the patch in previous comment was against 10.x branch.

This is now against 11.x branch.

MR has a solution that works around https://github.com/ckeditor/ckeditor5/issues/14613

I assume some tests will fail since the plugin added an option, but that's easy enough to address. I wanted to get this goofy workaround up asap so this could officially be un-postponed.

Workaround implemented and tests added.

Seems upgrade path is there. But test failures seem legit.

Do think this is ready to go but till [#3396628 am postponing it. But that issue is RTBC so hopefully lands very soon and this could be marked RTBC right after.

The patches in this comment are for Drupal 10.1.x, but I can only presume the issue I discuss below applies equally to 11.x.

* Patch 'a' is a re-roll of the (11.x) MR from commit ea2a4f1a1368ec4347419e87fd43ebeb3764a219 against 10.1.x.
* Patch 'b' is a modified version of the same (see below)

The interdiff is between the two. The modifications are removing all cases of:

ul:not([type]) {
  list-style-type: disc;
}
 
ol:not([type]) {
  list-style-type: decimal;
}

Those cause problems but I believe also shouldn't be necessary. User agents should be defaulting to those same styles when a list has no type attribute and no list-style-type style; so setting them explicitly should be a no-op at best.

At worst, the :not([type]) makes these selectors more specific than plain ul and ol and therefore pre-existing styles for the less-specific selector get overridden. This is bad when, say, a list was previously being styled to have a list style of none and winds up with disc instead because it didn't have a type attribute in the markup. Using Claro as my admin theme, I was observing that issue in practice in, for instance, the Operations drop-down menus at admin/content.

#42b did not seem to fix the problem in our case. We went with this as a temporary workaround:

.ck-content ol[type='A'] {
  list-style-type: upper-latin;
}

...and so on down through the list hierarchy.

That's certainly the other option. I was hoping it wouldn't be needed, but there must be something else styling list-style-type for those list items in your case, meaning that you then need to override it. I guess it's going to be safest to assume that will be the case, even if it's not true for everyone.

In some cases it might be preferable to only allow the list style on ordered lists, but not unordered lists. With this patch there is no option to enable these separately.

Haven't retested but appears to have test failures.

Re #48 I didn't realise that was upstream, so I agree we should leave that for now. In our case the editors have asked for numbering options only. I'm sure we can get away with giving them unordered list style options too, but our stylesheet displays them all the same anyway, so we'll probably end up with a bug report.

Same here RE: the desire to only allow style options for ordered lists. Is there an existing upstream issue for this? I can make my case for it there if so

Needs Work on account of #42 as well -- it's not safe for the stylesheet to use :not([type]).

#42 patch b works well on a clean install with Claro and Olivero. I think we should update the MR accordingly, and the approach in #43 should be left to individual themes to decide if they need.

Untagging from release highlights as this did nto make it into 10.2. Would be great in 10.3 highlights once it lands though :)

Having issues with this patch while upgrading to 10.2.

We already had the latest MR changes applied as a patch to 10.1.7 and list styles were working well. After upgrading to 10.2.0 and running db updates it set plugins.ckeditor5_list.properties.styles to false

Manually going through the UI to edit the editor/filter config, I had to remove a large amount of tags from sourceEditing (probably unrelated) until it would let me save.

When it finally let me save, I tried multiple times to check "Allow the user to choose a list style type", wait for the AJAX request to finish, then save the form. After each save, I'd return to the edit form and the checkbox would be unticked.

Debugging into ListPlugin::submitConfigurationForm I can see the config is set to TRUE so I'm not sure what's going wrong here.

In both 10.1 and 10.2 config I have <ol class="ol--counters" type reversed start> in Allowed HTML tags, and do NOT have <ol type> in the allowed source editing tags.

EDIT: This worked - manually editing the properties.styles to TRUE AND adding <ol type> and <ul type> to the sourceEditing allowed tags.

I'd prefer not to see https://github.com/ckeditor/ckeditor5/issues/15554 as a blocker to the current issue as it seems to be a separate issue. While waiting for that issue, a developer could use display:none to not render the drop down for unordered lists in the WYSIWYG, no?

@Wim Leers re #56 - that may be related but sounds quite different. Sorry if I confused things in my comment by over explaining haha.

The main issue I'm seeing is that saving a text format is not respecting the styles or source editing settings that relate to this particular issue.

I.e, I save the text format without changing anything and export config and that results in this diff:

The only way I can keep the list styles enabled is manually editing the format yaml and importing.

@acbramley I wanted to make sure I understand what you were reporting in #55 and #59. This is important to me because this issue is blocking the upgrade of my Drupal websites to 10.2. I am not going to tell my content editors that I now have to take away what is really a basic, essential feature of the content editor.

It sounds like you were saying that, following the upgrade to Drupal 10.2, I will need to:

- re-save my text format configurations
- export the website configuration to xml
- manually update the editor.editor.x.yml files to set the list properties.styles to true and add <ol type> and <ul type> to the allowed tags
- import the configuration

It also sounds like I will need to repeat this process if, in the future, I need to change my text format configurations for some other reason besides list styles. Is that correct? Are there any other steps that are necessary to get this to work? Is there a new patch I need to apply or are these steps sufficient without a patch? Thanks for working through this.

---------

I very much hope some of this can be resolved via patch before 10.3. This is really basic functionality for the editor that should never have been missing in the initial release of 10.0. More than a year later, it would be nice to see this finally cleaned up.

Directly accessing the 'styles' index in $this->getConfiguration()['properties']['styles'] (in ListPlugin.php) was throwing me warnings when editing nodes, so I'm using isset() to check if the index exists before accessing it.

@aaronpinero yes, except now I'm seeing for some reason that I don't need to keep the tags in ckeditor5_sourceEditing - our config export kept forcing the removal of those (but it would keep styles: true) even without editing the filter format. I ended up testing the list styles feature without those allowed tags and it works just fine.

So all you need to do is:

1. Save your filter format
2. Maunally edit the editor yaml file and set settings.plugins.ckeditor5_list.properties.styles to true
3. Import the config again.

If you edit the filter format again you will indeed need to make sure to revert the styles: false change

This is indeed a nice feature - I think it can be shipped as is (i.e. without control over specifics for UL/OL). Maybe consider unpostponing it?

MR is important, hid everything else

#63 is now fixed with the latest MR changes - thanks @Taran2L!

The MR is green too, should we consider unpostponing?

hi @acbramley, we had a discussion with @Wim Leers the other day in Slack: https://drupal.slack.com/archives/C1BMUQ9U6/p1708010000803059

The problem is that not having that choice from the start means that some sites who only want <ol type>and not <ul type> (or vice versa) will see it appear on both. And so then they’re allowing markup that they don’t want to allow. We don’t want to force that upon them.

So, probably this is no go for now. Don't know

My response is that: this fix is a direct upgrade path from https://www.drupal.org/project/ckeditor_liststyle

I don't understand the argument that making this an option is currently problematic.

Sure, in its current state such an option would mean "enabled for both ul and ol" or "disabled for both ul and ol", and some users might want only one or the other. But at the moment those users don't have any way to do that, so they'll be no worse off. Meanwhile the other users who want it enabled for both ul and ol also don't have any way to do that, and their requirement could be fulfilled. (And I'm struggling to imagine that the latter would not be the majority.)

This would in no way preclude a more granular config down the track, and in the interim it would resolve the issue for lots of people.

Do we need to hang this on a ckeditor feature with an indeterminate timeline? Seems like we could provide this as a feature with the caveat and support the new feature after ckeditor supports. That would provide some immediate functionality for people that can use it as is but also highlight the feature request to more users.

With that in mind though, we should probably default this off and make sure administrators opt in and understand what they're opting in to.

A quick test of the merge request, this seems to work pretty well as a user. Very nice!

Totally agree with #70 and #71 - why not commit as is and open a follow up to track upstream to allow enabling only one or the other?

I believe the issue is that currently users may have <ul type> enabled in source editing, but not <ol type>

With the introduction of this plugin you would not be able to do that - if you left the plugin option disabled and tried to add <ul type> to the source editing you would get this validation error:

The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: List (<ul type>).

I agree we shouldn't need to wait for CKEditor to proceed with this, but I do think this scenario is another use case for reverting the validation of optional attributes that requires you to use the plugin #3410100: [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified - that way the editor would be free to keep using source editing for support type on only 1 of the elements, and users who want both get the benefit of this awesome work.

Yes, I think #74 summarizes this nicely. Unless #3410100: [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified is resolved, I think this needs to be blocked on the upstream issue. So the fastest way to get this resolved is to show your support in the upstream issue by adding a thumbs up.

I believe it should not be postponed as list style is not the part of source editing and it is important feature which was working till 10.1.5 along with ckeditor 5 and all the patches are either considering 10.1.x or 11.x but nothing seems to be working in 10.2.x version.

Fix for Drupal 10.2.x

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

PHPCS Fixes

@saurabh rawat, please stop producing patches, and use MR, thanks

Hiding patches but MR needs a rebase.

I personally appreciate the patch files from saurabh rawat to have something usable in D10.2.

I also appreciate having a patch available. I tested the patch in #77 and it worked for me in Drupal 10.2.4. I understand the desire to have folks follow the current practice of using a merge request, but it is also convenient to have the patch. I think we can encourage the following of desired practice without discouraging helpful contributions.