Create a CKEditor plugin to select and embed a media item from the Media Library

I changed the order of these issues as we first need a working CKEditor plugin before we can actually start parsing the code. I have been looking for a workaround for two days so therefore I recommend this approach.

IMO: we should create an additional issue for getting approval of the UX team for the layout of the dialog / button of this plugin so that we can focus on UX in a single issue.

This patch adds the plugin to CKEditor and parses the drupal-entity tags.

Quoting #2994696-8: Render embedded media items in CKEditor:

As of #5, this has been marked as postponed on #2994699: Create a CKEditor plugin to select and embed a media item from the Media Library. I understand the original reasoning, but … we have to have a @Filter plugin first, that's what #2940029: Add an input filter to display embedded Media entities does. If we want to do the filter first, initially one would have to craft the HTML manually anyway (and note that this won't ship with a Drupal release as-is, we'll want to complete this and #2994699 too in the same release!). So, then it makes much more sense to have the same thing be visible both in a rendered blog posts (/node/1 has a visible embedded image media item thanks to the filter) and in edited blog posts (/node/1/edit has a visible embedded image media item thanks to a CKEditor plugin).

So, swapping the order.

Because we were still very deep in stabilizing https://www.drupal.org/project/entity_embed, yet we wanted to already validate some core issues that we knew would block this core issue (such as #3044649: Delegate media library selection handling to the "thing" that opened the library and #3038254: Delegate media library access to the "thing" that opened the library), we opened #2998005: [PP-1] Support Drupal core's Media Library in the Entity Embed issue queue to do some early prototyping.

Now that both #2940029: Add an input filter to display embedded Media entities and #2994696: Render embedded media items in CKEditor have core patches, it's time to bring that to Drupal core too :)

I'm currently porting #2998005-51: [PP-1] Support Drupal core's Media Library to a core patch that builds on top of both #2940029 and #2994696.

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.

Quoting one very relevant bit from #2998005-51: [PP-1] Support Drupal core's Media Library:

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.

That's why I think this issue should deal only with point 1: a smooth media selection and embedding experience. Per-embed overrides and changing alignment and toggling captions are for #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`

This patch:

1. adds a new DrupalMediaLibrary CKEditor plugin
2. this DrupalMediaLibrary requires the DrupalMedia CKEditor plugin that #2994696: Render embedded media items in CKEditor is adding
3. this CKEditor plugin is very simple: it provides a button, and that button triggers the Media Library dialog using the infrastructure that #3044649: Delegate media library selection handling to the "thing" that opened the library added; upon getting a response from the server it just generates<drupal-media …> HTML and inserts that!
4. Per the discussion that was cited in #12, there's no selection of view mode, alignment, alt overiding, nothing: you select media, and then it appears in CKEditor.
5. Consequently, defaults matter a lot. The most important default that is chosen is the view mode. As described in #2940029-25: Add an input filter to display embedded Media entities, we believe that we should introduce a new embed or embedded view mode, which will help ensuring a sensible out-of-the-box experience.

Click button, see the media library

Select media:

After clicking "Insert selected"

Forgot one screenshot.

Are you also a bit disappointed by that last screenshot in #13? I am. But it's the smallest scope possible.

That's why I wrote:

5. Consequently, defaults matter a lot. The most important default that is chosen is the view mode. As described in #2940029-25: Add an input filter to display embedded Media entities, we believe that we should introduce a new embed or embedded view mode, which will help ensuring a sensible out-of-the-box experience.

Perhaps we want to add this new view mode here. Perhaps we want it in a separate issue. TBD.

But what we can do right now is make the insertion from the media library set sane defaults for alignment and captioning: center aligned and allowing the end user to enter a caption immediately after insertion:

Forgot #15's screenshot.

A few things worth calling out for reviewers:

