Problem/Motivation

If CKEditor 5 is selected as text editor for the Basic HTML format then the "Language" button is added automatically to the toolbar. This is also the case, if the text editor is switched from a CKEditor configuration where the Language button is disabled.

For the other default text formats (Full and Restricted HTML), the "Language" button is not automatically added.

The "Language" button has very limited use in a Drupal site because content languages are organized through the Core modules, and the user experience can be really confusing if HTML markup for languages can also be added within a field. Therefore enabling the "Language" button should be a conscious choice of a site builder, not the default option. Enabling it just to support the <span> tag is not appropriate and should therefore be considered a regression.

Steps to reproduce

  • Enable the CKEditor 5 module
  • Go to the configuration of Basic HTML at /admin/config/content/formats/manage/basic_html
  • Change the text editor from "CKEditor" to "CKEditor 5"

(Also see GIFs in User interface changes.)

Proposed resolution

Do not add the "Language" button to the default CKEditor 5 toolbar configuration for Basic HTML to support <span>.

How? See #5.

Remaining tasks

None.

User interface changes

Switching from CKEditor 4 to CKEditor 5 when <span> was supported in the text format but not the "Language" plugin

Before
After

Enabling a plugin which only is able to add attributes to a tag but not create the tag

Now when you try to enable the Language plugin when <span> is not yet supported, you will get a warning:

And it disappears/never appears if <span> is supported:

API changes

CKEditor 5 plugin metadata now needs to explicitly list tags it can create, by listing it under drupal.elements as just <tag>, in addition to listing it as <tag attrA attrB>.

Data model changes

None.

Release notes snippet

The drupal.elements metadata in CKEditor 5 plugin definitions must now explicitly list which tags are creatable. Previously, any listed tag was assumed to be creatable by the CKEditor 5 module, even if it was only able to create attributes on an already existing tag.

Issue fork drupal-3273983

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

ifrik created an issue. See original summary.

ifrik’s picture

As discussed: the Language button is added as a way to allow for the span tag, so there might be another way around that.

Wim Leers’s picture

Title: Do not add Language button by default to Basic HTML » Special case <span> for SourceEditingRedundantTags and upgrade path
Assigned: Unassigned » Wim Leers
Priority: Normal » Major
Issue tags: +Needs issue summary update, +Upgrade path, +Usability

Discussed with @ifrik. I know what to do next. Will expand tomorrow.

Wim Leers’s picture

Title: Special case <span> for SourceEditingRedundantTags and upgrade path » Special case <span> and <div> for SourceEditingRedundantTags and upgrade path
Assigned: Wim Leers » Unassigned
Status: Active » Needs review

Sorry for the delay! Here's the promised write-up based on discussing this with @ifrik at DDD Ghent.

Many sites have <span> in their filter_html configuration. The only way for CKEditor 5 to meet that goal is to enable ckeditor5_language. So that's what SmartDefaultSettings does. Problem solved, right?

But … that also places the textPartLanguage toolbar item in the CKE5 toolbar, which drastically changes the UX. Many sites do not need nor want it.

So what's going on here? Well, <span> is different from 99% of other HTML tags in that it has basically no meaning. It's a container element without semantics. <span> is the inline container, <div> is the block container.

In fact, the HTML spec says something along those lines too:

Therefore these two tags deserve to be treated differently by SmartDefaultSettings. Because these tags on their own do not have semantics. So if a text format has <span> or <div> allowed, or even <span foo> or <div foo>, then SmartDefaultSettings should not try to partially fulfill the need by enabling a plugin that allows <span bar> or <div bar> to be created.

Marking Needs review to gather input from fellow maintainers.

lauriii’s picture

Based on where I ran into #3276207: SourceEditingRedundantTagsConstraintValidator logic handles attribute supersets incorrectly which was with <section> element, I'm wondering if we need something more generic for this than what's proposed here. What if we only triggered SourceEditingRedundantTags for plugins that provide an exact matches?

