Problem/Motivation

#3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal resolves the <$block> wildcard based on incorrect list of tags. The good thing is that the current list should be a superset of what it actual should resolve into. However, the list that it should resolve into is significantly smaller, and for that reason we should update the list.

Proposed resolution

Come up with a shorter list of tags <$block> should resolve into. This can be hardcoded based on all CKEditor 5 core plugins. Rename Drupal's <$block> to <$text-container> to distinguish it from CKEditor 5's config $block in a manner that better describes its purpose.

Remaining tasks

  1. Come up with a shorter list of tags <$block> should resolve into. This can be hardcoded based on all CKEditor 5 core plugins
  2. Update test assertions to use tags listed on the new list
  3. Discuss if we want to continue <$block> as the wildcard name since it's not tied into block level HTML elements
  4. Open follow-up for providing an API to extend the wildcard tags so that contrib modules can provide additional tags that should be resolved from a wildcard

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3268307

Command icon 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

lauriii created an issue. See original summary.

lauriii’s picture

lauriii’s picture

Based on some additional research I did on $block, we should not include <drupal-media> as part of elements that resolve from $block, since <drupal-media> element should not contain anything inside.

I think the list of elements that we resolve $block into might be too expansive. The $block in the metadata doesn't actually resolve to all block level elements, but to elements that have been configured to inheritAllFrom: '$block' which is different from isBlock: true. For example, <imageBlock> has isBlock: true but does not have inheritAllFrom: '$block', therefore it's should not be considered as a $block element.

I think this is the most accurate possible list of elements $block should resolve into: https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-par.... I think we should use that instead of the current list returned by \Drupal\ckeditor5\HTMLRestrictions::getBlockElementList. We need to decide if there should be an API to extend that list somehow. It might not be necessary for now - it might be something that could be added later if there's demand for that.

wim leers’s picture

#3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal is in!

Let's push this forward now.

@lauriii we discussed this in last week's CKEditor 5 meeting. You had the best understanding of next steps. What do you propose we do here?

lauriii’s picture

I think the next steps here are:

  1. Come up with a shorter list of tags <$block> should resolve into. This can be hardcoded based on all CKEditor 5 core plugins. (I can take care of this)
  2. Update test assertions to use tags listed on the new list
  3. Discuss if we want to continue <$block> as the wildcard name since it's not tied into block level HTML elements
  4. Open follow-up for providing an API to extend the wildcard tags so that contrib modules can provide additional tags that should be resolved from a wildcard
lauriii’s picture

Title: $block wildcard does not include <drupal-media> » $block wildcard resolves into a superset of the actual $block tags
Issue summary: View changes

Clarifying the title and issue summary.

wim leers’s picture

#5:

  1. Devil's advocate: if we do what is described in #5, then couldn't we have avoided #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal altogether?
  2. /
  3. I think renaming <$block> to <$CKEditor5BlockElement> is clearer.
  4. +1
wim leers’s picture

lauriii’s picture

#7.1 We still need the algorithm from #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal for resolving the wildcards so I don't think we could have avoided that.
#7.3 Something like <$ImplementsCKEditor5Block would be most accurate but I think its a bit strange wildcard name.

lauriii’s picture

If anyone wants to give this a try, I believe the list we should use is this: <div> <p> <h1> <h2> <h3> <h4> <h5> <h6> <pre> <li>. This is all elements that are registered to schema with inheritAllFrom: '$block'.

wim leers’s picture

#9

  1. Good point! 😊
  2. Who cares if it's a long name? It's only going to be visible in code anyway, in CKEditor 5 plugin definitions? 🤓 Clarity above brevity?

#10: 👍🙏

lauriii’s picture

Started wondering about #7.1 more. I think we potentially need some of the logic from #3260869: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal but maybe some of it could be simplified. Since we now have a reliable list of elements to resolve to, I'm wondering if we for example need the GHS fallback configuration? I'm also wondering if we should allow using wildcards in the source editing plugin configuration? If we removed the source editing wildcards, that would potentially allow us to simplify some of the code.

wim leers’s picture

Status: Active » Needs work

Posted detailed review on MR.

Two failures:

  1. 1) Drupal\Tests\ckeditor5\Kernel\CKEditor5PluginManagerTest::testProvidedElements with data set "blockquote combo"
    

    core/modules/ckeditor5/tests/modules/ckeditor5_plugin_elements_test/ckeditor5_plugin_elements_test.ckeditor5.yml contained <$block data-everyblock> and has been updated, but it does not apply to <blockquote> anymore with the updated list.

    It's not clear to me yet what the correct solution is here, it really depends on what we define the list of tags corresponding to <$implements-ckeditor5-block> to be. https://github.com/ckeditor/ckeditor5/blob/master/packages/ckeditor5-par... lists <blockquote>, but our implementation does not. This confirms the need to really thoroughly document the rationale for the list of tags we hardcode!

  2. 1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "basic_html_with_alignable_p can be switched to CKEditor 5 without problems, align buttons added automatically" 
    

    → the expected superset should change now; several tags that <$block> used to expand into and <$implements-ckeditor5-block> does not means that the expectations need to change. This is definitely a net improvement! 🚀

