[Style] Add tests for inability to apply styles to <div>, <ul>, <ol>, <table> etc. in CKEditor 5 — and <a> allows applying it to all elements

Any chance you could also post a GIF of it working in CKEditor 4? And one for it failing in CKEditor 5? 🙏

The CKEditor 5 team has been very consumed with making sure CKEditor 5 is equal or superior accessibility-wise compared to CKEditor 4, and that’s why they have not made as much progress on https://github.com/ckeditor/ckeditor5/issues?q=is%3Aissue+is%3Aopen+labe... as they were hoping.

The upstream bug report is https://github.com/ckeditor/ckeditor5/issues/11577 — please add a 👍-reaction to that GitHub issue to prioritize it higher for them 🙏

@Reinmar just confirmed that fixing this is scheduled for Q1 2023 👍

Postponing on that.

Updating issue title to clarify this is no limited to lists — see #3324225-10: Support CKEditor 5's table properties and cell properties plugins in Full HTML.

Confirming this bug after migrating from CKE4 -> CKE5 our styles on ul elements no longer work. They appear in the styles list but are greyed out and not clickable.

Discussed this with @Reinmar from CKEditor 5!

This is one of their top priorities for this quarter. The fix should get merged in March, and will ship in their April 2–5 release. 👍

However, it's not a single fix: it cannot be fixed generically. It requires changes in many parts of CKEditor 5: for <ul> the list plugin will have to be modified, for <table> the table plugin, and so on. So it's not known yet for which tags it will be fixed and for which ones it won't be — but expect a big leap forward in any case.

@Reinmar just confirmed that fixing this is scheduled for Q1 2023 👍

Due to unforeseen challenges with the conversion of the CKEditor 5 codebase to TypeScript, this has been delayed.

Update directly from @Reinmar: expect the first PRs solving parts of this issue to land mid-May, and for most/all of this to be solved by Q2.

As I reported almost a year ago at https://github.com/ckeditor/ckeditor5/issues/11709, this also applies to <a>. Clarifying that here.

Closing #3334617: CKEditor 5 Text Styles can be applied to any element even when a tag is specified as a duplicate.

As a temporary work-around for <a> specifically, you could install @ChrisSnyder's https://www.drupal.org/project/ckeditor_link_styles module.

Crediting the contributors of that issue.

Also note: #3347721: [Style] Warn the user about styles for unsupported elements would reduce the "WTF" factor — hoping to hear from the CKEditor team soon on that one! 🤞

Note for <a>, it seems to work fine. My defined styles for them can be applied to links I added. The problem is that it allows applying those styles to any element instead of just <a>. That wasn't super clear to me when reading thru this issue

Good point — clarified.

Thanks for keeping us up-to-date on this. I've hoped it was resolved by this month but I can see that it's not until mid May / Q2.

If not solved in time, can we use CKE4 with D10 ?

@brulain Yes, you can use CKEditor 4 with Drupal 10 for another ~6 months. See https://www.drupal.org/docs/core-modules-and-themes/deprecated-and-obsol....

<div> is also affected.

1. Drupal 9. Set CKEditor 4 in Full HTML text format.
2. Define the style div.alert.succcess|Success alert.
3. Click on “Success alert” in “Styles” dropdown on a node body field and it inserted <div class="alert success"> </div> and placed the cursor inside.
4. Switch Text editor to CKEditor 5 in the Full HTML text format.
5. Expand the “Styles” dropdown on a node body field, and the block style is disabled.
6. Explicitly allow the <div> tag in the manually editable tags setting for CKEditor5.
7. Manually add a <div> in Source mode on the node edit form body field.
8. Switch back to normal editing mode and ensure cursor is inside the DIV.
9. Expand the "Styles" dropdown and the block style is still disabled.

Upstream issue here:

#13341 Can't set the style on <div> using Styles dropdown

Please thumbs up that issue to help get it resolved faster and on maintainers radar.

There is also an issue with heading tags. They are disabled. Here is a list of styles I currently have.

h2|Heading 2
h2.jump-link|Jump Link
h3|Heading 3
h4|Heading 4
p.p-lg|Large Paragraph Copy
p|Body Copy
small|Mouse Type
span.notranslate|No Translate
span.sl_swap|Content Swap

@jeremy.sloan:

That is an interesting point about the functional regression. In CKEditor 4 you could actually create an "empty" style for block-level tags like div, h2, h3 etc with no additional classes (eg h2|Heading 2), and that would be enough to wrap the currently selected text in the associated tag.