We could allow plugins to define that they support both <div> and <div foo> by defining two items in the elements. That would mean that both <div> and <div foo> are supported by the plugin. On SourceEditingRedundantTags if the user wants to use <div> we could consider that plugin a superset that contains an exact match. If the plugin only provided <div foo>, we would not consider <div> an exact match to the plugin.

Wim Leers’s picture

We could allow plugins to define that they support both <div> and <div foo> by defining two items in the elements. That would mean that both <div> and <div foo> are supported by the plugin.

Interesting proposal! I think that makes sense.

That would mean that for example ckeditor5_link would have to change from

    elements:
      - <a href>

to:

    elements:
      - <a>
      - <a href>

and ckeditor5_table from

    elements:
      - <table>
      - <tr>
      - <td rowspan colspan>
      - <th rowspan colspan>
      - <thead>
      - <tbody>
      - <tfoot>

to

    elements:
      - <table>
      - <tr>
      - <td>
      - <th>
      - <td rowspan colspan>
      - <th rowspan colspan>
      - <thead>
      - <tbody>
      - <tfoot>

That looks a bit odd, but not unreasonable.

Wim Leers’s picture

On SourceEditingRedundantTags if the user wants to use <div> we could consider that plugin a superset that contains an exact match. If the plugin only provided <div foo>, we would not consider <div> an exact match to the plugin.

Would this mean that SmartDefaultSettings would do BOTH:

  1. enable the SourceEditing plugin and configure it to allow <div> tag
  2. enable this plugin that provides <div foo>, to add support for the foo attribute on the <div> tag

?

If so: I think that's reasonable, and perfectly complementary to #6: if a plugin supports the tag explicitly, then we won't need to add it to SourceEditing, otherwise we would need to.

lauriii’s picture

#6: 💯

#7: That's how I would imagine it work, so long as the plugin hasn't explicitly stated that it supports <div>.

I agree all of this is a bit strange but I think it would make sense because CKEditor 5 does provide a very easy way for plugins to support HTML elements only when certain attributes exist. I think the original proposed solution where we would special case <span> and <div> could also be helpful. I'm wondering if we should decide which approach to take or should we implement both approaches?

Wim Leers’s picture

While we could do both, your <section> example made it clear that special-casing a few tags doesn't necessarily work. I think your generic proposed solution in #5 (which I provided concrete examples for in #6) will do just fine 🤓

In other words: if you're +1 to it, I'd be happy to implement exactly what you proposed in #5. Then you can also remain the reviewer? 🤞

Wim Leers’s picture

Title: Special case <span> and <div> for SourceEditingRedundantTags and upgrade path » Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path

Retitling per #8.

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

Assigned: Unassigned » Wim Leers
Wim Leers’s picture

Wim Leers’s picture

Status: Needs review » Active

faa9df5d is identical to the patch in #14. The MR is now in sync with the patch. All next steps will happen in the MR.

Wim Leers’s picture

0177e520 documents the architectural change/addition this issue is making.

3de2fa09 added a unit test for new (low-level) infrastructure in HTMLRestrictions. It failed. 👍

3c1cc5ef implemented the infrastructure. It made the unit test pass. 👍

Wim Leers’s picture

0dc9f848 and 6db6453 introduced new (higher level) infrastructure, which is not yet used, nor tested. They will be soon.

This should still pass tests, because it adds unused infra. 🤞

This passed tests, because it added unused infra. 👍

Wim Leers’s picture

0bbb4f8107 changed the unit test expectations for SmartDefaultSettings::getCandidates() → it should now take into account the ability of a plugin to create such tags into account. This should fail. 🤞

Wim Leers’s picture

0bb4f81 updated a unit test for a low-level detail in SmartDefaultSettings that will need to behave differently. It failed 👍

32abc3d1 updated the SmartDefaultSettings logic. It made the unit test pass 👍 Indirect consequences:

  • it made the SmartDefaultSettings kernel test fail: expected, that's what eaef245f1d will fix … partially — because more changes are necessary, and this kernel test is really at the hart of it all!
  • it also made a functional JS test fail. I expect it to pass again when the SmartDefaultSettings kernel test passes again! 🤓
