[PP-1] Support Drupal core's Media Library

I need this for a new 8.6 site, and would be happy to help with the coding if given some guidance.

I see in demos from the core team they have a working embed feature, is that only available in 8.7? Was their code based on this module?Would it be possible to backport their work to 8.6, since media library exists in 8.6 it would be nice to have.

Will the need for this module be eliminated once core releases their embed / media library solution as part of 8.7?

This seems pretty critical…

#2938116: Allow media to be uploaded with the Media Library field widget landed a few months ago, just in time for Drupal 8.6! Let's get this to work :)

The core media library was written without any preliminary support for CKEditor, or anything Entity Embed does. But support for CKEditor is a feature target for Drupal core 8.7, and if we implemented that in this issue, we'd have a clear patch to spruce up and commit to core. So yes, this is critical ;)

Given your experience with Media Library, could you sketch out at a high level in the issue summary how this should work ideally, what the tricky bits are, what pieces of code this should look to as an example, and any gotcha you can think of that we'd likely run into?

Having looked at Media Library's code, I have a few thoughts on how we can do this.

Every media library "widget", for lack of a better term, has some code which sends an AJAX response containing the IDs of the selected/created media items. Normally, the commands contained in the response will update the value of a hidden field, and then click a hidden button to trigger an AJAX refresh.

However, with a few changes in core, there is no real reason why those AJAX commands couldn't, say, insert a drupal-entity tag into a CKEditor instance...

That, I think, would be the quickest path for Entity Embed to work with Media Library. We'll need a few upstream changes in core, I believe, but that's not impossible.

there is no real reason why those AJAX commands couldn't, say, insert a drupal-entity tag into a CKEditor instance...

I'd be very hesitant to do this, because it'd make Media directly aware of CKEditor-specific stuff. Why not use a dialog (like \Drupal\editor\Form\EditorLinkDialog and \Drupal\editor\Form\EditorImageDialog), whose submitted values are mapped to CKEditor commands by a CKEditor plugin? Then any site that uses a text editor other than CKEditor would have to do far less work to integrate their text editor.

Which brings me to my key question for you: how would you imagine that a \Drupal\editor\Form\MediaLibraryDialog would work? How would it integrate with the Media Library? Could we reuse the code you're referring to there? Do you foresee any challenges?

Ever since #3020716: Add vertical tabs style menu to media library, the direction of the Media Library has been cemented. Time to get this moving forward.

Here's an initial patch which:

1. checks if the Media Library is installed
2. if it is, then it automatically uses the Media Library's selection dialog instead of EntityEmbedDialog
3. it respects the bundles selected for the EmbedButton at /admin/config/content/embed/button/manage/media
4. it correctly integrates with the Media Library infrastructure, particularly it uses MediaLibraryState as introduced and intended by #3038241: Implement a tamper-proof hash for the media library state

Remaining tasks:

1. it looks crappy, because Drupal.ckeditor.openDialog() adds its own dialog settings — solution TBD
2. it should probably only allow a single item to be selected at a time rather than multiple
3. it doesn't support saving yet
4. it doesn't support editing existing embeds yet
5. tests
6. probably more

While working on this, discovered a minor bug for which I created #3057370: MediaLibraryState::fromRequest() may result in invalid MediaLibraryState::create() call.

Updating IS per #10.

3. it should probably only allow a single item to be selected at a time rather than multiple

Fixed.

Now we have 0 of 1 item selected or 1 of 1 item selected in the bottom right. I don't think this makes sense in the context of CKEditor.

4. it doesn't support saving yet

This task is evidently the most important one right now.

Unfortunately, \Drupal\media_library\Plugin\views\field\MediaLibrarySelectForm::updateWidget() currently is hardcoded to a entity reference-triggered update:

  public static function updateWidget(array &$form, FormStateInterface $form_state) {
    $field_id = $form_state->getTriggeringElement()['#field_id'];
    $selected = array_filter(explode(',', $form_state->getValue($field_id, [])));

    $response = new AjaxResponse();
    $response->addCommand(new CloseDialogCommand());

    $ids = implode(',', $selected);

    $opener_id = MediaLibraryState::fromRequest($form_state->get('view')->getRequest())->getOpenerId();
    if ($field_id = MediaLibraryWidget::getOpenerFieldId($opener_id)) {
      $response
        ->addCommand(new InvokeCommand("[data-media-library-widget-value=\"$field_id\"]", 'val', [$ids]))
        ->addCommand(new InvokeCommand("[data-media-library-widget-update=\"$field_id\"]", 'trigger', ['mousedown']));
    }

    return $response;
  }

