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.
Comment | File | Size | Author |
---|---|---|---|
#37 | ckeditor5--basic-html.png | 9.51 KB | ifrik |
#37 | ckeditor--basic-html.png | 8.2 KB | ifrik |
#33 | before.gif | 954.55 KB | Wim Leers |
#33 | after.gif | 917.46 KB | Wim Leers |
#32 | Screenshot 2022-05-31 at 20.11.07.png | 187.8 KB | Wim Leers |
Issue fork drupal-3273983
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
ifrikAs discussed: the Language button is added as a way to allow for the span tag, so there might be another way around that.
Comment #3
Wim LeersDiscussed with @ifrik. I know what to do next. Will expand tomorrow.
Comment #4
Wim LeersSorry for the delay! Here's the promised write-up based on discussing this with @ifrik at DDD Ghent.
Many sites have
<span>
in theirfilter_html
configuration. The only way for CKEditor 5 to meet that goal is to enableckeditor5_language
. So that's whatSmartDefaultSettings
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>
, thenSmartDefaultSettings
should not try to partially fulfill the need by enabling a plugin that allows<span bar>
or<div bar>
to be created.Marking
to gather input from fellow maintainers.Comment #5
lauriiiBased 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 triggeredSourceEditingRedundantTags
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. OnSourceEditingRedundantTags
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.Comment #6
Wim LeersInteresting proposal! I think that makes sense.
That would mean that for example
ckeditor5_link
would have to change fromto:
and
ckeditor5_table
fromto
That looks a bit odd, but not unreasonable.
Comment #7
Wim LeersWould this mean that
SmartDefaultSettings
would do BOTH:SourceEditing
plugin and configure it to allow<div>
tag<div foo>
, to add support for thefoo
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.Comment #8
lauriii#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?Comment #9
Wim LeersWhile 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? 🤞
Comment #10
Wim LeersPer #3276207-4: SourceEditingRedundantTagsConstraintValidator logic handles attribute supersets incorrectly and per @ifrik in Drupal Slack: bumping to stable blocker.
Comment #11
Wim LeersRetitling per #8.
Comment #13
Wim LeersComment #14
Wim LeersComment #15
Wim Leers#3231328: SmartDefaultSettings should select the CKE5 plugin that minimizes creation of HTML restriction supersets touched on a related area.
Comment #17
Wim Leersfaa9df5d is identical to the patch in #14. The MR is now in sync with the patch. All next steps will happen in the MR.
Comment #18
Wim Leers0177e520 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. 👍
Comment #19
Wim Leers0dc9f848 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. 👍
Comment #20
Wim Leers0bbb4f8107 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. 🤞Comment #21
Wim Leers0bb4f81 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: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!SmartDefaultSettings
kernel test passes again! 🤓Comment #22
Wim Leers🥳 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.)Comment #23
Wim Leers🤬 I hate it when I carefully curate commits and then
phpcs
screws it up … especially if it does not complain locally.Comment #24
Wim LeersWhew, that was tricky to sort out!
The failure was on
One expected entry was missing:
Root cause: I missed an edge case in https://git.drupalcode.org/project/drupal/-/merge_requests/2310/diffs?co....
Fixed in 4a8a61781b.
Comment #25
Wim LeersMy fix in 4a8a6178 was wrong, because there is a number of new failures. Explicit test coverage added in 6b58fddff9. Will continue on Monday.
Comment #26
Wim LeersThe failures in b2eed581d851f2a58104ecebff7216bf7f79c6e9 indicate our validation logic needs to be updated too:
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.)Comment #27
Wim Leers#3274651: Impossible to enable <ol type> or <ul type> with GHS: switch to List's successor, DocumentList conflicted. Resolved.
Comment #28
Wim Leersd2be0cac7b7b76a30c3ea14329e9dd09fce3c10e expanded validation logic. This caused many more failures. One concrete example:
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:→ i.e. the
<drupal-media>
tag is not being returned byMedia::getElementsSubset()
. 2ee82ada24db05bbd09ff8c80663eae237c21c91 fixes that. 👍Comment #29
Wim Leers8bbcc46 introduced many new failures. Even though the code is definitely correct.
They're all attributable to errors like this:
… 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. 🥳
Comment #30
Wim LeersPer #29, this is now postponed on #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers: the commit which added that patch solved many failures. 👍
Comment #31
Wim LeersThose 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
.Comment #32
Wim LeersComment #33
Wim LeersThat is green 😊
Improved issue summary. Added change record. Added release note.
Pinging @ifrik for a review 😊
Comment #34
Wim Leers#3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers is in! Reverted that commit, unpostponing 👍
Comment #35
Wim LeersThe follow-up @lauriii requested: #3283776: Make CKEditor5PluginDefinition::getElements() consistent with CKEditor5PluginDefinition::get*().
Comment #36
lauriiiThank you! 🙏
All (including the draft CR) looks good for me now!
Comment #37
ifrikI'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
Comment #38
bnjmnmSome 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).
Comment #39
Wim LeersGood feedback! 🙏👏
Will address on Monday 😊
Comment #40
Wim LeersTo 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.
Comment #45
bnjmnmEverything appears to be addressed. Committed to 10.0.x and cherry-picked to 9.5.x 9.4.x 9.3.x
Comment #46
Wim LeersThis unblocked #3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style 🥳
Comment #47
quietone CreditAttribution: quietone at PreviousNext commented