Problem/Motivation

Drupal 8/9 ship with CKEditor 4's StylesCombo plugin/feature: https://ckeditor.com/docs/ckeditor4/latest/features/styles.html

This shipped with the original CKEditor 4 issue: #1890502: WYSIWYG: Add CKEditor module to core. It provides a configuration UI:

When talking to the CKEditor developers:

We didn’t work on a replacement yet. We’re not even sure whether it’s needed and what’d be the use cases.
There’s a very old ticket: https://github.com/ckeditor/ckeditor5/issues/648 where we started some discussion. There’s also https://github.com/ckeditor/ckeditor5/issues/5700. But in general it’s unclear for us what would this feature be for.

What use cases do you see for it?

My response:

Well … how do we provide an automatic upgrade path in Drupal? 🤔

StylesCombo in CKE4 can only be mapped to heading.Heading in CKE5 as long as it’s only touching <h*> — but StylesCombo allowed much more. How do we automatically take any arbitrary StylesCombo configuration and make it work in CKE5?

It’s fine if it maps to multiple plugins but we do need to provide an upgrade path.

To which they responded:

Yeah, from the upgrade path perspective, there’s certainly a clear reason to do something on our side. What was blocking us so far is that we don’t want to create again a feature that duplicates a big part of the headings dropdown functionality. Thus, product-wise, it’s tricky.

Proposed resolution

Wait for https://github.com/ckeditor/ckeditor5/pull/11349 to ship with the April 6 release of CKEditor 5.

It shipped: #19. 👍

Remaining tasks

  • add @ckeditor/ckeditor5-style package
  • basic functionality
  • TEST: unit
  • TEST: kernel: SmartDefaultSettingsTest expectations should be updated
  • TEST: functional JS for plugin settings
  • TEST: functional JS for using the plugin
  • Upgrade path
  • Additional validation constraints to ensure config always is valid
  • TEST: kernel: new test cases in ValidatorsTest:
    Validator test coverage, which conveys just how precise validation is, which is extra important because as described in #46, the CKEditor 5 Style plugin itself does not do any validation:
    1. INVALID: Style plugin with no styles → when no styles are configured, having the toolbar item enabled is pointless (this is the only one already passing because it's the only one already implemented)
    2. INVALID: Style plugin configured to add class to unsupported tag → it does not make sense to allow adding a class to a tag that cannot actually be generated!
    3. INVALID: Style plugin configured to add class that is supported by a disabled plugin → it does not make sense to allow adding specific classes through a Style that can be supported by enabling a disabled plugin (for example "Alignment")
    4. INVALID: Style plugin configured to add class that is supported by an enabled plugin → it does not make sense to allow adding specific classes through a Style that are already supported by another plugin (for example "Alignment")
    5. INVALID: Style plugin has multiple styles with same label → without unique labels, it'd have terrible usability and accessibility
    6. INVALID: Style plugin has styles with invalid elements → not possible to configure this through the UI, but … it is possible if you modify the configuration directly.
    7. VALID: Style plugin has multiple styles with different labels → the sole example of a sane configuration, and hence the only not resulting in a validation error

Follow-up to discuss configuration validation restrictions rules between Styles and Source Editing: #3294908: Configuration overlaps between Styles and other CKE5 plugins

User interface changes

New plugin settings form.

API changes

None.

Data model changes

None.

Release notes

The CKEditor 5 successor of CKEditor 4's StylesCombo functionality has been added. This was the last remaining widely adopted functionality of CKEditor 4 that did not yet have an equivalent! When upgrading from CKEditor 4 editor, the configuration will be migrated automatically.

CommentFileSizeAuthor
#119 3222797-style-101.patch158.36 KBbnjmnm
#119 3222797-style--94x.patch158.63 KBbnjmnm
#119 3222797-style--95x.patch158.41 KBbnjmnm
#115 core-style-3222797-115.patch143.21 KBnod_
#102 Schermafbeelding 2022-07-01 om 11.42.47.png50.88 KBmpp
#102 Schermafbeelding 2022-07-01 om 11.43.16.png36.52 KBmpp
#101 Schermafbeelding 2022-07-01 om 11.14.17.png21.8 KBmpp
#100 Schermafbeelding 2022-07-01 om 10.50.52.png46.58 KBmpp
#94 style-3222797-93-10.patch138.77 KBWim Leers
#94 interdiff-10.0.txt1.94 KBWim Leers
#93 style-3222797-93.patch142.22 KBWim Leers
#52 Screenshot from 2022-05-07 11-15-25.png25.01 KBlarowlan
#52 Screenshot from 2022-05-07 11-05-22.png108.81 KBlarowlan
#52 Screenshot from 2022-05-07 11-04-53.png87.12 KBlarowlan
#52 Screenshot from 2022-05-07 11-03-36.png288.88 KBlarowlan
#52 Screenshot from 2022-05-07 11-03-08.png78.74 KBlarowlan
#52 Screenshot from 2022-05-07 11-02-59.png193.58 KBlarowlan
#47 Screenshot from 2022-05-06 19-59-24.png4.09 KBlarowlan
#47 Screenshot from 2022-05-06 19-59-09.png14.27 KBlarowlan
#47 Screenshot from 2022-05-06 19-58-36.png9.63 KBlarowlan
#47 Screenshot from 2022-05-06 19-58-02.png6.34 KBlarowlan
#47 Screenshot from 2022-05-06 19-57-44.png14.55 KBlarowlan
#47 Screenshot from 2022-05-06 19-57-01.png10.03 KBlarowlan
#47 Screenshot from 2022-05-06 19-56-40.png146.81 KBlarowlan
#21 Screenshot 2022-04-14 at 14.00.02.png28.68 KBWim Leers
#21 Screenshot 2022-04-14 at 13.58.42.png122.81 KBWim Leers
#17 image-20220406-123908.png92.03 KBmpp
#16 Screenshot 2022-02-24 at 15.42.09.png235.06 KBWim Leers
#6 the-best-ux.png232.94 KBLuke.Leber
#6 a-better-ux.png29.15 KBLuke.Leber
#6 holy-ux-nightmare-batman.png147.5 KBLuke.Leber

Issue fork drupal-3222797

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

Wim Leers’s picture

Status: Active » Postponed
Issue tags: +needs upstream feature

@reinmar said we'll discuss it next week; he'll gather more information as it’s a long time since they discussed this.

Wim Leers’s picture

Wim Leers’s picture

Title: Upgrade path from CKEditor 4's StylesCombo » [needs upstream feature] Upgrade path from CKEditor 4's StylesCombo
Priority: Normal » Major

Discussed during today's meeting.

@Reinmar said that other than the upgrade path the StylesCombo plugin would be completely useless in CKE5. In CKE4 we had both “Format” (now “Heading”) and “StylesCombo”, with lots of overlap. It was always stupid to configure CKE to have both.

The fact that Drupal limits it to just classes makes an upgrade path more feasible. Needs more thought/investigation.

Today people use CKE5’s “Heading” dropdown as an alternative to CKE4’s StylesCombo, but that was never the intention.


Since we are being told we should not use the Heading dropdown as a partial successor and the proper solution is TBD, explicitly marking this as needing an upstream feature.

Luke.Leber’s picture

Following up from a slack conversation with @bnjmnm. I'm in agreement with Reinmar in that the styles combo plugin has always felt like an afterthought. I feel that there are some things wrong with it.

  1. There's a confusion of responsibility. It's possible to change the semantic element type through something that's clearly labeled as a Style. This blends semantics with presentation.

    Headers are especially troublesome in this area because sequential ordering is very important. This important accessibility topic is sometimes in direct opposition to design requirements. Who knows what the correct heading level will be for any given spot where a WYSIWYG might be used? Not only is it currently difficult to reconcile the two needs, but it also leads to my second point.

  2. It's very hard to implement flexible block level elements for an accessible design system without introducing internal SPAN elements. Take the following image for example.

    Image depicting dozens of block level style options

    This is obviously an exaggeration that we've been able to work around by limiting styles (which sometimes also limits design options...). Conversely, editors like TinyMCE can effectively gray out or even remove style options that don't make sense given the current context. IE, if one is editing an H2 element, then styles applicable to blockquote elements wouldn't be selectable. Wouldn't it be better if the user was only presented with the style options that make sense given what they've currently selected?

    An image depicting a better user experience with irrelevant styles disabled

Would it be possible to tap into contextual bubble toolbars to provide users with a more streamlined and easy to use interface? We can already see a precedence for this in the CKEditor5 image plugin. I feel that this could be the best UX for styling block level elements with CKEditor5. Only showing users the options that make sense, when they make sense.

Depiction of the CKEditor5 image plugin contextual bubble toolbar

I'd love to discuss this further, so feel free to reach out with any questions or comments.

Thanks!

Wim Leers’s picture

@Luke.Leber: wow! Thanks for that thoughtful write-up! I've just pointed @Reinmar to it :)

Wim Leers’s picture

FYI: Development for the CKEditor 5 successor will start in ~2 months at the earliest.

Wim Leers’s picture

Per #3231364-57: Add CKEditor 5 module to Drupal core: work on this will begin soon upstream! 🥳

Wim Leers’s picture

Project: CKEditor 5 » Drupal core
Version: 1.0.x-dev » 9.3.x-dev
Component: Code » ckeditor5.module
Issue tags: +Needs tests
Wim Leers’s picture

Title: [needs upstream feature] Upgrade path from CKEditor 4's StylesCombo » [upstream] Upgrade path from CKEditor 4's StylesCombo

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

esch’s picture

If the StylesCombo plugin is not going to be migrated and/or there is no upgrade path, how will editors add CSS class to HTML tags?

Are we now expecting editors to go into source view and fat-finger in CSS class on the correct HTML tags?

Is the expectation to have the Drupal community contribute a CK5 plugin for this, even though it is a default on the current CK4?

I would just like to know how CSS classes will be managed by editors when this is a part of Drupal 10. Will we need to train our editors how to use source view to add classes in the proper areas/tags?

Any insights would be much appreciated as I'm trying to figure out we are going to handle this major editor experience change.

Cheers.

Wim Leers’s picture

If the StylesCombo plugin is not going to be migrated and/or there is no upgrade path, how will editors add CSS class to HTML tags?

  1. It will be.
  2. Work-around for now: use the "Source Editing" plugin to manually add classes.

Are we now expecting editors to go into source view and fat-finger in CSS class on the correct HTML tags?

We're not expecting that. See above.

Is the expectation to have the Drupal community contribute a CK5 plugin for this, even though it is a default on the current CK4?

No.

I would just like to know how CSS classes will be managed by editors when this is a part of Drupal 10. Will we need to train our editors how to use source view to add classes in the proper areas/tags?

Definitely not! 🙈 We absolutely do not want that 😱

Any insights would be much appreciated as I'm trying to figure out we are going to handle this major editor experience change.

It's not a change — it's just not yet available.

I'll answer this very explicitly and clearly: Without this feature available, we will not be able to mark the CKEditor 5 module stable. (That's what the stable blocker tag means, but I totally understand that just the presence of that tag does not quite make you feel comfortable. Hopefully that phrasing that explains the meaning of the tag puts your mind at ease!)


