Problem/Motivation
Follow-up of #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style, to fix the edge case reported by @DanielVeza at #3222797-97: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style.
When using the Styles and the sourceEditing plugins there can be situation with conflicting configuration.
The main idea with CKE5 config is to not have two plugins with overlaping supported elements.
Steps to reproduce
- Set up a CKE5 text format with a Styles config of
span.test|Test - and a sourceEditing config of
<span class> - An error message is shown:
A style must only specify classes not supported by other plugins. The test classes on
<span>are already supported by the enabled Source editing plugin.
Proposed resolution
Discuss what needs to happen in that situation.
- Do nothing, since the user is expected to use sourceEditing to alter the span classes, the styles plugin shouldn't be able to touch those.
- It can make sense to have the Styles plugin add classes to an element for less technical users, while still allowing the source editing plugin to allow all classes for more technical users. (Removing the validation constraints, it's working as expected in the UI). It can introduce a grey area where unexpected behaviors happens though.
In that case the resulting Allowed HTML tags will be as followed
<span class>
Remaining tasks
Agree on a solution
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #22 | core-3294908-22.patch | 9.73 KB | wim leers |
| #22 | interdiff.txt | 4.66 KB | wim leers |
| #10 | interdiff-9-10.txt | 2.08 KB | nod_ |
| #10 | core-3294908-10.patch | 8.44 KB | nod_ |
| #10 | core-3294908-10-TEST-ONLY.patch | 6.41 KB | nod_ |
Comments
Comment #2
nod_Comment #3
nod_Comment #4
nod_no test yet but seems to be working.
When a plugin declare support for class without value restriction, make sure the styles plugin can add classes to the same tag without problem.
code is not pretty and docs needs work but putting as NR if someone is inpired to take a look and improve this before I get back to it next week.
Comment #5
nod_Added some tests
Comment #6
nod_Removed that because it ends up being 2 different plugins that supports the same tag, and we have a "reset" after that makes sure we only take one.
Not sure how to fix the assert in this case.
Comment #7
nod_Comment #8
nod_Comment #9
nod_Comment #10
nod_Comment #16
nod_all good now :)
Comment #17
wim leersComment #18
xjmComment #20
wim leersSo this is to fix the edge case reported by @DanielVeza at #3222797-97: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style. Credited them!
Comment #21
wim leers🤔 Removing this assertion is a significant change: it means that multiple plugins may be adding support for a particular tag.
We should not need this part.
👍
👍
👍 … but (!) this is only testing the case of a concrete plugin supporting a particular element, it's not testing the case of a
SourceEditing-provided element.I think we need more test coverage.
If it weren't for that one line and the missing extra test coverage, I'd RTBC 😊
Comment #22
wim leersThis addresses #21.1 (a single line revert) and #21.4 (more test cases).
Comment #23
wim leersNote: the single line revert is trivial, and so are the test cases. Explaining the test cases was more difficult than writing them. So if @nod_ +1s the added test cases, I'm comfortable RTBC'ing.
Comment #24
nod_ah yes, since I changed the element in the test it doesn't fail anymore
Comment #25
nod_new tests make sense, thanks for adding those :)
Comment #29
bnjmnmI was pretty worried about #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style going in with this edge case happening, but it was also a bad idea to expand the scope of that issue further. It happens to make this solution one of the easier CKEditor 5 patches I've reviewed. Fix makes sense, as do the tests.
The fact that this problem was happening indirectly showcases how robust validation is in the CKeditor 5 modules. For every hassle such as this one, we've been spared a dozen larger ones.
Committed to 10.1.x and cherry picked to 10.0.x and 9.5.x. Good fix folks!
Comment #30
danielvezaAmazing team! ❤️
Comment #31
eric_a commentedThat issue went into 9.4.x. Should this one as well?
Comment #33
wim leersThe fix should indeed still be backported to
9.4.x, @Eric_A!Will provide a patch. Currently not cleanly cherry-pickable. So determining what the optimal cherry-pick order is …
Comment #34
wim leers#22 still applies cleanly to
9.4.xtoo! Test queued 👍Comment #36
lauriiiCommitted b7b5e1a and pushed to 9.4.x. Thanks!
Comment #37
wim leersRestoring previous status.
Comment #38
jksloan2974 commentedI am running php 8.1.14 and Drupal 9.5.3. I am still unable to add a span with class in the styles plugin.
Here is the error message after trying to add span.test_class|Test Class to the Styles configure textarea
Error message
The Style plugin needs another plugin to create , for it to be able to create the following attributes: . Enable a plugin that supports creating this tag. If none exists, you can configure the Source Editing plugin to support it.
Comment #39
bnjmnm@jksloan2974 I recommend bringing up your concern in #38 in Drupal Slack or opening a new issue describing the steps to reproduce. Issues are rarely re-activated, and in those rare instances it's within a few days of it being completed. Since it's been 5 months, this will not re-open to address your concerns, but if it were a new issue (or question in Slack), it can get some attention.