Problem/Motivation

Discovered while working on #3228334: Refactor HTMLRestrictionsUtilities to a HtmlRestrictions value object.

#3216015: Generate CKEditor 5 configuration based on pre-existing text format configuration introduced SmartDefaultSettings, to compute upgrade paths automatically.

Right now, there is a single ckeditor5_alignment plugin with 5 toolbar items (align left, align center, align right, justify — plus a dropdown providing access to all 5), with the following plugin definition:

ckeditor5_alignment:
  …
  drupal:
    …
    elements:
      - <$block class="text-align-left text-align-center text-align-right text-align-justify">

This means that any Drupal 8|9 text format configured to allow one or more of those classes will get all 5 of those toolbar items configured in CKEditor 5 and will have all four of those attribute values allowed.

Steps to reproduce

Update from a text format using CKEditor with <p class="text-align-center"> in the list of allowed HTML tags of filter_html.

Proposed resolution

In order for SmartDefaultSettings to be able to generate more precise CKEditor 5 configuration, we need to split up this plugin definition; so we know that for example the alignment:center button only generates <$block class="text-align-center">, so that it can automatically only allow that plugin.

Remaining tasks

User interface changes

None.

API changes

None.

Data model changes

None.

Release notes snippet

N/A

CommentFileSizeAuthor
#2 3259179-2.patch2.2 KBwim leers
#2 3259179-after-3259174-do-not-test.patch4.37 KBwim leers

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Active » Needs review
StatusFileSize
new 3259179-after-3259174-do-not-test.patch4.37 KB
new 3259179-2.patch2.2 KB

This patch could land right away — it's an uncontroversial splitting of an existing CKEditor 5 plugin definition. At least, if I'm right, and this will not cause any test failures 🤞

I've also shown what additions would be necessary if this lands after #3259174: Add missing CKE5 SmartDefaultSettings test coverage (wildcard tag with unsupported attribute).

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Needs review » Reviewed & tested by the community

It makes sense to me that users might want to limit what options are available for aligning the text. The solution is straight forward and works for me. I guess an alternative approach would be to allow configuring which options are available in the dropdown, but what's here works for me.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented

Reviewing this made it apparent that the alignment controls being buttons AND dropdowns should be addressed, but not in this scope. Created #3259593: Alignment being available as separate buttons AND in dropdown is confusing as a followup.

bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/ckeditor5/ckeditor5.ckeditor5.yml
@@ -313,23 +313,59 @@ ckeditor5_alignment:
+    <<: *alignment_drupal_section

This results in the addition of all 4 text-align-x classes added to "allowed tags", even if a single button is chosen. Removing the merge key use address that. That leads me to wonder if it's expected behavior and I did not correctly interpret the issue summary.

In Proposed Solution I see

so we know that for example the alignment:center button only generates <$block class="text-align-center">

so that leads me to believe the expectation is to add the classes per-button, which is isn't currently doing, so this either needs the patch changed or the issue summary updated to make the expected result a little clearer.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#3259593: Alignment being available as separate buttons AND in dropdown is confusing

#4++ — thanks for creating that!

#5: No, it does not. YAML syntax is weird. The << operator inherits everything from the referenced variable except for explicitly defined keys. It's basically the YAML equivalent of PHP's + operator on arrays. Copy/paste it into https://yamlvalidator.com/ to observe the behavior.

This results in the addition of all 4 text-align-x classes added to "allowed tags", even if a single button is chosen.

That's not what I see — neither in the UI, nor if I skip the YAML validation logic above and instead inspect it using the plugin definition:

$ vendor/bin/drush ev "var_dump(\Drupal::service('plugin.manager.ckeditor5.plugin')->getDefinition('ckeditor5_alignment.center')->getElements())"
array(1) {
  [0]=>
  string(34) "<$block class="text-align-center">"
}

  • bnjmnm committed 9b0a3b4 on 9.3.x 
    Issue #3259179 by Wim Leers, lauriii: Split ckeditor5_alignment CKEditor...

  • bnjmnm committed f7da300 on 9.4.x 
    Issue #3259179 by Wim Leers, lauriii: Split ckeditor5_alignment CKEditor...

  • bnjmnm committed 109ceec on 10.0.x 
    Issue #3259179 by Wim Leers, lauriii: Split ckeditor5_alignment CKEditor...
bnjmnm’s picture

bnjmnm
he/him
Primary language English
Location Ann Arbor, MI
commented
Status: Needs work » Fixed

What I ran into #5 surprised me as well and deviated from my understanding of the << operator. Whatever the circumstances were that caused the odd behavior reported in #5, I was unable to reproduce despite considerable attempts do to so.

This is a very straightforward and welcome change that is behaving exactly as I'd expect it to, so I'm committing to 9.4.x, 10.0.x, as well as 9.3.x since it is a safe change to an experimental module that adds important upgrade path functionality.

Status: Fixed » Closed (fixed)

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

dom.’s picture

dom. commented

This issue introduced "regression" in the sense that is crashes the CKE5 when multiple alignments buttons are activated to the toolbar.
See #3273510: CKEditor 5 crash when multiple alignment buttons are activated due to duplicate configuration