Wim Leers’s picture

🥳 All of the above has been leading up to this moment: 1b20949794 is the essential algorithmic change (along with 32abc3d1), and will solve the reported bug! 👍

(But bear with me while there are still some tests failing — we for example still need to update SourceEditingRedundantTagsConstraintValidator. Until then, it will complain. That's why I'm not marking this as needing review yet.)

Wim Leers’s picture

🤬 I hate it when I carefully curate commits and then phpcs screws it up … especially if it does not complain locally.

Wim Leers’s picture

Whew, that was tricky to sort out!

The failure was on

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" (array(array(array('heading', 'bold', 'italic', 'link', 'sourceEditing', 'textPartLanguage')), array(array(array('heading2', 'heading3', 'heading4', 'heading5', 'heading6')), array('un'), array(array('', '', '', '', '', '', '', '')))), array(true), array(), array('The following tag(s) are alre....', 'The following tag(s) are alre....', 'The following attribute(s) ar....', 'The following tag(s) are alre....', 'The following attribute(s) ar....'))
Failed asserting that two arrays are identical.

One expected entry was missing:

-    'settings.plugins.ckeditor5_sourceEditing.allowed_tags.5' => 'The following tag(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 tags: Code Block (&lt;code class=&quot;language-*&quot;&gt;).'

Root cause: I missed an edge case in https://git.drupalcode.org/project/drupal/-/merge_requests/2310/diffs?co....

Fixed in 4a8a61781b.

Wim Leers’s picture

My fix in 4a8a6178 was wrong, because there is a number of new failures. Explicit test coverage added in 6b58fddff9. Will continue on Monday.

Wim Leers’s picture

The failures in b2eed581d851f2a58104ecebff7216bf7f79c6e9 indicate our validation logic needs to be updated too:

1) Drupal\Tests\ckeditor5\FunctionalJavascript\MediaTest::testMediaArbitraryHtml
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'The following tag(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Media caption (&lt;drupal-media&gt;), Media (&lt;drupal-media&gt;), Media align (&lt;drupal-media&gt;).'
+    1 => 'The following tag(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 tags: TEST — Layercake (&lt;div&gt;).'
+)

8f366fac124dde62fb52061faca63270d372438b added the first piece of that.

But we also need to do run-time compliance checking for the return value of getElementsSubset(). We already have that. But we need more of it: we need to ensure that <foo> is still returned if it was listed and <foo attr> is still in the subset. (This is what is causing the Media failure.)

Wim Leers’s picture

Wim Leers’s picture

d2be0cac7b7b76a30c3ea14329e9dd09fce3c10e expanded validation logic. This caused many more failures. One concrete example:

1) Drupal\Tests\ckeditor5\Functional\MediaEntityMetadataApiTest::testApi
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
-Array &0 ()
+Array &0 (
+    0 => 'The current CKEditor 5 build requires the following elements and attributes: &lt;br&gt; &lt;p&gt; &lt;* dir=&quot;ltr rtl&quot; lang&gt; &lt;drupal-media data-entity-type data-entity-uuid alt data-view-mode&gt;The following tags cannot be created: &lt;drupal-media&gt;'
+)

But … this worked fine before, so why is this now invalid? Well, the runtime validation logic for ::getElementsSubset() added in 5be4ff7560839eb6aca8afa10171e68352f79ed1 explains it, because now that same test fails like this:

1) Drupal\Tests\ckeditor5\Functional\MediaEntityMetadataApiTest::testApi
LogicException: The "media_media" CKEditor 5 plugin implements ::getElementsSubset() and did return a subset ("") but the following tags can no longer be created: "".

/var/www/html/core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php:347
/var/www/html/core/modules/ckeditor5/src/Plugin/CKEditor5PluginManager.php:179
/var/www/html/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php:199
/var/www/html/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php:73
/var/www/html/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:196
/var/www/html/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:148
/var/www/html/core/lib/Drupal/Core/TypedData/Validation/RecursiveContextualValidator.php:100
/var/www/html/core/lib/Drupal/Core/TypedData/Validation/RecursiveValidator.php:93
/var/www/html/core/lib/Drupal/Core/TypedData/TypedData.php:132
/var/www/html/core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php:224
/var/www/html/core/modules/ckeditor5/tests/src/Functional/MediaEntityMetadataApiTest.php:159
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:726

