Entity embed dialog can no longer use custom data- attributes in CKEditor 5

Created on 20 December 2023, 11 months ago
Updated 11 April 2024, 7 months ago

Problem/Motivation

We have customized the Entity Embed dialog that appears when embedding a media item to include a custom set of radio buttons that allow the user to select an aspect ratio for the embedded media item.

Now that we're trying to upgrade from Entity Embed 1.4 and CKEditor 4 to Entity Embed 1.5 and CKeditor 5, one of the issues we're running into is with this custom form field that we've added to the dialog no longer working like it used to.

<!--break-->

The selected aspect ratio value is supposed to be stored in the <drupal-entity data-aspect-ratio=""> attribute. However, after upgrading to EE1.5/CKE5 our custom data-aspect-ratio attribute is no longer getting set on the <drupal-entity> element.

Steps to reproduce

Here's our code that adds the custom aspect ratio form element to the dialog, linked to the data-aspect-ratio attribute:

I've verified that this is an allowed attribute in the CKEditor 5 source editing:

When I inspect what form data gets passed to the server when the dialog is closed, the data-aspect-ratio is missing in the query parameters:

Proposed resolution

If I change our custom form element to use a different attribute name like data-caption or alt then the data does get passed. So it seems like there's a new whitelist of allowed attributes within Entity Embed.

Can you provide some way to customize what attributes can be passed from the Entity Embed dialog to the <drupal-entity> element? Or can it use whatever is allowed in the CKEditor Source Editing configuration?

Remaining tasks

User interface changes

API changes

Data model changes

πŸ› Bug report
Status

RTBC

Version

1.0

Component

Code

Created by

πŸ‡ΊπŸ‡ΈUnited States RichardDavies Portland, Oregon

Live updates comments and jobs are added and updated live.
  • JavaScript

    Affects the content, performance, or handling of Javascript.

Sign in to follow issues

Merge Requests

Comments & Activities

  • Issue created by @RichardDavies
  • πŸ‡ΊπŸ‡ΈUnited States RichardDavies Portland, Oregon
  • πŸ‡§πŸ‡ͺBelgium wim leers Ghent πŸ‡§πŸ‡ͺπŸ‡ͺπŸ‡Ί

    FYI: this logic must be modified to pass all attributes to the server side: also the GHS ones, and not just the ones explicitly used/supported/consumed by the entity_embed CKE5 plugin:

          this.listenTo(buttonView, 'execute', (eventInfo) => {
            const element = editor.model.document.selection.getSelectedElement();
            const libraryURL = Drupal.url('entity-embed/dialog/' + options.format + '/' + element.getAttribute('drupalEntityEmbedButton'));
    
            let existingValues = {};
    
            for (let [key, value] of element.getAttributes()) {
              let attribute = entityEmbedEditing.attrs[key]
              if (attribute) {
                existingValues[attribute] = value
              }
            }
    
            // Open a dialog to select entity to embed.
            this._openDialog(
              libraryURL,
              existingValues,
              ({ attributes }) => {
                editor.execute('insertEntityEmbed', attributes);
                editor.editing.view.focus();
              },
              dialogSettings,
            )
    

    β€” https://drupal.slack.com/archives/C01GWN3QYJD/p1703092541221209?thread_t...

    P.S. there already is explicit test coverage for additional attributes not getting stripped. So this is NOT a data loss issue! This issue is requesting the ability for the server side rendered dialog to be hook_form_alter()ed to support editing additional attributes.

  • First commit to issue fork.
  • Merge request !31support ghs attribute setting in dialogs β†’ (Open) created by bnjmnm
  • Status changed to Needs review 11 months ago
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI
  • πŸ‡ΊπŸ‡ΈUnited States bnjmnm Ann Arbor, MI

    It . I'm guessing this may be 10.2 conflicts that impact the entire module

  • @bnjmnm Thanks for the MR! I've confirmed it fixes the issue described, but I did find some bugs:

    1. If a wildcard is defined in the Source editing plugin's allowed tags, e.g. <drupal entity data-*>, then a TypeError: attr.key.replace is not a function is thrown.

    2. If one of the default data attributes is defined in the allowed tags, e.g. <drupal-entity data-entity-type data-entity-uuid>, then the media doesn't render after embedding. I believe this is because the default key in the attrs object, e.g. "drupalEntityEntityType", gets superseded by the now-automatically-added "dataEntityType", causing some other part of the code to break.

    I've attached a patch to your MR that works around these two issues.

    For #2, the fix is easy enough: add a check that the attribute is not already defined in the attrs array.

    For #1, it's a bit trickier. It appears CKEditor's SchemaItemDefinition.register method does not accept RegExp objects in the allowAttributes property. That means there's no way to simply pass the wildcard along, AFAICT.

    Therefore, I simply added a typecheck to simply skip if attr.key is not a string, but this is quite opaque - I'm not sure if it'd be better to actually throw an error instead, so the user isn't confused if their wildcard is not respected. That is, of course, unless you have a better idea of how to handle wildcards.

  • πŸ‡¦πŸ‡ΊAustralia yovince Melbourne

    Thank you @bnjmnm and @odensc for your contributions. After applying the two patches you suggested, everything is working perfectly. For my convenience, I've combined both patches into one.

  • Status changed to RTBC 10 months ago
  • πŸ‡­πŸ‡ΊHungary szato

    Using patch #9 with D10.2.2 solved our issue (broken custom attributes)

    Thank you!

  • πŸ‡¦πŸ‡ΊAustralia Nadim Hossain

    Re-rolled the patch for the new release of Entity Embed version 1.6
    I couldn't push the merge request resolved conflict though,

Production build 0.71.5 2024