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

10.0.x is out. We can totally do this now in 10.1.x 🤓

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'd love to see this land in 10.2.x.

@joaopauloc.dev ~5 months have passed since #13, and Drupal 10.1.x is now using a newer major version of CKEditor 5 — could you please re-test? 🙏😊

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

and

fail as they override that style set by the attribute.

As metaism describes, one of these styles is in a

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 . The former is particularly interesting as it suggests that ckeditor5 isn't really fully supporting the type attribute due to this style. 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 🌱 Make ckeditor_liststyle compatible with ckeditor5 RTBC . That MR's patch + the patch in the previous comment together will make those list style types available in Drupal until the ckeditor issues are sorted out and core updated to the releases that sorted them out.

Thanks for your thorough research, @s_leu! 🤩🙏 Updated issue status accordingly.

Hi, I'm the owner/maintainer of previously mentioned CKEditor List Style → module. Sorry for joining 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.

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.

That works great! (Which is proven by the additional functional JS test coverage you added 👏)

Unfortunately, there are some genuine failures:

    1)
    Drupal\Tests\ckeditor5\FunctionalJavascript\SourceEditingTest::testAllowingExtraAttributes
    with data set "no numberedList-related additions to the Source Editing
    configuration" ('foo...>', 'foobar')
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -'foobar'
    +'foobar'
    
    /builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:122
    /builds/project/drupal/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:79
    /builds/project/drupal/core/modules/ckeditor5/tests/src/FunctionalJavascript/SourceEditingTest.php:111
    /builds/project/drupal/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
    
    2)
    Drupal\Tests\ckeditor5\FunctionalJavascript\SourceEditingTest::testAllowingExtraAttributes
    with data set "no bulletedList-related additions to the Source Editing
    configuration" ('foo', 'foobar')
    Failed asserting that two strings are identical.
    --- Expected
    +++ Actual
    @@ @@
    -'foobar'
    +'foobar'

… except that those are literally for the test cases that were only necessary for now, because <ol type> and <ul type> were only supported today through the Source Editing UX (aka by typing HTML manually), not through the UI! 🤩🥳 These tests were added explicitly in #3274651: Impossible to enable

or
with GHS: switch to List's successor, DocumentList → and are now obsolete.

The correct solution is to:

1. remove those tests
2. add an upgrade path that removes <ol type> and <ul type> from the Source Editing plugin's configuration, and moves it to the List plugin's configuration
3. add upgrade path tests

Done:

1. remove those tests
2. add an update path that removes <ol type> and <ul type> from the Source Editing plugin's configuration, and moves it to the List plugin's configuration
3. add update path tests

It should now be reviewable 👍

I only wrote the update path + update path tests + revised unit tests. @bnjmnm did the hard work. IMHO @bnjmnm can RTBC my part, and I can RTBC his part.