We're meeting with the CKEditor 5 team later today.

esch’s picture

Cheers @wim-leers,

Thanks for explaining the current status of this issue.

Wim Leers’s picture

Issue summary: View changes
FileSize
235.06 KB

I realize I forgot to cross-post to this issue what I posted to the #ckeditor5 Drupal Slack channel on February 28:

🥳🥳🥳 StylesCombo has epic progress, see attached screenshot from a live demo @lauriii and I got! See the attached screenshot 🤩 This is what we talked about the majority of the meeting. They were looking for more feedback, especially with regards to which styles to show (all or the eligible choices based on the current caret location), how to discover through the UI all available styles. They’re still dealing with complex edge cases (i.e. when caret is in a paragraph in a table cell, it should also show table styles). But they’re well on their way and were mostly just looking for confirmation 👍🚀

The upstream WIP can be seen at https://github.com/ckeditor/ckeditor5/pull/11349 😊

Best news yet: it's going to ship in the upcoming release of CKEditor 5, on April 6: https://github.com/ckeditor/ckeditor5/issues/11387!

mpp’s picture

FileSize
92.03 KB

A use case we have also supports translations for the styles:
CKeditor4

Wim Leers’s picture

#17: that should be just a matter of Configuration Translation, just like for the CKEditor 4 StylesCombo plugin.

Wim Leers’s picture

Visible in the config UI now:

And also working in CKEditor 5 itself:

Next up: making this configurable. After that: upgrade path.

Wim Leers’s picture

Now configurable using the exact same syntax/config UX as for CKEditor 4. But we're storing things in a much clearer way, with better validation:

h2.title.dsf|dsf
p.foo|Foo

gets stored as

    ckeditor5_style:
      styles:
        -
          label: dsf
          element: '<h2 class="title dsf">'
        -
          label: Foo
          element: '<p class="foo">'

→ this reuses the ckeditor5.element config schema type's validation constraints, makes the config file easier to read/interpret using git and makes the getElementsSubset() implementation trivial. While simultaneously not reinventing the UX: that is kept identical to the CKEditor 4 UX. We don't want to start that bikeshed.

⚠️ But I expect this MR to fail tests now, due to:

        // The default case: all other plugins that implement this interface are
        // explicitly checked for compliance: only subsets are allowed. This is
        // essential for \Drupal\ckeditor5\SmartDefaultSettings to be able to
        // work: otherwise it would not be able to know which plugins to enable.
        elseif (isset($editor)) {
          $subset = $this->getPlugin($id, $editor)->getElementsSubset();
          $subset_violations = array_diff($subset, $defined_elements);
          if (!empty($subset_violations)) {
            throw new \LogicException(sprintf('The "%s" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "%s".', $id, implode(' ', $subset_violations)));
          }
          $defined_elements = $subset;
        }

Why? Because turns out that SourceEditing isn't the only special case, Style also is special, because it too does not know which superset of elements it will generate. So we'll need to special case this one too. I think this is reasonable. (And if we discovered a need for more "supersets" in the future in contrib, we can add the necessary API capabilities for it.)

Wim Leers’s picture

⚠️ But I expect this MR to fail tests now, due to:

This is not yet happening … because there's no test yet that configures the Style plugin to be enabled 😅 Hence the Needs tests tag on the issue!

Wim Leers’s picture

  • Unit test added, passes tests 👍
  • Upgrade path added, fails one test, with this exception: LogicException: The "ckeditor5_style" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<p class="callout"> <blockquote class="interesting highlighted"> <blockquote class="famous">". → this is due to the same reason as I mentioned in #22.
  • Functional JS test added for testing the settings form → not yet passing, due to various reasons.

Tests still needed: new test cases in ValidatorsTest (for the validators that still need to be written) and functional JS test (to verify that it works as expected).

Wim Leers’s picture

The functional JS test for testing the settings form fails on

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
RuntimeException: Unable to complete AJAX request.

/var/www/html/core/tests/Drupal/FunctionalJavascriptTests/JSWebAssert.php:54
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:29
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

The root cause:

ckeditor5.plugin.ckeditor5_style:
…
  mapping:
    styles:
…
      constraints:
        NotBlank:
          message: "Enable at least one style, otherwise disable the Style plugin."

→ initially, there are no styles configured, and that's why this validation error is triggered even before showing the plugin settings form for the very first time. We need to make the infrastructure a bit smarter.

Just fixed that in 419df3ae964de9151ba20a9acadcc0d1effff84e. Now it should fail not on line 29 in StylesTest, but later.

Wim Leers’s picture

I wrote:

Now it should fail not on line 29 in StylesTest, but later.

And the failure confirms it:

There was 1 error:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
Behat\Mink\Exception\ElementNotFoundException: Link with id|title|alt|text "Style" not found.

/var/www/html/vendor/behat/mink/src/Element/TraversableElement.php:70
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:49

From line 29 to line 49! 👍

But I'm not yet satisfied. I want to add explicit test coverage for the absence of validation errors upon enabling the Style plugin.

Did that in 992fd87505ce3b9f7cf7360de0ef16c2212e7af3.

Wim Leers’s picture

992fd87505ce3b9f7cf7360de0ef16c2212e7af3's test result:

There was 1 error:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
Behat\Mink\Exception\ResponseTextException: The text "has been updated" was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:785
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:898
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:49

