Support CKEditor 5's table properties and cell properties plugins in Full HTML

Created an upstream CKE5 feature issue as suggested by Wim for TableProperties.

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

Additional details may need to be added.

To those who need this, this is a regression that potentially blocks an update to CKEditor 5 from 4.

Sorry, I acted a bit too fast here. I misinterpreted this as being about rowspan and colspan attributes.

But this appears to be primarily about attributes on the <table> element, which is why the issue summary is referring to <table style>.

What's missing here is an overview of the attributes that you're missing. That's also missing in https://github.com/ckeditor/ckeditor5/issues/13879. Before the CKEditor team can act on this, we need more precision 😊

I think this is about the following "properties":

Those map to the following attributes/attribute values:

• <table align="left center right">
• <table border>
• <table bgcolor>
• <table width>

IOW: see https://www.w3.org/TR/html4/struct/tables.html#h-11.2.1 — this is all deprecated in HTML5 but allowed in HTML4.

That means everything in that CKEditor 5 UI would be usable except for: border style (just on/off would be supported), border color, border width and table height (only width is supported).

Which also means that that CKEditor 5 UI would need to be reduced in Drupal, since we specifically don't allow <style>.

Can you confirm that would fulfill your needs?

I did update the CKEditor5 issue thread with the 4 attributes you mentioned align, border, bgcolor, width.

For me, those should be sufficient.

I'm not sure I updated the CKEditor5 issue thread correctly; please let me know if I missed something.

Thanks! :-)

Re-uploading the patch from ✨ CKEditor 5 is missing table properties and table cell properties plugins Closed: duplicate to this issue. Credit to: jordan.jamous

No idea what the patch test result will be here.

Thanks for the initial patch, @BramDriesen!

But … it's enabling this functionality that relies on <table style=…> for all text formats whose CKEditor 5 configuration has the Table functionality enabled. We need to enable these features conditionally: in text formats that are unrestricted and hence allow style attributes.

An example of how to do that can be found in #3353010-6: Add table.TableColumnResize for formats with arbitrary HTML supported → 🤓

Yeah of course, this was just a copy pasta from the other issue rebased on 10.1.x 😉 that's also why I left it on NW. I doubt the MR will even work at the current moment.

👍 Great!

The condition stuff to automatically enable based on Full HTML or not is pretty powerful, but also not that intuitive, so I was just making sure I kept you unblocked! If you already figured this out on your own, you deserve the 🧙 emoji 😄

I did not figure that out on my own, but happy to take the 🧙 emoji 😅.

I *guess* this last commit(s) is a better implementation more in line with the one you referenced as example. I tested this out (with the tugboat URL) and was able to set table and cell properties. One thing I noticed was that cell properties were shown while doing the edit, table properties not until after saving and viewing the page.

Also adding the need manual testing tag since automated tests will be difficult like you said in that other issue.

Something is setting the <tr/> background colour to white, so that is what's causing the "issue" that you can't see the background colour being applied while editing.

tr,.draggable-table.tabledrag-disabled tr {
    color: var(--color-text);
    background: var(--color-white);
}

Pretty sure that last removal will not pass review, but I also have no idea how to properly fix that.

Woah, did not expect this to run into Claro CSS troubles 😅 Looks like that Claro CSS was introduced in #3021388: Table style update → .

I'm surprised that a not so specific selector like tr, .draggable-table.tabledrag-disabled tr {…} can trump style attributes?! 😳

Can you show what the computed CSS specificity levels are?

can trump style attributes?!

It's not really the case (I think). The issue I was having is that the style on the <table> element was not properly showing because of the background colours introduced in that ticket.

Setting a style on a <td> or <th> did work as expected and overrides what is set in CSS. I was not able to set a style on a <tr> element using the UI.

CSS specificity levels

I had to google that 😅 fron-end stuff is not my forte haha. But I don't think that this is causing an issue here. It's just *where* that background colour is being set by CKEDITOR and Claro.

Here is an example of what CKEDITOR is generating.

<table style="background-color:hsl(120, 75%, 60%);width:400px;">
    <caption>caption</caption>
    <thead>
        <tr>
            <th>
                th
            </th>
            <th>
                th
            </th>
        </tr>
    </thead>
    <tbody>
        <tr>
            <td>
                td
            </td>
            <td style="background-color:hsl(0, 75%, 60%);">
                td
            </td>
        </tr>
        <tr>
            <td>
                td
            </td>
            <td>
                td
            </td>
        </tr>
    </tbody>
</table>

And a screenshot of how it looks

I guess a better solution (instead of me removing CSS that is used on so many places 😅) would have been to exclude that table/tr style on the CKEDITOR element with one of these classes. ck ck-content ck-editor__editable ck-rounded-corners ck-editor__editable_inline ck-blurred

I think it can go through a review now 😇

Looking for a particular screenshot to confirm that there is no visual regression 🤓

When using the default Basic HTML text format with just the table added I get the following. It basically just doesn't show you the options. Removing the background colour in the CKeditor with the CSS I added doesn't have an impact.

Ah, I see, we're overriding