But unfortunately, this will not be able to land for now, because I was forced to add a work-around for 🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed . 😬 Keeping at though, because other than that one line in that one commit (https://git.drupalcode.org/project/drupal/-/merge_requests/5079/diffs?co...) that is out of scope, this is totally reviewable.

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

Just realized something: \Drupal\ckeditor5\Plugin\CKEditor4To5Upgrade\Core::computeCKEditor5PluginSubsetConfiguration()'s case 'ckeditor5_list': should also be updated.

That was easy 😊

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.

Wonderful, thank you!

I do think this would merit being in the release highlights, if it still makes it 😇

📌 [upstream] Update CKEditor 5 to 40.0.0 Needs review landed, this will need a rebase.

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.

🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed landed, this is unblocked.

This conflicted with both 🐛 Fix → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by Fixed and 📌 [upstream] Update CKEditor 5 to 40.0.0 Needs review — that was quite the unraveling 😅

@mstrelan I know. I agree. But that seems even more edge-casey. If we require that, we'd have to wait for yet another upstream feature. I think this is a net improvement for the majority?

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

in the allowed source editing tags.

@Gábor Hojtsy in #54: thanks — and sorry 🙈

@acbramley in #55: That sounds related to the 🐛 Several elements are missing from ckeditor5 Basic HTML Active / 🐛 Change to edit source behavior in 10.2 leads to data loss on edit Active / 🐛 [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified Needs review triple duplicate reports in the past day. I'll try to get that sorted out later today.

Should we block this on https://github.com/ckeditor/ckeditor5/issues/15554? Per #50 and #51, we will probably have to do that.

Calling it. 😞

Please give a 👍 to https://github.com/ckeditor/ckeditor5/issues/15554 🙏

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 from 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

and

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.

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

and not

(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 (

).

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 🐛 [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified Needs review

Yes, I think #74 summarizes this nicely. Unless 🐛 [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified Needs review 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 patche 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.

Tried to rebase for 11.x but it has diverged too far and I got lost. Ended up rerolling for 10.3.x for use on a client project. This is from MR 5079 commit 94fde602. Attaching patch for others to use but setting it to hidden so as not to derail the issue.

I also don't understand why this is blocked, I doubt it's all that common to want to allow <ul type> but not <ol type>.

@berliner @aaronpinero creating a static patch from the MR is ok-ish, but continue working on the issue in patch mode - not

@Taran2L No disagreement then :)

Guys tested patch #80 for 10.2.5 and 10.2.6 - I can see the other types of lists but when selected the list stays numerical as usual. Anyone experiencing the same?

[Subscribing]

Patch from 80 ✨ [upstream] Use CKEditor 5's native and UX Needs work is working for me in the editor and saving content, but produces an error message on editing page where CKEditor appears:

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig() (line 99 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php).
Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig(Array, Object) (Line: 316)
Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getCKEditor5PluginConfig(Object) (Line: 934)
Drupal\ckeditor5\Plugin\Editor\CKEditor5->getJSSettings(Object) (Line: 81)
Drupal\editor\Plugin\EditorManager->getAttachments(Array) (Line: 100)
Drupal\editor\Element->preRenderTextFormat(Array)
call_user_func_array(Array, Array) (Line: 111)
Drupal\Core\Render\Renderer->doTrustedCallback(Array, Array, 'Render #pre_render callbacks must be methods of a class that implements \Drupal\Core\Security\TrustedCallbackInterface or be an anonymous function. The callback was %s. See https://www.drupal.org/node/2966725', 'exception', 'Drupal\Core\Render\Element\RenderCallbackInterface') (Line: 859)
Drupal\Core\Render\Renderer->doCallback('#pre_render', 'element.editor:preRenderTextFormat', Array) (Line: 421)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 240)
Drupal\Core\Render\Renderer->render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 89)
__TwigTemplate_ab337cfa89a1457340e8d67a133a7335->doDisplay(Array, Array) (Line: 394)
Twig\Template->displayWithErrorHandling(Array, Array) (Line: 367)
Twig\Template->display(Array) (Line: 379)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 39)
twig_render_template('core/themes/claro/templates/form/field-multiple-value-form.html.twig', Array) (Line: 348)
Drupal\Core\Theme\ThemeManager->render('field_multiple_value_form', Array) (Line: 480)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 493)
Drupal\Core\Render\Renderer->doRender(Array) (Line: 493)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 240)
Drupal\Core\Render\Renderer->render(Array) (Line: 475)
Drupal\Core\Template\TwigExtension->escapeFilter(Object, Array, 'html', NULL, 1) (Line: 44)
__TwigTemplate_815234656e4a4de144202255a926142d->doDisplay(Array, Array) (Line: 394)
Twig\Template->displayWithErrorHandling(Array, Array) (Line: 367)
Twig\Template->display(Array) (Line: 379)
Twig\Template->render(Array) (Line: 38)
Twig\TemplateWrapper->render(Array) (Line: 39)
twig_render_template('core/themes/claro/templates/node-edit-form.html.twig', Array) (Line: 348)
Drupal\Core\Theme\ThemeManager->render('node_edit_form', Array) (Line: 480)
Drupal\Core\Render\Renderer->doRender(Array, ) (Line: 240)
Drupal\Core\Render\Renderer->render(Array, ) (Line: 238)
Drupal\Core\Render\MainContent\HtmlRenderer->Drupal\Core\Render\MainContent\{closure}() (Line: 627)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 239)
Drupal\Core\Render\MainContent\HtmlRenderer->prepare(Array, Object, Object) (Line: 128)
Drupal\Core\Render\MainContent\HtmlRenderer->renderResponse(Array, Object, Object) (Line: 90)
Drupal\Core\EventSubscriber\MainContentViewSubscriber->onViewRenderArray(Object, 'kernel.view', Object)
call_user_func(Array, Object, 'kernel.view', Object) (Line: 111)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch(Object, 'kernel.view') (Line: 186)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 76)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 58)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 28)
Drupal\Core\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 32)
Drupal\big_pipe\StackMiddleware\ContentLength->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 36)
Drupal\Core\StackMiddleware\AjaxPageState->handle(Object, 1, 1) (Line: 51)
Drupal\Core\StackMiddleware\StackedHttpKernel->handle(Object, 1, 1) (Line: 704)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

