Problem/Motivation

The change in validation introduced in #3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by triggers validation errors for previously valid config when "Source" / Source editing button is added and "Manually editable HTML tags" are specified.

The intent of the validation was that because a plugin supported <ol start> that tests should have failed when trying to manually configure source editing to allow start <ol start> on the basis that the UX for source editing is worse that the UX of a plugin.

There are several use cases where this has caused frustration, a lot related to the fact you can no longer allow the class attribute in source editing as it expects you to use the style plugin.

Several use cases were raised where the old behavior is preferred, such as:

  • Adding classes that are not supported by the style plugin, e.g. classes can not be added to img elements through the style plugin, but should be able to through manual source editing
  • Making classes available through CKEditor templates but not intending them to be selectable via the ckeditor style drop down
  • Frameworks such as bootstrap or tailwind which contain utility classes, which can be used by exerienced editors but are not appropriate to all be selected in the dropdown
  • Support for classes related to legacy styles or content, which need to be retained but not necessarily "promoted" to the styles plugin to be used

There are many other use cases for arbitrary-but-allowed source editing throughout the issue including migrations and upgrades for existing Drupal installations with these patterns in place since the allowed HTML feature was converted to source editing with the CKEditor 4 to 5 upgrade.

Steps to reproduce

1. Install using the Standard installation profile on Drupal 10.2.3
2. Create a new text format, setting the editor to "CKEditor 5" and adding the following toolbar items: Blockquote, Styles, and Source editor.
3. Configure one style for the dropdown, p.fancy|Fancy Paragraph
4. Add the "Limit allowed HTML tags and correct faulty HTML" text format filter.
5. Save the form. Everything should be fine.
6. Edit the text format. Under "Source editing" > "Manually editable tags", add <blockquote class>, indicating that you want content editors to manually add the `class` attribute to the `blockquote` tag, which is allowed on the CKEditor toolbar.
7. Attempt to save the form. A validation check will prevent the form save: The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (<blockquote class>).

Proposed resolution

Several options have been discussed in this issue with the aim of allow source editing to define what can be edited in source regardless of other plugins that might also allow the change.

Option 1 - make the validation configurable

Drupal core provides a setting to opt out of this locked behavior to accommodate the use cases mentioned above.

Suggested in https://www.drupal.org/project/drupal/issues/3410100#comment-15474342

Option 2 - revisit if the validation is needed at all

Suggested in https://www.drupal.org/project/drupal/issues/3410100#comment-15409300

Option 3 - allow class attributes

Suggested in https://www.drupal.org/project/drupal/issues/3410100#comment-15578880

Remaining tasks

Need maintainer discussion / decision on suitable approach.

User interface changes

TBC

API changes

TBC

Data model changes

TBC

Release notes snippet

TBC

Issue fork drupal-3410100

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:

Comments

KlemenDEV created an issue. See original summary.

klemendev’s picture

Title: CKEditor breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified » CKEditor5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified
joaquinve’s picture

Hello,

The problem also occurs in version 10.1.6 and without adding any tags to the "Manually editable HTML tags" setting of the plugin.

Just by adding the "Source" button the text formatting interface is damaged.

wim leers’s picture