tr,
.draggable-table.tabledrag-disabled tr {
  color: var(--color-text);
  background: var(--color-white);
}

to

.ck tr,
.ck td {
  background-color: transparent;
}

Questions:

1. Shouldn't we also do this for th, where Claro has background: var(--color-gray-050);?
2. More importantly, shouldn't we generalize this to work for all themes? We can do that by:
   1. Creating a new core/modules/ckeditor5/css/table.css file, just like we already have core/modules/ckeditor5/css/image.css which does serves the same pattern: .ck .image {…}, .ck figcaption … { … } etc.
   2. Add this to a new internal.drupal.ckeditor5.table asset library (modeled after internal.drupal.ckeditor5.image), which only adds this CSS file — besides that it just depends on core/ckeditor5.table
   3. Tweak the CKEditor 5 plugin definitions for ckeditor5_table_properties and ckeditor5_table_cell_properties to load this asset library

   Tada! This should now work consistently across all themes, and we've managed to avoid making Claro CKEditor-aware 🤓

More importantly, shouldn't we generalize this to work for all themes?

That's why we do code reviews 😁. Totally agree with your approach! Will see if I can put that together in the coming days!

I think leaving the hover state background colour is fine, see screenshot below. (unless there is some CSS magic to detect the colour that is set and to tone it down a bit to give the hover effect in the same colour)

I guess we can remove Claro as tag since there is now a general fix? :) also tagging DrupalCamp Ruhr 2023 where I worked on the issue.

This looks great now!

Ideally we'd have automated tests for this, but then we'd just be duplicating the test coverage that CKEditor 5 already has upstream.

I think the only thing that remains is for me (or another CKEditor 5 module maintainer) to do manual testing and scrutinize every line of code 🤓

I hope to RTBC this next week! Great work here 😊🙏

Re-tagging Claro because this specifically needs testing in Claro, even if it's not modifying Claro :)

Thanks @Wim Leers 😉 short feedback loops are nice!

Fixed broken tag.

Tested out all kinds of various combos

Table
Added some background color
Moved the alignment
Added some border at different sizes

But if we are going to add the ability to set background color we should allow them to set the text color. Or auto change it to an accessibility passing color.

Example if I set the background to black the text should be white.

Would be nice to be able to control what properties are available. Though that may be out of scope of this one.
But I do think there should be some way to turn off properties all together for now. That's a lot of power to put into a users hand.
Existing sites may have given some roles the ability to create tables but now they can create tables that could be made into an accessibility issue very easily.

But if we are going to add the ability to set background color we should allow them to set the text color. Or auto change it to an accessibility passing color.

Example if I set the background to black the text should be white.

Isn't that the responsibility of the content editor? E.g we also don't cover someone setting a white text on a white background. Also, core CKEditor is also not covering this. You can test it out here: https://ckeditor.com/docs/ckeditor5/latest/features/tables/tables.html

Would be nice to be able to control what properties are available. Though that may be out of scope of this one.
But I do think there should be some way to turn off properties all together for now. That's a lot of power to put into a users hand.
Existing sites may have given some roles the ability to create tables but now they can create tables that could be made into an accessibility issue very easily.

Yeah, but the feature is only available for full_html anyway. Which should only be given to experienced users because it has implications. I would say it's out of scope? But who am I to say that 😁.

Setting it back to NR for now so @Wim Leers can also take a look/comment about the above.

Fair point about the accessibility being on the editors.

Would still say a yes/no switch would be needed. Can see many sites allowing editors to create tables but not wanting them to have this level of control. Without a switch it’s all or nothing.

Not sure how we can implement that. That would mean we need to conditionally add (or enable/disable) checkbox options to only the full_html text format editor under text styles. Won't that cause an UI quirk?

Don't get me wrong, I get where you are coming from :-) but the fact full_html is already an "elevated rights" thingie, I feel like adding such checkbox logic adds unnecessary complexity.

Hmm sorry, it's not really only "full html".

when not using filter_html (meaning, on a "Full HTML"-esque format).

I guess in that case it would be possible to add a checkbox if that would be desired. Let's see what one of the maintainers of CKEditor thinks about this.

@smustgrave already did the manual testing. Removing tag.

Configuration

Adding configuration for this is definitely possible.

But.

Any Full HTML-esque format already allows ANY markup anyway. So I don't see the point. Even if it would then not be available in the CKEditor 5 table (cell) properties UI, they could still achieve the same by modifying the markup directly.

Which means that for once I must disagree with you, @smustgrave 🙈 I'm curious to hear if you found the above convincing!

Review

1. I have only one nitpick and 2 requests for a clarifying comment (posted a suggestion).
2. Since the very recent changes, AFAIK all MRs need to target the 11.x branch: everything gets merged their first. Would you mind first addressing my feedback on the MR and then transplanting your commits to a new MR that targets 11.x? 🙏

Then this will IMHO be RTBC. It won't ship in 10.1.x anymore though, but it would in 10.2!

Re #47 Will do! Already applied the nits and suggestions to the comments.

The issue fork is confusing me, can't select 10.2.x as a target for the merge request, nor is 11.x available (I do see 11.0.x which seems a very very old branch from Dries himself. Will check later on how to fix that 😅 will probably have to do a fetch manually on the issue fork.

