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
| Comment | File | Size | Author |
|---|---|---|---|
| #108 | 3410100-108-revert-3396628-11-x.patch | 6.81 KB | joegraduate |
| #97 | ckeditor5_enable_class_attribute-3410100-97.patch | 1.12 KB | morvaim |
| #76 | 3410100-nr-bot.txt | 88 bytes | needs-review-queue-bot |
| #75 | fix-ckeditor5.patch | 13.43 KB | ethant |
| #50 | editable_html.jpg | 276.13 KB | rick hood |
Issue fork drupal-3410100
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
Comment #2
klemendev commentedComment #3
joaquinve commentedHello,
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.
Comment #8
wim leersMerging #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.
Comment #9
bsnodgrass commentedAdded another related possibly duplicate issue
Comment #10
wim leersInteresting! #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.
Comment #11
adrianm6254 commented@Wim Leers you asked for a couple of files from my other issue,
https://www.drupal.org/project/drupal/issues/3410380so 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).Comment #12
joshuami@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 affectsspanis 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.
Comment #13
joao.ramos.costa commented@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.Comment #14
damienmckennaComment #15
jesss commentedComing 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:
Of course, the field where I would add those elements back in is read-only...
I do have source editing enabled (as well as the CodeMirror plugin), and everything worked without issue prior to updating core.
Comment #16
klemendev commentedIn 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
Comment #17
matt_paz commentedMight 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.
Comment #18
klemendev commentedCould 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.
Comment #19
jesss commentedFrom 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.
Comment #20
longwaveThat 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.
Comment #21
klemendev commentedOk 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
Comment #22
klemendev commented#3410303: FilterHtml data loss when iframe and/or textarea is allowed indeed fixes this issue
Comment #23
klemendev commentedComment #24
joshuami@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.
Comment #25
joao.ramos.costa commentedHello @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="").
Comment #26
ryan-l-robinson commentedI 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:
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.
Comment #27
phil stringer commentedI'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.
Comment #28
joao.ramos.costa commentedHi @Phil Stringer, indeed, should be related with
SourceEditingRedundantTagsConstraintValidator.phpas mentioned by #12.Comment #29
johns996 commentedI'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.
Comment #30
joelpittetTo 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
We worked around this with a creative MODULE.ckeditor5.yml plugin definitions
But that is because there were buttons and added iframe support with a module for that (but regretting that already)
Comment #31
joelpittetComment #32
joelpittetWhile 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
Comment #33
mark_fullmerUpdating the issue title to indicate that this was not introduced in 10.2, but rather 10.1.6.
Comment #34
joshuami@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.
Comment #35
ericgsmith commented#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.
Comment #36
mark_fullmerYep, y'all are right. I tested again and this was not present in 10.1.6. Changing the title back.
Comment #37
mark_fullmerComment #38
ericgsmith commentedFor 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.
Comment #39
pavelculacov commented#38 +1
Comment #40
odenscComment #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.
Comment #41
jaydarnellPatch 38 appears to work
Comment #42
pawel.traczynski commentedComment #43
mkindred commentedI was running into the issue mentioned in #12, and the patch in #38 fixes it for now.
Comment #44
rick hood commentedPatch #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>).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>Comment #45
joao.ramos.costa commentedFor those looking for an intermediate solution, instead of a rollback of the aforementioned commit, with a custom implementation as mentioned above:
Comment #46
rick hood commentedThis is a screenshot of what it looks like when I try to save a switch to CKEditor 5 from 4 on Basic HTML.
^ 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."
Comment #47
ericgsmith commentedIt 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.
Comment #48
ericgsmith commentedAdding another potential use case - https://www.drupal.org/project/drupal/issues/3274635#comment-15457719
Comment #49
ericgsmith commentedComment #50
rick hood commentedComment #51
mark_fullmerComment #52
mark_fullmerComment #53
mark_fullmerI'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.
Comment #54
daddison commentedI confirm that the updated steps in the issue description reproduce the error.
Comment #55
rick hood commentedAlso 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.
Comment #57
mingsongThanks @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
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.
Comment #59
mingsongApart 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.
Comment #63
mingsongMR 7888 is aiming for 10.2.x branch.
The patch for 10.2.x
https://git.drupalcode.org/project/drupal/-/merge_requests/7888.patch
Comment #64
mingsongThose two MRs are ready for review.
Comment #65
jacobupal commentedI 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!Comment #66
mingsongAttribute name missing in the error message is another problem. More details see
https://www.drupal.org/project/drupal/issues/3445375
Comment #67
alfattal commentedI'm having the same issue as in #65. The patch in #63 did NOT solve the issue.
Comment #68
mingsong#65 is different issue from this one.
I don't think we can fix all CKEditor 5 Source editing issues in one go.
Comment #69
ericgsmith commentedAgreed, setting back to needs review.
Comment #70
alfattal commentedMy apologies, I overlooked comment #66
Comment #71
alfattal commentedComment #72
ethantI had two issues:
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.
Comment #75
ethantComment #76
needs-review-queue-bot commentedThe 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.
Comment #77
cobasa commentedChanged from version 11 to 10.2
Comment #78
quietone commentedFixes 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.
Comment #79
jacobupal commentedJust to note for those using ^10.3; #38 still seems to apply fine (10.3.1).
Comment #81
joegraduateConfirmed 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>).Comment #82
b_sharpe commentedWhy 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.
Comment #83
ericgsmith commentedUpdating 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.
Comment #84
gordon commentedMy 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.
Comment #85
eduardo morales albertiComment #86
eduardo morales albertiAs workaround we defined the style tag on another core plugin:
Comment #87
chrisgross commentedI 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.
Comment #88
amy_farrell commentedHere'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.
Comment #89
lauriiiHere'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_styledefines<$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 theclassis 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.
Comment #90
joshuami@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
idor adata-attributeif 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.
Comment #95
morvaim commentedComment #96
morvaim commentedThe 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.
Comment #97
morvaim commentedComment #98
odensc@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.Comment #99
alfattal commentedI second the suggestion in #87. Also, the patch in #97 didn't work.
Comment #100
ericgsmith commentedThis 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.
Comment #101
smustgrave commentedSorry 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.
Comment #103
morvaim commentedSorry, 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.
Comment #104
eduardo morales albertiWorkaround:
Comment #105
pere orgaI'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:*
This, alongside enabling Linkit, produced the following diff after exporting the site configuration:
*It may be too early to be sure nothing else broke after applying that workaround.
Comment #106
aherczeg commentedWe 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.
Comment #108
joegraduateRe-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.
Comment #109
tichris59 commentedHi @joegraduate, your patch works like a charm in drupal 11.2.x thanks