1. +++ b/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.js
   +++ b/core/modules/media_library/src/MediaLibraryCkeditorOpener.php
   @@ -0,0 +1,46 @@
   +namespace Drupal\media_library;
   ...
   +class MediaLibraryCKEditorOpener implements MediaLibraryOpenerInterface {
   
   +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   
   +namespace Drupal\media_library\Plugin\CKEditorPlugin;
   

   It's worth calling out that this CKEditor plugin explicitly lives in the media_library module, not in the media module.

   Because sites that only have the media module installed of course cannot select media from a non-existent Media Library.

   That's why this CKEditor plugin is called DrupalMediaLibrary and why it's a separate plugin in the first place.

2. +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   @@ -0,0 +1,90 @@
   +  public function getDependencies(Editor $editor) {
   +    return [
   +      'drupalmedia'
   +    ];
   +  }
   

   This guarantees that the DrupalMedia CKEditor plugin from #2994696: Render embedded media items in CKEditor is loaded — which provides the nice previews thanks to CKEditor Widgets.

3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -169,6 +181,12 @@
   +            // When starting out with an empty caption, CKEditor automatically injects a <br> that we need to delete.
   +            // @see core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.es6.js
   +            if (captionEditable.$.childNodes.length === 1 && captionEditable.$.childNodes.item(0).nodeName === 'BR') {
   +              captionEditable.$.removeChild(captionEditable.$.childNodes.item(0));
   +            }
   

   This is copied verbatim from core/modules/ckeditor/js/plugins/drupalimagecaption/plugin.es6.js, and was itself originally written by the CKEditor 4 Lead Developer of the time (@reinmarpl).

+++ b/core/modules/media_library/src/MediaLibraryCkeditorOpener.php
@@ -0,0 +1,46 @@
+  public function checkAccess(MediaLibraryState $state, AccountInterface $account) {
+    throw new \Exception('Not yet implemented, see https://www.drupal.org/project/drupal/issues/3038254.');
+  }

This should be updated now, because #3038254: Delegate media library access to the "thing" that opened the library landed a few minutes ago! 🥳

Addressed #21.

A few rounds of feedback have been incorporated into #2940029, rebasing #22 on top of #2940029-91: Add an input filter to display embedded Media entities.

Rerolled #22 to match #2940029-97: Add an input filter to display embedded Media entities's marking of data-view-mode as optional. #2994696-23: Render embedded media items in CKEditor was already updated for the same reason.

The defaults-to-solicit-feedback chosen in #15 were given feedback during yesterday's UX call. We should remove data-caption per #2994702-18: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`.2.

1. +++ b/core/modules/media_library/src/MediaLibraryCkeditorOpener.php
   

   vs

   +class MediaLibraryCKEditorOpener implements MediaLibraryOpenerInterface {
   

   @kcolaers in #2994702-21: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` noticed that there is a file vs class naming inconsistency that we should fix here.

2. +++ b/core/modules/media_library/src/MediaLibraryCkeditorOpener.php
   @@ -0,0 +1,46 @@
   +    $response->addCommand(new EditorDialogSave($values));
   

   In the same vein: we should not make this coupled to ckeditor.module, but instead to editor.module. Looks like the code already isn't CKEditor-specific. So this should be renamed to MediaLibraryEditorOpener.

#2940029: Add an input filter to display embedded Media entities landed. Rerolled, on top of #2994696-25: Render embedded media items in CKEditor.

Addressed #26.

Adding test coverage here:

/core/modules/media_library/tests/src/FunctionalJavascript/DrupalMediaLibraryTest.php

(See the interdiff in the next comment).

Removing "Needs tests" issue tag.

One question I have: I'm not sure why there is stuff about captions in the drupalmedia plugin.js in this patch. Is there a way to add a caption I'm not seeing? If so, we'll need to add test coverage for that. I think those changes might belong in the next issue: #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption`.

Update: It turns out the test failed because the class didn't match the filename, fixed in #33.

Here's an interdiff for the new test added in #29.

Some minor cleanup on test added in #29.

1. We need an access test for MediaLibraryEditorOpener::checkAccess()
2. MediaLibraryEditorOpener::checkAccess() only checks the use permission for the format. Perhaps we should also check if DrupalMediaLibrary plugin is enabled on the format. Perhaps that's redundant. Or we could check if the media_embed filter is enabled. Or both. We'll need to consider what additional access checks are useful here.
   This would be similar to what we did for the route check MediaFilterController::formatUsesMediaEmbedFilter
3. drupal_get_path('module', 'media_library') should be replaced with $this->moduleExtensionList->getPath('media_library')
4. coding style but found by testbot:
   

   /var/lib/drupalci/workspace/jenkins-drupal_patches-3182/source/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php ✗ 2 more
   6 Unused use statement
   35 A comma should follow the last multiline array item. Found: 'drupalmedia'

5. There's undo and redo integration that should be tested.
6. If DrupalMediaLibrary button is enabled on a text format, but media_embed filter disabled, they'd be able to insert an embed, but it wouldn't render. This would lead to bad UX. There are two two ways to handle this, I see. You could add javascript to the drupalmedialibrary plugin.js where it displays a warning of some type, or you could just prevent this from happening by adding validation, which is what I think is the better strategy. I think there should be validation at /admin/config/content/formats/manage/textformatname that you can't have the DrupalMediaLibrary button enabled without enabling the media_embed filter. Doing so should lead to a form validation error.

I had a cut-and-paste error where I failed to rename the class to match the file.

Addressing #32
1. ✅Added in comment #35.
2. ✅Added a check that media_embed filter is enabled to MediaLibraryEditorOpener::checkAccess(). I didn't add a check that DrupalMediaLibrary button enabled, as this should be true if this access check is being called. A check for media_embed filter is more important, as the media embed won't render without the filter. I also added form validation on the text format to assure this filter is enabled if you enable the button on a text format.
3. ✅updated
4. ✅fixed coding style
5. ✅Added test coverage of undo and redo
6. ✅Added form validation requiring media_embed filter to if DrupalMediaLibrary button in toolbar in editor. Added test coverage for this.

Also, converted drupalmedialibrary plugin.js to es6.

Addressing #32.1, adding test coverage for MediaLibraryEditorOpener::checkAccess().

Also, I noticed one bug with "Undo" functionality. I put a note in the test about it:

     $embed = $assert_session->waitForElementVisible('css', '.media-embed-widget drupal-media .media-embed', 1000);
     $this->assertNotEmpty($embed);
+    // @todo inserting media embed should enable undo.
+    // $this->assertEditorButtonEnabled('undo');

After you insert a media embed, the undo button should immediately be enabled, and you should be able to undo the embedding. In the test, the Undo button is only enabled after you hit the source button and then hit it again, re-rendering the embed, and presumably the Undo functionality at this point is the Undo handling in drupalmedia plugin.js, not in drupalmedialibrary plugin.js.

@covers for ::checkAccess() was wrong. I had forgot where it was when I was adding that. This method isn't in DrupalMediaLibrary that the test class is covering:
* @coversDefaultClass \Drupal\media_library\Plugin\CKEditorPlugin\DrupalMediaLibrary

Since I'm testing more than the one class, I think I should give the test a more generic name. I'll stick with the pattern previously established by several other modules and go with CKEditorIntegrationTest.

Re-roll against latest patch (194) from #2994696: Render embedded media items in CKEditor. Mostly dropping the CKEditorTestTrait.

Rerolling a combined patch. The patch in #37 still applies cleanly on top of 2994696-222.

#2994696: Render embedded media items in CKEditor landed so un-postponing.

Indeed! I'll rebase this and #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` 🥳

Rebased #37. Review of @oknate's changes since my last patch iteration in #28 to follow next.

In #28, this was a 8.94 KB patch. Today, it's a 27.4 KB patch, thanks to the test coverage @oknate added.

• #32 RE: MediaLibraryEditorOpener::checkAccess() also checking if the filter is enabled: +1
• #32.6 RE: DrupalMediaLibrary needing the media_embed filter: +1 for the form validation error.
• #34: Great progress, thanks! 👍
• #35: I cannot reproduce this. But we've discovered undo-related issues in CKEditor before in Entity Embed. #3060245: [upstream] CKEditor's "undo" functionality not working correctly for embeds: offers multiple undo steps where there should be only one and #3063964: [upstream][PP-2] Editing an embedded entity and submitting the `EntityEmbedDialog` without changing anything results in an unwanted "undo' step are still open and are both blocked on an upstream bug: https://github.com/ckeditor/ckeditor-dev/issues/3198.
• @oknate added all the necessary test coverage! 🙏🥳

Patch review + reroll

1. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,361 @@
   +  public function testMediaLibraryEditorOpenerAccess($media_embed_enabled, $can_use_format) {
   +
   +    $format = FilterFormat::create([
   

   🔎 Extraneous blank line. Fixed. ✅

2. +++ b/core/modules/media_library/media_library.module
   @@ -342,3 +343,68 @@ function _media_library_configure_view_display(MediaTypeInterface $type) {
   +  // filter enabled if DrupalMediaLibrary button is enabled.
   

   🔎 Nit: "filter enabled if button is enabled" sound strange. Improved. ✅

3. +++ b/core/modules/media_library/media_library.module
   @@ -342,3 +343,68 @@ function _media_library_configure_view_display(MediaTypeInterface $type) {
   +function media_library_filter_format_edit_form_validate($form, FormStateInterface $form_state) {
   +
   +  if ($form_state->getTriggeringElement()['#name'] !== 'op') {
   

   🔎 Nit: extraneous blank line. Fixed. ✅

4. +++ b/core/modules/media_library/media_library.module
   @@ -342,3 +343,68 @@ function _media_library_configure_view_display(MediaTypeInterface $type) {
   +  if ($button_groups = $form_state->getValue($button_group_path)) {
   

   👎 This validation logic should only run if the text editor is CKEditor. Fixed. ✅

5. +++ b/core/modules/media_library/media_library.module
   @@ -342,3 +343,68 @@ function _media_library_configure_view_display(MediaTypeInterface $type) {
   +          '%media-embed-filter-label' => new TranslatableMarkup('Embed media'),
   

   🔎 This should use the same logic as media_filter_format_edit_form_validate() to get the translated filter label. Fixed. ✅

6. +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   @@ -0,0 +1,129 @@
   +        // @todo: new icon!
   +        'image' => '',
   

   We still need this for sure 😁

7. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,361 @@
   +  protected $adminUser;
   ...
   +    $this->adminUser = $this->drupalCreateUser([
   +      'use text format test_format',
   +      'access media overview',
   +      'create blog content',
   +    ]);
   

   👎 This is hardly an "admin" user. Fixed. ✅

8. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,361 @@
   +  public function testFormatValidation() {
   

   🔎 Nit: "format validation" sounds pretty abstract. Let's rename it to "configuration validation". Done. ✅

9. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,361 @@
   +  public function testCKEditorIntegration() {
   

   👎 This matches the class name. That's odd. Let's instead name this testButton(). Fixed. ✅

10. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -0,0 +1,361 @@
    +    if ($can_use_format) {
    +      $user = $this->drupalCreateUser([
    +        'access media overview',
    +        $format->getPermissionName(),
    +      ]);
    +    }
    +    else {
    +      $user = $this->drupalCreateUser([
    +        'access media overview',
    +      ]);
    +    }
    +    $this->drupalLogin($user);
    

    🤔 This can be be made quite a bit simpler. Done. ✅

11. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -0,0 +1,361 @@
    +      $item = $assert_session->elementExists('xpath', '(//div[contains(@class,"media-library-item")])[1]');
    

    🤔 Why the (…)[1]? I've never seen that before in a test assertion. I grepped my Drupal installation to find something similar (using this regex: xpath.*\)\[\d\]['"]{1}), and found only one, in a contrib module: \Drupal\entity_reference_revisions\Tests\EntityReferenceRevisionsAdminTest::testMultipleTargetBundles() has this too.

    AFAICT this is selecting the second media library item. Why though?

12. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -0,0 +1,361 @@
    +   * Data Provider for ::testMediaLibraryEditorOpenerAccess.
    

    🔎 Übernit: s/Provider/provider/. Fixed. ✅

13. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -0,0 +1,361 @@
    +      'media_embed enabled on format' => [
    ...
    +      'media_embed disabled on format' => [
    ...
    +      'media_embed enabled, user lacks access to format' => [
    

    👎 These are oddly named. Fixed. ✅

14. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
    @@ -0,0 +1,361 @@
    +  protected function waitForEditor($instance_id = 'edit-body-0-value', $timeout = 10000) {
    ...
    +  protected function assignNameToCkeditorIframe() {
    ...
    +  protected function getEditorButton($name) {
    ...
    +  protected function pressEditorButton($name) {
    ...
    +  protected function assertEditorButtonDisabled($name) {
    ...
    +  protected function assertEditorButtonEnabled($name) {
    

    👎 These are identical to those in \Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest. Let's extract them into a trait that both test classes use. Done. ✅

I deliberately omitted one thing from #42 to keep #42 simpler:

• #29: One question I have: I'm not sure why there is stuff about captions in the drupalmedia plugin.js in this patch. […] I think those changes might belong in the next issue I included it here in #15, because at that point the proposed default insertion included data-caption="". Since then, it was decided in a UX call that we should not caption media embeds by default — see #25 for that decision. I should've removed this JS addition in #25. Well spotted!

Removed it from this patch. I'll add it to #2994702: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` instead.

#41's test results finally came back. The failure is because of a change in the previous patch in the chain, over at #2994696-214: Render embedded media items in CKEditor. Rather than a custom media-embed-widget class, we're now relying only on the CKEditor Widget class cke_widget_drupalmedia. Easy fix :)

Re #42.11 :

      $item = $assert_session->elementExists('xpath', '(//div[contains(@class,"media-library-item")])[1]');
      $this->assertNotEmpty($item);

AFAICT this is selecting the second media library item.

Actually, it selects the first from the list. It's rare in programming for something to not be zero-based.

see https://stackoverflow.com/a/14295136/1214689

I wrote a test for Entity Browser testing drag and drop re-ordering where I used this pattern a lot. It allows getting the first item from the list in one shot when there is more than one of a selector. I like it because I can use a more general class name as a selector and then grab a specific one using xpath. If you have a more elegant way, I'm all ears.

Re #42.14:

Phenaproxima recommended that we hold off on extracting a trait, see #2994696-191: Render embedded media items in CKEditor.1 and move that into a separate issue. I think the concern is the trait could slow RTBC for this issue (it could "raise committer eyebrows"), and that the methods might need to be reworked to be more generic. Maybe you can discuss it with him.

In #42 I was unable to reproduce #35. Now I've found a way:

1. Go to /node/add/article
2. Click the "Insert from Media Library" button without giving focus to anything in CKEditor first
3. Select media, insert it
4. Observe that the undo button is indeed not activatable

If before step 2 you click inside the CKEditor iframe instance, or for example type something first, then the undo button does work as expected.

Pinged CKEditor maintainer @kkrzton about this.

#47:

• RE: selector. Ahhh, right, XPath is weird like that! I forgot about that. That's fine then. Thanks for clarifying :)
• RE: test trait. It would've delayed things when we were introducing a trait with a single use. Because how could we have known it was sufficiently generic? But now there's two uses. So the need is validated, and the abstraction works. That's sufficient.

Fixing the tests (continuing the effort started in #45).

For those who are catching up, the classes changed in the previous commit (#2994696: Render embedded media items in CKEditor), so:

• .media-embed-widget no longer exists, so using .cke_widget_drupalmedia
• .media-embed no longer exists, so using .media

#52:

• 2–6: we're just moving test methods that already exist in core. Let's not spend time in this issue improving them.
• 8: CKEditor supports both wysiwyg mode (iframe) and inline mode (contenteditable).
• 9: Sure, but note that I pinged the CKEditor team. With some luck this is fixable without an upstream fix.
• 10: This exact same pattern exists in every other CKEditor plugin that has buttons. Let's not deviate from that.
• 11–13: +1, good finds!
• 15: +1 — my bad!
• 17: +1
• 18: it literally is impossible, since a client cannot tamper with the URL without invalidating the hash which would result in this code never getting called. So IMO this is unnecessary.
• 19: RE: question. Let's leave that to contrib. Just like the DrupalLink plugin also does only the 90%, and https://www.drupal.org/project/editor_advanced_link fills most of the remaining 10%. (Quite literally, since about 10% of Drupal 8 sites have that module installed.)
• 20: See my answer to 19. If image does not exist, it won't pick it. This is perfectly safe.
• 21: That would benefit like maybe three lines of code in all of Drupal ever … so I don't see the value in doing that :P
• 22: I'm getting distinct Batman spotlight vibes 😀🦇👊🏿

Addressing #52
1. ✅ Added some verbiage about functional javascript.
2-5. Moved these comments to follow up issue.
6. ✅ I like the idea of keeping pressEditorButton() for readability but replacing the redundant code within, so I implemented that suggestion.
7. This looks like it's only affected by classy. So this rule should probably go in classy, either in core/themes/classy/css/components/ui-dialog.css or in a file for media_library style overrides in classy. I need a FE dev who's familiar with the framework to give me some guidance on best practices there.
9. ✅ Added follow up issue: #3073294: Remove obsolete @todo for "Undo bug when first inserting media into unfocused CKEditor" and link to follow-up issue.
10. 🚫 Leaving alone per Wim feedback that this follows an established pattern.
11. ✅ Switched to using Json::decode().
12. ✅ Excellent find. We were only looking at the first row. I added the embed on the second row to verify this, and a fail patch to demonstrate the bug. I added a bug report to the entity_embed issue queue as well.
13. ✅
15. ✅
16. ✅ Injected the filter_format storage in the constructor and added code to forbid access if the config entity cannot be loaded.
17. ✅
18. ✅ Added logic exception. We should add test coverage for this (possibly?). Wim says it's impossible to trigger the error in #54 Perhaps with a Kernel test of some type?
19. ✅Added media type storage. The follow-up I left alone for now.
20. Added a follow up to see if the default selected_type_id can be improved https://www.drupal.org/project/drupal/issues/3073309

Addressing #58.
1. ✅
2. ✅
3. ✅ I've undone this change for now, and I'll try to avoid making unnecessary changes. This was just a personal thing . The name seemed odd to me. The other core CKEditor plugins have names like "Image", "Drupal link", "Drupal image caption widget", "Styles dropdown". The entity embed module calls it simply "Entity". These are all names, rather than descriptions. They start with nouns or adjectives. "Embed" is a verb, so it sounds like a command or a description of the action it performs.
4. ✅

I second #59 although don’t see why we shouldn’t expose a UI, I can see this being useful for a lot of sites.

• #56.7: Generally speaking, module-specific things don't belong in Classy, they belong in the module. Lots of examples of this in core. Furthermore, this specific thing is not a Classy styling thing, but a jQuery UI styling thing!
• #56 patch interdiff review: my nits have already been addressed in subsequent comments, but just wanted to point out one thing:
  

  +++ b/core/modules/media_library/src/MediaLibraryEditorOpener.php
  @@ -4,34 +4,68 @@
  +      return AccessResult::forbidden()
  +        ->addCacheTags(['filter_format_list'])
  +        ->setReason("The text format '$filter_format_id' could not be loaded.");
  

  👍🤩

• #58.2: Whew! :D
• #58.4: This sounds like a case of premature optimization.
• #59: If we want this, then it must be configuration for the media_embed filter, not for the CKEditor integration. Just like we already added a default_view_mode setting, we should then add a allowed_media_types setting. Then the CKEditor integration (but also a similar implementation for any other text editor) can just read that configuration. But before we even go that route, let's first address the reason for adding configuration:
  

  there is no easy way that I can see for contrib to modify this behavior, short of swapping out the entire plugin class

  I disagree. It'd be trivial. I should've explained why it's trivial. Let me explain by just writing everything that this contrib module would have to do:

  function mymodule_editor_js_settings_alter(array &$settings) {
    $allowed_media_type_ids = \Drupal::config('mymodule')->get('allowed_media_type_ids');
  
    foreach (array_keys($settings['editor']['formats']) as $format_id) {
      if ($settings['editor']['formats']['editor'] !== 'ckeditor') {
        continue;
      }
  
      $state = MediaLibraryState::create(
        'media_library.opener.editor',
        $allowed_media_type_ids,
        in_array('image', $allowed_media_type_ids, TRUE) ? 'image' : reset($media_type_ids),
        1,
        ['filter_format_id' => $format_id]
      );
      $media_library_url = Url::fromRoute('media_library.ui')
        ->setOption('query', $state->all())
        ->toString(TRUE)
        ->getGeneratedUrl();
      $settings['editor']['formats']['editorSettings']['DrupalMediaLibrary_url'] = $media_library_url;
  }
  

  Hopefully this convinces you :)

• #61: yes, "a lot of sites". https://www.drupal.org/project/editor_advanced_link is installed on 30K sites. That's also "a lot". It's not "most". I've shown above how a contrib module can easily implement this. I am arguing that on the vast majority of sites, "inserting from the media library" is what is needed, not "inserting from some subset of the media library".

Re #62:

If we want this, then it must be configuration for the media_embed filter, not for the CKEditor integration.

+1

#61: yes, "a lot of sites". https://www.drupal.org/project/editor_advanced_link is installed on 30K sites. That's also "a lot". It's not "most". I've shown above how a contrib module can easily implement this. I am arguing that on the vast majority of sites, "inserting from the media library" is what is needed, not "inserting from some subset of the media library".

Yeah actually that's a fair point regarding "inserting from the media library" vs "inserting from some subset of the media library".

That being said and regarding your specific example (BTW this is clearly going to be a tangent but still worth mentioning), 30K sites is around 10% of Drupal 8 sites (as reported by https://www.drupal.org/project/usage/drupal). In the case of Editor Advanced Link, 10% might be reporting using that module, but that doesn't mean that X% more sites wouldn't benefit from a feature like that being in Core. The percentage of sites currently using a feature is a good indicator of whether something should be in Core, but it can't be the only one and it shouldn't demerit the value added by including something in Core... How do you measure the true value added to the editing experience by including something in Core out of the box? Does including a particular feature tick another box for more evaluators who are trying to decide which CMS to use? Does adding more features available out of the box make it easier for site builders to choose Drupal because it simply "just works" for their needs? Maybe, or maybe not, but to me these leading questions do form part of the reasons why new Core modules like Media and Layout Builder are in Core and not contrib. That being said, maybe I'm thinking into this too deeply.

Anyway, my ramblings and questions above are more rhetorical than questions that can actually be answered, and I'm going to stop there because this is really more of a discussion for another time and place, and I don't want to side track this issue. But I'd say 30K sites using one Contrib feature is a pretty clear indicator that maybe that feature should at least be considered for inclusion in Core, and there's probably examples of other features which have been committed without having to be justified based on how many sites used an equivalent Contrib module.

How do you measure the true value added to the editing experience by including something in Core out of the box?

Great question :)

In the case of https://www.drupal.org/project/editor_advanced_link, the argument was that we shouldn't complicate the UX of something that is used millions of times per day for 100% of sites if only 10% of sites need it.

But in this case, it's just configuration. So it would only complicate the site builder experience.

Which means you've (accidentally? :D) convinced me that it's okay for us to add this to Drupal core! 😁 Let's continue this in #3073535: Allow only a subset of the media library to be exposed to CKEditor.

Addressing #64
1. ✅Removed unnecessary dependency.
2. ✅Added $page method for improved readability.
3. ✅Changed code to one line. I think I was confused by some feedback you had for me on another patch about checking if elements exist before calling methods on them. I guess for ->click() and ->getValue() it's fine because they'll throw an error if empty.
4. ✅Added link to the follow-up issue.
5. ✅same as 3
6. ✅Added comment explaining why we keep switching back to the same iframe.
7. ✅Converted test to Kernel test for speed. 🏎🏎🏎

1. Added the code for the icon. I took the image icon and added an extra moon.

2. Addressed nits in #68
3. Ran eslint and prettier and cleaned up the es6 and javascript.

Went through a WebchickTestCase with @phenaproxima and @oknate, and here were the results!

1) It would be good when we know what the final icon will be and can get confirmation that it will be visually distinct in some way from the Image button on CKEditor. Because by default, people are going to end up with two buttons that do almost the same thing, at least until their site administrator has configured some stuff differently. (If we don't already, we should maybe spin off a follow-up to talk about what we want the upgrade path to be here, esp. once Media Library is part of Standard profile.)

2) When using the icon to open Media Library, there's a visual bug in that Image is selected by default (+1) but Audio is also highlighted, causing a visual regression:

@phenaproxima explained that this is because there's code somewhere for accessibility reasons to automatically select the first item in a list. That's good, except that because Image isn't the first thing in the list, it's selecting both. Even though this is not the fault of this patch, I really don't think we can commit it with this because it's a pretty obvious visual problem in a primary content author user interface. (We don't want to introduce major bugs as we go.)

We talked about two possible solutions: one to change the logic of that code to act on the element that has focus first, should one exist, and only then falling back to the first option; or two, if that turns out to not be a simple fix, to hack around it by moving Image to the top for now and addressing it in a follow-up. (I would rather not postpone this important thing on a separate bug fix issue for it, so whatever's easiest to fix here +1.)

3) It's a little odd that checkboxes are presented here (indicating multiple selections are possible), but your choices are limited to only one. Radio buttons would be the standard UI element for this kind of choice. However this is a "pre-existing condition" with any time the Media Library is showing you a group of things you are limited to only being able to select 1 of, so fine to handle in a follow-up (and just "normal" priority; there are other visual indicators like the disabling of controls and the fading out of the other selections and the tiny, tiny text in the lower-right saying "0 of 1 selected" to help you understand that you can only select one thing).

4) Once you insert a media element, what I expected to be able to do was position the cursor to the right and press Enter a couple of times (like I do in Microsoft Word, Google Docs, etc.) to get to the place where I could type some more text and/or add another media element. However, this didn't work. @oknate helpfully pointed out that you need to click this tiny, tiny red arrow thingy in order to get another "paragraph" and be on your way and never in one gazillion lifetimes would I have figured that out by myself. :)

Anyway. WELL out of scope for this issue, but IMO worth another follow-up for that, probably "major" though. (If this is indeed something we can even do anything about; I'm not sure if this behaviour is something CKEditor just forces on people but IMO it's gonna trip people up horribly, and @oknate confirmed he has to train his content authors on this all the time).

TL;DR: This is looking great; just the icon and the double-activation bug to sort out and we are good to go! <3

2) When using the icon to open Media Library, there's a visual bug in that Image is selected by default (+1) but Audio is also highlighted, causing a visual regression:

The media library has a state parameter for the available types, and respects the order that is passed. I think the easiest solution for this would be to make the order configurable in the filter and always select the first media type. I think phenaproxima opened an issue for this already.

It would also be nice to be able to disable media types for embedding, but that is a completely separate issue.

Anyway, great to see how this is all coming together. Thanks everyone for the amazing work!

Just read the patch as well while I was at it. Very little to complain about. Great work!

1. +++ b/core/modules/media_library/media_library.module
   @@ -342,3 +344,73 @@ function _media_library_configure_view_display(MediaTypeInterface $type) {
   +  if ($form_state->getValue(['editor', 'editor']) !== 'ckeditor') {
   +    return;
   
   +++ b/core/modules/media_library/media_library.services.yml
   @@ -13,3 +13,6 @@ services:
   +  media_library.opener.editor:
   +    class: Drupal\media_library\MediaLibraryEditorOpener
   
   +++ b/core/modules/media_library/src/MediaLibraryEditorOpener.php
   @@ -0,0 +1,78 @@
   + * The media library opener for text editors.
   

   While looking at this, it one the one hand seems that this works for all editors, but at the same time we focus specifically on CKEditor.

   So my question is, should this work for all text editors? If not, it might help to remove any possibility for confusion in naming the classes, service, and documentation. So for example user media_library.opener.ckeditor, MediaLibraryCKEditorOpener etc.

2. +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   @@ -0,0 +1,140 @@
   +      in_array('image', $media_type_ids, TRUE) ? 'image' : reset($media_type_ids),
   

   As mentioned in my previous comment, let's make this sortable (can be a followup) and remove the "select image by default" weirdness for now. Or we can force the image type to be the first item. That would fix it as well.

3. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,193 @@
   +   * The sample Media entity to embed.
   

   We consistently try to use "media item" instead of "media entity."

Here's a bit procrastination sorry... I prepared several icons. I'm adding a Zip with several tests and the 2 I think are more understandable when they are on their smaller size:

Option 1:

Option2:

And some other options if someone else wants to jump in and try something else. All of them are on the ZIP as SVG too.

I'd go for option 2, but it isn't an strong opinion.

In #71 @webchick mentioned:

3) It's a little odd that checkboxes are presented here (indicating multiple selections are possible), but your choices are limited to only one.

That got me thinking... is there a reason the user is limited to inserting only 1 item? The Media Library does technically support inserting as many items as it's configured to, I could totally see a scenario where someone is writing an article and wants to insert a several images, but currently they'd have to open the Media Library, upload or select an item, close the Media Library, and then repeat several more times. A more efficient workflow would be if there wasn't a limit of 1 item and so the user could both upload and select, then insert all the media they needed in essentially one operation.

Re #75: I prefer option 2 over option 1, but I also like the last two icons on the last row of the grid of icons.

#76: I agree, given the functionality here, there's no reason this can't support more than one insertion. Here's a proof of concept. The test coverage has been updated to insert three media items at once.
#75: I like option 2 as well. The two eight notes (musical symbol) is more visually distinct from the image icon and works better when small. I also like option 1, as video is more common than music when it comes to embedding media, but when small it will be harder to see. So I agree with you option 2 is better. I'd like to see the final png icon files. I tried converting these into 16x16 png and 32x32 png to attach properly to a patch, but I ran into two issues:

• the white background should be removed, so it's transparent. See the core drupalimage.png for an example of transparency.
• I tried to use photoshop to reduce option 2 to 16x16 and I got poor results. So I might need some help there. I'll give it another go, but if you could produce 16x16 and 32x32 icons. I think those are the required sizes. I'll double check on that.

Here's the png included in #75 for the smaller size. You can see it looks weird in chrome. I think it's 17x17, which is part of the problem. If it were 16x16 chrome wouldn't resize it creating the fuzzy aliasing effect.

#74.3 Changed 'media entity' to 'media item'.
#71: Added the workaround for now for the focus vs. active styling bug. I tried to fix it by adding focus on the active element, but I haven't yet found the proper place to do this. I added test coverage to verify that image shows first.

I don't think it's so bad that they aren't alphabetical, despite what I was saying yesterday. But I do think when we add a config to allow choosing the allowed media types, it should allow setting the order too. Then, we don't need a separate config for the active item, it can always be the first one.

I also created a follow up issue for this: https://www.drupal.org/project/drupal/issues/3073799

Amazing progress here! 🥳

• #69.1: 😂👏 I have no idea where this came from, but it's genius!
• #71.1: +1 for follow-up issue for that. Standard should remove DrupalImage and stop allowing<img>, and add DrupalMediaLibrary, the media_embed filter and allow <drupal-media>
• #71.2+3: Both of these are pre-existing problems. Especially the latter: this also occurs if you configure a media field to be single-cardinality. @oknate opened an issue for point 2: #3073799: Fix visual bug in media library where active-tab highlight applied to non active tab.
• #71.4: Welcome to HTML rich text editors, they sometimes have to do crazy things :) The same already exists with all other block-level elements in CKEditor. That "tiny red arrow thingy" is something CKEditor added many, many years ago. We cannot fix this in Drupal. We'd have to work with the CKEditor team to come up with a better solution.
• #72: RE: order -> nice!
• #73: I disagree. Let's continue this in #3073535: Allow only a subset of the media library to be exposed to CKEditor though instead. Tight issue scope make issues easier to land :)
• #74.1: MediaLibraryEditorOpener doesn't do anything CKEditor-specific. Other text editor plugins can reuse this. But Drupal core cannot provide integration with other text editors. That's why the additions to media_library indeed are CKEditor-specific: because it's validating the configuration of the CKEditor integration combined with the filter. That's why specifically this naming was chosen. There's no reason to make MediaLibraryEditorOpener CKEditor-specific.
• #75 +1 for option 2, it's most visually distinct, and most clearly conveys that there is a whole range of possible things available by clicking this button :)
• #76: Please file a feature request for that :) The code is based on Entity Embed's, which was validated with many sites. Deviating from that is a risk. It is also conceptually very different from how the existing DrupalImage + EditorImageDialog have worked for years. Finally, it makes it impossible for contrib modules to alter EditorMediaDialog and turn it into a multi-step form and allow overriding metadata immediately upon insertion. (Or at least makes that much more complex.) In short: too many risks, too many unknowns. Let's not slow this issue down by expanding scope.
• #77.#76: I'd like to see this reverted. Like I said, it's too risky and out of scope. (Yes, the code changes are simple, I know. The full implications may be unclear though, and I don't want to risk derailing this capability over a single nice-to-have feature.)

I also only have nits!

1. The icon still needs to be properly integrated of course.

2. +++ b/core/modules/media_library/js/plugins/drupalmedialibrary/icons/drupalmedialibrary.png
   @@ -0,0 +1,7 @@
   IHDR��a	pHYs�� iTXtXML:com.adobe.xmp<?xpacket begin="" id="W5M0MpCehiHzreSzNTczkc9d"?> <x:xmpmeta xmlns:x="adobe:ns:meta/" x:xmptk="Adobe XMP Core 5.6-c140 79.160451, 2017/05/06-01:08:21        "> <rdf:RDF xmlns:rdf="http://www.w3.org/1999/02/22-rdf-syntax-ns#"> <rdf:Description rdf:about="" xmlns:xmp="http://ns.adobe.com/xap/1.0/" xmlns:dc="http://purl.org/dc/elements/1.1/" xmlns:photoshop="http://ns.adobe.com/photoshop/1.0/" xmlns:xmpMM="http://ns.adobe.com/xap/1.0/mm/" xmlns:stEvt="http://ns.adobe.com/xap/1.0/sType/ResourceEvent#" xmp:CreatorTool="Adobe Photoshop CC 2018 (Macintosh)" xmp:CreateDate="2018-04-05T16:29:05-04:00" xmp:ModifyDate="2019-08-07T19:10:44-04:00" xmp:MetadataDate="2019-08-07T19:10:44-04:00" dc:format="image/png" photoshop:ColorMode="3" photoshop:ICCProfile="sRGB IEC61966-2.1" xmpMM:InstanceID="xmp.iid:550a4a5f-73e4-41f8-82f0-cae04d84e074" xmpMM:DocumentID="xmp.did:550a4a5f-73e4-41f8-82f0-cae04d84e074" xmpMM:OriginalDocumentID="xmp.did:550a4a5f-73e4-41f8-82f0-cae04d84e074"> <xmpMM:History> <rdf:Seq> <rdf:li stEvt:action="created" stEvt:instanceID="xmp.iid:550a4a5f-73e4-41f8-82f0-cae04d84e074" stEvt:when="2018-04-05T16:29:05-04:00" stEvt:softwareAgent="Adobe Photoshop CC 2018 (Macintosh)"/> </rdf:Seq> </xmpMM:History> </rdf:Description> </rdf:RDF> </x:xmpmeta> <?xpacket end="r"?>55S��IDAT8�����a��;NF�
   

   🤓 I see XML metadata. This means this image has not yet been optimized. Please use https://imageoptim.com to do that :) The result is fewer bytes to load for our end users!

3. +++ b/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.js
   @@ -0,0 +1,47 @@
   +          label: Drupal.t('Insert from Media Library'),
   

   +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   @@ -0,0 +1,152 @@
   +        'label' => $this->t('Insert from Media Library'),
   

   👍 These match.

4. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,237 @@
   +    $expected_attributes = [
   +      'data-entity-type' => 'media',
   +      'data-entity-uuid' => $this->media->uuid(),
   +      'data-align' => 'center',
   +    ];
   +    foreach ($expected_attributes as $name => $expected) {
   +      $this->assertSame($expected, $drupal_media->getAttribute($name));
   +    }
   

   🤔 Perhaps we also want to assert that no other attributes were inserted?

5. +++ b/core/modules/media_library/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,237 @@
   +    // Why do keep switching to the 'ckeditor' iframe?  Because the buttons
   +    // are in a separate iframe from the markup, so after call to
   

   Übernit: two spaces after the question mark.
   Übernit: "are" on the second line can be moved to the first line.

That's been fast! Good that we're aligned with the icon :)
Here are the 16&32px transparent PNG versions.

Since @oknate is on his day job. @phenaproxima and I are pair-programming to address the last few things :)

Like @phenaproxima just wrote, this reverts the multi-insert functionality.

Re #78

#76: Please file a feature request for that :) The code is based on Entity Embed's, which was validated with many sites. Deviating from that is a risk. It is also conceptually very different from how the existing DrupalImage + EditorImageDialog have worked for years. Finally, it makes it impossible for contrib modules to alter EditorMediaDialog and turn it into a multi-step form and allow overriding metadata immediately upon insertion. (Or at least makes that much more complex.) In short: too many risks, too many unknowns. Let's not slow this issue down by expanding scope.

@Wim Leers Thanks for the feedback, that sounds reasonable, happy to visit this further in a follow-up.

Re #81 Thanks for creating the follow-up @phenaproxima

This patch:

• fixes the nits I raised in #79
• fixes the nits @phenaproxima raised while pairing :)
• adds the icons @ckrina uploaded in #80
• enables the "hidpi" mode that CKEditor plugins can choose to opt in to, which solves the fuzziness @oknate pointed out in #77

@phenaproxima and I are now doing a final round of review 🤓

Just noticed #84's interdiff and patch were wrong. Sorry about that.

@phenaproxima and I had one last round of review, and found a few nits to fix. We fixed them all.

As far as we are concerned, this is RTBC 🥳🥳🥳

1. +++ b/core/modules/ckeditor/tests/src/Traits/CKEditorTestTrait.php
   @@ -0,0 +1,100 @@
   +   *   The name of the button, such as drupalink, source, etc.
   

   Nit: s/drupalink/drupallink/. Fixed. ✅

2. +++ b/core/modules/ckeditor/tests/src/Traits/CKEditorTestTrait.php
   @@ -0,0 +1,100 @@
   +  }
   +
   +
   +  /**
   

   Nit: double blank line. Fixed. ✅

3. +++ b/core/modules/ckeditor/tests/src/Traits/CKEditorTestTrait.php
   @@ -0,0 +1,100 @@
   +   *   The name of the button, such as `drupallink`, `source`, etc.
   

   Nit: inconsistent with ::pressEditorButton(). Fixed.

4. +++ b/core/modules/media_library/css/media_library.module.css
   @@ -2,6 +2,11 @@
   +/* Work around dialogOptions imposed by Drupal.ckeditor.openDialog(). */
   +.ui-dialog--narrow.media-library-widget-modal {
   +  max-width: 75%;
   +}
   

   @phenaproxima and I documented this more clearly.

5. +++ b/core/modules/media_library/src/MediaLibraryEditorOpener.php
   @@ -0,0 +1,78 @@
   +class MediaLibraryEditorOpener implements MediaLibraryOpenerInterface {
   
   +++ b/core/modules/media_library/src/Plugin/CKEditorPlugin/DrupalMediaLibrary.php
   @@ -0,0 +1,152 @@
   +class DrupalMediaLibrary extends CKEditorPluginBase implements ContainerFactoryPluginInterface {
   

   @phenaproxima pointed out these classes should be @internal.

6. +++ b/core/modules/media_library/tests/src/Kernel/MediaLibraryAccessTest.php
   @@ -124,6 +126,76 @@ public function testFieldWidgetEntityCreateAccess() {
   +   * Tests MediaLibraryEditorOpener::checkAccess().
   

   Should use @covers.

Sorry it took me a while: I needed to check the no hdpi icon on a normal screen. I hope it's OK now, I've used Wim's suggestion to optimize the PNG (btw, great tool, thanks!).

Thanks @phenaproxima for letting me know the small icon had white background. Here's the patch again with the transparent one. Hope this time is OK 🤞

And here's the SVG source. Bot note that the PNGs have been edited per pixels to optimize them per each resolution.

Looks great, and seems like the parts of my feedback that are part of this issue has been addressed! Yay!

However, I can't commit this because:

10:14:12] ‘/Users/angie.byron/Sites/drupal/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.es6.js’ is being checked.
[10:14:13] ‘/Users/angie.byron/Sites/drupal/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.es6.js’ is not updated.
error Command failed with exit code 1.
[10:14:15] ‘/Users/angie.byron/Sites/drupal/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.es6.js’ is being checked.
[10:14:16] ‘/Users/angie.byron/Sites/drupal/core/modules/media_library/js/plugins/drupalmedialibrary/plugin.es6.js’ is not updated.
error Command failed with exit code 1.

So we need a new patch that updates the corresponding .eg6.js files.

Actually nevermind. :) @phenaproxima and @WimLeers taught me how to yarn build:js from the /core directory.

Committed and pushed to 8.8.x. WAHHOOOOO!1!1!1&^@*# Awesome stuff, folks!!

Sean B found a bug in the final patch.

#3075443: Follow-up for #2994699: Cardinality should be 1 for inserting embedded media

When I was doing a POC for allowing multiple insertions at a time, and then we reverted that change, there was a line of code that wasn't reverted. The cardinality should be 1 in the MediaLibraryState, not -1.

This should be a quick fix.

redacted

Removed release notes tag, this is already tagged for highlights, which I think is correct.

Please comment if I'm wrong and there is any potential for disruption from this that should go in release notes.

You're correct, I didn't realize release notes required something to be disruptive. Thanks!

There is a discussion in #3099878: Media Library: Default value for data-align attribute should not be center whether the default alignment should be 'center'

please check my comment here for the patch #91 https://www.drupal.org/project/drupal/issues/3073799#comment-14655505.