Problem/Motivation
Drupal 8/9 ship with CKEditor 4's StylesCombo
plugin/feature: https://ckeditor.com/docs/ckeditor4/latest/features/styles.html
This shipped with the original CKEditor 4 issue: #1890502: WYSIWYG: Add CKEditor module to core. It provides a configuration UI:
When talking to the CKEditor developers:
We didn’t work on a replacement yet. We’re not even sure whether it’s needed and what’d be the use cases.
There’s a very old ticket: https://github.com/ckeditor/ckeditor5/issues/648 where we started some discussion. There’s also https://github.com/ckeditor/ckeditor5/issues/5700. But in general it’s unclear for us what would this feature be for.What use cases do you see for it?
My response:
Well … how do we provide an automatic upgrade path in Drupal? 🤔
StylesCombo in CKE4 can only be mapped to
heading.Heading
in CKE5 as long as it’s only touching<h*>
— butStylesCombo
allowed much more. How do we automatically take any arbitraryStylesCombo
configuration and make it work in CKE5?It’s fine if it maps to multiple plugins but we do need to provide an upgrade path.
To which they responded:
Yeah, from the upgrade path perspective, there’s certainly a clear reason to do something on our side. What was blocking us so far is that we don’t want to create again a feature that duplicates a big part of the headings dropdown functionality. Thus, product-wise, it’s tricky.
Proposed resolution
Wait for https://github.com/ckeditor/ckeditor5/pull/11349 to ship with the April 6 release of CKEditor 5.
It shipped: #19. 👍
Remaining tasks
- ✅
add@ckeditor/ckeditor5-style
package - ✅
basic functionality - ✅
TEST: unit - ✅
TEST: kernel:SmartDefaultSettingsTest
expectations should be updated - ✅
TEST: functional JS for plugin settings - ✅
TEST: functional JS for using the plugin - ✅
Upgrade path - ✅
Additional validation constraints to ensure config always is valid - ✅
TEST: kernel: new test cases in:ValidatorsTest
Validator test coverage, which conveys just how precise validation is, which is extra important because as described in #46, the CKEditor 5 Style plugin itself does not do any validation:INVALID: Style plugin with no styles
→ when no styles are configured, having the toolbar item enabled is pointless (this is the only one already passing because it's the only one already implemented)INVALID: Style plugin configured to add class to unsupported tag
→ it does not make sense to allow adding aclass
to a tag that cannot actually be generated!INVALID: Style plugin configured to add class that is supported by a disabled plugin
→ it does not make sense to allow adding specific classes through a Style that can be supported by enabling a disabled plugin (for example "Alignment")INVALID: Style plugin configured to add class that is supported by an enabled plugin
→ it does not make sense to allow adding specific classes through a Style that are already supported by another plugin (for example "Alignment")INVALID: Style plugin has multiple styles with same label
→ without unique labels, it'd have terrible usability and accessibilityINVALID: Style plugin has styles with invalid elements
→ not possible to configure this through the UI, but … it is possible if you modify the configuration directly.VALID: Style plugin has multiple styles with different labels
→ the sole example of a sane configuration, and hence the only not resulting in a validation error
Follow-up to discuss configuration validation restrictions rules between Styles and Source Editing: #3294908: Configuration overlaps between Styles and other CKE5 plugins
User interface changes
New plugin settings form.
API changes
None.
Data model changes
None.
Release notes
The CKEditor 5 successor of CKEditor 4's StylesCombo
functionality has been added. This was the last remaining widely adopted functionality of CKEditor 4 that did not yet have an equivalent! When upgrading from CKEditor 4 editor, the configuration will be migrated automatically.
Comment | File | Size | Author |
---|---|---|---|
#119 | 3222797-style-101.patch | 158.36 KB | bnjmnm |
| |||
#119 | 3222797-style--94x.patch | 158.63 KB | bnjmnm |
#119 | 3222797-style--95x.patch | 158.41 KB | bnjmnm |
#115 | core-style-3222797-115.patch | 143.21 KB | nod_ |
#102 | Schermafbeelding 2022-07-01 om 11.42.47.png | 50.88 KB | mpp |
Issue fork drupal-3222797
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 #3
Wim Leers@reinmar said we'll discuss it next week; he'll gather more information as it’s a long time since they discussed this.
Comment #4
Wim LeersComment #5
Wim LeersDiscussed during today's meeting.
@Reinmar said that other than the upgrade path the StylesCombo plugin would be completely useless in CKE5. In CKE4 we had both “Format” (now “Heading”) and “StylesCombo”, with lots of overlap. It was always stupid to configure CKE to have both.
The fact that Drupal limits it to just classes makes an upgrade path more feasible. Needs more thought/investigation.
Today people use CKE5’s “Heading” dropdown as an alternative to CKE4’s StylesCombo, but that was never the intention.
Since we are being told we should not use the
dropdown as a partial successor and the proper solution is TBD, explicitly marking this as needing an upstream feature.Comment #6
Luke.LeberFollowing up from a slack conversation with @bnjmnm. I'm in agreement with Reinmar in that the styles combo plugin has always felt like an afterthought. I feel that there are some things wrong with it.
There's a confusion of responsibility. It's possible to change the semantic element type through something that's clearly labeled as a Style. This blends semantics with presentation.
Headers are especially troublesome in this area because sequential ordering is very important. This important accessibility topic is sometimes in direct opposition to design requirements. Who knows what the correct heading level will be for any given spot where a WYSIWYG might be used? Not only is it currently difficult to reconcile the two needs, but it also leads to my second point.
It's very hard to implement flexible block level elements for an accessible design system without introducing internal
SPAN
elements. Take the following image for example.This is obviously an exaggeration that we've been able to work around by limiting styles (which sometimes also limits design options...). Conversely, editors like TinyMCE can effectively gray out or even remove style options that don't make sense given the current context. IE, if one is editing an
H2
element, then styles applicable toblockquote
elements wouldn't be selectable. Wouldn't it be better if the user was only presented with the style options that make sense given what they've currently selected?Would it be possible to tap into contextual bubble toolbars to provide users with a more streamlined and easy to use interface? We can already see a precedence for this in the CKEditor5 image plugin. I feel that this could be the best UX for styling block level elements with CKEditor5. Only showing users the options that make sense, when they make sense.
I'd love to discuss this further, so feel free to reach out with any questions or comments.
Thanks!
Comment #7
Wim Leers@Luke.Leber: wow! Thanks for that thoughtful write-up! I've just pointed @Reinmar to it :)
Comment #8
Wim LeersFYI: Development for the CKEditor 5 successor will start in ~2 months at the earliest.
Comment #9
Wim LeersPer #3231364-57: Add CKEditor 5 module to Drupal core: work on this will begin soon upstream! 🥳
Comment #10
Wim LeersComment #11
Wim LeersComment #13
esch CreditAttribution: esch commentedIf the StylesCombo plugin is not going to be migrated and/or there is no upgrade path, how will editors add CSS class to HTML tags?
Are we now expecting editors to go into source view and fat-finger in CSS class on the correct HTML tags?
Is the expectation to have the Drupal community contribute a CK5 plugin for this, even though it is a default on the current CK4?
I would just like to know how CSS classes will be managed by editors when this is a part of Drupal 10. Will we need to train our editors how to use source view to add classes in the proper areas/tags?
Any insights would be much appreciated as I'm trying to figure out we are going to handle this major editor experience change.
Cheers.
Comment #14
Wim LeersWe're not expecting that. See above.
No.
Definitely not! 🙈 We absolutely do not want that 😱
It's not a change — it's just not yet available.
I'll answer this very explicitly and clearly: Without this feature available, we will not be able to mark the CKEditor 5 module stable. (That's what the
tag means, but I totally understand that just the presence of that tag does not quite make you feel comfortable. Hopefully that phrasing that explains the meaning of the tag puts your mind at ease!)We're meeting with the CKEditor 5 team later today.
Comment #15
esch CreditAttribution: esch commentedCheers @wim-leers,
Thanks for explaining the current status of this issue.
Comment #16
Wim LeersI realize I forgot to cross-post to this issue what I posted to the
#ckeditor5
Drupal Slack channel on February 28:The upstream WIP can be seen at https://github.com/ckeditor/ckeditor5/pull/11349 😊
Best news yet: it's going to ship in the upcoming release of CKEditor 5, on April 6: https://github.com/ckeditor/ckeditor5/issues/11387!
Comment #17
mpp CreditAttribution: mpp at AmeXio for District09 commentedA use case we have also supports translations for the styles:
Comment #18
Wim Leers#17: that should be just a matter of Configuration Translation, just like for the CKEditor 4 StylesCombo plugin.
Comment #19
Wim Leers#3274767: Update to CKEditor 5 v34.0.0 is in and it includes https://github.com/ckeditor/ckeditor5/commit/fc562455439f903bb655533f006.... Working on this now! 🥳
Comment #21
Wim LeersVisible in the config UI now:
And also working in CKEditor 5 itself:
Next up: making this configurable. After that: upgrade path.
Comment #22
Wim LeersNow configurable using the exact same syntax/config UX as for CKEditor 4. But we're storing things in a much clearer way, with better validation:
gets stored as
→ this reuses the
ckeditor5.element
config schema type's validation constraints, makes the config file easier to read/interpret usinggit
and makes thegetElementsSubset()
implementation trivial. While simultaneously not reinventing the UX: that is kept identical to the CKEditor 4 UX. We don't want to start that bikeshed.⚠️ But I expect this MR to fail tests now, due to:
Why? Because turns out that
SourceEditing
isn't the only special case,Style
also is special, because it too does not know which superset of elements it will generate. So we'll need to special case this one too. I think this is reasonable. (And if we discovered a need for more "supersets" in the future in contrib, we can add the necessary API capabilities for it.)Comment #23
Wim LeersThis is not yet happening … because there's no test yet that configures the
Style
plugin to be enabled 😅 Hence the tag on the issue!Comment #24
Wim LeersLogicException: The "ckeditor5_style" CKEditor 5 plugin implements ::getElementsSubset() and did not return a subset, the following tags are absent from the plugin definition: "<p class="callout"> <blockquote class="interesting highlighted"> <blockquote class="famous">".
→ this is due to the same reason as I mentioned in #22.Tests still needed: new test cases in
ValidatorsTest
(for the validators that still need to be written) and functional JS test (to verify that it works as expected).Comment #25
Wim LeersThe functional JS test for testing the settings form fails on
The root cause:
→ initially, there are no styles configured, and that's why this validation error is triggered even before showing the plugin settings form for the very first time. We need to make the infrastructure a bit smarter.
Just fixed that in 419df3ae964de9151ba20a9acadcc0d1effff84e. Now it should fail not on line 29 in
StylesTest
, but later.Comment #26
Wim LeersI wrote:
And the failure confirms it:
From line 29 to line 49! 👍
But I'm not yet satisfied. I want to add explicit test coverage for the absence of validation errors upon enabling the Style plugin.
Did that in 992fd87505ce3b9f7cf7360de0ef16c2212e7af3.
Comment #27
Wim Leers992fd87505ce3b9f7cf7360de0ef16c2212e7af3's test result:
Still at line 49, but now with a more explicit assertion. 👍
Let's harden test coverage further, because there's another (related) edge case to be fixed. Just pushed 8a12470d2caa9d2ee7bd8ba51caf0676be7ed7cb, which should cause the failure to happen earlier than line 49.
Comment #28
Wim LeersThat worked:
Pushed the fix for that edge case in 65521a476c80b642a5b352235404d2714206460c.
Comment #29
Wim LeersSo the functional JS test for the settings form now fails like so:
Because:
👍 → also failing on the problem described in #22. Working to fix that now.
I pushed
62760b9a9856834621444962fe202fdb21f11a55
because the "has been updated" expectation was wrong too: the test is creating a new text format, not updating one 😅Comment #30
mpp CreditAttribution: mpp at AmeXio for District09 commentedThanks and 🤞 :-)
Comment #31
Wim LeersAfter careful consideration, I realized that using the "global attribute" infrastructure introduced by #3231334: Global attributes (<* lang> and <* dir="ltr rtl">): validation + support (fix data loss) earlier today is not appropriate for this use case.
I was first thinking that
ckeditor5_style
should claim to support<* class>
, and it'd then return a subset such as['<p class="fancy">', '<blockquote class="famous">]
.… but that does not quite make sense, because the semantics of
<* attr>
mean that it is not a superset of<p attr>
.HTMLRestrictions
specifically treats<*>
as a "concrete tag", so the union of<* attr>
and<p attr>
is not['<* attr>']
but['<* attr>', '<p attr>']
. In other words: the semantics that we've just carefully established in #3231334 mean that we cannot overload it for this purpose too.Fortunately we already have the necessary infrastructure in place: wildcard tags! We just need to introduce a new wildcard tag. That's what b2092eabdd07f5fab26610a490a58e8785e1c4cf just did.
I have more commits locally that I need to clean up/untangle — expect more progress tomorrow :) I'm pretty confident that I'm on a path that will lead to a fully green patch/MR in the next few days.
Comment #32
catchThis seems no longer blocked by upstream now? Tentatively removing the tag.
Comment #33
Wim LeersCorrect!
Comment #34
Wim LeersOne of the remaining failures (
Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "cke4_plugins_with_settings can be switched to CKEditor 5 without problems, settings are upgraded too"
) is due to a bug in\Drupal\ckeditor5\HTMLRestrictions::diff()
: #3278394: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result.Comment #35
Wim LeersClarify remaining tasks. Especially for the
issue tag.Comment #36
Wim Leers#3261599: Use CKEditor 5's native <ol start> support (and also support <ol reversed>) conflicted with this heavily. Merged upstream changes and resolved conflicts, hope I got it right 🤞
Comment #37
Wim LeersThose last 2 failures (DrupalCI miscounts them as 4 failures) are only solveable if I apply the patch from #3278394. So … did that.
Comment #38
Wim LeersPROGRESS! 🥳
Committing the #3278394-7: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result patch allowed us to go from:
4bcb76e2
: 4 failures (2 real ones: one inSmartDefaultSettingsTest
and one inStyleTest
, both counted double)6a892ebf
: 2 failures (both real: one inStyleTest
, one inValidatorsTest
)The end is close now 🤞
b1816e84 should fix the
StyleTest
failure.Comment #39
Wim LeersLast failure:
1) Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::testPair with data set "INVALID Source Editable tag already provided by plugin and another available in a not enabled plugin"
with the following failure:
Note that there is one addition there: besides
Alignment
andAlign center
being suggested as a solution to allowing<h2 class="text-align-center">
, a third suggestion has appeared now:Style
. That's because the Style plugin this MR is adding claims it supports<$any-html5-element class>
— which is obviously a superset of<h2 class="text-align-center">
. So … that's actually normal.Of course,
Alignment
is clearly the better/more specific fit for supporting that particular markup. To improve the message that validator provides, we need to optimize the suggestions it makes. And for that, we already have an issue: #3271179: Update SourceEditingRedundantTagsConstraintValidator to reuse SmartDefaultSettings 👍I don't think it makes sense to block this stable-blocking issue on that detail. So, I'm updating the expected validation message here and adding a
@todo
. 👍Pushed that in 1dc5a2b9c2102c5b6ebd305ce23cf6161d344822.
Comment #40
Wim LeersComment #41
Wim LeersGreen! Time to add the remaining test coverage + validators.
Comment #42
Wim LeersValidator test coverage:
INVALID: Style plugin with no styles
→ when no styles are configured, having the toolbar item enabled is pointless (this is the only one already passing because it's the only one already implemented)INVALID: Style plugin configured to add class to unsupported tag
→ it does not make sense to allow adding aclass
to a tag that cannot actually be generated!INVALID: Style plugin has multiple styles with same label
→ without unique labels, it'd have terrible usability and accessibilityINVALID: Style plugin has styles with invalid elements
→ not possible to configure this through the UI, but … it is possible if you modify the configuration directly.VALID: Style plugin has multiple styles with different labels
→ the sole example of a sane configuration, and hence the only not resulting in a validation errorPushed in 397b5af76079eb17a71010a5260c05f4247eb314.
Comment #43
Wim LeersYay, #42 predicted 3 failures (for test cases 2, 3 and 4) and this also happened:
Time to implement validators! 🤓
Pushed ee4d1838c5148885bb19a81cfd36976aaa2811ff for that.
Comment #44
Wim LeersYay,
ValidatorsTest
is all green now!But looks like
SmartDefaultSettingsTest
started failing due to the stricter validation. 🤓 Good! Pushed a minor fix to the CKE4 configuration.Now the only remaining task is:
.Comment #45
Wim LeersJust pushed the functional JS test coverage!
Comment #46
Wim LeersJust met with the CKEditor 5 team. Discussed this too.
Bugs
They confirmed two bugs I found, and one has already been fixed a few hours ago! 🥳😄
Those now all have explicit test coverage, with the failing pieces commented out with
@todo
s to uncomment them.Validation
The
ckeditor5-style
plugin does not do any validation right now. This confirms the importance of doing validation on our side. All aspects we're validating so far make sense. 👍They suggested one more: ensuring that you do not configure the Style plugin to add classes that other plugins already provide — for example
ckeditor5_alignment
's ability to specifyclass=”text-align-(left|center|right|justify)”
. Will add that tomorrow. Meanwhile, this is definitely ready for review, so unassigning :)Comment #47
larowlanManual testing of the happy path
a.read-more__link|Read more link
to the styles listLooking 🔥
Special mention to https://github.com/dpi/dogit which made it easy to checkout the MR locally
dogit issue:mr 3222797
and done 😎Comment #48
Wim Leers#47: Does that mean … you found everything works as you expected on the first try, on an existing site? 🤓🤞🤞🤞
Comment #49
larowlanNew site, will try an existing site at the weekend
Comment #50
Wim Leers#49: great, thank you!
Quoting #46:
Just pushed that in f1ffbf677d8d3277a1cfb6fe0da81ad7f9b85d02. 👍
Comment #51
Wim LeersUpdating issue summary to show the full set of validation constraints and why (since #42 no longer provides a complete overview).
Comment #52
larowlanManual testing on an existing site with a lot of styles
* Applied patch
* Attempted to update to ckeditor 5, received warning about 'non html' filter (we have a token style custom filter that is
\Drupal\filter\Plugin\FilterInterface::TYPE_MARKUP_LANGUAGE
(unrelated, but we can probably make it HTML)* Turned that off and tried again
* Update looked to add as many items as it could, some items didn't have CKEditor 5 equivalents (we need to port some custom ones, and I guess we need a version of Embed module that is compatible with CKEditor 5) - I couldn't find an issue in its queue for a CKEditor 5 version, but I think that's likely to be a major blocker for many folks updating
* All the styles looked to convert correctly, and the allowed HTML list looked good too, but when I saved I received an exception (see screenshot for trace)
* Also attached screenshot of extensive list of styles and allowed HTML
* Here's the shape of html_restrictions when that fails
The
[1 => FALSE]
obviously being the issueThe full list of allowed tags:
<br> <p class="time icon--calendar-white icon-text icon--location icon--location-white icon--phone icon--phone-white icon--mail icon--rsvp icon--clock icon--clock-white icon--download icon--link icon--external-link icon--info icon--faq icon--arrow-blue-right icon--inline icon icon--arrow-blue-left box box--float-right box--float-left box--blue box--black box--bordered clear-float text-align-left text-align-center text-align-right text-align-justify"> <h2 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h3 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h4 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h5 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <h6 class="block-title clear-float text-align-left text-align-center text-align-right text-align-justify" id> <a class="button button--red button__nav button--prev button--next button--arrow-circle-blue" hreflang title accesskey id rel target name href> <table class="js-table--responsive"> <td class="table--cell-width-s table--cell-width-m table--cell-width-l table--cell-width-xl table--cell-width-xll" rowspan colspan> <div class="clear-float text-align-left text-align-center text-align-right text-align-justify" id> <ul class="listing-links" type> <cite> <dl> <dt> <dd> <blockquote cite> <ol type start> <li class> <span class lang dir> <img style src alt data-entity-type data-entity-uuid data-align data-caption width height> <drupal-entity data-caption data-entity-type data-entity-uuid data-entity-id data-view-mode data-entity-embed-display data-entity-embed-settings data-entity-embed-display-settings data-align data-embed-button data-entity-label alt title class> <section id class> <strong> <em> <code class="language-*"> <pre class="text-align-left text-align-center text-align-right text-align-justify"> <hr> <tr> <th rowspan colspan> <thead> <tbody> <tfoot> <caption>
The issue is coming from this line
elseif (self::intersectionWithClasses($style_element, $other_enabled_plugin_elements)) {
line 64 in StyleSensibleElementConstraintValidator
Here's a var_export of the two items its trying to merge
And
Comment #53
Wim LeersPerfect, thank you for the superb and detailed feedback!
On Monday, I will dig into this. I think the crash you’re running into is a bug we’ve seen once before, but the issue was not considered important … but this will likely change that 👍
The overwhelming messages are already being worked on.
Expect a solid update on Monday!
Comment #54
Wim Leers#52:
The PHP exception … I cannot reproduce. 😬 Already spent a lot of time trying. Could you please share a config export of the text format + editor so I can reproduce it locally? 🤞Eventually I was able to … and this caused half a day of digging around to figure out why the hell this was happening! Root cause found, tests + fix at #3279470: HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state. I don't think that needs to block this.#3278394: HTMLRestrictions' diff operation bug: diff(<tag attr="A B">, <tag attr>) should return an empty result landed, so removed the
[PP-1]
prefix.Next:
HEAD
, then it should be green again.Comment #55
Wim LeersComment #56
Wim LeersI wrote this in #54:
I did that in https://git.drupalcode.org/project/drupal/-/merge_requests/2118/diffs?co..., but it passed.
WTF.
Looks like this is yet another GitLab merge request + DrupalCI weirdness thing. Looks like it automatically pulled and rebased instead of testing that exact commit. 🤪🤷♂️
Because here:
very clearly yields multiple failures, including this fast-to-reproduce one:
Anyway … pushed the merge commit and the vertical tabs summary support (including test coverage) too.
Comment #57
Wim LeersAddressed all of @larowlan's feedback at the start of this week, just finished addressing all of @lauriii's feedback! 👍
Comment #58
larowlan@Wim Leers, I've DM'd you some YAML files on slack to help diagnose the issue after applying #3279470: HTMLRestrictions::merge() can crash due to array_merge_recursive() being affected by internal PHP array state
Failing that I could do an early EU meeting on Wednesday or Thursday to step through it
Comment #59
Wim LeersComment #61
Wim LeersPicking this up again. Keeping this against
9.4.x
because A) it needs to land there too, B) keeps the merge request simple.Forwarding the merge request to the latest and greatest
origin/9.4.x
now 🤓Comment #62
Wim LeersDone. Conflicting changes were: #3277438: Update to CKEditor 5 v34.1.0 + #3259593: Alignment being available as separate buttons AND in dropdown is confusing.
Next: address @lauriii's feedback.
Comment #63
Wim LeersYay,
testStyleFunctionality()
is failing because it was asserting the previously broken state, #3277438: Update to CKEditor 5 v34.1.0 brought in the upstream fix at https://github.com/ckeditor/ckeditor5/issues/11576! 😄The other failures are due to the upstream changes that have been merged in. Should be green again now 🤞
Comment #64
Wim Leers@larowlan I'm 90% confident that #3274648: HTMLRestrictions::merge() and ::toGeneralHtmlSupportConfig() fail on allowed attribute values that can be interpreted as integers by @nod_ which landed earlier today will have solved all your "merging troubles" 🤓🥳 Haven't checked yet with your YAML files (thanks for sending them to me! I haven't gotten to this in a while because I was helping with getting
9.4.0-beta1
stuff) yet, but I'm pretty confident I won't need them anymore 😄 I will test this tomorrow morning (it's been a very long day), unless you get to re-test this before I can 🤞Comment #65
Wim LeersComment #66
Wim LeersPer https://git.drupalcode.org/project/drupal/-/merge_requests/2118/diffs#no..., this is now blocked on #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path — because I found a bug thanks to @lauriii's question 🤓
@lauriii, please review that thread on the merge request — I'm now less convinced of the value of this new
HTMLRestrictions::notInResolvedSuperset()
utility method 🤔 Curious about your thoughts.Comment #67
Wim Leers@lauriii thanks! That makes this officially blocked on #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path.
Comment #68
Wim Leers#3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path landed, this is now unblocked!
Comment #69
Wim LeersAll ready for final review now! 🥳
Comment #70
Wim LeersComment #71
lauriiiMR looks good! Great job @Wim Leers 👏
Comment #72
Luke.LeberHmm...this is what I get when trying to enable cke5 on our main site on 9.4.x-dev and applying the latest from https://git.drupalcode.org/project/drupal/-/merge_requests/2118.diff:
Here are the list of styles:
Please let me know if you need more information :)
Comment #73
Wim LeersI'll investigate in a few hours (currently working on something else).
Comment #74
Wim LeersReproduced. ✔
Comment #75
Wim LeersRoot cause is two-fold:
SmartDefaultSettings
was not correctly computing the provided elements from the enabled plugins: it did not pass in the text editor configurationSmartDefaultSettings
was not yet correctly computing the$still_needed
elements: it did not properly account for creatable tags (new since #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path), which is also only now a problem because this is the first time we have a wildcard tag providing support for a specific attribute (Style
supports<$any-html5-element class>
, i.e. any class on any HTML5 element)IOW: the presence of even a single
span.test|My Test Span Style
style definition would've triggered this edge case!Explicit test coverage pushed just now.
Comment #76
Wim LeersDrupalCI says:
DrupalCI unfortunately strips the critical information away 😬 It's actually:
This shows the problem:
SmartDefaultSettings
thinks thatclass="llama"
on<span>
is also not yet supported, but it is!Fix pushed in 8bc6a27a3b1723c58240769f6ea15e2c70031f12.
Comment #77
Wim LeersThat should be green again 🤞
@Luke.Leber Please try again 😄
Comment #78
Wim LeersLooking very green, so unassigning and running all tests again 🥳
Comment #79
lauriiiThank you for addressing the feedback on the MR! Looks all good now!
Comment #80
bnjmnmI found some things on manual testing. Perhaps some of these are acceptable, but would be good to have the justification documented in the issue.
when the Code plugin isn't enabled. With my default setup, saving results in a big white page with
The website encountered an unexpected error. Please try again later.
and the log hasThis seems like an issue because despite the solution being fairly easy (add support for the tag), that solution isn't particularly clear. I also notice that this deviates from the really helpful validation messages that appear for many similar config issues.
<p>
tags are always available - i.e. in a blank editor, the styles options for<p>
are available while the options for other tags are disabled. I can choose the<p>
style and it will add that paragraph with the class. For styles that are applied to other elements, I must first create that element (use header to create an h2 for example). Then the style option will be available. (confirmed this is possible in CKE4, f.e. an h2 can be created with styles combo)<p>
style that adds.exclusive-paragraph-class
then use the Header plugin to change it to an<h2>
. That<h2>
will still have.exclusive-paragraph-class
. That class gets stripped from theh2
on save since my text format doesn't support that tag/class combo so there's not a huge consequence, but it's worth notingdrupal-media
in styles. Similar to my first point, submitting the form gets me anunexpected error
and the logged error isI don't know if something like
<drupal-media>
necessarily needs to be supported or if it would ever happen in an actual site scenario, but I wonder if validation should make this limitations more clear & friendly.<li>
and<ul>
element in the text editor config, but in the actual editor it's not possible to add them even when a<li>
or<ul>
is the active element (confirmed this is possible in CKE4)Comment #81
Wim Leers👏
The tl;dr of @bnjmnm's review is
🤣🤣🙈IOW: Ben is the new Lauri, but fortunately instead of finding critical upstream bugs, Ben is kindly finding only critical downstream bugs 👍 😄
😱 Will add an explicit test case for this.
Those are upstream bugs. There are known bugs with
Style
, some of them were already fixed in #3277438: Update to CKEditor 5 v34.1.0, but not all. For example, the functional JS test coverage still contains this:See https://github.com/ckeditor/ckeditor5/issues?q=is%3Aissue+is%3Aopen+labe.... Upcoming releases will fix more bugs. They have made it clear that
Style
is currently "experimental" and will be getting lots of bugfixes in upcoming releases.(Writing an explicit test for every detail is pointless IMHO, because then we'll just be duplicating their test suite.)
That's intentional, because
Style
does not work for widgets, just like CKEditor 4'sStylesCombo
did not.But of course the validation logic should not crash! 😱
That's also an upstream bug. (See above.)
+1 — I thought the validation I added covered this but apparently not 🙈
Working on adding failing tests 👍
Comment #82
Luke.Leberre: #77 - everything works great on our setup now!
Comment #83
Wim Leers@bnjmnm's first bug
@bnjmnm reported this crash:
when configuring
s.blah|Blah
when<s>
was not supported. That's the output he saw because he's running PHP with assertions disabled. If you enable assertions, you see this instead:… which is what de686af0 showed in https://www.drupal.org/pift-ci-job/2403676 👍
This was fixed in 48b466fa, and now the test result (https://www.drupal.org/pift-ci-job/2403699) shows something else instead:
That's right …
\Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraint::$nonCreatableTagMessage
is awfully similar to\Drupal\ckeditor5\Plugin\Validation\Constraint\StyleSensibleElementConstraint::$unsupportedTagMessage
! That's because this issue/MR predate #3273983: Do not assume that plugin supporting <tag attr> also supports <tag> in SourceEditingRedundantTags and upgrade path, but that landed first.
Before removing the duplicate validation error messages (by removing duplicate validation logic), I wanted to make sure that the bug that @bnjmnm found through the UI was actually tested through the UI as well. 7eb9597c + the two subsequent commits prove that. (Although once again the DrupalCI test results are … so you'll have to run the test locally if you want to verify this. 😬)
The functional JS test is green because it only checks the validation message from
FundamentalCompatibilityConstraintValidator
— which is why 713fd839 was able to just remove all of that.ℹ️ To view the full set of changes for fixing this bug, do
git diff a24f3b215b95990e01ef79a3a510da0fab7ad2e5713fd8393be8ca39c56f6d6504cf42ac5903a668
.Comment #84
Wim Leers@bnjmnm's second bug
Because this was a logic exception being thrown for invalid configuration at a time when invalid configuration is acceptable (while CKE5 is being configured in the admin UI!), there is no clear best place to add explicit test coverage for this.
The best/only place: expanding the functional JS test coverage. Did that in 23a0868c. It fails magnificently due to the fatal error 🤣 — see for yourself:
Then fixed it in 0bbdece3, added explicit test coverage for the logic exception in e3336eaf and expanded the existing validation test coverage in to prove how it behaves in "actually saved text editor" circumstances in 0b5f0bef.
ℹ️ To view the full set of changes for fixing this bug, do
git diff 713fd8393be8ca39c56f6d6504cf42ac5903a668 0b5f0bef2895cabef953c339864139c646632e71
.Comment #85
Wim LeersComment #86
lauriiiNow that we are relying on
\Drupal\ckeditor5\Plugin\Validation\Constraint\FundamentalCompatibilityConstraintValidator
for validating the supported elements, there's a warning triggered instead of an error. The warning also is tied to the toolbar button, not to the Styles textarea. Because the textarea doesn't have an error state, if you are scrolled down so that the toolbar configuration is outside of screen, there's no indication of warning or error at all for the user, unless they scroll up or try to save the form. This has inferior UX compared to what we had previously. I'm wondering if you have any thoughts on how we could improve this?Comment #87
Wim LeersI think that is arguably better than before, because now the user is being told what to do to make the Style configuration they just entered actually work.
Perhaps we then finally have a good reason to fix #3215897: Toolbar items validation errors not able to target the toolbar? 🤓
I think fixing #3215897 would bring us back to the great UX :)
Comment #88
Wim LeersDiscussed #86 + #87 with @lauriii, I have an idea about how to approach this differently now 🤓
Comment #89
Wim LeersComment #90
Wim Leers😬 I pushed the “run all tests again” commit while the previous build still had to start. Result: they’re both testing the last commit. This is defeating the purpose of having separate pushes: verifying that the subset passes first, then the superset 😞
Comment #91
Wim LeersI forgot to update the kernel test in e3a2aa4db73ef8cca97efdb3c95b1853afabf384! Fixed now!
Comment #92
lauriiiConfirmed that all of my feedback was addressed. Did some manual testing and in my opinion the UX is in line with other CKEditor 5 plugins now. 👍
Comment #93
Wim LeersAFAICT the same patch applies cleanly to all branches! 🥳
EDIT: just realized it's an unfortunate coincidence that the patch is named
*-93.patch
, that's just because it was comment #93 😅Comment #94
Wim Leers😬 Forgot that
yarn run build:js
yields different results in D10 …Comment #95
DanielVezaCouple of issues found so far when testing this on a site.
<button class>
in the manual html field, it will throw a fatal when you try save when an OOB exception. Having just<button>
works fine. Can we add some validation here?button.btn-primary|Button
it is always disabled in the styles dropdown - This looks to be related to the upstream issue raised in #81Comment #96
Wim Leers@DanielVeza Thanks for testing!
🤔 Great point. But … isn't that true for many AJAXy forms in Drupal? 😅
In any case, this works exactly the same as it does for CKEditor 4. If you have a concrete suggestion on how we could make this work better, I'd love to see that happen, but then as a non-stable-blocking follow-up
tagged 🤓But that's not the
Style
plugin, that's theSource Editing
plugin. I already added tons of test coverage for edge cases around this, but maybe you found a new one.This issue/MR is getting very unwieldy, so let's add more test coverage for more edge cases in a follow-up issue so that this stable-blocking issue can land 🤓
Would you mind creating a follow-up issue with the exact steps to reproduce? Because just adding
<button class>
to theSource Editing
plugin does not reproduce this for me 😅Thanks!
Correct! Will be fixed in an upcoming CKEditor 5 release. 👍
Comment #97
DanielVezaThanks for the quick follow up Wim!
Heh, agree. I'll have a think about this one.
Sorry, I wasn't clear on this one. If you -
<button class>
to the source editingbutton.btn-primary
to the Style plugin, you'll get a OOB exception<button>
to the source editing and it will save correctlyThis was testing on an existing site but I don't think it would be a site specific issue from some debugging, happy to be wrong though!
Love ya work and don't want to delay this getting in. I don't think either thing raised should block it and can be done in follow ups so leaving at RTBC with confirmation that this is working on an existing site.
Comment #99
mpp CreditAttribution: mpp at AmeXio for District09 commentedWhen adding the Styles button the following error message is displayed:
"Enable at least one style, otherwise disable the Style plugin."
However, there's no UI under "CKEditor5 plugin settings" to enter these styles.
Edit: it's not appearing due to a JavaScript bug:
The id is "erjqrv8b1j-edit-editor-editor--5b1BdFoJwK0":
<select data-drupal-selector="erjqrv8b1j-edit-editor-editor-5b1bdfojwk0" id="erjqrv8b1j-edit-editor-editor--5b1BdFoJwK0" name="editor[editor]" class="form-select" data-once="drupal-ajax"><option value="">None</option><option value="ckeditor">CKEditor</option><option value="ckeditor5" selected="selected">CKEditor 5</option></select>
Seemed to be related to https://www.drupal.org/project/drupal/issues/1852090.
Once that was solved the following popped up:
Removing
the Styles button appears but the tr/td styles don't apply.
Comment #100
mpp CreditAttribution: mpp at AmeXio for District09 commentedComment #101
mpp CreditAttribution: mpp at AmeXio for District09 commentedComment #102
mpp CreditAttribution: mpp at AmeXio for District09 commentedAfter some tweaks, the Styles UI is working. Not sure why but ckeditor 5 doesn't detect the td or tr element, only the p is enabled:
@wim any idea why ckeditor detects a p when the cell is highlighted, perhaps a default paragraph setting in ckeditor config or a bug in ckeditor itself?
Comment #103
xjmComment #104
xjmUpdating tag for the next window.
Comment #106
nod_last commit fix part of #97, there is no crash anymore, but we get a validation error that says we can't have two plugins supporting the same elements + attributes.
The error is that styles support of
<button class="test">
is in conflict with source editing plugin support of<button class>
. The validation error makes sense to me.Do we want to be able to use the UI to edit a subset of what's allowed with source editing? If we keep things strictly separate the fix should be enough, but I can see some cases where this type of config might be wanted (although it might not be a good idea).
Comment #107
nod_test is not actually failing without the code change so will need to fix that
Comment #108
nod_Comment #109
nod_created #3294908: Configuration overlaps between Styles and other CKE5 plugins to agree on where we stop peeling the validation oignons.
Comment #110
nod_Comment #111
DamienMcKennaThe release tag change was accidentally reverted.
Comment #112
nod_Comment #113
xjmThe release note here does not describe the disruption or consequences of the change. It should explain the upgrade path implications for site owners and module developers. See the release note instructions for more info on what should go in a release note. Thanks!
A CR would also probably help.
Comment #114
nod_Last push doesn't fix the test failure but gets closer. With the latest changes to the upgrade messages not sure what's happening here.
When I run the test I get the First message, but when I relaunch it I get the second, so I can never seem to get the right string at the right time to match. Help welcome.
Comment #115
nod_Comment #116
xjmUntagging for now since this is not in yet, but we can re-tag for the release notes if there is a disruptive change associated with this on commit. (The current release note does not describe a disruptive change as per #113.)
Comment #117
nod_Tried to add some details to the release note. Essentially, nothing to do for site owners, when they switch to the CKE5 editor, the config is migrated automatically.
Not sure what to put in the change record though, we don't have change records for additional plugins we support in core, only the Drupal-specific API or use of CKE5.
As for the configuration overlap issue it's fine to keep in a follow up because:
Comment #118
lauriiiI think the release note is more of a highlight than describing something disruptive. I don't think this is disruptive to existing users as #117 already points out. I don't see how we would need a change record for this but I'm keeping the tag for now.
I've reviewed all of the changes since my last RTBC and it addresses all of the feedback that has been raised after it.
Comment #119
bnjmnmComment #125
bnjmnmI'm tagging with 9.4.6 release notes despite being aware of this comment (and it's associated policy)
#3222797: Upgrade path from CKEditor 4's StylesCombo to CKEditor 5's Style.
The release note in the issue summary does not meet the criteria of describing a disruptive because nothing disruptive is introduced here (also why I removed the CR tag). This is a release highlight, but those are typically not tagged on patch releases, and I wanted to be sure this was discoverable in case there was a desire to make this known to 9.4.6 users waiting on Styles Combo migration support.