Problem/Motivation

We "solved" the problem of supporting <ol start> by using the SourceEditing functionality for that.

But in CKEditor 5 32.0.0, they've added native support for <ol start> (and even for <ol reversed>).

Which means that rather than having to edit HTML by hand, the content creator can now do that through the UI!

Steps to reproduce

N/A

Proposed resolution

Use it!

Clearest example I could find of how to configure this: https://github.com/ckeditor/ckeditor5/commit/99c818c7b9ddf62aa958b9d265d...

Remaining tasks

After Drupal is updated to that version of CKEditor 5, we should modify the ckeditor5_list plugin definition to:

  1. have a plugin class that allows the site builder to configure the list functionality to specify the starting index and whether the ordered list should be reversed (two separate booleans to store). See \Drupal\ckeditor5\Plugin\CKEditor5Plugin\Heading for an example — note this will also require an addition to the config schema, but that too has an example
  2. this should cause \Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest to fail — because a better upgrade path becomes possible, so we should update that test coverage

User interface changes

TBD

API changes

None.

Data model changes

None.

Release notes snippet

TBD

Issue fork drupal-3261599

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Wim Leers created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Version: 9.3.x-dev » 10.0.x-dev
Issue tags: -Needs screenshots +stable blocker
Parent issue: » #3238333: Roadmap to CKEditor 5 stable in Drupal 9

See #3261600-29: Update to CKEditor5 v32.0.0 — this will soon be unblocked. The configuration I used there:

 core/modules/ckeditor5/ckeditor5.ckeditor5.yml | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
index 3467d34ec8..a1c9d60eb9 100644
--- a/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
@@ -270,7 +270,14 @@ ckeditor5_linkMedia:
 
 ckeditor5_list:
   ckeditor5:
-    plugins: [list.List]
+    plugins:
+      - list.List
+      - list.ListProperties
+    config:
+      list:
+        properties:
+          startIndex: true
+          reversed: true
   drupal:
     label: List
     library: core/ckeditor5.list

which yielded:

Wim Leers’s picture

Title: [PP-1] Use CKEditor 5's native <ol start> support (and also support <ol reversed>) » Use CKEditor 5's native <ol start> support (and also support <ol reversed>)
Status: Postponed » Active

#3261600: Update to CKEditor5 v32.0.0 landed — this is now unblocked!

hooroomoo made their first commit to this issue’s fork.

bnjmnm’s picture

I was checking this out with @hooroomoo and ran into a test failure that (at least at the moment) cant tell if it's a problem with the test, this implementation, or a new edge case.
SmartDefaultSettingsTest is failing on the filter_only__filter_html can be switched to CKEditor 5 without problems (3 upgrade messages) iteration. With the error The following tags/attributes are not allowed in the updated text format:<ol type="1 A I"> This failure is when $unsupported_tags_attributes isn't empty

Here's how $unsupported_tags_attributes made:

 $allowed_tags = HTMLRestrictions::fromTextFormat($text_format);
      $enabled_plugins = array_keys($this->manager->getEnabledDefinitions($updated_text_editor));
      $updated_allowed_tags = new HTMLRestrictions($this->manager->getProvidedElements($enabled_plugins, $updated_text_editor));
      $unsupported_tags_attributes = $allowed_tags->diff($updated_allowed_tags);

So it's the diff of $allowed_tags against $updated_allowed_tags. $updated_allowed_tags includes <ol type> (now offered in the), which implicitly allows the <code><ol type="1 A I"> that is deemed unsupported. diff() is intentionally written this way - if the caller is more restrictive it is returned as part of the diff.

So two questions come to mind:

  1. Is adding the type attribute in this issue's scope?
  2. If yes, should the test be refactored to account for situations where SmartDefaultSettings grants any-attribute-value in CKeditor5 when it's predecessor was resticted to specific ones?

These may answer themselves once I'm away from the IDE for a moment, but now it's documented.

Wim Leers’s picture

#7:

  1. No, it is not! 👍 Good catch, I independently caught it too :D
  2. N/A because "no".