I also can't select 10.2.x yet from the version dropdown here in the issue queue.

Hm … maybe we needed to switch the issue first? 🤔

Hah, looks like you figured it out in the meantime!

Did some manual fetching on the issue fork and then pushed a new branch 😅

So now we have a MR for 11.x and 10.1.x (which will become 10.2.x I guess once it's created)

But what if some formats aren’t full htm but allow for adding a table?

For them to edit the markup directly they’d need the source button no? That may not be present

Still think this will force some existing sites to alter their formats and how they let editors add tables that could be solved with an on/off switch like we do for other buttons.

@smustgrave You won't get the options then. See screenshot in #32

But what if some formats aren’t full htm but allow for adding a table?

You won't get the options then. See screenshot in #32

To elaborate:

+    conditions:
+      plugins:
+        - ckeditor5_table
+        # When arbitrary HTML is already allowed, it's harmless to enable CKEditor 5's UI for table cell properties.
+        - ckeditor5_arbitraryHtmlSupport

This says that this new plugin is automatically enabled if and only if a plugins condition is met. This particular plugins condition says that these 2 plugins (ckeditor5_table and ckeditor5_arbitraryHtmlSupport) must be enabled.

ckeditor5_arbitraryHtmlSupport is only enabled when there are zero HTML restrictions, which typically means that the filter_html filter is not enabled — see \Drupal\ckeditor5\Plugin\CKEditor5PluginManager::getEnabledDefinitions():

    // Only enable the arbitrary HTML Support plugin on text formats with no
    // HTML restrictions.
    // @see https://ckeditor.com/docs/ckeditor5/latest/api/html-support.html
    // @see https://github.com/ckeditor/ckeditor5/issues/9856
    if ($editor->getFilterFormat()->getHtmlRestrictions() !== FALSE) {
      unset($definitions['ckeditor5_arbitraryHtmlSupport']);
    }

This is the same mechanism that e.g. ckeditor5_linkMedia uses to declare it should be enabled whenever ckeditor5_link and media_media are enabled:

ckeditor5_linkMedia:
  ckeditor5:
    plugins:
      - drupalMedia.DrupalLinkMedia
    config:
      # Append the "Link" button to the media balloon toolbar.
      drupalMedia:
        toolbar: [drupalLinkMedia]
  drupal:
    label: Linked Media
    elements: false
    conditions:
      plugins:
        - ckeditor5_link
        - media_media

IOW: it's declarative composability.

Won't die on this hill haha but still seems like something that should be controllable.

@smustgrave Yeah I hear you. But for two decades now, Drupal core's stance has been that if you choose or anything like it, anything goes. Which does help keep the configuration simple of course!

Note that nothing is stopping a contrib module from adding such configuration options :)

If I understood the argument from #53 correctly, it's saying that you should be able to use this feature without having to use Full HTML which I agree with. Ideally we wouldn't introduce use cases where you can do things through the UI that are otherwise not available. Not feeling super strongly about this though, but moving to NR to get input from @Wim Leers & others on this.

you should be able to use this feature without having to use Full HTML

This has been discussed multiple times already.

(The filter_html filter strips 100% of style attributes for security reasons: it's a commonly used attack vector.)

See: #2 to #11-ish. And then again by @smustgrave
😉

Makes sense 👍 I misunderstood the discussion because I only read the few of the last comments 🙈

Yeah sure, no problem 😇 Think Wim already explained his thoughts about it a few times 😉

Committed and pushed 93aa6832a5 to 11.x. Thanks!

Automatically closed - issue fixed for 2 weeks with no activity.

Just uploading a static patch to be used on a D10.1.

Hi murilohp,

Just uploading a static patch to be used on a D10.1.

This patch does not work with the 10.1.5 version, maybe you know why?

@Zulljin: Patch in #66 seemed to run ok for us on 10.1.5.

Hi,
Here is the #66 patch but for Drupal 9 :)

Hi,
updating my last patch (there was a wrong path)

Aaaaand here's a re-roll of the #70 for use in 10.1.5:

Hi There,

I'm on Core 10.1.6 but I still don't see the table property. Has this been fixed in core at all?

Thanks,
Queenie

@queenielow it should be on 10.2.0 or the 11.x branch on which it was committed.

Do we have the possibility to hide some ckeditor's Cell properties, for example, 'background'

Hi @BramDriesen
I just updated to Drupal 10.2 and the

The merge request was not merged, but the diff got committed directly (see #62). Not sure why that was done. The two merge requests should indeed get closed.

Ok. Using this patch works for me on Drupal 10 and I can see the Table properties. How can I import, modify and export the Table and TableProperties now in Drupal 10 custom module plugin for CKEditor5?

Normally I do:
import { Plugin } from 'ckeditor5/src/core'

But this is not found:
"import { Table } from 'ckeditor5/src/table'" or "import { Table } from 'ckeditor5-table/src/table' ". I need to add a new field to that Table Properties Balloon popup, but I am not able to import `Table` or `TableProperties` class. Please advise.

Drupal 9.5.11 patch in case anybody need.