Problem/Motivation

Elements other than <a> wrapping <drupal-media> are not retained on upcast.

Steps to reproduce

  1. Create text format with CKEditor 5, source editing allowing <div> elements, enabled media library + media embed, and HTML filtering disabled
  2. Create content using the text format and embed media
  3. Using source editing, wrap <drupal-media> element with a <div>
  4. 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

Issue fork drupal-3268860

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

lauriii created an issue. See original summary.

Wim Leers’s picture

Wim Leers’s picture

Title: Elements wrapping <drupal-media> are not retained » [drupalMedia] Elements wrapping <drupal-media> are not retained
Issue tags: +JavaScript

I'm assuming this is indeed a bug on our end, and not upstream — can you confirm that, @lauriii?

lauriii’s picture

Title: [drupalMedia] Elements wrapping <drupal-media> are not retained » [upstream] Elements wrapping <drupal-media> are not retained
Status: Active » Postponed
Wim Leers’s picture

🙏

Wim Leers’s picture

Title: [upstream] Elements wrapping <drupal-media> are not retained » Elements wrapping <drupal-media> are not retained
Status: Postponed » Needs work
Issue tags: -Needs tests

Well that is a wonderfully simple solution! Great!

All this needs is a slight comment tweak, then this is RTBC.

lauriii’s picture

Status: Needs work » Needs review

Updated the documentation 👍

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

🚢

lauriii’s picture

bnjmnm’s picture

Status: Reviewed & tested by the community » Needs work

Since 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 👍👍👍👍👍

  1. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
    @@ -240,7 +242,7 @@ function (ConstraintViolation $v) {
    +    $this->host->body->value = '<div data-foo="bar">' . str_replace('drupal-media', 'drupal-media data-foo="bar" ', $original_value) . '</div>';
    

    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)

  2. +++ b/core/modules/ckeditor5/tests/src/FunctionalJavascript/MediaTest.php
    @@ -252,6 +254,10 @@ function (ConstraintViolation $v) {
    +    $this->assertNotEmpty($editor_dom->query('//div[@data-foo="bar"]/drupal-media'));
    

    May as well check for the div/attribute in the upcasted editor markup too.

lauriii’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
73.95 KB
74.11 KB
23.56 KB

Addressed feedback from #11. Moving back to RTBC since the changes are trivial.

bnjmnm’s picture

Issue summary: View changes

  • bnjmnm committed a382219 on 10.0.x
    Issue #3268860 by lauriii, Wim Leers: Elements wrapping <drupal-media>...

  • bnjmnm committed f740064 on 9.4.x
    Issue #3268860 by lauriii, Wim Leers: Elements wrapping <drupal-media>...

bnjmnm’s picture

Version: 10.0.x-dev » 9.3.x-dev

Committed 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.)

lauriii’s picture

Here's a patch for 9.3.x.

  • bnjmnm committed 9bfe62a on 9.3.x
    Issue #3268860 by lauriii, Wim Leers: Elements wrapping <drupal-media>...
bnjmnm’s picture

Status: Reviewed & tested by the community » Fixed

Thanks @lauriii!

Status: Fixed » Closed (fixed)

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