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
- Come up with a shorter list of tags
<$block>should resolve into. This can be hardcoded based on all CKEditor 5 core plugins - Update test assertions to use tags listed on the new list
- Discuss if we want to continue
<$block>as the wildcard name since it's not tied into block level HTML elements - 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
| Comment | File | Size | Author |
|---|---|---|---|
| #29 | 3268307-28.patch | 46.73 KB | bnjmnm |
| #18 | 3268307-18-PoC-no-sourceediting-wildcards-do-not-test.patch | 6.22 KB | wim leers |
Issue fork drupal-3268307
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:
- 3268307-block-wildcard-resolves
changes, plain diff MR !2011
Comments
Comment #2
lauriiiComment #3
lauriiiBased 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 fromisBlock: true. For example,<imageBlock>hasisBlock: truebut does not haveinheritAllFrom: '$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.Comment #4
wim leers#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?
Comment #5
lauriiiI think the next steps here are:
<$block>should resolve into. This can be hardcoded based on all CKEditor 5 core plugins. (I can take care of this)<$block>as the wildcard name since it's not tied into block level HTML elementsComment #6
lauriiiClarifying the title and issue summary.
Comment #7
wim leers#5:
<$block>to<$CKEditor5BlockElement>is clearer.Comment #8
wim leersAFAICT the plan in #5 would also solve #3259367: [upstream] Allow configuring *which* HTML tags can be aligned … marked that postponed on this.
Comment #9
lauriii#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
<$ImplementsCKEditor5Blockwould be most accurate but I think its a bit strange wildcard name.Comment #10
lauriiiIf 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 withinheritAllFrom: '$block'.Comment #11
wim leers#9
#10: 👍🙏
Comment #13
lauriiiStarted 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.
Comment #14
wim leersPosted detailed review on MR.
Two failures:
→
core/modules/ckeditor5/tests/modules/ckeditor5_plugin_elements_test/ckeditor5_plugin_elements_test.ckeditor5.ymlcontained<$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!→ 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! 🚀Comment #15
lauriiiThank you @Wim Leers! Do you have thoughts on #13?
Comment #16
wim leersWe need it if we in the future add more wildcards.
This I'm also wondering about, sort of … but …
… while this is true, it's also kind of valuable to be able to say "allow
idattribute 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:
Are we changing our minds about this being valuable then? It's an important discussion to have for sure.
Comment #17
lauriiiI 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.
Comment #18
wim leersI 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
WildcardHtmlSupportTestbecause 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.
Comment #19
wim leersAlmost there!
Comment #20
wim leersI think this is now RTBC! 🚀🥳
… as soon as tests are green 😅
Comment #21
lauriiiHopefully the most recent commit makes this pass 🤞
Comment #22
wim leersFeeling optimistic 🤓
Comment #23
wim leersYay, green!
🚢
Comment #25
bnjmnmI 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
$blockthat need to be changed. All the existing uses are referring to it's use in CKEditor 5 itself, which is still accurate.Comment #26
ravi.shankar commentedI have addressed a few unresolved threads of MR, please review.
Comment #27
lauriiiRemoving credit from #26 since it was simply applying Gitlab MR suggestions from @bnjmnm, meaning that the credit from those should go to @bnjmnm.
Comment #28
wim leersNice refinements 👍 Let's get this in! 🚀
Comment #29
bnjmnmUploading a patch so I can more easily commit starting with the highest Drupal version.
Comment #34
bnjmnmCommitted 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!