[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.

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 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).

Simply adding an isset() to that line fixes the error message for me.

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. (Sorry, it was the issue with the Seven theme)

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

EDIT: Ah, I see. The css has been changed for extant core themes, but we'll have to adjust them for contrib admin themes. We're using a custom subtheme based on Adminimal/Seven

I was able to apply the patch here to get it working: https://www.drupal.org/project/seven/issues/3436941

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 thought it was from a ckeditor js but I don't think so now. I am continuing the search for where this css is being injected from.

I'm fairly certain it's injected with this:

/docroot/core/assets/vendor/ckeditor5/list/list.js

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"] and ol[type="I"] is the same as ol[type="i"].

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.

Makes me wonder, however, if ckeditor are just doing this on purpose anyways.

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 does or contribute to ckeditor 5 directly.

Or include this library which works over on the liststyle module as well. I haven't looked at it in detail, but think the code in this library is what eliminates the forced ckeditor styles.

"ckeditor/liststyle": "4.16.2",

        "ckeditor/liststyle": {
            "type": "package",
            "package": {
                "name": "ckeditor/liststyle",
                "version": "4.16.2",
                "type": "ckeditor-library",
                "dist": {
                    "url": "https://download.ckeditor.com/liststyle/releases/liststyle_4.16.2.zip",
                    "type": "zip"
                },
                "extra": {
                    "installer-name": "liststyle"
                }
            }
        },

I have modified the patch to be compatible with version 10.3.1.

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.

I am also not sure if any of the changes in subsequent patches based on 80 are included or relevant to the MR given they have diverged.

*Edit* - for clarity - this patch is only for capturing a previous known state of the MR for other people that were using the patch in #86

#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).

Edit on Drupal v10.2.6 and the patch on #93 is working for me - https://www.drupal.org/files/issues/2024-06-13/3274635--ckeditor_list_st...

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.

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.

Patches in #119 and #120 seem to be missing something, again it's hard to figure out what since no interdiffs have been provided. When applied to 10.3.x I'm unable to save the editor config screen as it complains about the properties key in config schema.

Going all the way back to #86 is working on 10.3.

I accidentally rebased onto the closed branch so I've toggled MRs since the latest one had other issues.

Please, if you want to contribute to this issue do not upload more patches or open more MRs, instead get push access via the button under the issue summary and contribute directly to this MR

https://git.drupalcode.org/project/drupal/-/merge_requests/11100

Got this green, it would be great for people to test this on 11.x with the latest changes in the MR.

God help the next person that has to deal with SmartDefaultSettingsTest 💀

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.

Hi
have manually tested MR 11100 on Drupal 11 version.
The MR is applied Successfully...

For ul and ol type the styles are showing sa per the selection on the node page.

Attaching screenshot for reference

RTBC+1

Keeping in need review for code verification.

Changes on the MR11100 look good to me and likewise with #135 I have tested this and the functionality and upgrade path work as expected.

I've been running versions of this patch in production for a while now without issue.

I think this is ready to go to RTBC - the only thing missing was a change record. I have added one https://www.drupal.org/node/3529709

If somebody can please review / amend the CR then I think this should move to RTBC

@ericgsmith the CR reads very well! I can't RTBC myself because I did a lot of the code changes but I think it's fine for you to.

Also kicked off another rebase since the branch was quite behind. Thank goodness there weren't any more conflicts!

Thanks Adam, and thank you for all the work here!

Great work here - nice to see this finally land! Tagging as a release highlight and will publish the change record.

Committed e117838 and pushed to 11.x. Thanks!

I tried to credit everyone who provided code changes or helpful comments that moved the issue along in some way; I did not credit those who just uploaded patches, because we should be using MR workflows now. Apologies if I missed anyone as there were a lot of comments to read.

Great effort everyone, thanks @longwave

It still produces the 'Warning: Undefined array key "styles"' indicated in comment #118, but on line 111. I'm uploading a patch that fixes the warning.

Hi @pascuperbla that will need to be fixed in a new issue, are you able to create that and add the steps to produce the warning?

I'm interested in backporting this to 10.6 - should I file a separate issue? Is it the kind of thing that might make it in to that branch?

@cboyden is there a reason you can't upgrade to Drupal 11? 10.6 is a maintenance minor release where we try to minimise changes to ensure stability; the criteria for backports is listed at https://www.drupal.org/about/core/policies/core-change-policies/allowed-... - as a feature request this doesn't appear to fit the criteria.

I needed this before our D11 upgrade.

Here's a function-only patch I made for 10.4 that works alright so far. YMMV

