Drupal.ckeditor5.openDialog missing existingValues param

Attached patch in accordance to the proposed resolution

This can potentially break contribs that calls this function

Yes, that's right. Although I wonder how many contribs actually use this. TBH, I only discovered this method due to this bug. I couldn't find any documentation on this. Neither any info on the related Drupal.ckeditor.openDialog.

Especially due to the missing existingValues param this method (in contrast to the version for CK4) is almost useless, I guess. I resorted to a direct Drupal.ajax() call for now.

@mglaman Any chance you could write a test? 🤞🙏😊

Why not make existingValues the last parameter and default it to {}? This way we won't break any contrib calling this method without it

ckeditor5.es6.js has been removed since the previous patch

Also attaching the Drupal 9 patch version which includes ckeditor5.es6.js

The only argument against it would perhaps be the symmetry to the CKE4 version. But I don't really oppose the idea. Just a thought.

I see 2 options here:
- keep the CKE4 parameters order in sake of the symmetry. Expect issues with the contrib modules calling Drupal.ckeditor5.openDialog. Not sure if there is a way to evaluate how many of them actually do that. Anyways this approach would require collaboration with the maintainers of each of those modules
- move the existingValues param at the end and default it to {} as I did in the patches #11 and #12. Given the difference between CKE4 and CKE5 I think that the parameters inconsistency is less evil than having to adjust contrib modules

One way or another I needed these patches in order to make the insert_view_adv contrib module work https://www.drupal.org/project/insert_view_adv/issues/3356931

I'd be in favor of option 2.

Anyone who has used this function in CKE5 so far obviously hasn't used existing values or else would have needed a patch/custom solution, so better to keep the function backwards compatible

The primary thing missing here is test coverage. Any takers? 😊🤞

Just adding that there isn't currently a test for this without the patch. Currently only the media library plugin calls this function in core (see core/modules/ckeditor5/ckeditor5.ckeditor5.yml)

#17: valuable contribution, thanks 😊 That raises another question: why does the Media Library CKEditor 5 plugin in Drupal core not run into this problem? 🤔

@Wim Leers. Very good question. I asked myself the same thing. The answer is: The Drupal Media Library plugin has no edit feature. If you click on the "Insert media" button a dialog is opened and you can select a media file. The media file is then inserted at the cursor position. However, there is no real way to "edit" the media file. You cannot double-click and if you highlight the media item and click on the button again, just normal dialog is opened again but the current file is not preselected. It's okay-ish I guess for this use-case. It would be nicer though, if the library would preselect the file. If you use sth. like media_directories this gets worse because you have to select the same folder again. Long story short, there currently isn't any information flow from CKEditor to the dialog - only from the dialog back to CKEditor. That's why this feature isn't needed.

...but this might be needed as more and more contrib issues surface.

For example, this one from ui_icons, where there is a request for allowing to edit an inserted icon on CKE5. This makes total sense from the user point of view - I insert an icon, i make use of a setting like "color" and "size", I want to change it, I am surprised that there is absolutely no way to edit it and the only solution is to remove it and insert it again.

Modules such as Embedded Content found a way around this, but it involves creating its very own openDialog function that mimicks core's but actually passes the existing values:

export const openDialog = (button, saveCallback, existingValues) => {
    const ckeditorAjaxDialog = Drupal.ajax(
        {
            dialog: {
                width: button.dialogSettings.width,
                height: button.dialogSettings.height,
                class: button.class,
            },
            (...)
            submit: existingValues, // <---- THIS IS THE MAIN CHANGE
        }
    );
  ckeditorAjaxDialog.execute();

  // Store the save callback to be executed when this dialog is closed.

  // We already take into account the possibility of supporting multiple modals.
  // @see https://www.drupal.org/project/drupal/issues/2741877
  if (Drupal.ckeditor5.saveCallback instanceof Map) {
    Drupal.ckeditor5.saveCallback.set('#embedded-content-dialog-form-' + button.editor_id, saveCallback);
  } else {
    Drupal.ckeditor5.saveCallback = saveCallback;
  }
};

So I think there is a case to retake this issue and move it forward...

Another module that is struggling with this issue is Ckeditor5 Bootstrap Grid, see here.

I have a custom module—one that allows the selection of an entity via an entity reference field and inserts a snippet—that also struggles with this, and had to implement my own dialog similar to #22 to take care of this. Swapping the referenced entity without destroying the text it was attached to proved impossible without a workaround.