The way CKEditor 5 appears to work now is that you must first change the selected text from "Paragraph" to "Heading 2" (so that the H2 tag can be applied to the html source) before the Heading 2 jump-link style can be applied. This workaround obviates the need for the naked H2 style in the styles list.

Closed #3356593: [upstream] [Style] Support adding class attribute to table captions as a duplicate.

Quoting @Luke.Leber:

When configuring a style for caption elements, it cannot be applied to the table plugin model through the styles drop-down. Incidentally, it also cannot be applied through source editing mode, as it's stripped out, which might be a GHS issue.

See https://github.com/ckeditor/ckeditor5/issues/13777 for the blocking upstream issue.

Steps to reproduce

1. Add caption.test|Caption test as a style option.
2. Observe that the style cannot be applied via the styles dropdown.
3. Add <caption class="test">Bah humbug</caption> via source editing mode, switch out of source editing mode, and back in, observing that the class attribute is gone.

The good news: https://github.com/ckeditor/ckeditor5/issues/13777 was fixed on April 28! 😄

@jwilson3 ok that makes sense.

1) Put in you text first with headings plugin
2) Highlight text and apply styles. (Only styles that can be applied to the tag will be enabled)

Whats the timeline for rolling this into core? Will it be in the next version of 9 as well?

Is this really fixed by https://github.com/ckeditor/ckeditor5/issues/13777? It looks like that issue is only about table based elements.

Looks like we are still waiting on:
https://github.com/ckeditor/ckeditor5/issues/13341
https://github.com/ckeditor/ckeditor5/issues/11577

Both are needed to close this issue.

Updated IS

@frob: I posted an update in the #ckeditor5 Drupal Slack channel after the meeting I had last Thursday with the CKEditor team. Sorry for not cross-posting here!

Paraphrasing what I wrote there:

• Most fixes for Style will ship in the next CKEditor 5 release, which will happen with 95% certainty on May 22 (twelve days from today!). See https://github.com/ckeditor/ckeditor5/issues/11574 for the list of things that is fixed (progress is still being made).
• I've already got confirmation from Drupal core release manager @catch that we'll update 10.1.x despite it already being in alpha from the current 37.1.0 version to the next release.
• In other words: many Style problems will be fixed in Drupal 10.1.0! 🥳

Rather than manually keeping track of a list of upstream issues here, I'd rather point to a single umbrella/meta upstream issue (so updated the IS), and then after the next release they will create a new umbrella/meta issue for the remaining ones.

I agree the clearest way forward then is for us on the downstream Drupal side to keep this issue open until Style works on all commonly used HTML tags it's known to not work on.

Will it be in the next version of 9 as well?

Drupal 9 will not get any CKEditor 5 updates, because Drupal 9.5.x is the last minor version for Drupal 9.

Drupal 9 will not get any CKEditor 5 updates, because Drupal 9.5.x is the last minor version for Drupal 9.

I am not quite sure I get this. If the fixes come before EOL of Drupal 9 why wouldn't 9.5 get the fixes? I thought 9.5 was a part of the CKE4-5 upgrade path for D10

Version 38 has been released: #3361800: Update CKEditor 5 to 38.0.0.

@frob: because each CKEditor 5 update is a major release with some backwards compatibility breaks. That's why only new Drupal core minors get CKEditor 5 updates: there's too much disruption risk otherwise.

#3361800: Update CKEditor 5 to 38.0.0 is green: tests are passing. That means it's time for manual testing here! 🥳

We know for a fact that things are better in this release, so Drupal core will update to this version. But now we need to construct a list of the remaining bugs.

What remains

Currently there are 13 open issues for the Style package and 29 closed. Here are the 13 open issues:

In the stabilization issue, 14 of the 20 issues are completed:

Impact of what remains to be fixed

I think the most impactful remaining bugs are:

1. <div> support: https://github.com/ckeditor/ckeditor5/issues/13341
2. <table> support: https://github.com/ckeditor/ckeditor5/issues/11577

Impact of what has already been fixed

• <blockquote> support: https://github.com/ckeditor/ckeditor5/issues/11576
• <ul> + <ol> support: https://github.com/ckeditor/ckeditor5/issues/11590
• <a> support: https://github.com/ckeditor/ckeditor5/issues/11709
• … and many more: https://github.com/ckeditor/ckeditor5/issues/11574

Please confirm!

Please confirm the above breakdown in your manual tests 😊🙏

#38: I think that might be the already known bug described in https://github.com/ckeditor/ckeditor5/issues/11606. Asking @witeksocha for confirmation… 😊

Yep, that's exactly what #11606 upstream says. Makes it more likely that @witeksocha will confirm my hunch.