Because MediaLibraryWidget::getOpenerFieldId() returns NULL, we're simply returning an empty AJAX response. #3044649: Delegate media library selection handling to the "thing" that opened the library is designed to address this. Currently investigating that.

I found a way to make this work without depending on #3044649: Delegate media library selection handling to the "thing" that opened the library, which allows us to make this work even in Drupal 8.7 without further changes.

For now, it's hardcoded to embed the Media entity in teaser view mode, and doesn't offer you to select any @EntityEmbedDisplay plugin (which we won't port to Drupal core either anyway). In Drupal core we aim to do something similar to #2246533: Add 'embed' view mode for all entity types, to allow sensible embedding out-of-the-box, so we'd hardcode it to embed view mode there (at least initially).

Also: while the UX is less than ideal, "editing" existing embeds already works too: upon double-clicking an embedded Media entity, you get to see the Media Library again and can select a different entity. Your new choice would overwrite the previous choice.

Let's ensure this only works on versions of Drupal core where the necessary Media Library infrastructure exists: \Drupal\media_library\MediaLibraryUiBuilder was added in 8.7, so none of this will work with Drupal 8.6's media_library.module.

d.o is drunk. I added a tag and attached files, but it lost both. :| Or rather, they're attached, but don't show up in #16…

2. it looks crappy, because Drupal.ckeditor.openDialog() adds its own dialog settings — solution TBD

Fixed.

Updated screenshot in IS.

#15 hardcoded teaser. Let's hardcode embed instead. And let's make it actually work.

I've configured my embed view mode for the Image MediaType to use the medium image style, which looks like this:

(One small problem: the .visually-hidden label is … visible.)

9. Ensure .visually-hidden is respected.

Fixed.

LOL, uploaded the same screenshot again in #20, oops.

If you switch to any of the non-default vertical tabs, insertion into the text editor doesn't work. entity_embed_form_views_form_media_library_widget_alter() doesn't work in this case because $form_state->getUserInput()['editor_object'] isn't set in this case. Will need to figure out a work-around tomorrow.

10. Ensure that after switching to another vertical tab (another media type), inserting into the text editor still works

Fixed.

Reverting this patch and using Entity Embed HEAD's behavior, I notice that there's the ability to specify alignment and a caption. It does that by using a multi-step form. Should we do the same here? Won't that make the UX much more complex? Besides data-align and data-caption, we'll also need to be able to specify an embed-specific alt for Image media for example, but like I wrote at #2994702-3: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`: the Media entity reference field also doesn't currently allow that to be specified, so why should we do that for CKEditor?

In short; the Media entity reference field doesn't yet allow this (just discovered #3023807: Override media fields from the reference field exists for that), and if it did, then we would be able to rely on the UX defined there to also specify alignment and captioning.

Assigning this to @phenaproxima to get input from him and @seanB (to whom I can't assign it) on how they believe we should proceed here. AFAICT this means this is blocked on core providing direction in #3023807.

Updating status per #25.

Reverting this patch and using Entity Embed HEAD's behavior, I notice that there's the ability to specify alignment and a caption. It does that by using a multi-step form. Should we do the same here? Won't that make the UX much more complex? Besides data-align and data-caption, we'll also need to be able to specify an embed-specific alt for Image media for example, but like I wrote at #2994702-3: [PP-3] Allow editors to alter embed-specific metadata in CKEditor: the Media entity reference field also doesn't currently allow that to be specified, so why should we do that for CKEditor?

I am not sure I can provide a good answer to this yet. We know there are strong wishes for users to add extra metadata when using media, whether this is an entity reference field or WYSIWYG editor. From a UX perspective it would be nice if providing metadata always works the same way. That being said, the WYSIWYG editor has the big disadvantage it has to show the rendered media item directly in the editor, which means the metadata fields have to be hidden in the "place where the user returns". We probably need to discuss this together with the UX team.

One idea could be to have some kind of metadata event, where openers could specify the metadata they allow. If there is extra metadata, the media library adds an extra step where the metadata can be provided for each media item. The entity reference widget could then display the metadata with an edit button or something like that. The WYSIWYG integration probably has to rely on a right-click or double-click to bring the metadata form back.

Just some thoughts.

Assuming #3038254-31: Delegate media library access to the "thing" that opened the library and #3044649-4: Delegate media library selection handling to the "thing" that opened the library land (the latter builds on top of the former), these are the changes we'd need to make. Note that we unfortunately still need entity_embed_form_views_form_media_library_widget_alter() to attach the editor/drupal.editor.dialog asset library.

#27

One idea could be to have some kind of metadata event, where openers could specify the metadata they allow. If there is extra metadata, the media library adds an extra step where the metadata can be provided for each media item. The entity reference widget could then display the metadata with an edit button or something like that. The WYSIWYG integration probably has to rely on a right-click or double-click to bring the metadata form back.

I really like this proposal!

The tricky bit is that there are two kinds of metadata then: those that override existing Media entity metadata (e.g. alt), and those that decorate this particular use of a Media entity (e.g. data-align). I think that can be workable though.

The tricky bit is that there are two kinds of metadata then: those that override existing Media entity metadata (e.g. alt), and those that decorate this particular use of a Media entity (e.g. data-align). I think that can be workable though.

From a media library perspective those "details" are for the opener to decide. The media library provides a standard interface to collect the values, whatever the opener wants/needs to do with it after that is up to the opener I guess. We might want to go as far as letting the openers provide the actual form elements to collect the data. That way the opener can use fieldsets to group metadata fields. Not sure about that last part, but I guess we should at least make sure the media library doesn't care about those details.

Right; the Media Library shouldn't care about that, but the UX for providing those "details" does need to form a coherent whole together with the selection experience. Since the selection experience is owned by the Media Library module, I think it makes sense to also expect the Media Library module to provide a standardized way to "provide details".

Created issue for #20: #3057578: .visually-hidden not respected in CKEditor preview.

#3057578: .visually-hidden not respected in CKEditor preview landed, rebased #24.

Now 100% of the patch is focused on the scope of this particular issue.

So, first of all, +1 to the general implementation direction Wim came up with in #28. That's quite elegant, in my opinion, and dovetails nicely with the event-based architecture we're working to get into core. The need for a form_alter is an unfortunate wart, but it also seems like something we might be able to address further down the line.

As far as the added UX complexity...I'd like an actual UX professional to validate this (or shoot it down), but IMHO, having a multi-step form for every single embedded item increases the cognitive load to unacceptable levels. In my opinion, choosing an item from the media library should immediately embed it into the editor, no questions asked, using whatever available default values can be gleaned from the embedded item itself (such as alt text). Then, the user should have the ability to edit the embed after the fact and tweak it to their liking -- maybe by right-clicking (or hopefully some kind of accessible equivalent, depending on what kind of wiggle room CKEditor gives us) to call up the dialog with the embedding options.

One idea could be to have some kind of metadata event, where openers could specify the metadata they allow. If there is extra metadata, the media library adds an extra step where the metadata can be provided for each media item. The entity reference widget could then display the metadata with an edit button or something like that. The WYSIWYG integration probably has to rely on a right-click or double-click to bring the metadata form back.

I also like this idea quite a bit. :)

but IMHO, having a multi-step form for every single embedded item increases the cognitive load to unacceptable levels

That's my thinking too.

In my opinion, choosing an item from the media library should immediately embed it into the editor, no questions asked, using whatever available default values can be gleaned from the embedded item itself (such as alt text).

Exactly.

Then, the user should have the ability to edit the embed after the fact and tweak it to their liking -- maybe by right-clicking (or hopefully some kind of accessible equivalent, depending on what kind of wiggle room CKEditor gives us) to call up the dialog with the embedding options.

This is where I'm not certain. I think many people will never discover this capability. It also means you get two completely different experiences, unlike just about everything else in CKEditor.

If you combine those answers of mine, I think that means I'm starting to question how essential per-use (either embedded in text or entity reference) metadata overrides truly are, combined with the fact that #3023807: Override media fields from the reference field hasn't happened and very few people seem to be complaining about that. Yes, people would like that, but people also wanted img[title] in core (https://www.drupal.org/project/editor_advanced_image exists for that and is installed on 0.2% of D8 sites) and a[class][rel] in core (https://www.drupal.org/project/editor_advanced_link exists for that and is installed on 10% of D8 sites).

We consciously chose to not complicate the UX for the 90% and make it not impossible but optional: we deferred that to contrib, and 90% of people indeed don't use it.

Is the same true here? I'm not sure, but I think we should seriously ask that question.

I think many people will never discover this capability. It also means you get two completely different experiences, unlike just about everything else in CKEditor.

Well, that is a pretty good point. But we are approaching this as primarily backend engineers with our own opinions about these features. If people really do need these capabilities, then figuring out how users should interact with it is a question that falls squarely in the province of the design and UX teams. And that leads us to...

I think that means I'm starting to question how essential per-use (either embedded in text or entity reference) metadata overrides truly are

It seems to me like we are not being well-served by speculating about this. Maybe what we should do is get hard information about this from actual users, the UX team, site builders, and the product managers before proceeding either way.

If you update entity_embed to support media-embed, or in core you use drupal-embed (what entity_embed uses), then the ability to use a contextual link to reopen the embed and edit the alt text could be supported by the entity_embed contrib module. That way core could focus on using media library with its own button to embed one or more embeds, and those who want the added functionality of being able to configure them from a dialog could install the contrib module.

It seems to me like we are not being well-served by speculating about this.

Right, so I will just proceed with what needs to happen anyway: inserting media entities from the media library. Metadata overrides can happen later.

Tomorrow is a national holiday over here, I'll continue on Friday.

I think this actually should happen post-1.0. This is a new non-essential feature. It's not yet clear how we'll make the existing alignment/caption per-embed choices from EntityEmbedDialog available in this scenario, since it's not designed to be part of multi-step forms. (Not to mention alt/title per-media-image-embed, which just became available in #2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`.) It also will need a way for sites to opt in to this alternative selection UI, because otherwise it'll disrupt existing workflows (different UX, and no more per-embed overrides yet).

So: postponing on #2577887: Entity Embed 8.x-1.0.0 release, but not setting the issue status to Postponed, because while it can't be shipped with a release yet, the work should still continue.

Rebased #33.

Also rebased this on top of #3044649-27: Delegate media library selection handling to the "thing" that opened the library. Still works. 👍

@Wim Leers,

I am trying to test the patch from #33, but I can't seem to get the media library to open from the media embed buttons.

I do realise this is difficult, but could you give me any pointers on how to set this up?

We were using entity_browser / entity_browser_enhanced before, which I am trying to replace with the core media library.

So far I have updated to Drupal 8.7.2, applied the patch from #33, uninstalled entity_browser & entity_browser_enhanced modules and checked the entity_embed buttons / settings. There is a button called 'media'.

However the entity_embed button now opens a dialog with an autocomplete reference field instead of the media library.

Debugging the code it does seem like DrupalEntity_mediaLibraryUrl and DrupalEntity_mediaLibraryDialogOptions are added to the config.

I have checked the code to see if I missed any settings, but as far as I can see that is not the case. Are there additional patches I need to install? Or any other steps I may have missed?

(This is on a thunder_admin based admin theme, but I have also switched it to a Drupal core theme, to make sure the Thunder alterations weren't causing this.)

Triggering Media's Entity Embed button in CKEditor opens the Media Library selection dialog the first time it's clicked. When an embedded media entity is selected and then the button is triggered, it opens the Media Library selection dialog again, but it does not show the selection.

That's because the Media Library selection dialog does not yet support this. @seanB pointed me to #3023809: Add a selection overview to the media library widget modal, which exists to add precisely that. Until then, changing the embedded media entity is not possible — users will first have to delete the current one and then insert a new one.

Discussed with @seanB at Drupal Dev Days — we concluded that it does not make sense to remove X of 1 item selected in bottom right of Media Library dialog.

HTML oopsie

I forgot something in #43:

Of course that should not exist either then.

  // When the Media Library is triggered from the text editor, override the
  // submit handler.
  if (\Drupal::request()->query->get('media_library_opener_id')) {
    $form['#attached']['library'][] = 'editor/drupal.editor.dialog';
    $form['actions']['submit']['#ajax']['callback'] = 'entity_embed_form_views_form_media_library_widget_ajax_update';
  }

This is not only triggered by opening the Media Library using the text editor, but also when having an entity reference field (to media) on a node form.

The "drupalentity" CKEditor plugin uses a fixed opener id: "editor_entity_embed".
We can check on this in entity_embed_form_views_form_media_library_widget_alter().

I was now able to get the patch #48 working (the issue in #42 turned out to be another module that made some alterations to the media embed button plugin.js).
So far it seems to be working nicely!
The issues I have run into so far:

• It is not (yet?) possible to select per instance the format in which the media item will be inserted;
• The 'select and insert' button doesn't seem to work yet. I think this has to do with getOpenerFieldId in MediaLibraryWidget not returning a valid field id.

Nice work so far!

Thanks for #48, @kcolaers! 👍🙏

Rebased #48 on top of latest entity_embed.

As discussed with @seanB, @phenaproxima and @ckrina at Drupal Dev Days:

1. we want to ensure that embedding from the media library is a smooth experience, forcing the user to think about a view mode, alignment, caption and metadata overrides (alt for example) is bad UX
2. we want to bring a consistent UX across all media embeds (either through entity reference fields or via CKEditor/filter) with regards to per-embed overrides — this is therefore blocked on #3023807: Override media fields from the reference field (reference field embed) and #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`) (text embed)
3. we want to bring a consistent UX across all media library experiences: the pencil and cross icons that #3039829-35: Remove link to media item from media library view. is adding to the media library should also be used in media reference fields (for overriding metadata and deleting the reference, respectively) and in text fields (for overriding metadata and deleting the embed, respectively)
4. There was a lot of discussion about whether to put the per-embed metadata overrides and alignment/captioning in either the same dialog, in separate dialogs, in vertical tabs in a dialog, in fieldsets in a dialog, in accordions in a dialog, … basically everything you can think of was suggested. There's no consensus on this yet. See #3023807-10: Override media fields from the reference field for the YouTube recording of the UX meeting where this was discussed at length, plus #3023807-12: Override media fields from the reference field for the mock-ups.

Since there now is a patch at #3039829-34: Remove link to media item from media library view. whose CSS we can reuse, it's time to get a patch up for this. That should allow us to figure out how to move forward with that last bullet.

The attached patch requires #3039829-34: Remove link to media item from media library view. to be applied! (It reuses the CSS from that patch.)

It enables this:

For context: this is what the Media Library looks like as of #3039829-34: Remove link to media item from media library view.

My embedded media now has these buttons overlaid, just like in the Media Library

After clicking the pencil button, I see this:

I unchecked the "Caption" checkbox, and hit "Save", now I see:

After re-checking it, I see this:

Quoting myself from #2994699-11: Create a CKEditor plugin to select and embed a media item from the Media Library:

The patch that will soon follow was ported from #2998005-51: [PP-1] Support Drupal core's Media Library, but I think it's probably more productive if all work now continues in this issue rather than in a contrib module. Because this will need product manager and UX meeting sign-off, i.e. it needs to have the full attention of core contributors for it to succeed, and hence it should be done solely in Drupal core. Now that the blocking puzzle pieces have core patches (see #10), that is now actually feasible. Closing #2998005.

This has been ported and improved upon: #2994699-13: Create a CKEditor plugin to select and embed a media item from the Media Library + #2994702-14: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`.