Review

  1. This MR looks great already! 🤩 Left a range of detailed remarks on the MR. Most of them should be trivial to address! 🥳
  2. #7: the root of the problem is PHP doing weird things with array keys: https://3v4l.org/fVk1K. This is a bug in HtmlRestrictions. Opened #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers for that. It's not a blocker for this issue, because … the metadata being added here is wrong (see review on the MR): this MR does not add support for <ol type> nor <ul type>. I added #3274635: [upstream] Use CKEditor 5's native <ol type> and <ul type> UX for that. And while investigating that … found #3274651: Impossible to enable <ol type> or <ul type> with GHS: switch to List's successor, DocumentList
  3. Test failure 1 out of 4: see comment on MR on how to fix it.
  4. Test failure 3 out of 4 (SmartDefaultSettingsTest::test with data set "basic_html_with_h1 can be switched to […]) → this is because #3273527: Upgrade path never configures the ckeditor5_heading plugin to allow <h1> just landed and added one more test case.
  5. Test failure 4 out of 4: see point 2.
hooroomoo’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Needs work

Looking great! There's only a few coding style nits and one missing comment … and then this is RTBC! 🥳

hooroomoo’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
Wim Leers’s picture

All commits up to 97b690a6a5c476895eb4878dfc686e8fc6355a79 in patch form, for testing against all 3 branches.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

Discussed with @hooroomoo — un-RTBC'ing only to add the unit test that #3229078: Unit tests for all @CKEditor5Plugin plugin classes would otherwise have to add.

Wim Leers’s picture

Issue tags: -Needs tests

Well that was a perfect example how making a slight mistake can lead one into a fall sense of confidence — unit tests may pass but the whole may not be telling y ou much: https://giphy.com/gifs/healthy-VldGsyiAmMW9a 😄

hooroomoo’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Needs work
hooroomoo’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Perfect! Time to land this one! 🥳🚀

Wim Leers’s picture

Note that this patch applies cleanly to all 3 branches — except for 9.3.x until #3261943: Confusing behavior after pressing "Apply changes to allowed tags" with invalid value gets cherry-picked onto 9.3.x. So intentionally not yet testing against 9.3.x.

Wim Leers’s picture

Priority: Normal » Major

Bumping to Major priority because this is a CKE5 stable blocker.

Wim Leers’s picture

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs reroll

Just a few nits that can be easily taken care of as part of the reroll.

  1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php
    @@ -442,4 +442,101 @@ public function testEmphasis() {
    +    $ordered_list_html = '<ol>
    +    <li>
    +        apple
    +    </li>
    +    <li>
    +        banana
    +    </li>
    +    <li>
    +        cantaloupe
    +    </li>
    +</ol>';
    

    Lets either get rid of the line breaks or make this HEREDOC. (No line breaks is fine, though, it's clear what is being added).

  2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5Test.php
    @@ -442,4 +442,101 @@ public function testEmphasis() {
    +    $numbered_list_dropdown = '.ck-splitbutton__arrow';
    

    NIT: This, and $reversed_order_button and $start_index_element should get renamed to make it clear they are selectors, not the actual element (such as what might be returned by $page->find())

hooroomoo’s picture

Status: Needs work » Reviewed & tested by the community

Self-RTBC'ing because super minor changes made

  • lauriii committed e935c43 on 10.0.x
    Issue #3261599 by hooroomoo, Wim Leers, bnjmnm: Use CKEditor 5's native...
lauriii’s picture

Committed e935c43 and pushed to 10.0.x. Thanks!

Leaving open for 9.4.x and 9.3.x backport.

  • bnjmnm committed 953a69b on 9.4.x
    Issue #3261599 by hooroomoo, Wim Leers, lauriii: Use CKEditor 5's native...

  • bnjmnm committed 4537221 on 9.3.x
    Issue #3261599 by hooroomoo, Wim Leers, lauriii: Use CKEditor 5's native...
bnjmnm’s picture

Version: 9.4.x-dev » 9.3.x-dev
Status: Reviewed & tested by the community » Fixed

Backported to 9.4.x + 9.3.x

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

Wim Leers’s picture