Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Elements other than <a>
wrapping <drupal-media>
are not retained on upcast.
Steps to reproduce
- Create text format with CKEditor 5, source editing allowing
<div>
elements, enabled media library + media embed, and HTML filtering disabled - Create content using the text format and embed media
- Using source editing, wrap
<drupal-media>
element with a<div>
- Go back to editing view by clicking source editing button, and go back to source editing to confirm that the
<div>
is now gone 😐
Proposed resolution
Per this thread, we need to register drupal-media as a block element with CKEditor 5, so this.editor.editing.view.domConverter.blockElements.push('drupal-media');
would take care of that
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#18 | 3268860-12-d93.patch | 73.91 KB | lauriii |
#12 | intediff.txt | 23.56 KB | lauriii |
#12 | 3268860-12-d94.patch | 74.11 KB | lauriii |
#12 | 3268860-12-d10.patch | 73.95 KB | lauriii |
| |||
#10 | 3268860-10-d94.patch | 73.72 KB | lauriii |
Issue fork drupal-3268860
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:
- 3268860-elements-wrapping-drupal-media changes, plain diff MR !1957
Comments
Comment #3
Wim LeersComment #4
Wim LeersI'm assuming this is indeed a bug on our end, and not upstream — can you confirm that, @lauriii?
Comment #5
lauriiiOpened upstream bug for this: https://github.com/ckeditor/ckeditor5/issues/11489.
Comment #6
Wim Leers🙏
Comment #7
Wim LeersWell that is a wonderfully simple solution! Great!
All this needs is a slight comment tweak, then this is RTBC.
Comment #8
lauriiiUpdated the documentation 👍
Comment #9
Wim Leers🚢
Comment #10
lauriiiComment #11
bnjmnmSince there is https://github.com/ckeditor/ckeditor5/issues/11489 to refer to, the solution is clearly the right approach, and a simple one.
Two things I spotted in the tests I think could be improved, but this is basically 👍👍👍👍👍
Giving the enclosing div and the drupal-media element the same attribute+value suggests a relationship that isn't the case (but is feasible considering media passes attributes around in all sorts of ways on conversion. This could be improved by making the div attr more distinct (even just a value change is OK)
May as well check for the div/attribute in the upcasted editor markup too.
Comment #12
lauriiiAddressed feedback from #11. Moving back to RTBC since the changes are trivial.
Comment #13
bnjmnmComment #17
bnjmnmCommitted to 10.0.x and 9.4.x. The 9.4.x commit isn't cherry pickable to 9.3.x, but I will commit a patch rolled for 9.3.x since CKEditor 5 is experimental and this might be a desirable change (I suppose that depends on there not being major differences between CKEditor 32/33 specific to this solution.)
Comment #18
lauriiiHere's a patch for 9.3.x.
Comment #20
bnjmnmThanks @lauriii!