lauriii’s picture

Thank you @Wim Leers! Do you have thoughts on #13?

wim leers’s picture

I'm wondering if we for example need the GHS fallback configuration

We need it if we in the future add more wildcards.

I'm also wondering if we should allow using wildcards in the source editing plugin configuration?

This I'm also wondering about, sort of … but …

If we removed the source editing wildcards, that would potentially allow us to simplify some of the code.

… while this is true, it's also kind of valuable to be able to say "allow id attribute on all tags matching wildcard X".

So I'm sort of torn.

OTOH, being explicit is probably better?

So I'm personally +1 to removing the ability to use wildcards in "source editing" — that crossed my mind as an odd capability in #3260869-11: Resolve mismatch between <$block> interpretation by CKEditor 5 and Drupal, but I considered it valuable then:

[…] somehow along the way we got sidetracked in this MR: we instead focused on adding support for <$block> to the Source Editing plugin's configuration, even though that's never been mentioned as being in scope. I'm fine with adding this ability (it's valuable!), so I'm fine with keeping this. But we've definitely not yet solved the critical portion of this — and looks like both @lauriii and I missed that so far 🙈

Are we changing our minds about this being valuable then? It's an important discussion to have for sure.

lauriii’s picture

Are we changing our minds about this being valuable then? It's an important discussion to have for sure.

I don't think my thinking has drastically changed on how valuable it's to have that behavior. The change is that we used to get that with a low cost as part of that issue - I think after this change, it would be the main reason we would have to keep some of the complex code, and therefore I think it could make sense to remove that capability.

wim leers’s picture

it would be the main reason we would have to keep some of the complex code

I don't think that's true. Unless I'm missing something, I think all we'd be able to remove is this (plus we'd need to change the test coverage in WildcardHtmlSupportTest because we can't use the source editing plugin anymore to do our testing).

👆 That's a genuine question: what else do you think we could remove/simplify if we were to do what you wrote in #17 besides what the attached patch does?

It would slightly simplify #3260857: Expand SourceEditingRedundantTagsConstraintValidator to also check attributes and attribute values though.

wim leers’s picture

Almost there!

wim leers’s picture

I think this is now RTBC! 🚀🥳

… as soon as tests are green 😅

lauriii’s picture

Status: Needs work » Needs review

Hopefully the most recent commit makes this pass 🤞

wim leers’s picture

Status: Needs review » Reviewed & tested by the community

Feeling optimistic 🤓

wim leers’s picture

Yay, green!

🚢

bnjmnm made their first commit to this issue’s fork.

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

I left some MR comments, just docs stuff (surprise!).

The rationale for changing to hard-coded tags is well explained and makes sense. I also confirmed there aren't any stray uses of $block that need to be changed. All the existing uses are referring to it's use in CKEditor 5 itself, which is still accurate.

ravi.shankar’s picture

Status: Needs work » Needs review

I have addressed a few unresolved threads of MR, please review.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

Removing credit from #26 since it was simply applying Gitlab MR suggestions from @bnjmnm, meaning that the credit from those should go to @bnjmnm.

wim leers’s picture

Nice refinements 👍 Let's get this in! 🚀

bnjmnm’s picture

Issue summary: View changes
StatusFileSize
new46.73 KB

Uploading a patch so I can more easily commit starting with the highest Drupal version.

  • bnjmnm committed 7b1e43d on 10.0.x
    Issue #3268307 by lauriii, Wim Leers: $block wildcard resolves into a...

  • bnjmnm committed 1a59838 on 9.4.x
    Issue #3268307 by lauriii, Wim Leers: $block wildcard resolves into a...

  • bnjmnm committed c17eea2 on 9.3.x
    Issue #3268307 by lauriii, Wim Leers: $block wildcard resolves into a...

bnjmnm’s picture

Version: 9.4.x-dev » 9.3.x-dev
Issue summary: View changes
Status: Reviewed & tested by the community » Fixed

Committed to 10.0.x and cherry picked to 9.4.x and to 9.3.x since CKEditor 5 is experimental and this is a desirable improvement.

The switch to hard-coded tags representing $block is nicely justified in this issue and removes a layer of confusion in a module with many layers of confusion. Pleased to see this in!

Status: Fixed » Closed (fixed)

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