Title: CKEditor5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified » [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified
Version: 10.2.x-dev » 11.x-dev
Assigned: Unassigned » wim leers
Priority: Major » Critical
Issue tags: +Needs issue summary update
Related issues: +#3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by

Merging #3410364: Change to edit source error handling in 10.2 leads required changes to configuration that cause data loss on edit and #3410380: Several elements are missing from ckeditor5 Basic HTML as duplicates of this, since this was the first issue created for this regression.

Already crediting all contributors of those issues here, next up: transfering relevant info.

bsnodgrass’s picture

Added another related possibly duplicate issue

wim leers’s picture

Interesting! #3410303 points to #2441811: Upgrade filter system to HTML5 as the culprit.

But the other issues pointed at #3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by!

Now this definitely needs a deeper investigation.

adrianm6254’s picture

StatusFileSize
new2.25 KB
new2.17 KB

@Wim Leers you asked for a couple of files from my other issue, https://www.drupal.org/project/drupal/issues/3410380 so I'll copy them here. Hope this helps. I'll also include what my Style Plugin & Source Editing settings are showing on the old site (D10.1.5.) and the new site (D10.2.0).


Source Editing:

Original (D10.1.5)
    <cite> <span> <dl class> <dt class> <dd class> <iframe allow allowfullscreen src width height frameborder> <blockquote cite> <ul type> <ol type> <h2 id class> <h3 id class> <h4 id class> <h5 id class> <h6 id class> <a hreflang accesskey id rel target title class> <img data-entity-substitution class> <p class>
    
Current (D10.2.0)
    <cite> <span> <dl class> <dt class> <dd class> <iframe allow allowfullscreen src width height frameborder>
    
Style Plugins:

Original (D10.1.5)
    a.button|Button
    p.note|Note
    p.large|Larger text
    h1.node__title|Page title
    img.border|Add a border
    img.display-left|Float left
    img.display-right|Float right
    p.h2|H2
    p.h3|H3
    p.h4|H4

Current (D10.2.0)
    a.button|Button
    p.note|Note
    p.large|Larger text
    h1.node__title|Page title
    img.border|Add a border
    img.display-left|Float left
    img.display-right|Float right
    p.h2|H2
    p.h3|H3
    p.h4|H4

joshuami’s picture

@Wim Leers, I think #3396628 may be a separate issue.

What I was experiencing was definitely related to the changes in SourceEditingRedundantTagsConstraintValidator.php as part of #3396628. At https://git.drupalcode.org/project/drupal/-/commit/ec24a58c6dd7e7d3a1ead..., the validation is comparing elements enabled in source editing to all other enabled plugins. That check prevents someone from adding an attribute like class to anything that might be covered by another plugin that also enables aspects of that element.

The error message I was getting that prevented saving a valid config is The following @element_type(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: %overlapping_tags.

Here's an example:

We had a couple of CKEditor 4 plugins that we wrote to support adding phone numbers and email addresses with an enforced format and classes that allowed them to be editable and styled. That code required the following to be enabled in source editing: <span class="cke-phone cke-email">. (Note: this is a temporary need to avoid data loss until we can write a proper CKEditor 5 solution.

When we attempt to save the text format after the 10.2 upgrade, we get the error message:

The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (<span class="cke-phone cke-email">).

That is directly coming from $enabled_plugin_elements_optional, but the only other plugin enabled that affects span is the Language selector. Clearly, we are not going to get the ability to save those classes from a completely unrelated plugin, but the overlap filter is preventing span from being in source editing.

I propose we remove the overlap optional filtering so that source editing can truly allow the developer to say "I know I don't have another plugin for this so I want to allow it in source just to prevent data loss until I can write something way more complex and robust (possibly never 😆)".

To put this another way, source editing should allow us to define exceptions that are not covered by another plugin rather than forcing us to write a plugin from scratch.

joao.ramos.costa’s picture

@joshuami I’m facing exactly the same issue. I managed to workaround some missing classes and prevent data loss by adding some styles to the Styles plugin ie span.my-class. But that doesn’t cover all cases, for instance the <ul class>, where it complains with the same constraint validation you’ve described.

damienmckenna’s picture

Issue tags: +Regression
jesss’s picture

Coming here from #3410380: Several elements are missing from ckeditor5 Basic HTML.

My site's content is missing basic HTML tags after updating to 10.2.0 -- most noticeably bold and links. Turning off the Limit HTML Tags filter restored the missing tags but as others have noted, that is not a good long-term solution.

When I turned the Limit HTML Tags filter back on again, I was presented with the following error message:

The current CKEditor 5 build requires the following elements and attributes:
<br> <p> <h2> <h3> <h4> <h5> <h6> <* dir="ltr rtl" lang> <cite> <dl> <dt> <dd> <span> <figure> <figcaption> <iframe src height width allowfullscreen frameborder> <strong> <em> <code> <blockquote> <a href data-entity-type data-entity-uuid data-entity-substitution> <ul> <ol reversed start> <li> <img src alt height width data-entity-uuid data-entity-type data-caption data-align> <drupal-media data-entity-type data-entity-uuid alt data-caption data-align>
The following elements are missing:
<strong> <em> <code> <blockquote> <a href data-entity-type data-entity-uuid data-entity-substitution> <ul> <ol reversed start> <li> <img src alt height width data-entity-uuid data-entity-type data-caption data-align> <drupal-media data-entity-type data-entity-uuid alt data-caption data-align>

Of course, the field where I would add those elements back in is read-only...

With CKEditor 5 this is a read-only field. The allowed HTML tags and attributes are determined by the CKEditor 5 configuration. Manually removing tags would break enabled functionality, and any manually added tags would be removed by CKEditor 5 on render.

I do have source editing enabled (as well as the CodeMirror plugin), and everything worked without issue prior to updating core.

klemendev’s picture

In further tests, it seems this only happens if one defines HTML tags that already exist or variations of them with custom attributes or attribute values allowed. If a tag added here is a tag that is not added by any other plugin, it seems to work fine.

EDIT: This is only the case for some of the text formats on the website.

EDIT2: Ok so this only happens when both "Limit allowed HTML tags and correct faulty HTML" and "Source editing" is present. This is why Full HTML was not affected by this

matt_paz’s picture

Might this be a duplicate of #3410303: FilterHtml data loss when iframe and/or textarea is allowed? I noticed both Adrianm6254 and jesss have an iframe reference in their respective configs.

klemendev’s picture

Could be, I am also using iframe as the HTML tag allowed, alongside some div tags with custom classes.

However, the linked issue mentions data loss after saving, but in this issue I reported, data loss happens immediately upon update where certain tags no longer work.

jesss’s picture

From what I'm seeing on my site, there is no actual data loss. It's simply that when rendering the field, the text filter is stripping out tags that should be allowed. If you go to edit the content, all those "missing" tags are still there.

longwave’s picture

That issue is fixed in 10.2.1 (released today) so if you can still reproduce this after updating then this must be a separate issue. Multiple other regressions in 10.2.0 were fixed in 10.2.1 so this might be solved by one of those, too.

klemendev’s picture

Ok I have used a wrong term. I mean data loss on render, not in the database. Sorry for the confusion.

I will spin up local instance of our system and try it out and report back

klemendev’s picture

#3410303: FilterHtml data loss when iframe and/or textarea is allowed indeed fixes this issue

klemendev’s picture

Status: Active » Closed (duplicate)
joshuami’s picture

Status: Closed (duplicate) » Active

@KlemenDev, I don't feel this is a duplicate. The issue summary is not fixed by 3410303. The problem, for me and a couple others in this issue, is that you can't save a valid configuration for source editing after 10.2.

Unless we want to reopen issue #3396628 which I mention in comment 12, we still have a validation issue which forces resolution of errors on the text format creating a state that will cause data loss upon editing existing content.

For any element that has a valid plugin, the current validations requires you remove the element and its classes from source editing. This is a problem, because not every plugin covers every attribute that may be needed in an existing site.

This is currently a hard blocker for upgrading a site from CKEditor 4, which allowed pretty much any source editing pattern that you desired, to CKEditor 5, which now requires new plugin for every possible pattern in source editing.

joao.ramos.costa’s picture

Hello @joshuami, I have the same idea.
I still haven't seen clearly which tags the problem is most evident in, but as above refered I have <iframe> and other element types for instance <ul class>.
For now, in the concerning project I've detected the same issue, since we already use ckeditor5, I've implemented a hook_config_schema_info_alter() to temporarly remove the constraint mentioned on #12 and #13, starting from https://git.drupalcode.org/project/drupal/-/commit/ec24a58c6dd7e7d3a1ead.... But again, it's far from ideal.

I think that the solution you indicated in #12, in the final part, could work if it is fine-tuned.
For example, in my case, this constraint would make sense to be checked right after the CKEditor5Plugin Style, but before other plugins that enable the same tag, with a different but unspecified attribute (like class="").

ryan-l-robinson’s picture

I am encountering this error on 10.2.1, or at least a closely related one. I tried to remove allowing dl, dt, and dd tags after the 10.2.1 update, and I get this error:

The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: .

So it's not citing any plugins that are already supported and should not be added, but still throwing that error anyway.

Edit: I've narrowed it down to a few tags and tomorrow I'll test if they are indeed already allowed by other plugins. If that is true, it might be the case that showing me the error was accurate, but it would really help if it could have told me what the conflict was instead of trial and error testing.

Edit again: one that is left is a case where a module ckeditor_indentblock allows the element and some specific class values, but not other class values. Prior to the 10.2.1 update, I was able to specify to allow all class values. That now throws the error. So I can use the classes that are allowed by that module, but I don't seem to have any way to allow other class values again.

The other that is left I don't think ever worked anyway. It's the ckeditor_details and this configuration page throws an error if I try to allow class on details elements. Previously I could set it in that config to allow it, but then it never worked in practice anyway, so we didn't actually use it. In practice losing this one won't hurt us.

Short version: it may not be as big of an issue as some of the initial reports here, but there's still something going on, particularly when it comes to class and whether to allow all classes or only specific ones.

phil stringer’s picture

I've encountered a problem in 10.2.1 whereby and tags are always deleted by the editor but are present when viewing the node before an edit. My problem is fixed by disabling source editing.

joao.ramos.costa’s picture

Hi @Phil Stringer, indeed, should be related with SourceEditingRedundantTagsConstraintValidator.php as mentioned by #12.

johns996’s picture

I'm not sure if this is intended, but the new "source editing" box carried over all of the custom tags and styles from my drupal 9 (ck4) and 10.1 (ck5) upgrades. I rarely touch the text editor definitions on my sites but I recently encountered the error mentioned in #26 when trying to add a new class to the allowed list of tags. It was only then that I discovered there is no longer an allowed list of tags but, here's the strange thing, the tags that I previously defined and were migrated during the last two upgrades still seem to be working.

In my list of allowed tags I had the tag: <td class="w-100"> and that class would not get cleaned out when the page was edited as long as I didn't try to edit the text format. After I edited the format and got the #26 error I removed the tag and checked the page with the td and custom class. When I saved that page the class was removed from the <td>. So, I guess I'm not going to edit any of my text editors or I risk Ckeditor purging all of my custom classes and tags.

The only workaround I think will work is adding each of those custom classes to the list of custom styles. Adding custom styles like this puts the class into the read-only "Allowed HTML tags" box and stops the Ckeditor purge. The problem with this workaround is I used wildcard classes previously and those don't work as as custom styles.

joelpittet’s picture

To add to the cases:

We have these tags in our output (from D6 migration) and we want to preserve allowing them but they can’t be added to the Allowed Tags anymore (I'm ok with that actually, as long as I can find a way to add them back in somewhere) and are not accepted in the Source Code allowed tags because other buttons define them

<iframe src allowfullscreen width height frameborder>
<div class>
<ul type class>
<ol type class>
<img title class>
<table class>

We worked around this with a creative MODULE.ckeditor5.yml plugin definitions

MODULE_ckeditor5_allowHtml:
  ckeditor5:
    plugins: [htmlSupport.GeneralHtmlSupport]
    config:
      htmlSupport:
        allow:
          - name: table
            classes: true
          - name: ol
            classes: true
            attributes:
              - key: type
            ...ETC...
  drupal:
    label: Arbitrary HTML support
    elements: 
      - <table class>
      - <ol class type>
            ...ETC...
    conditions: [ckeditor5_table]

But that is because there were buttons and added iframe support with a module for that (but regretting that already)

joelpittet’s picture

StatusFileSize
new184.8 KB

Styles + Source error

The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (

).

In the above example, I don't want to allow editors to add classes to tables through Styles, but it prevents me from allowing the class attribute to preserve the classes added to tables manually in the source because it thinks that plugin can handle the use-case. But Styles will only allow specific classes to be added (and I don't want my editors to know they can do that)

joelpittet’s picture

While I don't recommend this module (read the project page near the end to see what this means) I am putting it here to show that it's a problem that people are trying to solve.
https://www.drupal.org/project/ckeditor5_allowed_html

mark_fullmer’s picture

Title: [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified » [10.1.6 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified

Updating the issue title to indicate that this was not introduced in 10.2, but rather 10.1.6.

joshuami’s picture

@mark_fullmer, can you point to the commit that introduced the bug for you? I was fairly certain it was introduced in 3396628, which added the validation that compared source editing to the currently enabled plugins—among some other changes related to lists.

I'm not able to reproduce the validation error in a site running 10.1.6 and CKEditor 5.

ericgsmith’s picture

#3410364: Change to edit source error handling in 10.2 leads required changes to configuration that cause data loss on edit was closed as a duplicate / merged into this issue, but I don't think the issue summary currently accurately describes to the same level of detail exactly what the issue is that multiple users are still reporting in this comments.

The original author of this issue closed it as resolved by #3410303: FilterHtml data loss when iframe and/or textarea is allowed so I think we potentially have a lot of confusion in this issue, potentially 2 unrelated issues.

Are there any objections to updating the issue summary to more clearly highlight what is reported in comment 12 onwards? That is to highlight that the issue is to do with the change in validation logic?

The alternative would be reopening the issue that was marked as duplicate, but there are a lot of comments in this issue that seem to being relating to that issue.

I also agree with above comment that this is a 10.2 regression and I'm not sure why the title was changed.

mark_fullmer’s picture

Title: [10.1.6 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified » [10.1.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified

Yep, y'all are right. I tested again and this was not present in 10.1.6. Changing the title back.

mark_fullmer’s picture

Title: [10.1.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified » [10.2 regression] CKEditor 5 breaks when "Source"/Source editing button is added and "Manually editable HTML tags" are specified
ericgsmith’s picture

For those needing to work around this issue here is a patch to revert the change that tightened the validation from #3396628: Fix <ol start> → native CKEditor 5 functionality and fix bug in SourceEditingRedundantTagsConstraintValidator that allowed it to slip by as noted comment #34 by @joshuami

Tests would fail as this doesn't revert the additional tests related to the ol start functionality which introduced this change.

Not expecting a review so leaving as active - this is just if people need to revert to the behaviour of Drupal 10.1.

pavelculacov’s picture

#38 +1

odensc’s picture

Comment #35 is exactly correct: there are two issues at play here.

There was a data loss issue that was fixed in #3410303: FilterHtml data loss when iframe and/or textarea is allowed, but there still exists a separate issue (fixed/reverted by the patch in comment #38), which is that saving a valid CKEditor "Source editing" configuration in the admin interface will cause a validation error. You can still technically edit it through drush, but it's not ideal.

jaydarnell’s picture

Patch 38 appears to work

pawel.traczynski’s picture

StatusFileSize
new65.42 KB

Preview of the problem

mkindred’s picture

I was running into the issue mentioned in #12, and the patch in #38 fixes it for now.

rick hood’s picture

Patch #38 works for me. I don't like patching core, but is that really the answer for now?

I created a Google Doc to explain exactly what was happening with me. If it's OK I link to it here:

https://docs.google.com/document/d/1H3p6rH-oi0kSfeqc79b-BbE6wLLOf83r8Eju...

If that is not OK I will put it all here, but it's a lot.

The main issue is this error when trying to save after switching to 5 (this is a Drupal 10.2.2 site):

The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (<ul class>).

editable html

In CKEditor 4 I have this in allowed HTML:

<a href hreflang data-entity-type data-entity-uuid target data-colorbox-inline name id class> <em> <strong> <cite> <blockquote cite> <code> <ul type class> <ol start type> <li> <dl> <dt> <dd> <h2 id style class> <h3 id style class> <h4 id style class> <h5 id style class> <h6 id style class> <s> <sup> <sub> <table id class> <caption> <tbody> <thead> <tfoot> <td colspan class> <tr colspan class> <hr> <p class="text-align-left text-align-center text-align-right text-align-justify inline"> <h1> <pre> <button> <img src alt height width data-entity-type data-entity-uuid data-align data-caption id class="image-30"> <drupal-media data-entity-type data-entity-uuid data-view-mode data-align data-caption alt title width height> <div id class> <br> <b> <i class> <u> <th colspan class> <span id class=" orange orange black gray gold teal pale-blue coboat red orange black gray gold teal pale-blue cobalt red"> <iframe src style frameborder allow allowfullscreen> <script src>

joao.ramos.costa’s picture

For those looking for an intermediate solution, instead of a rollback of the aforementioned commit, with a custom implementation as mentioned above:

/**
 * Implements hook_config_schema_info_alter().
 */
function MODULE_config_schema_info_alter(&$definitions) {
  if (isset($definitions['ckeditor5.plugin.ckeditor5_sourceEditing'])) {
    unset($definitions['ckeditor5.plugin.ckeditor5_sourceEditing']['mapping']['allowed_tags']['sequence']['constraints']['SourceEditingRedundantTags']);
  }
}
rick hood’s picture

StatusFileSize
new482.74 KB

This is a screenshot of what it looks like when I try to save a switch to CKEditor 5 from 4 on Basic HTML.

The following attribute(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: Style (<ul class>).

^ I am not understanding what that really means and what the fix is.

UPDATE: I just realized that this post described the exact problem I am having: https://www.drupal.org/forum/support/post-installation/2024-01-11/is-it-...

And in that post a comment says "But the patch, proposed in #38, can be started point for a solution."

ericgsmith’s picture

Issue summary: View changes

It seems we have consensus since I asked in #35 that some of the original issue details were related to the new resolved #3410303: FilterHtml data loss when iframe and/or textarea is allowed so I have update to include some of the use cases mentioned here and make it clearer (hopefully) what the recent comments have been.

I have added the propose resolution from one of the closed duplicates - while I'm in favor of reverting the validation, there hasn't been much discussion on making the validation "opt out" or perhaps just smarter about what is being prevented.

ericgsmith’s picture

ericgsmith’s picture

Issue summary: View changes
rick hood’s picture

StatusFileSize
new276.13 KB
mark_fullmer’s picture

Issue summary: View changes
mark_fullmer’s picture

Issue summary: View changes
mark_fullmer’s picture

I've updated the issue's steps to reproduce; previously, they did not capture all of the elements required to trigger this scenario. Specifically, the CKEditor 5 toolbar needs to be configured to use both the "Source" and the "Styles" plugins, and at least one style needs to be defined, and finally, one manually editable tag attribute needs to be added for an element that is already allowed by the CKEditor toolbar.

My take: if this is the new expectation of how the "Limit allowed HTML tags and correct faulty HTML" filter and the "Manually editable HTML tags" should interact, as of Drupal 10.2, then I think there are three potential resolutions:

1. Drupal core provides a setting to opt out of this locked behavior to accommodate the use cases mentioned in the issue description.
2. Sites with those use cases apply the equivalent of the alter hook in #45 to opt out themselves.
3. Sites use a different HTML limiter, such as https://www.drupal.org/project/htmlpurifier , in place of Drupal core's "Limit allowed HTML tags and correct faulty HTML" filter.

daddison’s picture

I confirm that the updated steps in the issue description reproduce the error.

rick hood’s picture

Also confirming that the updated steps in the issue description reproduce the error.

I very much appreciated the summary from mark_fulmer in #53 I am going with option 2 (#45) -- more testing to do but so far that seems to work great.

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

mingsong’s picture

Thanks @Mark Fullmer for the suggested solutions in #53.

I would vote for option 1. For a better user experience, in the error message where said

The following @element_type(s) can optionally be supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: %overlapping_tags.'

In that message, If we could provide a link to where an admin user who has the permission to ignore the constraint conflicts without coding/module required, that would give a better user experience.

mingsong’s picture

Apart from the proposed resolutions from #53, another one could be:

4. Accept the wildcard in the class attribute name, such as 'class*', for example, in the 'Manually editable HTML tags' box, if you put in <a class*> then all classes for an A tag will be allowed.

The MR 7887 introduces functionality to support the wildcard character of '*' in the class attribute names within the 'Manually editable HTML tags' setting.

If you need a patch for your local test, you can get the patch from

https://git.drupalcode.org/project/drupal/-/merge_requests/7887.patch

This MR is aiming for 11.x branch.

Mingsong changed the visibility of the branch 3410100-10.2-regression-ckeditor to hidden.

Mingsong changed the visibility of the branch 10.2.x to hidden.

mingsong’s picture

MR 7888 is aiming for 10.2.x branch.

The patch for 10.2.x

https://git.drupalcode.org/project/drupal/-/merge_requests/7888.patch

mingsong’s picture

Status: Active » Needs review

Those two MRs are ready for review.

jacobupal’s picture

I recently updated to version 10.2.6 from 10.1.something and I'm getting the error but with whitespace where it should tell me what the offending tag/attribute is, so I can't do anything to resolve it:
The following attribute(s) are already supported by enabled plugins and should not be added to the Source Editing "Manually editable HTML tags" field: . I don't know what's caused this, but it made me wish extra hard that I could switch off this annoying validation... therefore very thankful for #38, cheers for that!

mingsong’s picture

Attribute name missing in the error message is another problem. More details see

https://www.drupal.org/project/drupal/issues/3445375

alfattal’s picture

Status: Needs review » Needs work

I'm having the same issue as in #65. The patch in #63 did NOT solve the issue.

mingsong’s picture

#65 is different issue from this one.

I don't think we can fix all CKEditor 5 Source editing issues in one go.

ericgsmith’s picture

Status: Needs work » Needs review

Agreed, setting back to needs review.

alfattal’s picture

Status: Needs review » Needs work

My apologies, I overlooked comment #66

alfattal’s picture

Status: Needs work » Needs review
ethant’s picture

I had two issues:

  1. In Acquia site studio's page builder tool, the in place editor was throwing fatals / not saving
  2. Source code editing was being lost.

This pr resolves these issues on the site I'm working on, but ymmv.
My fork was way behind and I'm going to bed, so gonna punt on the resolution of conflicts for an mr. Uploading a patch file instead.

ethant’s picture

StatusFileSize
new13.43 KB
needs-review-queue-bot’s picture

Status: Needs review » Needs work
StatusFileSize
new88 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

cobasa’s picture

Version: 11.x-dev » 10.2.x-dev

Changed from version 11 to 10.2

quietone’s picture

Version: 10.2.x-dev » 11.x-dev

Fixes are made on on 11.x (our main development branch) first, and are then back ported as needed according to our policies. Also, 10.2 is in security mode now.

jacobupal’s picture

Just to note for those using ^10.3; #38 still seems to apply fine (10.3.1).

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

joegraduate’s picture

Status: Needs work » Needs review

Confirmed that the changes in MR !7887 fix the problem described in the steps to reproduce in the issue summary but only if <blockquote class*> is used for the "Manually editable tags" value in step 6 (instead of <blockquote class>).

b_sharpe’s picture

Status: Needs review » Needs work

Why only class? This applies to any attribute a plugin is defining. If a plugin defines <div id> you now can no longer add ID's to any source edited divs.

I believe we either need an optout of the constraint as suggested in #53 or we need to re-evaluate why this constraint is needed in the first place.

ericgsmith’s picture

Issue summary: View changes
Issue tags: -Needs issue summary update

Updating issue summary with the proposed options that have been discussed in the issue.

From what I can gather option 1 has the most support - I would like to get some form of feedback from ckeditor5 subsytem maintainers before progressing.

gordon’s picture

My client is having this issue.

Basically it is manifesting itself on the table element. Basically we have written a plugin for tables which allow us to add classes to the table element. This worked perfectly under D10.1, but then broken when we migrated to 10.2. The problem is that when we edit content with a table element, all the classes are stripped from the table. So before we save any content we need to re-apply the styles again.

I have tested all the patches and none of them are working.

#75 didn't really do anything. I had to reroll it so it would work with 10.2.6, but didn't resolve the issue.
#38 Fixed the issue when configuring the source editing, but didn't solve the issue with editing content that has tables with classes. It did resolve the issue of fixing up the classes in the filter html

I also tried MR!7888 but this didn't do anything as well, but then again this MR doesn't really change much.

eduardo morales alberti’s picture

eduardo morales alberti’s picture

As workaround we defined the style tag on another core plugin:

ckeditor5_removeFormat:
  ckeditor5:
    plugins: [removeFormat.RemoveFormat]
  drupal:
    label: Remove Format
    library: core/ckeditor5.removeFormat
    admin_library: ckeditor5/internal.admin.removeFormat
    toolbar_items:
      removeFormat:
        label: Remove Format
    elements:
     - <span>
chrisgross’s picture

I am running into this, too, and have seen it for a long time now. If this isn't easily fixable, shouldn't core maintainers roll back the commit that caused this problem? This is a majorly critical issue because it makes it impossible to edit text formats, which is, needless to say, really important.

amy_farrell’s picture

Here's another use case: We want to allow our editors to add "data-*" attributes to divs when in source editing mode. We use the uswds_ckeditor_integration module, which provides several editor plugins, of which we use several, but not the "USWDS Grid" plugin, which happens to specify "div class data-*" in its "elements" list.
I also like "option 1" in the current description.

lauriii’s picture

Here's how I understand this issue. It seems that this problem is specific to the class attribute. It seems that the root cause to the problem is that ckeditor5_style defines <$any-html5-element class> as it's elements, which then triggers the validation if anyone tries to add a class attribute to a HTML5 element, because theoretically, they could add that using the style plugin. The plugin documents that the class is dynamically restricted to a subset of class attributes, but this is not taken into account in the validation.

Requiring the use of the style plugin might be problematic because it a) requires someone to know and specify every class that may be used and it b) exposes them through the user interface (which might not be always desired). Because of this, people are specifying class as an attribute in the source editor plugin.

If this is a somewhat correct understanding of the problem, it seems that an ideal solution would be to special case the class attribute on the style plugin somehow. This way we could allow adding class attribute to elements even though it's an attribute that's supported by the style plugin.

It seems that #59 is proposing something pretty close to this. However, it seems to be adding a new syntax for this. I'm not sure that we need a new syntax for this; in my mind we would just special case the class attribute to not trigger this validation.

joshuami’s picture

@lauriii, while the patch in #59 is focused on class and the style widget, the text-format-save blocking error can occur for any attribute that is defined by a module and in source editing. It could be an id or a data-attribute if there is a module that defines those attributes for the element in question.

The validation introduced in 3396628 triggers on any "overlap". While the class issue is probably the most common, it's not the only scenario that can cause that error to throw and prevent the text format from being saved.

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

morvaim changed the visibility of the branch 10.4.x to hidden.

morvaim’s picture

Status: Needs work » Needs review
morvaim’s picture

The validation is indeed triggered on any "overlap" but is that a problem? If an attribute or tag is already enabled by another plugin then that attribute or tag can be used in Source editing mode when editing a content. So it can make sense to not add it as a duplicate to the "Manually editable HTML tags" list if it is already allowed and can be used. Class attribute is an exception in this regard because although the Style plugin adds it in its definition as an allowed attribute, in reality it is not allowed but only those specific classes are allowed which are added to the "Styles" list. This is why it is not "valid" to make the validation fail when someone tries to add the class attribute to a tag in "Manually editable HTML tags" in my opinion.

morvaim’s picture

odensc’s picture

@morvaim: The issue is in my case at least, is that I'm trying to make an attribute more permissive than the CKEditor plugin allows, so it needs to be added to "Manually editable HTML tags".

I have a custom text filter that handles some custom attributes that can be set in data-align. This issue restricts that field to only the values that the CKEditor media plugin allows (left, right, etc). So if I want to allow additional values, I'd have to create a dummy CKEditor plugin even if I don't need to do anything client-side.

alfattal’s picture

I second the suggestion in #87. Also, the patch in #97 didn't work.

ericgsmith’s picture

It seems that this problem is specific to the class attribute

This is the most common example but this problem is more generic and there have been multiple comments with examples of this. I feel like special handling for class attribute is going to help maybe 95% of the people in this issue, but there is not reason why we can't help everybody.

I have previously voiced support for reverting in #47 and still hold that opinion.

smustgrave’s picture

Status: Needs review » Needs work

Sorry to send this one back but summary mentions 3 options but which was chosen?

There are 4 MRs up, we should focus on 11,x first.

Also not sure there is test coverage.

morvaim’s picture

Sorry, I closed the 10.4 MR, I wasn't aware of the process.
And yeah, I see now that this can affect other attributes too but I'm not quite sure that reverting the stricter validation should be the way to go, because the need for that seems also valid for me.

eduardo morales alberti’s picture

Workaround:

/**
 * Implements hook_ckeditor5_plugin_info_alter().
 */
function my_module_forms_ckeditor5_plugin_info_alter(array &$plugin_definitions): void {
  // Adding span to remove format plugin to avoid error reported on issue
  // https://www.drupal.org/project/drupal/issues/3449576.
  if ($plugin_definitions['ckeditor5_removeFormat'] instanceof CKEditor5PluginDefinition) {
    $removeFormat_plugin_definition = $plugin_definitions['ckeditor5_removeFormat']->toArray();
    $removeFormat_plugin_definition['drupal']['elements'] = [
      '<span>',
    ];
    $plugin_definitions['ckeditor5_removeFormat'] = new CKEditor5PluginDefinition($removeFormat_plugin_definition);
  }
}
pere orga’s picture

Priority: Critical » Major

I'm not sure if this is critical, setting it to major for now.

In my case, I noticed the Linkit profile was disabled in the text format. This may have happened ages ago, maybe when I updated Drupal 9 to Drupal 10, but my user (yes, that website has a single user, and by the way, he is 83yr old) didn't notice or didn't tell me. I experienced this issue when trying to re-enable it, saving the text format.

I haven't read the workarounds described in this issue, but this is what worked for me:*

  1. Disable "Limit allowed HTML tags and correct Faulty HTML"
  2. Enable it again

This, alongside enabling Linkit, produced the following diff after exporting the site configuration:

diff --git a/config/sync/editor.editor.basic_html.yml b/config/sync/editor.editor.basic_html.yml
index c91277c28..070d8825b 100644
--- a/config/sync/editor.editor.basic_html.yml
+++ b/config/sync/editor.editor.basic_html.yml
@@ -56,7 +56,7 @@ settings:
         - '<i>'
         - '<b>'
         - '<span>'
-        - '<img src alt height width data-entity-type data-entity-uuid data-align data-caption>'
+        - '<img data-entity-type data-entity-uuid data-align data-caption>'
         - '<blockquote cite>'
         - '<ul type>'
         - '<ol type>'
@@ -65,7 +65,7 @@ settings:
         - '<h4 id>'
         - '<h5 id>'
         - '<h6 id>'
-        - '<a data-entity-substitution data-entity-type data-entity-uuid title>'
+        - '<a title>'
     ckeditor5_style:
       styles:
         -
@@ -75,6 +75,7 @@ settings:
           label: Versaleta
           element: '<span class="versaleta">'
     linkit_extension:
-      linkit_enabled: false
+      linkit_enabled: true
+      linkit_profile: default
 image_upload:
   status: false
diff --git a/config/sync/filter.format.basic_html.yml b/config/sync/filter.format.basic_html.yml
index 199a11d71..5a955e5c5 100644
--- a/config/sync/filter.format.basic_html.yml
+++ b/config/sync/filter.format.basic_html.yml
@@ -42,7 +42,7 @@ filters:
     status: true
     weight: -50
     settings:
-      allowed_html: '<br> <p class="nota text-align-left text-align-center text-align-right text-align-justify"> <h2 id class="text-align-left text-align-center text-align-right text-align-justify"> <h3 id class="text-align-left text-align-center text-align-right text-align-justify"> <h4 id class="text-align-left text-align-center text-align-right text-align-justify"> <h5 id class="text-align-left text-align-center text-align-right text-align-justify"> <h6 id class="text-align-left text-align-center text-align-right text-align-justify"> <span class="versaleta"> <cite> <dl> <dt> <dd> <i> <b> <img src alt height width data-entity-type data-entity-uuid data-align data-caption> <blockquote cite> <ul type> <ol type start> <a data-entity-substitution data-entity-type data-entity-uuid title href> <strong> <em> <u> <sup> <li>'
+      allowed_html: '<br> <p class="nota text-align-left text-align-center text-align-right text-align-justify"> <h2 id class="text-align-left text-align-center text-align-right text-align-justify"> <h3 id class="text-align-left text-align-center text-align-right text-align-justify"> <h4 id class="text-align-left text-align-center text-align-right text-align-justify"> <h5 id class="text-align-left text-align-center text-align-right text-align-justify"> <h6 id class="text-align-left text-align-center text-align-right text-align-justify"> <span class="versaleta"> <cite> <dl> <dt> <dd> <i> <b> <img data-entity-type data-entity-uuid data-align data-caption src alt height width> <blockquote cite> <ul type> <ol type start> <a title href data-entity-type data-entity-uuid data-entity-substitution> <strong> <em> <u> <sup> <li>'
       filter_html_help: false
       filter_html_nofollow: false
   filter_html_escape:

*It may be too early to be sure nothing else broke after applying that workaround.

aherczeg’s picture

We encountered the issue with a custom plugin that adds a class to selected text, in our case the patch from #97 solved it, currently on Drupal core 10.4.7.

joegraduate’s picture

StatusFileSize
new6.81 KB

Re-rolled @ericgsmith's patch from #38 into a new MR targeting 11.x.

Also attaching MR diff as a static patch that should be usable with Drupal 11.2.x.

Leaving status as "Needs work" since I'm not sure whether reverting the validation changes added in 10.2.x is still being considered as an option for resolving this issue or not.

tichris59’s picture

Hi @joegraduate, your patch works like a charm in drupal 11.2.x thanks

Version: 11.x-dev » main

Drupal core is now using the main branch as the primary development branch. New developments and disruptive changes should now be targeted to the main branch.

Read more in the announcement.