Still at line 49, but now with a more explicit assertion. 👍

Let's harden test coverage further, because there's another (related) edge case to be fixed. Just pushed 8a12470d2caa9d2ee7bd8ba51caf0676be7ed7cb, which should cause the failure to happen earlier than line 49.

Wim Leers’s picture

That worked:

There was 1 failure:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
Failed asserting that two strings are identical.
--- Expected
+++ Actual
@@ @@
-''
+'Enable at least one style, otherwise disable the Style plugin.'

/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:121
/var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/IsIdentical.php:90
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5TestBase.php:147
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:37

Pushed the fix for that edge case in 65521a476c80b642a5b352235404d2714206460c.

Wim Leers’s picture

So the functional JS test for the settings form now fails like so:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
Behat\Mink\Exception\ResponseTextException: The text "has been updated" was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:785
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/tests/Drupal/Tests/WebAssert.php:898
/var/www/html/core/modules/ckeditor5/tests/src/FunctionalJavascript/StyleTest.php:54

Because:

LogicException: The "ckeditor5_style" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<p class="foo bar">". in Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getProvidedElements() (line 338 of core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php).

👍 → also failing on the problem described in #22. Working to fix that now.

I pushed 62760b9a9856834621444962fe202fdb21f11a55 because the "has been updated" expectation was wrong too: the test is creating a new text format, not updating one 😅

mpp’s picture

Thanks and 🤞 :-)

Wim Leers’s picture

After careful consideration, I realized that using the "global attribute" infrastructure introduced by #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) earlier today is not appropriate for this use case.

I was first thinking that ckeditor5_style should claim to support <* class>, and it'd then return a subset such as ['<p class="fancy">', '<blockquote class="famous">].

… but that does not quite make sense, because the semantics of <* attr> mean that it is not a superset of <p attr>. HTMLRestrictions specifically treats <*> as a "concrete tag", so the union of <* attr> and <p attr> is not ['<* attr>'] but ['<* attr>', '<p attr>']. In other words: the semantics that we've just carefully established in #3231334 mean that we cannot overload it for this purpose too.

Fortunately we already have the necessary infrastructure in place: wildcard tags! We just need to introduce a new wildcard tag. That's what b2092eabdd07f5fab26610a490a58e8785e1c4cf just did.

I have more commits locally that I need to clean up/untangle — expect more progress tomorrow :) I'm pretty confident that I'm on a path that will lead to a fully green patch/MR in the next few days.

catch’s picture

Title: [upstream] Upgrade path from CKEditor 4's StylesCombo » Upgrade path from CKEditor 4's StylesCombo

This seems no longer blocked by upstream now? Tentatively removing the tag.

Wim Leers’s picture

Title: Upgrade path from CKEditor 4's StylesCombo » Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style
Issue summary: View changes

Correct!

Wim Leers’s picture

Title: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style » [PP-1] Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style
Related issues: +#3278394: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result

One of the remaining failures (Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "cke4_plugins_with_settings can be switched to CKEditor 5 without problems, settings are upgraded too") is due to a bug in \Drupal\ckeditor5\HTMLRestrictions::diff(): #3278394: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result.

Wim Leers’s picture

Issue summary: View changes

Clarify remaining tasks. Especially for the Needs tests issue tag.

Wim Leers’s picture

#3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) conflicted with this heavily. Merged upstream changes and resolved conflicts, hope I got it right 🤞

Wim Leers’s picture

Those last 2 failures (DrupalCI miscounts them as 4 failures) are only solveable if I apply the patch from #3278394. So … did that.

Wim Leers’s picture

PROGRESS! 🥳

Committing the #3278394-7: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result patch allowed us to go from:

  • 4bcb76e2: 4 failures (2 real ones: one in SmartDefaultSettingsTest and one in StyleTest, both counted double)
  • to
  • 6a892ebf: 2 failures (both real: one in StyleTest, one in ValidatorsTest)

The end is close now 🤞

b1816e84 should fix the StyleTest failure.

Wim Leers’s picture

Last failure: 1) Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::testPair with data set "INVALID Source Editable tag already provided by plugin and another available in a not enabled plugin"
with the following failure:

-    'settings.plugins.ckeditor5_sourceEditing.allowed_tags.6' => 'The following attribute(s) are already supported by available plugins and should not be added to the Source Editing "Manually editable HTML tags" field. Instead, enable the following plugins to support these attributes: Alignment (&lt;h2 class=&quot;text-align-center&quot;&gt;), Align center (&lt;h2 class=&quot;text-align-center&quot;&gt;).'
+    'settings.plugins.ckeditor5_sourceEditing.allowed_tags.6' => 'The following attribute(s) are already supported by available plugins and should not be added to the Source Editing "Manually editable HTML tags" field. Instead, enable the following plugins to support these attributes: Style (&lt;h2 class=&quot;text-align-center&quot;&gt;), Alignment (&lt;h2 class=&quot;text-align-center&quot;&gt;), Align center (&lt;h2 class=&quot;text-align-center&quot;&gt;).'

Note that there is one addition there: besides Alignment and Align center being suggested as a solution to allowing <h2 class="text-align-center">, a third suggestion has appeared now: Style. That's because the Style plugin this MR is adding claims it supports <$any-html5-element class> — which is obviously a superset of <h2 class="text-align-center">. So … that's actually normal.

Of course, Alignment is clearly the better/more specific fit for supporting that particular markup. To improve the message that validator provides, we need to optimize the suggestions it makes. And for that, we already have an issue: #3271179: Update SourceEditingRedundantTagsConstraintValidator to reuse SmartDefaultSettings 👍

I don't think it makes sense to block this stable-blocking issue on that detail. So, I'm updating the expected validation message here and adding a @todo. 👍

Pushed that in 1dc5a2b9c2102c5b6ebd305ce23cf6161d344822.

Wim Leers’s picture

Wim Leers’s picture

Green! Time to add the remaining test coverage + validators.

Wim Leers’s picture

Issue summary: View changes