New patch based on #80 with isset() added to stop error warning.

When configuring a text_format using patch from #80, I was getting this error:

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->buildConfigurationForm() (line 61 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php).

Same as in #91, this is fixed by adding an isset()

New patch added with this fix

The #93 patch is functioning perfectly on the frontend, but we are still not seeing the list changes in the CKEditor 5 backend field.

The patches aren't applying for me on D10.3.0.

Yeah. This seems to be working for us better than https://www.drupal.org/project/ckeditor_liststyle/issues/3326957 🌱 Make ckeditor_liststyle compatible with ckeditor5 RTBC which is throwing major errors right now.

However, this plugin seems to be only displaying the default list style in the wysiwyg, but is displaying it after publishing. (see screenshots)

Also please note, out of an interest for security, we will not ever use the Merge Request for our patching on our sites unless absolutely necessary.

I think these are the salient parts of the code where the style resides. This might be an relatively easy fix but I am just swamped right now.

I spoke too soon. That worked when I refreshed the page, but then after clearing some caches, it seems that ckeditor injected more styles into the page. I believe they came from here:

So, just to rule out an admin theme issue, I switched to Claro for my admin theme and this still seems to be happening.

The list.js is being injected making it impossible to re-style the list items because css is case-sensitive and ol[type="A"] is the same to css as ol[type="a"].

The only thing I can think to do at this point is write some asynchronous javascript to find the css when it appears and remove it from the page.

I put this in my theme and it works. It's kind of terrrible, but it works. It lags pretty badly when the editor page opens or when you expand a paragraph that has a wysiwyg in it.

(function (Drupal, once) {
  Drupal.behaviors.lastUpdated = {
    attach: function (context, settings) {
      // temove any CSS rule containing list-style-type for ol or ul
      function removeCSSRule(styleTag) {
        if (styleTag) {
          // Get the current CSS content
          let cssContent = styleTag.innerHTML;
          // Create a regular expression to match ol or ul rules with list-style-type
          const regex = /[^{]+\s*(ol|ul)[^,{]*\{[^}]*list-style-type:\s*[^;]+;[^}]*\}/gi;
          // Find all matches
          let matches = cssContent.match(regex);
          // Remove all matching rules and log them
          if (matches) {
            matches.forEach(match => {
              cssContent = cssContent.replace(match, '');
            });
          }
          // Set the updated CSS content back to the style tag
          styleTag.innerHTML = cssContent.trim();
        }
      }
      // Function to check and remove the rule from new style tags
      function checkAndRemoveRule(node) {
        if (node.nodeType === 1 && node.matches('style[data-cke="true"]')) {
          removeCSSRule(node);
        }
      }
      // Initial call to remove the rule from already present style tags
      document.querySelectorAll('style[data-cke="true"]').forEach(function (styleTag) {
        removeCSSRule(styleTag);
      });
      // Observer to detect when new style elements are added to the DOM
      const observer = new MutationObserver(function (mutations) {
        mutations.forEach(function (mutation) {
          mutation.addedNodes.forEach(function (node) {
            checkAndRemoveRule(node);
          });
        });
      });
      // Start observing the document for changes
      observer.observe(document.body, {
        childList: true,
        subtree: true
      });
    }
  };
})(Drupal, once);

I think it might be better to either have our own list.js like https://www.drupal.org/project/ckeditor_liststyle/issues/3326957 🌱 Make ckeditor_liststyle compatible with ckeditor5 RTBC does or contribute to ckeditor 5 directly.

