Problem/Motivation

Follow-up of #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style, to fix the edge case reported by @DanielVeza at #3222797-97: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style.

When using the Styles and the sourceEditing plugins there can be situation with conflicting configuration.

The main idea with CKE5 config is to not have two plugins with overlaping supported elements.

Steps to reproduce

  1. Set up a CKE5 text format with a Styles config of
    span.test|Test
    
  2. and a sourceEditing config of
    <span class>
    
  3. An error message is shown:
    A style must only specify classes not supported by other plugins. The test classes on <span> are already supported by the enabled Source editing plugin.

Proposed resolution

Discuss what needs to happen in that situation.

  1. Do nothing, since the user is expected to use sourceEditing to alter the span classes, the styles plugin shouldn't be able to touch those.
  2. It can make sense to have the Styles plugin add classes to an element for less technical users, while still allowing the source editing plugin to allow all classes for more technical users. (Removing the validation constraints, it's working as expected in the UI). It can introduce a grey area where unexpected behaviors happens though.

    In that case the resulting Allowed HTML tags will be as followed

    <span class>
    

Remaining tasks

Agree on a solution

User interface changes

API changes

Data model changes

Release notes snippet

Comments

nod_ created an issue. See original summary.

nod_’s picture

Issue summary: View changes
nod_’s picture

Status: Active » Needs review
StatusFileSize
new2.32 KB

no test yet but seems to be working.

When a plugin declare support for class without value restriction, make sure the styles plugin can add classes to the same tag without problem.

code is not pretty and docs needs work but putting as NR if someone is inpired to take a look and improve this before I get back to it next week.

nod_’s picture

StatusFileSize
new8.45 KB
new6.42 KB

Added some tests

nod_’s picture

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
@@ -207,7 +207,6 @@ private function checkAllHtmlTagsAreCreatable(EditorInterface $text_editor, Fund
-        assert(count($matching_plugins) === 1);
         $plugin_definition = reset($matching_plugins);

Removed that because it ends up being 2 different plugins that supports the same tag, and we have a "reset" after that makes sure we only take one.

Not sure how to fix the assert in this case.

nod_’s picture

StatusFileSize
new6.42 KB
new8.45 KB
nod_’s picture

Title: Configuration overlaps between Styles and sourceEditing plugins » Configuration overlaps between Styles and other CKE5 plugins
nod_’s picture

StatusFileSize
new6.41 KB
new8.44 KB
new2.09 KB
nod_’s picture

StatusFileSize
new6.41 KB
new8.44 KB
new2.08 KB

The last submitted patch, 7: core-3294908-7-TEST-ONLY.patch, failed testing. View results

The last submitted patch, 7: core-3294908-7.patch, failed testing. View results

The last submitted patch, 9: core-3294908-9-TEST-ONLY.patch, failed testing. View results

The last submitted patch, 9: core-3294908-9.patch, failed testing. View results

The last submitted patch, 10: core-3294908-10-TEST-ONLY.patch, failed testing. View results

nod_’s picture

all good now :)

wim leers’s picture

Assigned: Unassigned » wim leers
xjm’s picture

Priority: Normal » Critical
Issue tags: +Drupal 10 beta blocker
Parent issue: » #3238333: Roadmap to CKEditor 5 stable in Drupal 9

wim leers’s picture

Issue summary: View changes

So this is to fix the edge case reported by @DanielVeza at #3222797-97: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style. Credited them!

wim leers’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
    @@ -207,7 +207,6 @@ private function checkAllHtmlTagsAreCreatable(EditorInterface $text_editor, Fund
    -        assert(count($matching_plugins) === 1);
    

    🤔 Removing this assertion is a significant change: it means that multiple plugins may be adding support for a particular tag.

    We should not need this part.

  2. +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php
    @@ -1253,6 +1253,125 @@ public function providerPair(): array {
    +    $data['INVALID: Style plugin configured to add class already added by an other plugin'] = [
    

    👍

  3. +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php
    @@ -1253,6 +1253,125 @@ public function providerPair(): array {
    +    $data['VALID: Style plugin configured to add new class to and already restricted tag'] = [
    

    👍

  4. +++ b/core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php
    @@ -1253,6 +1253,125 @@ public function providerPair(): array {
    +    $data['VALID: Style plugin configured to add class to an element that already allows all classes'] = [
    

    👍 … but (!) this is only testing the case of a concrete plugin supporting a particular element, it's not testing the case of a SourceEditing-provided element.

    I think we need more test coverage.

If it weren't for that one line and the missing extra test coverage, I'd RTBC 😊

wim leers’s picture

Assigned: wim leers » Unassigned
Status: Needs work » Needs review
StatusFileSize
new4.66 KB
new9.73 KB

This addresses #21.1 (a single line revert) and #21.4 (more test cases).

wim leers’s picture

Note: the single line revert is trivial, and so are the test cases. Explaining the test cases was more difficult than writing them. So if @nod_ +1s the added test cases, I'm comfortable RTBC'ing.

nod_’s picture

ah yes, since I changed the element in the test it doesn't fail anymore

nod_’s picture

Status: Needs review » Reviewed & tested by the community

new tests make sense, thanks for adding those :)

  • bnjmnm committed fb85980 on 10.1.x
    Issue #3294908 by nod_, Wim Leers, DanielVeza: Configuration overlaps...

  • bnjmnm committed a620ced on 10.0.x
    Issue #3294908 by nod_, Wim Leers, DanielVeza: Configuration overlaps...

  • bnjmnm committed d6da999 on 9.5.x
    Issue #3294908 by nod_, Wim Leers, DanielVeza: Configuration overlaps...
bnjmnm’s picture

Version: 10.0.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Fixed

I was pretty worried about #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style going in with this edge case happening, but it was also a bad idea to expand the scope of that issue further. It happens to make this solution one of the easier CKEditor 5 patches I've reviewed. Fix makes sense, as do the tests.

The fact that this problem was happening indirectly showcases how robust validation is in the CKeditor 5 modules. For every hassle such as this one, we've been spared a dozen larger ones.

Committed to 10.1.x and cherry picked to 10.0.x and 9.5.x. Good fix folks!

danielveza’s picture

Amazing team! ❤️

eric_a’s picture

I was pretty worried about #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style going in with this edge case happening

That issue went into 9.4.x. Should this one as well?

Status: Fixed » Closed (fixed)

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

wim leers’s picture

Version: 9.5.x-dev » 9.4.x-dev
Status: Closed (fixed) » Patch (to be ported)

The fix should indeed still be backported to 9.4.x, @Eric_A!

Will provide a patch. Currently not cleanly cherry-pickable. So determining what the optimal cherry-pick order is …

wim leers’s picture

Status: Patch (to be ported) » Reviewed & tested by the community

#22 still applies cleanly to 9.4.x too! Test queued 👍

  • lauriii committed b7b5e1a on 9.4.x
    Issue #3294908 by nod_, Wim Leers, DanielVeza: Configuration overlaps...
lauriii’s picture

Status: Reviewed & tested by the community » Fixed

Committed b7b5e1a and pushed to 9.4.x. Thanks!

wim leers’s picture

Version: 9.4.x-dev » 9.5.x-dev
Status: Fixed » Closed (fixed)

Restoring previous status.

jksloan2974’s picture

I am running php 8.1.14 and Drupal 9.5.3. I am still unable to add a span with class in the styles plugin.

Here is the error message after trying to add span.test_class|Test Class to the Styles configure textarea

Error message
The Style plugin needs another plugin to create , for it to be able to create the following attributes: . Enable a plugin that supports creating this tag. If none exists, you can configure the Source Editing plugin to support it.

bnjmnm’s picture

@jksloan2974 I recommend bringing up your concern in #38 in Drupal Slack or opening a new issue describing the steps to reproduce. Issues are rarely re-activated, and in those rare instances it's within a few days of it being completed. Since it's been 5 months, this will not re-open to address your concerns, but if it were a new issue (or question in Slack), it can get some attention.