Validator test coverage:

  1. INVALID: Style plugin with no styles → when no styles are configured, having the toolbar item enabled is pointless (this is the only one already passing because it's the only one already implemented)
  2. INVALID: Style plugin configured to add class to unsupported tag → it does not make sense to allow adding a class to a tag that cannot actually be generated!
  3. INVALID: Style plugin has multiple styles with same label → without unique labels, it'd have terrible usability and accessibility
  4. INVALID: Style plugin has styles with invalid elements → not possible to configure this through the UI, but … it is possible if you modify the configuration directly.
  5. VALID: Style plugin has multiple styles with different labels → the sole example of a sane configuration, and hence the only not resulting in a validation error

Pushed in 397b5af76079eb17a71010a5260c05f4247eb314.

Wim Leers’s picture

Yay, #42 predicted 3 failures (for test cases 2, 3 and 4) and this also happened:

Testing Drupal\Tests\ckeditor5\Kernel\ValidatorsTest
...............FFF...................                             37 / 37 (100%)

Time: 00:25.446, Memory: 4.00 MB

There were 3 failures:

Time to implement validators! 🤓

Pushed ee4d1838c5148885bb19a81cfd36976aaa2811ff for that.

Wim Leers’s picture

Issue summary: View changes

Yay, ValidatorsTest is all green now!

But looks like SmartDefaultSettingsTest started failing due to the stricter validation. 🤓 Good! Pushed a minor fix to the CKE4 configuration.

Now the only remaining task is: TEST: functional JS for using the plugin.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs tests

Just pushed the functional JS test coverage!

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Just met with the CKEditor 5 team. Discussed this too.

Bugs

They confirmed two bugs I found, and one has already been fixed a few hours ago! 🥳😄

Those now all have explicit test coverage, with the failing pieces commented out with @todos to uncomment them.

Validation

The ckeditor5-style plugin does not do any validation right now. This confirms the importance of doing validation on our side. All aspects we're validating so far make sense. 👍

They suggested one more: ensuring that you do not configure the Style plugin to add classes that other plugins already provide — for example ckeditor5_alignment's ability to specify class=”text-align-(left|center|right|justify)”. Will add that tomorrow. Meanwhile, this is definitely ready for review, so unassigning :)

larowlan’s picture

Manual testing of the happy path

  1. installed umami
  2. enabled ckeditor5
  3. switched the basic html format to use ckeditor5
  4. added the style plugin
  5. added a.read-more__link|Read more link to the styles list
  6. verified it updated the allowed HTML accordingly
  7. saved the text format
  8. edited some content and added a link
  9. selected the read more link style
  10. saved the node
  11. verified it output with the correct class

Looking 🔥

Special mention to https://github.com/dpi/dogit which made it easy to checkout the MR locally dogit issue:mr 3222797 and done 😎

Wim Leers’s picture

#47: Does that mean … you found everything works as you expected on the first try, on an existing site? 🤓🤞🤞🤞

larowlan’s picture

New site, will try an existing site at the weekend

Wim Leers’s picture

#49: great, thank you!


Quoting #46:

They suggested one more: ensuring that you do not configure the Style plugin to add classes that other plugins already provide — for example ckeditor5_alignment's ability to specify class=”text-align-(left|center|right|justify)”. Will add that tomorrow. Meanwhile, this is definitely ready for review, so unassigning :)

Just pushed that in f1ffbf677d8d3277a1cfb6fe0da81ad7f9b85d02. 👍

Wim Leers’s picture

Issue summary: View changes

Updating issue summary to show the full set of validation constraints and why (since #42 no longer provides a complete overview).

larowlan’s picture

Manual testing on an existing site with a lot of styles

* Applied patch
* Attempted to update to ckeditor 5, received warning about 'non html' filter (we have a token style custom filter that is \Drupal\filter\Plugin\FilterInterface::TYPE_MARKUP_LANGUAGE (unrelated, but we can probably make it HTML)
* Turned that off and tried again
* Update looked to add as many items as it could, some items didn't have CKEditor 5 equivalents (we need to port some custom ones, and I guess we need a version of Embed module that is compatible with CKEditor 5) - I couldn't find an issue in its queue for a CKEditor 5 version, but I think that's likely to be a major blocker for many folks updating

* All the styles looked to convert correctly, and the allowed HTML list looked good too, but when I saved I received an exception (see screenshot for trace)

The website encountered an unexpected error. Please try again later.

InvalidArgumentException: The "p" HTML tag has attribute restrictions, but it is not an array of key-value pairs, with HTML tag attribute names as keys. in Drupal\ckeditor5\HTMLRestrictions::validateAllowedRestrictionsPhase3() (line 190 of core/modules/ckeditor5/src/HTMLRestrictions.php).

* Also attached screenshot of extensive list of styles and allowed HTML

* Here's the shape of html_restrictions when that fails

The [1 => FALSE] obviously being the issue

The full list of allowed tags:
<br> <p class="time icon--calendar-white icon-text icon--location icon--location-white icon--phone icon--phone-white icon--mail icon--rsvp icon--clock icon--clock-white icon--download icon--link icon--external-link icon--info icon--faq icon--arrow-blue-right icon--inline icon icon--arrow-blue-left box box--float-right box--float-left box--blue box--black box--bordered clear-float text-align-left text-align-center text-align-right text-align-justify"> <h2 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h3 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h4 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h5 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h6 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <a class="button button--red button__nav button--prev button--next button--arrow-circle-blue" hreflang title accesskey id rel target name href> <table class="js-table--responsive"> <td class="table--cell-width-s table--cell-width-m table--cell-width-l table--cell-width-xl table--cell-width-xll" rowspan colspan> <div class="clear-float text-align-left text-align-center text-align-right text-align-justify" id> <ul class="listing-links" type> <cite> <dl> <dt> <dd> <blockquote cite> <ol type start> <li class> <span class lang dir> <img style src alt data-entity-type data-entity-uuid data-align data-caption width height> <drupal-entity data-caption data-entity-type data-entity-uuid data-entity-id data-view-mode data-entity-embed-display data-entity-embed-settings data-entity-embed-display-settings data-align data-embed-button data-entity-label alt title class> <section id class> <strong> <em> <code class="language-*"> <pre class="text-align-left text-align-center text-align-right text-align-justify"> <hr> <tr> <th rowspan colspan> <thead> <tbody> <tfoot> <caption>

The issue is coming from this line

elseif (self::intersectionWithClasses($style_element, $other_enabled_plugin_elements)) {

line 64 in StyleSensibleElementConstraintValidator

Here's a var_export of the two items its trying to merge

Drupal\ckeditor5\HTMLRestrictions::__set_state(array(
   'elements' => 
  array (
    'p' => 
    array (
      'class' => 
      array (
        'time' => true,
      ),
    ),
    'br' => false,
    'h2' => false,
    'h3' => false,
    'h4' => false,
    'h5' => false,
    'h6' => false,
    'cite' => false,
    'dl' => false,
    'dt' => false,
    'dd' => false,
    'blockquote' => false,
    'ol' => false,
    'li' => false,
    'span' => false,
    'img' => false,
    'drupal-entity' => false,
    'section' => false,
    'a' => false,
    'div' => false,
    'ul' => false,
    'strong' => false,
    'em' => false,
    'code' => false,
    'pre' => false,
    'hr' => false,
    'table' => false,
    'tr' => false,
    'td' => false,
    'th' => false,
    'thead' => false,
    'tbody' => false,
    'tfoot' => false,
    'caption' => false,
  ),
   'unrestricted' => false,
))

And

Drupal\ckeditor5\HTMLRestrictions::__set_state(array(
   'elements' => 
  array (
    'br' => false,
    'p' => 
    array (
      'class' => 
      array (
        'time' => true,
        'icon--calendar-white' => true,
        'icon-text' => true,
        'icon--location' => true,
        'icon--location-white' => true,
        'icon--phone' => true,
        'icon--phone-white' => true,
        'icon--mail' => true,
        'icon--rsvp' => true,
        'icon--clock' => true,
        'icon--clock-white' => true,
        'icon--download' => true,
        'icon--link' => true,
        'icon--external-link' => true,
        'icon--info' => true,
        'icon--faq' => true,
        'icon--arrow-blue-right' => true,
        'icon--inline' => true,
        'icon' => true,
        'icon--arrow-blue-left' => true,
        'box' => true,
        'box--float-right' => true,
        'box--float-left' => true,
        'box--blue' => true,
        'box--black' => true,
        'box--bordered' => true,
        'clear-float' => true,
      ),
    ),
    'h2' => 
    array (
      'id' => true,
      'class' => 
      array (
        'block-title' => true,
        'clear-float' => true,
      ),
    ),
    'h3' => 
    array (
      'id' => true,
      'class' => 
      array (
        'block-title' => true,
        'clear-float' => true,
      ),
    ),
    'h4' => 
    array (
      'id' => true,
      'class' => 
      array (
        'block-title' => true,
        'clear-float' => true,
      ),
    ),
    'h5' => 
    array (
      'id' => true,
      'class' => 
      array (
        'block-title' => true,
        'clear-float' => true,
      ),
    ),
    'h6' => 
    array (
      'id' => true,
      'class' => 
      array (
        'block-title' => true,
        'clear-float' => true,
      ),
    ),
    '*' => 
    array (
      'dir' => 
      array (
        'ltr' => true,
        'rtl' => true,
      ),
      'lang' => true,
    ),
    'cite' => false,
    'dl' => false,
    'dt' => false,
    'dd' => false,
    'blockquote' => 
    array (
      'cite' => true,
    ),
    'ol' => 
    array (
      'type' => true,
      'start' => true,
    ),
    'li' => 
    array (
      'class' => true,
    ),
    'span' => 
    array (
      'class' => true,
      'lang' => true,
      'dir' => true,
    ),
    'img' => 
    array (
      'style' => true,
      'src' => true,
      'alt' => true,
      'data-entity-type' => true,
      'data-entity-uuid' => true,
      'data-align' => true,
      'data-caption' => true,
      'width' => true,
      'height' => true,
    ),
    'drupal-entity' => 
    array (
      'data-caption' => true,
      'data-entity-type' => true,
      'data-entity-uuid' => true,
      'data-entity-id' => true,
      'data-view-mode' => true,
      'data-entity-embed-display' => true,
      'data-entity-embed-settings' => true,
      'data-entity-embed-display-settings' => true,
      'data-align' => true,
      'data-embed-button' => true,
      'data-entity-label' => true,
      'alt' => true,
      'title' => true,
      'class' => true,
    ),
    'section' => 
    array (
      'id' => true,
      'class' => true,
    ),
    'a' => 
    array (
      'hreflang' => true,
      'title' => true,
      'accesskey' => true,
      'id' => true,
      'rel' => true,
      'target' => true,
      'name' => true,
      'href' => true,
    ),
    'div' => 
    array (
      'id' => true,
      'class' => 
      array (
        'clear-float' => true,
      ),
    ),
    'ul' => 
    array (
      'type' => true,
    ),
    'strong' => false,
    'em' => false,
    'code' => 
    array (
      'class' => 
      array (
        'language-*' => true,
      ),
    ),
    'pre' => false,
    'hr' => false,
    '$text-container' => 
    array (
      'class' => 
      array (
        'text-align-left' => true,
        'text-align-center' => true,
        'text-align-right' => true,
        'text-align-justify' => true,
      ),
    ),
    'table' => false,
    'tr' => false,
    'td' => 
    array (
      'rowspan' => true,
      'colspan' => true,
    ),
    'th' => 
    array (
      'rowspan' => true,
      'colspan' => true,
    ),
    'thead' => false,
    'tbody' => false,
    'tfoot' => false,
    'caption' => false,
  ),
   'unrestricted' => false,
))
Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

Perfect, thank you for the superb and detailed feedback!

On Monday, I will dig into this. I think the crash you’re running into is a bug we’ve seen once before, but the issue was not considered important … but this will likely change that 👍

The overwhelming messages are already being worked on.

Expect a solid update on Monday!

Wim Leers’s picture

Title: [PP-1] Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style » Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style

#52:

#3278394: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result landed, so removed the [PP-1] prefix.

Next:

  • Remove the commit that added #3278394's code to this MR already. Then it should fail.
  • Merge HEAD, then it should be green again.
  • Vertical tabs summary! (While working on #52, I noticed that that was lost when going from CKE4 to 5's admin UI.)
Wim Leers’s picture

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review

I wrote this in #54:

Remove the commit that added #3278394's code to this MR already. Then it should fail.

I did that in https://git.drupalcode.org/project/drupal/-/merge_requests/2118/diffs?co..., but it passed.

WTF.

Looks like this is yet another GitLab merge request + DrupalCI weirdness thing. Looks like it automatically pulled and rebased instead of testing that exact commit. 🤪🤷‍♂️

Because here:

git reset --hard f5408517

very clearly yields multiple failures, including this fast-to-reproduce one:

/usr/local/bin/php /private/var/folders/4l/5sndwsz50tqgxjh2525n10br0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml --filter Drupal\\Tests\\ckeditor5\\Kernel\\SmartDefaultSettingsTest --test-suffix SmartDefaultSettingsTest.php /Users/wim.leers/Work/d8/core/modules/ckeditor5/tests/src/Kernel
Testing started at 14:40 ...
#!/usr/bin/env php
PHPUnit 8.5.26 #StandWithUkraine

Testing /Users/wim.leers/Work/d8/core/modules/ckeditor5/tests/src/Kernel
.............E..                                                  16 / 16 (100%)

Time: 57.24 seconds, Memory: 8.00 MB

There was 1 error:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "cke4_plugins_with_settings can be switched to CKEditor 5 without problems, settings are upgraded too" ('cke4_plugins_with_settings', array(), array(array(array('textPartLanguage', 'style', 'blockQuote')), array(array('all'), array(array(array('Callout', '<p class="callout">'), array('Interesting & highlighted quote', '<blockquote class="interestin...hted">'), array('Famous', '<blockquote class="famous">'))))), '', array(), array(array('The CKEditor 4 button <em cla...ality.', 'The <em class="placeholder">l... path.')))
LogicException: The "ckeditor5_style" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<p class="callout"> <blockquote class="famous">".

<snip>

ERRORS!
Tests: 16, Assertions: 163, Errors: 1.

Anyway … pushed the merge commit and the vertical tabs summary support (including test coverage) too.

Wim Leers’s picture

Addressed all of @larowlan's feedback at the start of this week, just finished addressing all of @lauriii's feedback! 👍

larowlan’s picture

@Wim Leers, I've DM'd you some YAML files on slack to help diagnose the issue after applying #3279470: HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state
Failing that I could do an early EU meeting on Wednesday or Thursday to step through it

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Wim Leers’s picture

Version: 9.5.x-dev » 9.4.x-dev
Status: Needs review » Needs work

Picking this up again. Keeping this against 9.4.x because A) it needs to land there too, B) keeps the merge request simple.

Forwarding the merge request to the latest and greatest origin/9.4.x now 🤓

Wim Leers’s picture

Wim Leers’s picture

Yay, testStyleFunctionality() is failing because it was asserting the previously broken state, #3277438: Update to CKEditor 5 v34.1.0 brought in the upstream fix at https://github.com/ckeditor/ckeditor5/issues/11576! 😄

The other failures are due to the upstream changes that have been merged in. Should be green again now 🤞

Wim Leers’s picture

@larowlan I'm 90% confident that #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers by @nod_ which landed earlier today will have solved all your "merging troubles" 🤓🥳 Haven't checked yet with your YAML files (thanks for sending them to me! I haven't gotten to this in a while because I was helping with getting 9.4.0-beta1 stuff) yet, but I'm pretty confident I won't need them anymore 😄 I will test this tomorrow morning (it's been a very long day), unless you get to re-test this before I can 🤞

Wim Leers’s picture

Wim Leers’s picture

Title: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style » [PP-1] Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style
Assigned: Wim Leers » Unassigned
Status: Needs work » Postponed
Related issues: +#3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path

Per https://git.drupalcode.org/project/drupal/-/merge_requests/2118/diffs#no..., this is now blocked on #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path — because I found a bug thanks to @lauriii's question 🤓

@lauriii, please review that thread on the merge request — I'm now less convinced of the value of this new HTMLRestrictions::notInResolvedSuperset() utility method 🤔 Curious about your thoughts.

Wim Leers’s picture

Wim Leers’s picture

Title: [PP-1] Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style » Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style
Assigned: Unassigned » Wim Leers
Status: Postponed » Needs work
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review

All ready for final review now! 🥳

Wim Leers’s picture

Issue summary: View changes
Issue tags: +9.4.0 release highlights
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

MR looks good! Great job @Wim Leers 👏

Luke.Leber’s picture

Hmm...this is what I get when trying to enable cke5 on our main site on 9.4.x-dev and applying the latest from https://git.drupalcode.org/project/drupal/-/merge_requests/2118.diff:

The website encountered an unexpected error. Please try again later.

OutOfBoundsException: in Drupal\ckeditor5\Plugin\Validation\Constraint\StyleSensibleElementConstraintValidator->findStyleConflictingPluginLabel() (line 159 of core/modules/ckeditor5/src/Plugin/Validation/Constraint/StyleSensibleElementConstraintValidator.php).
Drupal\ckeditor5\Plugin\Validation\Constraint\StyleSensibleElementConstraintValidator->validate() (Line: 196)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateConstraints() (Line: 148)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 158)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validateNode() (Line: 100)
Drupal\Core\TypedData\Validation\RecursiveContextualValidator->validate() (Line: 93)
Drupal\Core\TypedData\Validation\RecursiveValidator->validate() (Line: 132)
Drupal\Core\TypedData\TypedData->validate() (Line: 224)
Drupal\ckeditor5\Plugin\Editor\CKEditor5::validatePair() (Line: 667)
Drupal\ckeditor5\Plugin\Editor\CKEditor5->validateConfigurationForm() (Line: 223)
editor_form_filter_admin_format_validate()
call_user_func_array() (Line: 82)
Drupal\Core\Form\FormValidator->executeValidateHandlers() (Line: 275)
Drupal\Core\Form\FormValidator->doValidateForm() (Line: 118)
Drupal\Core\Form\FormValidator->validateForm() (Line: 588)
Drupal\Core\Form\FormBuilder->processForm() (Line: 320)
Drupal\Core\Form\FormBuilder->buildForm() (Line: 73)
Drupal\Core\Controller\FormController->getContentResult() (Line: 39)
Drupal\layout_builder\Controller\LayoutBuilderHtmlEntityFormController->getContentResult()
call_user_func_array() (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 564)
Drupal\Core\Render\Renderer->executeInRenderContext() (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 158)
Symfony\Component\HttpKernel\HttpKernel->handleRaw() (Line: 80)
Symfony\Component\HttpKernel\HttpKernel->handle() (Line: 58)
Drupal\Core\StackMiddleware\Session->handle() (Line: 48)
Drupal\Core\StackMiddleware\KernelPreHandle->handle() (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass() (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle() (Line: 49)
Asm89\Stack\Cors->handle() (Line: 48)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() (Line: 51)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() (Line: 23)
Stack\StackedHttpKernel->handle() (Line: 702)
Drupal\Core\DrupalKernel->handle() (Line: 19)

Here are the list of styles:

h2.heading.heading--medium|Heading 2 Medium
h2.heading.heading--small|Heading 2 Small
h3.heading.heading--small|Heading 3 Small
span.h2|H2 Table Caption
span.h4|H4 Table Caption
span.visually-hidden|Visually hidden
a.button|Button
a.button--pdf|PDF Button
span.deadline|Application Deadline
p.footnote|Table Footnote
ul.column.column--two|UL Two Column
ol.column.column--two|OL Two Column
a.read-more|Read more link

Please let me know if you need more information :)

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I'll investigate in a few hours (currently working on something else).

Wim Leers’s picture

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

Reproduced. ✔

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests

Root cause is two-fold:

  • SmartDefaultSettings was not correctly computing the provided elements from the enabled plugins: it did not pass in the text editor configuration
  • SmartDefaultSettings was not yet correctly computing the $still_needed elements: it did not properly account for creatable tags (new since #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path), which is also only now a problem because this is the first time we have a wildcard tag providing support for a specific attribute (Style supports <$any-html5-element class>, i.e. any class on any HTML5 element)

IOW: the presence of even a single span.test|My Test Span Style style definition would've triggered this edge case!

Explicit test coverage pushed just now.

Wim Leers’s picture

DrupalCI says:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "cke4_stylescombo_span can be switched to CKEditor 5 without problems, only  in Source Editing" ('cke4_stylescombo_span', array(), array(array(array('style', 'sourceEditing')), array(array(array(array('Llama span', ''))), array(array('')))), '', array(), array(array('The following tags were permi...n&gt;.')))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
         )
         'ckeditor5_sourceEditing' => Array &7 (
             'allowed_tags' => Array &8 (
-                0 => ''
+                0 => ''
             )
         )
     )
 )

DrupalCI unfortunately strips the critical information away 😬 It's actually:

--- Expected
+++ Actual
@@ @@
         )
         'ckeditor5_sourceEditing' => Array &7 (
             'allowed_tags' => Array &8 (
-                0 => '<span>'
+                0 => '<span class="llama">'
             )
         )
     )
 )