I have modified the patch to be compatible with version 10.3.1. I discovered that we need to run the command drush updb -y if this is the first time we are using the patch.

Also please note, out of an interest for security, we will not ever use the Merge Request for our patching on our sites unless absolutely necessary.

The new best practice workflow for this is to do all issue work in merge requests and then to create/download a patch file from the MR and include it as part of your project repo rather than posting it in an issue comment. There are no security concerns with this approach.

download a patch file from the MR and include it as part of your project repo rather than posting it in an issue comment.

Ah, I see. So, drupal.org just wants us to stop linking to patch files on thier site?

Patch #103 works beautifully for me. I'm reluctant to add it to a production site until it's officially merged into core, but I look forward to the change.

The upstream MR was merged yesterday so hopefully a new release shortly.

Update patch to be compatible with 10.3.2

Hiding patches - please note at #80 onwards the patch appears to have introduced changes from another issue. This has diverted from the MR and and I do not see a reason that these would be intentional changes.

I have updated the patch from #86 as it was no longer applying to 10.3.2 - hiding so not to derail further.

I have not reviewed the additional commits to the MR.

#108 is working fine but #109 is giving this warning

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig() (line 99 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php).

Using the MR plaindiff (https://git.drupalcode.org/project/drupal/-/merge_requests/5079.diff) to patch and the lists show up but when on the node edit page I get a

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig() (line 99 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php).

as well!

For anyone running into the prb

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig() (line 99 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php).

Just loop on each of your CKEditor format Basic HTML, ... and click Save to trigger an update to your configuration

styles: true should be added to every settings.plugins.ckeditor5_list.properties

I'm sure that can be done using the Update hook, I will check it another time

When I apply the patch with Drupal 10.2.6, I could see patch applied successfully. but wysiwyg is not loading for editors.
I could see console errors. app.js

The patch on #109 seems to be working and applies well, on both 10.3.12-dev and 10.4.2-dev.

The Drupal.behaviors.editorStyleFix kludge does have an issue whenever the CKEditor styles aren't loaded into the page during the intial page load. For example a content type which has CKEditor(s) that are only AJAX-loaded inside collapsed Paragraphs, won't get the style fix. Removing the if (context.querySelector('style[data-cke]')) condition inside the behavior does seem to make the behavior trigger on AJAX-loaded editors as well. I guess this is a bit less performant this way, but anyways only the CKEditor-related sheets should be altered as the JS filter() has the hasAttribute('data-cke') in it.

Anyhow, attached is a patch based on #109, only change being the removal of the aforementioned condition, making the list-styles work on AJAX-loaded editors as well.

#114 works great on Drupal 10.3.11 - thank you, @isampo!

I've started tackling rolling #114 on to 11.x but there are some pretty major issues:

1. the presave hook is gone, I've moved it into the new OOP hooks class
2. SmartDefaultSettingsTest is an absolute nightmare to maintain. I've done my best to fix all the failures but there's 8 test cases still failing on the expected db log messages
3. There were some other pretty major conflicts which I hopefully have fixed properly.

Please don't upload any more patches, let's try to get this in!

patch from 114 works for me but produces this error as well:

Warning: Undefined array key "styles" in Drupal\ckeditor5\Plugin\CKEditor5Plugin\ListPlugin->getDynamicPluginConfig() (line 99 of core/modules/ckeditor5/src/Plugin/CKEditor5Plugin/ListPlugin.php)

wrapping new condition in ListPlugin.php with an isset() fixed my error. New patch here for 10.4

I was still getting the error from #112 after adding the patch from #119 on 10.4.5. Turns out when submitting the form values to the plugin config, it was trying to set the "styles" setting outside of the "properties" array. This patch fixes that.

acbramley → changed the visibility of the branch 3274635-upstream-use-ckeditor to hidden.

acbramley → closed merge request !11100

Creating new MRs for rebases is not ideal, now reviewers/contributors don't know where to look. Same goes for uploading patches for various fixes, now it's not clear what the canonical source is. We need to make sure the fixes from the patches in #119 and #120 are valid and they are rolled into the canonical MR.

Closing the old MR for now.