→ i.e. the <drupal-media> tag is not being returned by Media::getElementsSubset(). 2ee82ada24db05bbd09ff8c80663eae237c21c91 fixes that. 👍

Wim Leers’s picture

8bbcc46 introduced many new failures. Even though the code is definitely correct.

They're all attributable to errors like this:

InvalidArgumentException: The "blockquote" HTML tag has attribute restrictions, but it is not an array of key-value pairs, with HTML tag attribute names as keys.
InvalidArgumentException: The "drupal-media" HTML tag has attribute restrictions, but it is not an array of key-value pairs, with HTML tag attribute names as keys.

… caused by \Drupal\ckeditor5\HTMLRestrictions::merge() calls, due to a bug that #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers is fixing 👍

So, committed #3274648-19: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers to this merge request. 🥳

Wim Leers’s picture

Title: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path » [PP-1] Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path
Wim Leers’s picture

Those 2 commits made SmartDefaultSettingsTest go from 3 failures to 1.

These next 2 commits should make it go from 1 to 0, and also fix the last failure in ValidatorsTest.

Wim Leers’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: -Needs issue summary update +Needs release note
FileSize
210.19 KB
187.8 KB
Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Issue summary: View changes
Issue tags: -Needs release note
FileSize
917.46 KB
954.55 KB

That is green 😊

Improved issue summary. Added change record. Added release note.

Pinging @ifrik for a review 😊

Wim Leers’s picture

Title: [PP-1] Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path » Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path
lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Thank you! 🙏

All (including the draft CR) looks good for me now!

ifrik’s picture

I've tested this with Drupal 9.5.x, enabled CKEditor5 and then changed the text format Basic HTML to use CKEditor5.

Without the patch, the 'Language' button is automatically added to the toolbar. Using this branch, the 'Language' button is not added, A button for 'Code' is added automatically, but it can be removed. As a sitebuilder I need to review the changes that appear in the active toolbar anyway, so that it okay.

I've also tested it with a customized set of available buttons in the Basic HTML, and again the Language button is _not_ added. Blockquote and Code are added if they haden't been there before, but can be removed.


Basic HTML with CKEditor


Basic HTML after changing to CKEditor5

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

Some feedback in MR.

This seems like the right approach, but I have to mention that it pushes some already challenging DX into even bumpier territory. Ultimately it's probably the best choice as more people benefit from an elegant migration than they do straightforward CK5 plugin development (which isn't feasible anyway as it's already very complex).

Wim Leers’s picture

Assigned: Unassigned » Wim Leers

Good feedback! 🙏👏

Will address on Monday 😊

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
Status: Needs work » Reviewed & tested by the community

To address @bnjmnm's review, the vast majority of what I had to do were incorporate his comment suggestions. I made one UI string change to clarify things. I think that means this is straight back to RTBC, since it's the only functional change was a single UI string change.

  • bnjmnm committed 02e337e on 10.0.x
    Issue #3273983 by Wim Leers, ifrik, lauriii: Do not assume that plugin...

  • bnjmnm committed 5fa54e8 on 9.5.x
    Issue #3273983 by Wim Leers, ifrik, lauriii: Do not assume that plugin...

  • bnjmnm committed 5dc0ff5 on 9.3.x
    Issue #3273983 by Wim Leers, ifrik, lauriii: Do not assume that plugin...

  • bnjmnm committed 835ccb9 on 9.4.x
    Issue #3273983 by Wim Leers, ifrik, lauriii: Do not assume that plugin...
bnjmnm’s picture

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

Everything appears to be addressed. Committed to 10.0.x and cherry-picked to 9.5.x 9.4.x 9.3.x

Wim Leers’s picture

quietone’s picture

Status: Fixed » Closed (fixed)

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