This shows the problem: SmartDefaultSettings thinks that class="llama" on <span> is also not yet supported, but it is!

Fix pushed in 8bc6a27a3b1723c58240769f6ea15e2c70031f12.

Wim Leers’s picture

That should be green again 🤞

@Luke.Leber Please try again 😄

Wim Leers’s picture

Assigned: Wim Leers » Unassigned

Looking very green, so unassigning and running all tests again 🥳

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for addressing the feedback on the MR! Looks all good now!

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

I found some things on manual testing. Perhaps some of these are acceptable, but would be good to have the justification documented in the issue.

  • If I add an item to styles that includes an unsupported tag I get a pretty unhelpful error on save. Lets say I add
    code.cool-code-class|Cool Code
    

    when the Code plugin isn't enabled. With my default setup, saving results in a big white page with The website encountered an unexpected error. Please try again later. and the log has

    Error: Call to a member function getElements() on bool in                     
                                          Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator->checkAllHtmlTagsAreCreatable()

    This seems like an issue because despite the solution being fairly easy (add support for the tag), that solution isn't particularly clear. I also notice that this deviates from the really helpful validation messages that appear for many similar config issues.

  • I'm not sure if this is expected behavior, but it seems a little odd to me. Styles for <p> tags are always available - i.e. in a blank editor, the styles options for <p> are available while the options for other tags are disabled. I can choose the <p> style and it will add that paragraph with the class. For styles that are applied to other elements, I must first create that element (use header to create an h2 for example). Then the style option will be available. (confirmed this is possible in CKE4, f.e. an h2 can be created with styles combo)
  • Building on the prior point (this one might not be a huge concern): Lets say I have a <p> style that adds .exclusive-paragraph-class then use the Header plugin to change it to an <h2>. That <h2> will still have .exclusive-paragraph-class. That class gets stripped from the h2 on save since my text format doesn't support that tag/class combo so there's not a huge consequence, but it's worth noting
  • I can't add classes to drupal-media in styles. Similar to my first point, submitting the form gets me an unexpected error and the logged error is
    LogicException: The "ckeditor5_style" CKEditor 5 plugin implements            
                                             ::getElementsSubset() and did not return a subset, the following tags are     
                                             absent from the plugin definition: "". in                                     
                                             Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getProvidedElements() (line   
                                             350 of                                                                        
                                             /Users/ben.mullins/Sites/d9/core/modules/ckeditor5/src/Plugin/CKEditor5Plugi  
                                             nManager.php).       

    I don't know if something like <drupal-media> necessarily needs to be supported or if it would ever happen in an actual site scenario, but I wonder if validation should make this limitations more clear & friendly.

  • I can add styles for <li> and <ul> element in the text editor config, but in the actual editor it's not possible to add them even when a <li> or <ul> is the active element (confirmed this is possible in CKE4)
  • It is possible to add styles without labels. Ultimately this still works, but perhaps this is something that should be caught by validation? (validation catches this with CKEditor 4)