The issue will be fixed in Drupal 11.3, but for 11.2 there’s currently no patch available. I recently upgraded from Drupal 10.5 to 11.2, and the functionality has stopped working. The available patches are also failing to apply.
We can use the patch of this MR (https://git.drupalcode.org/project/drupal/-/merge_requests/11100) for the 11.2.x .

I doubt the MR diff would ever change anymore but still here's a static patch from the MR11100 changes for composer installs.

I have successfully applied 3274635-cke-lists-10-4-x.patch to Drupal 10.5. I needed to leave out the changes to ckeditor5.js. With those changes in place, there was no styling on the lists inside CKEditor.

It would be helpful to be able to configure useAttribute.

Bumping priority & we should fix the upgrade path before 11.3.0

This change makes sense to me. Please ship it as a super-critical bug fix. Can we add an explicit test in a follow-up MR?

Filed #3561755: Add an update path test for #3274635 for update path tests.

@catch and I discussed this hotfix. We agree it can be committed with the upgrade tests as a followup if the upgrade has been manually tested so that we confirm the issue is actually resolved. It should be tested both on a site where the update already broke and on a site that has not yet updated. Thanks!

In order to commit this we need to manually test with a site that already broke as well, but given they have already run the update hook I don't see how this fix will help them.

Am I correct in thinking that we could do this to cover both cases:

1. Rename ckeditor5_post_update_list_type() to ckeditor5_post_update_list_type_2()
2. Add ckeditor5_post_update_list_type() back as a no-op

I think this should:

1. Run the update only once for users that haven't run it before
2. Run the update again for users that don't have source editing enabled, which will fix it for them
3. Run the update again (but that will do nothing) for users that do have source editing enabled

I haven't tested this to see if it works, however.

Yes I think we need to do #161 to fix any sites that already updated to the release candidate. Moving to needs work for that.

Pushed a commit that does #161. The comments might need refinement but I think that's what we need for sites that went to the release candidate.

Needs manual testing both from 11.2.x direct to 11.3.x, and from 11.2.x -> 11.3.0-rc1 -> 11.3.x

Did part of the necessary manual testing:

1. Pulled 11.2.x HEAD and installed Standard
2. Confirmed that the Full HTML format did not have the list style control
3. Checked out 11.3.x HEAD and applied the patch
4. Ran database updates
5. Confirmed that the Full HTML format now has the list style control, and it's enabled

Tested the already-at-RC scenario:

1. Checked out 11.2.x HEAD and installed Drupal
2. Skipped confirming that the Full HTML format doesn't have the style control; this wouldn't have changed since my last test
3. Checked out 11.3.0-rc1 and updated to it. "Updates Text Editors using CKEditor 5 to native List "type" functionality" was in the list of pending updates.
4. Ran the update and confirmed that Full HTML now has the style control, and it's enabled
5. Checked out 11.3.x and applied the patch
6. Visited update.php and confirmed that "Updates Text Editors using CKEditor 5 to native List "type" functionality" is the only update
7. Ran the update and confirmed that Full HTML still has the style control, and it's still enabled

That seems right, then!

Drupal CMS is working around this, so not a blocker, but tagging it as an issue of interest. It'd be nice for this to be fixed before our stable release!

It looks like that test confirms it for the case where the update would already have worked, but not for the case where it didn't?

I can confirm the upgrade path is working for the scenario where the config was missed (in addition to verifying steps from 164/1165) - however there is an additional bug introduced now where the config can not be correctly updated after the update hook has run.

Update hook working:

1. Checked out 11.2.x HEAD and installed Drupal using standard profile
2. Create a new text format using CKEditor5 as text editor, with the "Bulleted List" button enabled and source editing not enabled:
   

   $ drush config:get editor.editor.test_list_only
   uuid: f84bfb98-2071-44d8-9ef0-e7544358422e
   langcode: en
   status: true
   dependencies:
     config:
       - filter.format.test_list_only
     module:
       - ckeditor5
   format: test_list_only
   editor: ckeditor5
   settings:
     toolbar:
       items:
         - heading
         - bold
         - italic
         - bulletedList
     plugins:
       ckeditor5_heading:
         enabled_headings:
           - heading2
           - heading3
           - heading4
           - heading5
           - heading6
       ckeditor5_list:
         properties:
           reversed: true
           startIndex: true
         multiBlock: true
   image_upload:
     status: false

3. Checked out 11.3.0-rc1 and updated to it
4. Ran the update and confirmed that "Updates Text Editors using CKEditor 5 to native List "type" functionality" was run.
5. Observed the issue - full html and basic htl editor config has correct config, test_list_only is missing as per comment #160
   

   drush config:get editor.editor.test_list_only settings.plugins.ckeditor5_list
   'editor.editor.test_list_only:settings.plugins.ckeditor5_list':
     properties:
       reversed: true
       startIndex: true
     multiBlock: true

6. applied patch from MR14034 and confirmed update ran again
7. Confirm config missed from the initial update is now as expected:
   

   $ drush config:get editor.editor.test_list_only settings.plugins.ckeditor5_list
   'editor.editor.test_list_only:settings.plugins.ckeditor5_list':
     properties:
       reversed: true
       startIndex: true
       styles: false
     multiBlock: true

New bug

After the update hook is run I edit the format to enable the plugin as per the change record:

Sites without text formats currently allowing the type attribute can enable it manually using the "Allow the user to choose a list style type" option from the List CKEditor 5 plugin settings:

1. Edit the text format use in the test above (e.g. the one without source editing enabled)
2. Check "Allow the user to choose a list style type" from the list fieldset
3. Save
4. Return to the edit form, value is unchecked - and the functionality is not enabled when using the format

What is happening with this change is that the pre save hook runs, source editing is still disabled so the else if condition array_key_exists('ckeditor5_list', $settings['plugins']) is still true so the config gets reset to FALSE again despite the user enabling it.

I think we need to change to something like this similar to the check on L425 to ensure that we are only setting it to false it it doesn't exist already, e.g:

      elseif (array_key_exists('ckeditor5_list', $settings['plugins']) && !array_key_exists('styles', $settings['plugins']['ckeditor5_list']['properties'])) {
        $settings['plugins']['ckeditor5_list']['properties']['styles'] = FALSE;
      }

Summar of Slack discussion with info from @catch and @xjm:

• The latest MR suggestion needs review and applying.
• Then it needs a full round of manual testing again.
• If an upgrade path test is also added, that would be best (not an absolute must have).
• Probably also need a follow-up to add tests for the UI step @ericgsmith found.

Note that deferring the upgrade path tests is dependent on manual testing of the upgrade path itself (in addition to the bug fixes), both from 11.2 and from an 11.3 beta with the broken update. See the comment in #159. That goes for subsequent regressions as well as the original one.

I applied the suggested after testing it - I note comment above was for somebody to review it first, but to move the issue forward I've applied it so I can set it to Needs review.

I repeated my manual tests above and the upgrade path is still work, and the issue I found in 168 is resolved although somebody else would need to verify that.

Manual test - 11.2.x => 11.3.0-rc1 => 3274635-add-missing-update-path-if-using-CKE5-list-plugin-only at 2f315be

Config before upgrade:

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
  multiBlock: true

$ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
  multiBlock: true

After update to 11.3.0-rc1

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
  multiBlock: true

  $ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
    styles: true
  multiBlock: true

After update to 3274635-add-missing-update-path-if-using-CKE5-list-plugin-only at 2f315be

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
    styles: false
  multiBlock: true

  $ drush config:get editor.editor.basic_html settings.plugins.ckeditor5_list
'editor.editor.basic_html:settings.plugins.ckeditor5_list':
  properties:
    reversed: false
    startIndex: true
    styles: true
  multiBlock: true

After enabling the styles option via UI it saved correctly and is now present:

$ drush config:get editor.editor.list_only settings.plugins.ckeditor5_list
'editor.editor.list_only:settings.plugins.ckeditor5_list':
  properties:
    reversed: true
    startIndex: true
    styles: true
  multiBlock: true

Setting to needs review for the remaining tasks from #169:

• The latest MR suggestioncommit needs review and applying.
• Then it needs a full round of manual testing again.
• If an upgrade path test is also added, that would be best (not an absolute must have).
• Probably also need a follow-up to add tests for the UI step @ericgsmith found.

Rebased and added the update path test and UI test.

Test-only job: https://git.drupalcode.org/issue/drupal-3274635/-/jobs/7635895
Update path test fails as expected.
(UI test does not fail, since UI was not broken previously).

Tests look great both for the upgrade path and the UI nearly-regression, glad we were able extend the existing tests for the new coverage.

Thanks for the swift fixes here. The new test coverage looks great, as simple as can be while proving the problem is fixed - nice work.

Committed and pushed 70a49ee7a2a to 11.x and 2a333220f57 to 11.3.x. Thanks!

Actually re-reading the diff Wim linked:

Shouldn't be

!array_key_exists('styles', $settings['plugins']['ckeditor5_list']['properties']['styles'])) {

?

Re: #180, no, because styles is a not a subkey of itself (styles). There was some conditional logic I thought about simplifying and tweaking when I added the tests, but since the existing code was already manually tested and there was urgency, I think it's fine.

Re-read the code and you are right, I misread this as it was isset and not array_key_exists 🤦🏽

FYI.
Drupal 10.5.3.
Applied #143.
Can't save text format when enabled styles.
"'properties' is an unknown key because settings.plugins.%key is ckeditor5_style (see config schema type ckeditor5.plugin.ckeditor5_style)"

I just encountered the same error after applying patch #143 in Drupal 10.5.2:
"'properties' is an unknown key because settings.plugins.%key is ckeditor5_style (see config schema type ckeditor5.plugin.ckeditor5_style).
Enable at least one style, otherwise disable the Style plugin."