I confirm this is the issue, I created a separate ticket for the list case https://github.com/ckeditor/ckeditor5/issues/14216. We will discuss it internally, and I will keep you posted.

@frob FYI I have done some work on back porting v38 to Drupal 9.5. I now understand why it is not being back ported past Drupal 10.1

I have done the back port and it is working well enough to test this issue and confirm that when we upgrade to 10.1 will be able to move to CkEditor 5. However in v36 up (which were both in D10.1) there was a change in the plugin API which caused crashes of CKEditor 5. (See https://www.drupal.org/project/editor_advanced_link/issues/3350254) which I have found it also effects link it as well.

For testing of v38 i think this is fine, but I do not think this should be used in production.

Tests

I personally don't think it makes sense to test every single one of these cases in Drupal if they're already being tested upstream. OTOH, it would make it crystal clear which precise cases are not yet working… 🤔

And in fact \Drupal\Tests\ckeditor5\FunctionalJavascript\StyleTest::testStyleFunctionality() already exists for that very purpose. It currently contains for example

    // @todo Uncomment this after https://github.com/ckeditor/ckeditor5/issues/11709 is fixed.
    // $this->assertSame('true', $buttons[3]->getAttribute('aria-disabled'));

So: self-assigning to update (and expand) that test coverage.

To follow up or not

IMHO we should not create a follow-up but instead keep this issue open. Because it already has ~46 followers; we shouldn't ask all of them to follow another issue? OTOH, that issue could be more narrowly scoped, and we'd stop notifying people whose problems are already solved.

Thoughts?

Remaining known bugs have a new upstream "meta" issue: https://github.com/ckeditor/ckeditor5/issues/14274.

Just wanted to add a comment to say that the upstream epic that was being tracked appears to now be complete and was marked for release in v38.0.0 of CKEditor 5.

https://github.com/ckeditor/ckeditor5/issues/11574

#48: Indeed, see #37 through #46.

Thanks to @Spokje in #3366633: Uncomment assertions in StyleTest related to https://github.com/ckeditor/ckeditor5/issues/11709, part of #45 is already addressed now 👍

Met with the CKEditor 5 team yesterday. I have GREAT news! 🥳

They made great progress on https://github.com/ckeditor/ckeditor5/issues/14274, with the first 3 being solved already:

1. [GHS/Style] List styles preserves itself and spreads to other list types and blocks. #14216
2. [Style dropdown] Inline styles retained when changing heading, inserting codeblock #11591
3. [Style dropdown][GHS] Clean up disallowed attributes upon editing #11606

👆 These will ship in a 38.x minor release, with no BC breaks, at the end of June! That will solve #38.

They are already working hard on "Can't set the style on

👆 These will ship in a 38.x minor release, with no BC breaks, at the end of June!

Most likely 🤞, we haven't yet pinned the version for the next release yet.

The fix for <div> support landed upstream 46 minutes ago: https://github.com/ckeditor/ckeditor5/commit/4b44f58c8dd280cafaadb10b2d0... 🥳

Did some testing and was able to apply custom style classes to <ul> and <ol> and <table>s without issue on the latest 10.1.x 🔥.... Unsure of <div>s though, don't have the plugin installed. We'll likely want to update to the latest CKEditor-dev for the fix Wim Leers linked.

I have expanded the FunctionaJavascript/StyleTest.php coverage to test styles for ul, ol and table elements (https://git.drupalcode.org/project/drupal/-/merge_requests/4462/diffs?co...).

It's not possible to apply a style to a div element is not working yet as far as I know.

My first commit failed because it worked in my 10.1.x version but fails in the latest code, probably because of this ckeditor issue that has been fixed: https://github.com/ckeditor/ckeditor5/issues/11709 in 10.0.x.

Committing an attempt to fix it

Updating to 10.1.x, that's why test was failing.

Tests passing, this is ready for review

Hi,

What about Drupal 9, the bug is still there for the div?

Thanks

#64: this will not be backported to Drupal 9. That would break contributed modules.

The test coverage that @juanolalla added looks great! 🤩👏

I've added test coverage for <a>. I'm working on test coverage for <div>. Will wrap that up in the next few days.

Ok fine, I will upgrade to 10 so, how can I apply the fix?

It’s already fixed 😊 This issue is only adding test coverage!

Ok because, I tried in Drupal 10.1.2 after having updated from 9.5 and when I save the styles in the ckeditor configuration, I got this error:

AssertionError: assert(NestedArray::keyExists($subform, $parts)) in assert() (line 892 of core/modules/ckeditor5/src/Plugin/Editor/CKEditor5.php).

I just tried to put that:
div.intertitre|intertitre

Thanks

Hi Again,

I found a solution, when you put the styles you have to first click out of the field to have the ajax call to save the styles only after you can save the form.

So now I have the style correclty setup, but the stlye button in CKEditor is still disabled, I have put those styles:
div.texte|Texte
div.intertitre|Intertitre
div.exerge|Exerge
div.encadrer-titre|Encadrer Titre
div.encadrer-texte|Encadrer Texte
div.itw-question|ITW Question
div.itw-reponse|ITW Réponse
div.note-bas|Note Bas

But the button is not working, do I need to do something ? I have the Drupal 10.1.3-dev installed

Thanks

#68 + #69: that's related to #3262484: AJAX desync when quickly changing multiple form values in plugin settings vertical tab and #3265626: Changes to "Manually editable HTML tags" lost if form is submitted without triggering AJAX. It's unrelated to this issue.

Added the missing test coverage.

This is now ready.

Yep, and that's also what the test coverage does 👍😊

Thanks for the very fast review! 😳🥳

Just to know, are there any plans to return to having a behavior similar to ckeditor4 regarding styles, where it was not necessary to manually add the <div> declaration to the source, just select the text and the <div> comes automatically with the style?

Yes, we plan to have a similar behavior in CKE5. We are focusing on the most impacting bugs and I don't have an ETA yet.

Indeed. That's also what I explained in #51 ~2 months ago.

If there's at least one more person who posts a comment asking about this, I'll create an explicit issue on Drupal.org to track this upstream issue, that way Drupal users can get notified when Drupal updates to a version of CKEditor 5 that supports this 😊

Hi,

Finally I have move from div to p, it work well. Quick question, how can we disable the "multiple style" behavior? The user need every time to click to remove the previous one, it's not really convenient like that.

Thanks

Quick question, how can we disable the "multiple style" behavior? The user need every time to click to remove the previous one, it's not really convenient like that.

Please note, that others desperately need that functionality. So perhaps that should be an option then to opt-in or out? (As separate feature request / follow-up discussion?)

Agreed with @Anybody. That's how it also worked in CKEditor 4 (try it for yourself at https://ckeditor.com/ckeditor-4/demo/), it just didn't show "Multiple styles" as the label.

I checked the docs (and demo) at https://ckeditor.com/docs/ckeditor5/latest/features/style.html, and there's nothing about this sort of "mutually exclusive style". But … there is a feature request for it: https://github.com/ckeditor/ckeditor5/issues/14206 — please add a 👍 reaction to that — that's how the CKEditor 5 team prioritizes issues based on their userbase's input! 😊

thanks a lot

It looks like we are still missing test coverage for the bug reported in #29. I think it would be great if we could expand the coverage to confirm that it has been addressed too.

Added test coverage for #29 — good catch! 👍

Happy to report that this one too is working correctly 😄

Re-RTBC'ing because this is identical to the pattern established by all the other test cases — this is just one more test case.

Committed 2014af7 and pushed to 11.x. Since this is test only change, backported to 10.1.x. Thanks!

image styles are not working on 9.5.10 cKEv5
exp:
img.m3|Margin

div , a tags are picking up

@thatipudir This only landed in 10.1.x and newer. Please update. 🙏

One additional regression was reported: <br class>. See #3398223: [upstream] [Style] [GHS] empty elements do not work: <br class>, <hr class>, <col class> etc.. The test coverage there builds on the test coverage that was introduced here 👍

@wim Leers - Issue on both 9.5.10 and 10.1.x. Thank you!
Created new issue here https://www.drupal.org/project/drupal/issues/3398786

@thatipudir I think it is safe to say that this will never work on Drupal 9.5

Quoting #51:

They're splitting that up in two pieces: the critical piece (being able to apply styles to <div>s) will happen first, the non-critical piece (being able to convert an existing element, for example a paragraph, into a <div class="something"> by just choosing a style).

There now is a Drupal.org issue to track this remaining upstream non-critical piece: #3362451-9: [upstream] [Style] Allow CKEditor 5 to *create* a <h2 class="something"> directly (instead of first <h2>, then adding a class).

Please consider opening another issue for ck5 and div, as that problem also appears to be still unsolved, and it is slightly different than for "headline" because divs can wrap multiple elements.

UPDATE: Issue for "div" has since been created here: https://www.drupal.org/project/drupal/issues/3418322

a.a-button|Button doesn't seem to work. Anchor tags don't work anymore?

Is there any projection to solve the image class problem in CK Editor 5?

It is still impossible to convert a <p> into a <div> automatically by selecting from the Styles dropdown?

And if yes, why are this issue and the upstream issue marked as closed ?