Wim Leers’s picture

Assigned: Unassigned » Wim Leers

I found some things on manual testing. Perhaps some of these are acceptable, but would be good to have the justification documented in the issue.

👏

The tl;dr of @bnjmnm's review is I found a whole lot of ways to trigger fatal PHP errors, how come y'all did not find these? 🤣🤣🙈

IOW: Ben is the new Lauri, but fortunately instead of finding critical upstream bugs, Ben is kindly finding only critical downstream bugs 👍 😄

  • If I add an item to styles that includes an unsupported tag I get a pretty unhelpful error on save. Lets say I add
    code.cool-code-class|Cool Code
    

    when the Code plugin isn't enabled. With my default setup, saving results in a big white page with The website encountered an unexpected error. Please try again later. and the log has

    Error: Call to a member function getElements() on bool in                     
                                          Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator->checkAllHtmlTagsAreCreatable()

    This seems like an issue because despite the solution being fairly easy (add support for the tag), that solution isn't particularly clear. I also notice that this deviates from the really helpful validation messages that appear for many similar config issues.

😱 Will add an explicit test case for this.

  • I'm not sure if this is expected behavior, but it seems a little odd to me. Styles for <p> tags are always available - i.e. in a blank editor, the styles options for <p> are available while the options for other tags are disabled. I can choose the <p> style and it will add that paragraph with the class. For styles that are applied to other elements, I must first create that element (use header to create an h2 for example). Then the style option will be available.
  • Building on the prior point (this one might not be a huge concern): Lets say I have a <p> style that adds .exclusive-paragraph-class then use the Header plugin to change it to an <h2>. That <h2> will still have .exclusive-paragraph-class. That class gets stripped from the h2 on save since my text format doesn't support that tag/class combo so there's not a huge consequence, but it's worth noting

Those are upstream bugs. There are known bugs with Style, some of them were already fixed in #3277438: Update to CKEditor 5 v34.1.0, but not all. For example, the functional JS test coverage still contains this:

// @todo Uncomment this after https://github.com/ckeditor/ckeditor5/issues/11709 is fixed.

See https://github.com/ckeditor/ckeditor5/issues?q=is%3Aissue+is%3Aopen+labe.... Upcoming releases will fix more bugs. They have made it clear that Style is currently "experimental" and will be getting lots of bugfixes in upcoming releases.

(Writing an explicit test for every detail is pointless IMHO, because then we'll just be duplicating their test suite.)

  • I can't add classes to drupal-media in styles. Similar to my first point, submitting the form gets me an unexpected error and the logged error is
    LogicException: The "ckeditor5_style" CKEditor 5 plugin implements            
                                             ::getElementsSubset() and did not return a subset, the following tags are     
                                             absent from the plugin definition: "". in                                     
                                             Drupal\ckeditor5\Plugin\CKEditor5PluginManager->getProvidedElements() (line   
                                             350 of                                                                        
                                             /Users/ben.mullins/Sites/d9/core/modules/ckeditor5/src/Plugin/CKEditor5Plugi  
                                             nManager.php).       

    I don't know if something like <drupal-media> necessarily needs to be supported or if it would ever happen in an actual site scenario, but I wonder if validation should make this limitations more clear & friendly.

That's intentional, because Style does not work for widgets, just like CKEditor 4's StylesCombo did not.

But of course the validation logic should not crash! 😱

  • I can add styles for <li> and <ul> element in the text editor config, but in the actual editor it's not possible to add them even when a <li> or <ul> is the active element

That's also an upstream bug. (See above.)

  • It is possible to add styles without labels. Ultimately this still works, but perhaps this is something that should be caught by validation?

+1 — I thought the validation I added covered this but apparently not 🙈


Working on adding failing tests 👍

Luke.Leber’s picture

re: #77 - everything works great on our setup now!

Wim Leers’s picture

@bnjmnm's first bug

@bnjmnm reported this crash:

Error: Call to a member function getElements() on bool in                     
                                      Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator->checkAllHtmlTagsAreCreatable()

when configuring s.blah|Blah when <s> was not supported. That's the output he saw because he's running PHP with assertions disabled. If you enable assertions, you see this instead:

assert(count($matching_plugins) === 1) in /var/www/html/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php:224

… which is what de686af0 showed in https://www.drupal.org/pift-ci-job/2403676 👍

This was fixed in 48b466fa, and now the test result (https://www.drupal.org/pift-ci-job/2403699) shows something else instead:

--- Expected
+++ Actual
@@ @@
 Array &0 (
+    'settings.toolbar.items.0' => 'The Style plugin needs another plugin to create &lt;blockquote&gt;, for it to be able to create the following attributes: &lt;blockquote class=&quot;highlighted&quot;&gt;. Enable a plugin that supports creating this tag. If none exists, you can configure the Source Editing plugin to support it.'
     'settings.plugins.ckeditor5_style.styles.0.element' => 'A style can only be specified for already supported tags. &lt;blockquote&gt; is not yet supported. Enable a plugin that supports creating this tag. If none exists, you can configure the Source Editing plugin to support it.'
 )

That's right … \Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraint::$nonCreatableTagMessage is awfully similar to \Drupal\ckeditor5\Plugin\Validation\Constraint\StyleSensibleElementConstraint::$unsupportedTagMessage
! That's because this issue/MR predate #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path, but that landed first.

Before removing the duplicate validation error messages (by removing duplicate validation logic), I wanted to make sure that the bug that @bnjmnm found through the UI was actually tested through the UI as well. 7eb9597c + the two subsequent commits prove that. (Although once again the DrupalCI test results are … so you'll have to run the test locally if you want to verify this. 😬)

The functional JS test is green because it only checks the validation message from FundamentalCompatibilityConstraintValidator — which is why 713fd839 was able to just remove all of that.

ℹ️ To view the full set of changes for fixing this bug, do git diff a24f3b215b95990e01ef79a3a510da0fab7ad2e5713fd8393be8ca39c56f6d6504cf42ac5903a668.

Wim Leers’s picture

@bnjmnm's second bug

Because this was a logic exception being thrown for invalid configuration at a time when invalid configuration is acceptable (while CKE5 is being configured in the admin UI!), there is no clear best place to add explicit test coverage for this.

The best/only place: expanding the functional JS test coverage. Did that in 23a0868c. It fails magnificently due to the fatal error 🤣 — see for yourself:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleSettingsForm
RuntimeException: Unfinished AJAX requests while tearing down a test

Then fixed it in 0bbdece3, added explicit test coverage for the logic exception in e3336eaf and expanded the existing validation test coverage in to prove how it behaves in "actually saved text editor" circumstances in 0b5f0bef.

ℹ️ To view the full set of changes for fixing this bug, do git diff 713fd8393be8ca39c56f6d6504cf42ac5903a668 0b5f0bef2895cabef953c339864139c646632e71.

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Needs review
lauriii’s picture

Now that we are relying on \Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator for validating the supported elements, there's a warning triggered instead of an error. The warning also is tied to the toolbar button, not to the Styles textarea. Because the textarea doesn't have an error state, if you are scrolled down so that the toolbar configuration is outside of screen, there's no indication of warning or error at all for the user, unless they scroll up or try to save the form. This has inferior UX compared to what we had previously. I'm wondering if you have any thoughts on how we could improve this?

Wim Leers’s picture

there's a warning triggered instead of an error

I think that is arguably better than before, because now the user is being told what to do to make the Style configuration they just entered actually work.

The warning also is tied to the toolbar button, not to the Styles textarea. Because the textarea doesn't have an error state

Perhaps we then finally have a good reason to fix #3215897: Toolbar items validation errors not able to target the toolbar? 🤓

This has inferior UX compared to what we had previously. I'm wondering if you have any thoughts on how we could improve this?

I think fixing #3215897 would bring us back to the great UX :)

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs review » Needs work

Discussed #86 + #87 with @lauriii, I have an idea about how to approach this differently now 🤓

Wim Leers’s picture

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

😬 I pushed the “run all tests again” commit while the previous build still had to start. Result: they’re both testing the last commit. This is defeating the purpose of having separate pushes: verifying that the subset passes first, then the superset 😞

Wim Leers’s picture

I forgot to update the kernel test in e3a2aa4db73ef8cca97efdb3c95b1853afabf384! Fixed now!

lauriii’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -9.4.0 release highlights +Needs reroll, +9.4.2 release notes

Confirmed that all of my feedback was addressed. Did some manual testing and in my opinion the UX is in line with other CKEditor 5 plugins now. 👍

Wim Leers’s picture

AFAICT the same patch applies cleanly to all branches! 🥳

EDIT: just realized it's an unfortunate coincidence that the patch is named *-93.patch, that's just because it was comment #93 😅

Wim Leers’s picture

😬 Forgot that yarn run build:js yields different results in D10 …

DanielVeza’s picture

Couple of issues found so far when testing this on a site.

  • It's not clear that the styles form has ajax so you can fill out the form and press save before it processes
  • If you have <button class> in the manual html field, it will throw a fatal when you try save when an OOB exception. Having just <button> works fine. Can we add some validation here?
  • If I add a button, e.g. button.btn-primary|Button it is always disabled in the styles dropdown - This looks to be related to the upstream issue raised in #81
Wim Leers’s picture

@DanielVeza Thanks for testing!

  • It's not clear that the styles form has ajax so you can fill out the form and press save before it processes

🤔 Great point. But … isn't that true for many AJAXy forms in Drupal? 😅

In any case, this works exactly the same as it does for CKEditor 4. If you have a concrete suggestion on how we could make this work better, I'd love to see that happen, but then as a non-stable-blocking follow-up Normal Task tagged Usability 🤓

  • If you have <button class> in the manual html field, it will throw a fatal when you try save when an OOB exception. Having just <button> works fine. Can we add some validation here?

But that's not the Style plugin, that's the Source Editing plugin. I already added tons of test coverage for edge cases around this, but maybe you found a new one.

This issue/MR is getting very unwieldy, so let's add more test coverage for more edge cases in a follow-up issue so that this stable-blocking issue can land 🤓

Would you mind creating a follow-up issue with the exact steps to reproduce? Because just adding <button class> to the Source Editing plugin does not reproduce this for me 😅

Thanks!

  • If I add a button, e.g. button.btn-primary|Button it is always disabled in the styles dropdown - This looks to be related to the upstream issue raised in #81

Correct! Will be fixed in an upcoming CKEditor 5 release. 👍

DanielVeza’s picture

Thanks for the quick follow up Wim!

Great point. But … isn't that true for many AJAXy forms in Drupal?

Heh, agree. I'll have a think about this one.

But that's not the Style plugin, that's the Source Editing plugin. I already added tons of test coverage for edge cases around this, but maybe you found a new one.

Sorry, I wasn't clear on this one. If you -

  • Add <button class> to the source editing
  • Add button.btn-primary to the Style plugin, you'll get a OOB exception
  • Add just <button> to the source editing and it will save correctly

This was testing on an existing site but I don't think it would be a site specific issue from some debugging, happy to be wrong though!

Love ya work and don't want to delay this getting in. I don't think either thing raised should block it and can be done in follow ups so leaving at RTBC with confirmation that this is working on an existing site.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 94: style-3222797-93-10.patch, failed testing. View results

mpp’s picture

When adding the Styles button the following error message is displayed: "Enable at least one style, otherwise disable the Style plugin."

However, there's no UI under "CKEditor5 plugin settings" to enter these styles.

Edit: it's not appearing due to a JavaScript bug:

ckeditor5.filter.admin.js?rec1ny:24 Uncaught TypeError: Cannot read properties of null (reading 'value')
    at Drupal.Ajax.ckeditor5AjaxEventResponse [as eventResponse] (ckeditor5.filter.admin.js?rec1ny:24:61)
    at HTMLTextAreaElement.<anonymous> (ajax.js?v=9.4.1:279:19)
    at HTMLTextAreaElement.dispatch (jquery.min.js?v=3.6.0:2:43064)
    at v.handle (jquery.min.js?v=3.6.0:2:41048)
    at updateSelectedButtons (ckeditor5.admin.js?rec1ny:107:14)
    at ckeditor5.admin.js?rec1ny:369:11
    at ckeditor5.admin.js?rec1ny:70:18
    at Array.forEach (<anonymous>)
    at Observable.notify (ckeditor5.admin.js?rec1ny:69:25)
    at Observable.set (ckeditor5.admin.js?rec1ny:86:16)

The id is "erjqrv8b1j-edit-editor-editor--5b1BdFoJwK0":

<select data-drupal-selector="erjqrv8b1j-edit-editor-editor-5b1bdfojwk0" id="erjqrv8b1j-edit-editor-editor--5b1BdFoJwK0" name="editor[editor]" class="form-select" data-once="drupal-ajax"><option value="">None</option><option value="ckeditor">CKEditor</option><option value="ckeditor5" selected="selected">CKEditor 5</option></select>

Seemed to be related to https://www.drupal.org/project/drupal/issues/1852090.

Once that was solved the following popped up:

OutOfBoundsException: in Drupal\ckeditor5\Plugin\Validation\Constraint\StyleSensibleElementConstraintValidator->findStyleConflictingPluginLabel() (line 150 of core/modules/ckeditor5/src/Plugin/Validation/Constraint/StyleSensibleElementConstraintValidator.php).

Removing

from the allowed html list fixed this (added styles for td & tr).

the Styles button appears but the tr/td styles don't apply.

mpp’s picture

mpp’s picture

mpp’s picture

After some tweaks, the Styles UI is working. Not sure why but ckeditor 5 doesn't detect the td or tr element, only the p is enabled:

s

s

@wim any idea why ckeditor detects a p when the cell is highlighted, perhaps a default paragraph setting in ckeditor config or a bug in ckeditor itself?

xjm’s picture

Issue tags: +Drupal 10 beta blocker
xjm’s picture

Updating tag for the next window.

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

nod_’s picture

last commit fix part of #97, there is no crash anymore, but we get a validation error that says we can't have two plugins supporting the same elements + attributes.

The error is that styles support of <button class="test"> is in conflict with source editing plugin support of <button class>. The validation error makes sense to me.

Do we want to be able to use the UI to edit a subset of what's allowed with source editing? If we keep things strictly separate the fix should be enough, but I can see some cases where this type of config might be wanted (although it might not be a good idea).

nod_’s picture

test is not actually failing without the code change so will need to fix that

nod_’s picture

Status: Needs work » Needs review
nod_’s picture

created #3294908: Configuration overlaps between Styles and other CKE5 plugins to agree on where we stop peeling the validation oignons.

nod_’s picture

Issue summary: View changes
DamienMcKenna’s picture

The release tag change was accidentally reverted.

nod_’s picture

Status: Needs review » Needs work
xjm’s picture

Issue tags: +Needs change record

The release note here does not describe the disruption or consequences of the change. It should explain the upgrade path implications for site owners and module developers. See the release note instructions for more info on what should go in a release note. Thanks!

A CR would also probably help.

nod_’s picture

Status: Needs work » Needs review

Last push doesn't fix the test failure but gets closer. With the latest changes to the upgrade messages not sure what's happening here.
When I run the test I get the First message, but when I relaunch it I get the second, so I can never seem to get the right string at the right time to match. Help welcome.

      'expected_db_logs' => [
        'status' => [
          //"The following tags were permitted by the <em class=\"placeholder\">A CKEditor 4 configured to have span styles</em> text format's filter configuration, but no plugin was available that supports them. To ensure the tags remain supported by this text format, the following were added to the Source Editing plugin's <em>Manually editable HTML tags</em>: &lt;span&gt;. The text format must be saved to make these changes active.",
          //'To maintain the capabilities of this text format, <a target="_blank" href="/admin/help/ckeditor5#migration-settings">the CKEditor 5 migration</a> did the following:  Added these tags/attributes to the Source Editing Plugin\'s <a target="_blank" href="/admin/help/ckeditor5#source-editing">Manually editable HTML tags</a> setting: &lt;span&gt;. Additional details are available in your logs.',
        ],
      ],


nod_’s picture

xjm’s picture

Issue tags: -9.4.4 release notes

Untagging for now since this is not in yet, but we can re-tag for the release notes if there is a disruptive change associated with this on commit. (The current release note does not describe a disruptive change as per #113.)

nod_’s picture

Issue summary: View changes

Tried to add some details to the release note. Essentially, nothing to do for site owners, when they switch to the CKE5 editor, the config is migrated automatically.

Not sure what to put in the change record though, we don't have change records for additional plugins we support in core, only the Drupal-specific API or use of CKE5.

As for the configuration overlap issue it's fine to keep in a follow up because:

  1. If you do not use the styles plugin, there is no disruptions on existing configuration
  2. The validation error we have follows the existing behavior of making sure only one plugin owns a specific tag/attribute/value, the follow-up will need to change this assumption in the case of the styles plugin.
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I think the release note is more of a highlight than describing something disruptive. I don't think this is disruptive to existing users as #117 already points out. I don't see how we would need a change record for this but I'm keeping the tag for now.

I've reviewed all of the changes since my last RTBC and it addresses all of the feedback that has been raised after it.

bnjmnm’s picture

The last submitted patch, 119: 3222797-style--95x.patch, failed testing. View results

  • bnjmnm committed 0603b8c on 10.1.x
    Issue #3222797 by Wim Leers, nod_, bnjmnm, larowlan, mpp, Luke.Leber,...

  • bnjmnm committed 95be604 on 9.5.x
    Issue #3222797 by Wim Leers, nod_, bnjmnm, larowlan, mpp, Luke.Leber,...

  • bnjmnm committed f0a7851 on 9.4.x
    Issue #3222797 by Wim Leers, nod_, bnjmnm, larowlan, mpp, Luke.Leber,...

  • bnjmnm committed f109ec0 on 10.0.x
    Issue #3222797 by Wim Leers, nod_, bnjmnm, larowlan, mpp, Luke.Leber,...
bnjmnm’s picture

Status: Reviewed & tested by the community » Fixed
Issue tags: -Needs change record +9.4.6 release notes, +9.5.0 release highlights
  • Committed to 10.1.x, and that commit was cherry picked to 10.0.x
  • Committed to 9.5.x
  • Committed to 9.4.x since CKEditor 5 is experimental and this is a migration feature we'd like available to users as soon as possible

I'm tagging with 9.4.6 release notes despite being aware of this comment (and it's associated policy)
#3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style.
The release note in the issue summary does not meet the criteria of describing a disruptive because nothing disruptive is introduced here (also why I removed the CR tag). This is a release highlight, but those are typically not tagged on patch releases, and I wanted to be sure this was discoverable in case there was a desire to make this known to 9.4.6 users waiting on Styles Combo migration support.

Status: Fixed » Closed (fixed)

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