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.

Ironically, this means that the first sentence in the issue summary is once again accurate: This is a follow-up to #2940029: Add an input filter to display embedded Media entities, and the second step of #2801307: [META] Support WYSIWYG embedding of media entities. 🥳

Here we go! 🚀

• Initial patch with the basics: the <drupal-media> HTML tag is upcast by the CKEditor Widget into a visual representation closely matching that on the front end (a preview).
• No configuration needed: thanks to \Drupal\ckeditor\CKEditorPluginContextualInterface it is automatically enabled whenever @Filter=media_embed is enabled.
• Testing steps added to the issue summary, they build on those for #2940029: Add an input filter to display embedded Media entities! It looks like … well, see the screenshot below :)
• Also see the attached screencast showing how it behaves on high-latency connections (I simulated 500 ms latency).

P.S.: I know that this still needs to be converted to *.es6.js, but brew install npm broke my PHP, because https://github.com/Homebrew/homebrew-core/issues/39567 🤷‍♂️ 🤯🤬I don't want to waste time on fixing my dev environment again. We can do that in a future patch iteration 🙂

Foundations are green :)

Now let's add the next important feature:

offer exactly the same great in-place caption editing UX as we currently have for images (core's DrupalImageCaption CKEditor plugin)

In the previous patch, the caption is visible (thanks to the server-side rendered preview), but it's not yet editable. That's what this interdiff adds.

Again with test coverage of course — note that Drupal core's existing captioning support for images does not have any test coverage!

Still green! ✅

Time to add the last feature:

offer exactly the same linking support

Now you can select the embedded media and press CTRL+K/CMD+K or click the "Link" button, to make the embedded media link somewhere.

Again with test coverage. And again Drupal core doesn't have any test coverage for this. (It predates us having JavaScript tests.)

And still green ✅🥳

With regards to

have its code reviewed & approved by the CKEditor team to ensure long-term maintainability and stability

I can share this, from @kkrzton, the CKEditor 4 Lead Developer:

What is your current assessment is of the code quality and particularly long-term maintainability?

I would say the code is CK-like 🙂 What I mean is that when working on it it felt familiar to our code base (apart from some code styling and spacing but it’s not important), easy to understand and has proper separation so it’s 👍 from me 😉 When it comes to long-term maintainability it would be good to have some significant code coverage. AFAIR you already created some test with Selenium, so maybe that’s already covered? It would be good to have basic scenarios cover (like single insert, edit, one undo/redo, etc). and then a few more tests with more complex scenarios (e.g. combining insertion of 2 widgets and undo).

So we're in a good place. Many thanks to the CKEditor team for their feedback and advice!

I want to provide a little bit of context:

1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -0,0 +1,230 @@
   +  function getFocusedWidget(editor) {
   ...
   +  function linkCommandIntegrator(editor) {
   ...
   +      linkCommandIntegrator(editor);
   

   This is copied verbatim from drupalimage. It can be confusing to those who don't know this code (it was to me after not having seen this for about five years), but this was written by the CKEditor team themselves.

2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -0,0 +1,230 @@
   +      // drupallink has a hardcoded integration with drupalimage. Work around that, to reuse the same integration.
   +      var originalGetFocusedWidget = null;
   +      if (CKEDITOR.plugins.drupalimage) {
   +        originalGetFocusedWidget = CKEDITOR.plugins.drupalimage.getFocusedWidget;
   +      }
   +      else {
   +        CKEDITOR.plugins.drupalimage = {};
   +      }
   +      CKEDITOR.plugins.drupalimage.getFocusedWidget = function () {
   +        var ourFocusedWidget = getFocusedWidget(editor);
   +        if (ourFocusedWidget) {
   +          return ourFocusedWidget;
   +        }
   +        // If drupalimage is loaded, call that next, to not break its link command integration.
   +        if (originalGetFocusedWidget) {
   +          return originalGetFocusedWidget(editor);
   +        }
   +        return null;
   +      };
   

   This can presumably be simplified by modifying the drupalimage CKEditor plugin to make fewer assumptions. Then all this code could go away. The entity_embed contrib module of course could not do this, but here we can 🙂

#15:

1. 🤔 I disagree. How many filters will the media module add?
2. 🤔 This is just following an existing pattern in core, see \Drupal\editor\EditorController::filterXss().
3. 🤔 See previous point.
4. ✅ Nice catch! Fixed :)
5. ✅ Another nice catch! This is apparently wrong in many places in the Entity Embed module :P
6. 👎There's plenty of examples of this in Drupal core already.
7. 🤓 That was the most fun and coolest test I've written in like a year!

This uncovered two bugs in CKEditor itself by the way; those will need follow-ups.

For #15.4 and #15.5, I added an issue to Entity Embed to fix those in the corresponding test. #3064846: [META] Port good ideas from core media embed to entity embed

Thanks, Phenaproxima.

Probably only this one, but if we ever add another one, the route name will become ambiguous.

I think we can change it at that time. The route name says it's for the media module's filter, namely to preview it. Hence media.filter.preview I don't see what's ambiguous about that?

I think the suggested media.embed_filter.preview route name is worse, because there is nothing named embed_filter. If you want to disambiguate between different filters, then media.filter.media_embed.preview perhaps makes sense. But … this route does not actually do anything specific to the media_embed filter at all. Its controller docs even say exactly that!

But perhaps media.editor.preview would be a better name? Because it's about previewing in the context of a text editor.

I don't feel strongly about the route name either, but if we're going to change it, I think we should all agree it's clearer. Hopefully this comment helps us get there 🙂

Other important context:

• This patch only allows the caption text to be edited in-place.
• It does not allow the caption itself to be turned on or off (that requires going into "source" mode and adding/removing the data-caption attribute yourself right now).
• It does not allow the alignment to be changed (that too requires going into "source" mode and adding/removing the data-align attribute yourself right now).

Why?

This is blocked on design and UX team sign-off. See #2994702-12: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` for details. See #2994702-13: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` and #2994702-14: Allow editors to alter embed-specific metadata, as well as `data-align` and `data-caption` for patches that add support for this.

In other words: we're not punting on this, it just doesn't make sense to be included in the scope of this issue.

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

Rerolled #16 to match #2940029-97: Add an input filter to display embedded Media entities's marking of data-view-mode as optional.

#2940029: Add an input filter to display embedded Media entities landed, unpostponing and rebasing 🥳

One last self-review now that I haven't looked at this in a few weeks:

1. +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   --- /dev/null
   +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   

   This still needs to be converted to *.es6.js. Last time I installed the necessary tooling, my dev env stopped working. I'm traveling next week. I am hence very hesitant to do this myself right now. If anybody else would like to do that, that'd be splendid 🙏

   (This hasn't happened yet in the Entity Embed contrib module because apparently https://www.drupal.org/node/2815083 doesn't work for contrib, only for core. #2957390: Use ES6 for contrib and custom JavaScript development exists to bring this to contrib too.)

2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -0,0 +1,230 @@
   +      // drupallink has a hardcoded integration with drupalimage. Work around that, to reuse the same integration.
   +      var originalGetFocusedWidget = null;
   +      if (CKEDITOR.plugins.drupalimage) {
   +        originalGetFocusedWidget = CKEDITOR.plugins.drupalimage.getFocusedWidget;
   +      }
   +      else {
   +        CKEDITOR.plugins.drupalimage = {};
   +      }
   +      CKEDITOR.plugins.drupalimage.getFocusedWidget = function () {
   +        var ourFocusedWidget = getFocusedWidget(editor);
   +        if (ourFocusedWidget) {
   +          return ourFocusedWidget;
   +        }
   +        // If drupalimage is loaded, call that next, to not break its link command integration.
   +        if (originalGetFocusedWidget) {
   +          return originalGetFocusedWidget(editor);
   +        }
   +        return null;
   +      };
   

   Now that this is being brought to Drupal core, it probably makes more sense to instead make the drupallink plugin not be as tightly coupled to drupalimage.

   Done. ✅

3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.js
   @@ -0,0 +1,230 @@
   +        // Minimum HTML which is required by this widget to work.
   

   This comment needs to be moved one line. ✅

4. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -0,0 +1,93 @@
   + *   This is an internal part of the oEmbed system and should only be used by
   

   C/P leftover. Fixed. ✅

5. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   @@ -0,0 +1,84 @@
   +      drupal_get_path('module', 'system') . '/css/components/hidden.module.css',
   

   🤔

   This arguably should not be done here, in the drupalmedia CKEditor plugin, but in \Drupal\ckeditor\Plugin\Editor\CKEditor::buildContentsCssJSSetting() directly. But … this only actually becomes relevant once Drupal-rendered content is embedded in CKEditor. So that arguably means this is correct.

   I don't feel strongly either way, let's find if reviewers have strong opinions about this :)

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -0,0 +1,487 @@
   +    File::create([
   +      'uri' => $this->getTestFiles('image')[0]->uri,
   +    ])->save();
   

   Nit: this could use File::setFileUri(). Argh, but because File::setFileUri() has a broken implementation, we actually can't. Never mind 🙄

Updating IS to match changes made in #2940029: Add an input filter to display embedded Media entities. #23 should already have done that.

Here too I want to make sure all people who ever contributed are credited. People who contributed to Entity Embed's js/plugins/drupalentity/plugin.js, which is the spiritual predecessor:

a.milkovsky
Ambidex
artis
BartK
Berdir
cs_shadow
CTaPByK
Dave Reid
duncan.moo
idflood
JamesK
John Pitcairn
kmoll
ls206
marcoscano
MobliMic
narnua
oknate
phenaproxima
pixelmord
Reinmar
root_brute
Ruuds
slashrsm
thenchev
webflo
Wim Leers
wrd
yched

The failures are in SkipOpTest, which was added yesterday by #2982684: Add a composer scaffolding plugin to core.

Looks like a random failure:

1) Drupal\Tests\Component\Scaffold\Integration\SkipOpTest::testProcess
RuntimeException: /tmp/composer-scaffold-test-805077626069a387b07f5263c386f338 does not exist and could not be created.

Retesting.

For what it's worth. The test CKEditorIntegrationTest isn't passing on my local.

1) Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testLinkability
WebDriver\Exception\ElementNotVisible: element not interactable
  (Session info: chrome=75.0.3770.100)
  (Driver info: chromedriver=2.46.628411 (3324f4c8be9ff2f70a05a30ebc72ffb013e1a71e),platform=Mac OS X 10.11.6 x86_64)

/Users/oknate/dev/entity_browser_testing/web/vendor/instaclick/php-webdriver/lib/WebDriver/Exception.php:155
/Users/oknate/dev/entity_browser_testing/web/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:157
/Users/oknate/dev/entity_browser_testing/web/vendor/instaclick/php-webdriver/lib/WebDriver/AbstractWebDriver.php:218
/Users/oknate/dev/entity_browser_testing/web/vendor/instaclick/php-webdriver/lib/WebDriver/Container.php:224
/Users/oknate/dev/entity_browser_testing/web/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:775
/Users/oknate/dev/entity_browser_testing/web/vendor/behat/mink-selenium2-driver/src/Selenium2Driver.php:763
/Users/oknate/dev/entity_browser_testing/web/vendor/behat/mink/src/Element/NodeElement.php:153
/Users/oknate/dev/entity_browser_testing/web/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:330
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:1071
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:939
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:698
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:894

and

Drupal\Tests\media\FunctionalJavascript\CKEditorIntegrationTest::testPreviewUsesDefaultThemeAndIsClientCacheable
Failed asserting that 913 is greater than 1024.

/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:117
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/Constraint/Constraint.php:62
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/Assert.php:2116
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/Assert.php:687
/Users/oknate/dev/entity_browser_testing/web/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php:198
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:1071
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:939
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestResult.php:698
/Users/oknate/dev/entity_browser_testing/web/vendor/phpunit/phpunit/src/Framework/TestCase.php:894

I can fix these errors with these changes:

diff --git a/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
index b81e636891..5bfbe86d42 100644
--- a/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
+++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
@@ -189,13 +189,13 @@ public function testPreviewUsesDefaultThemeAndIsClientCacheable() {
     $element = $this->assertSession()->elementExists('css', '[data-media-embed-test-active-theme]');
     $this->assertSame('stable', $element->getAttribute('data-media-embed-test-active-theme'));
 
-    // Assert that the first preview request transferred >1 KB over the wire.
+    // Assert that the first preview request transferred >912 B over the wire.
     // Then toggle source mode on and off. This causes the CKEditor widget to be
     // destroyed and then reconstructed. Assert that during this reconstruction,
     // a second request is sent. This second request should have transferred 0
     // bytes: the browser should have cached the response, thus resulting in a
     // much better user experience.
-    $this->assertGreaterThan(1024, $this->getLastPreviewRequestTransferSize());
+    $this->assertGreaterThan(912, $this->getLastPreviewRequestTransferSize());
     $this->pressEditorButton('source');
     $this->assertSession()->waitForElement('css', 'textarea.cke_source');
     $this->pressEditorButton('source');
@@ -327,7 +327,7 @@ public function testLinkability() {
     $this->getSession()->switchToIFrame('ckeditor');
 
     // Select the CKEditor Widget and click the "link" button.
-    $this->assertSession()->elementExists('css', 'drupal-media')->click();
+    $this->assertSession()->waitForElementVisible('css', 'drupal-media')->click();
     $this->pressEditorButton('drupallink');
     $this->assertSession()->waitForId('drupal-modal');

I think these changes might make the test more robust. I sure would like it to work on my local. I certainly think the first change is harmless enough:

-    $this->assertSession()->elementExists('css', 'drupal-media')->click();
+    $this->assertSession()->waitForElementVisible('css', 'drupal-media')->click();

For the second one, with the Byte size, can you think of why that might be different for me? I'm just running it with chromedriver with php PHP 7.1.26 .

#30:

• For that ElementNotVisible error: interesting that it passes both for me and testbot locally and it's not new test coverage, it's coming from Entity Embed! Is Entity Embed's CKEditor integration test coverage passing for you?

  I guess your computer must be running the test significantly slower, but it's certainly better to change >elementExists('css', 'drupal-media')->click() to waitForElementVisible('css', 'drupal-media')->click(). Go ahead and make that change! 👍

• RE: 913 is greater than 1024 — hah, this 1024 byte thing was fairly arbitrary, but it seemed like a lower bound (since I'm test with d8.test as my domain, no subdirectory, so it can't get much shorter than that). Apparently I underestimated how much shorter it can be. Let's change it to >500 then to err on the safe side.

@wim: re:

For that ElementNotVisible error: interesting that it passes both for me and testbot locally and it's not new test coverage, it's coming from Entity Embed! Is Entity Embed's CKEditor integration test coverage passing for you?

No. Some of the entity embed tests don't pass for me. You recently changed it from waitForElementVisible to elementExists. I'm suggesting to change it back in one place I encountered this. See #3064846.5. https://www.drupal.org/files/issues/2019-06-29/3064846-5.patch

If you're on a slower system it takes a bit for the preview to display, I guess.

Updated patch based on #30 and #31.

#61: 👍
#62: 🙏 One nice bonus punctuation nitpick you fixed there! 🤓 (FYI: give interdiffs the extension .txt to ensure DrupalCI doesn't try to test them. Yes this is weird, but it's the convention we have, and it works, once you know it :)

#64:

The JavaScript parts are very complex and will take some serious brain time to puzzle out. Since there's a bunch of CSS and JavaScript changes here, tagging for front-end framework manager review.

+1, but remember that in #13 I shared that the CKEditor 4 Lead Developer @kkrzton already reviewed this code; his suggestions were incorporated. In fact, I should credit him too!

1. Sure, I'm sadly not familiar with ES6 yet.
2. Good catch. Feel free to fix :)
3. See 1 :)
4. Answers can be found in #3037316: Show an outline around the <drupal-entity> element within CKEditor , specifically #3037316-24: Show an outline around the <drupal-entity> element within CKEditor . There's no elegant explanation for every one of these, they're the ones that worked through trial and error. I'd love this to be reviewed (and improved!) by a CSS expert. 🙏
5. Those comments can largely be lifted from drupalimage/plugin.js.
6. 👎 This is the name that CKEditor core maintainers chose to use in drupalimage/plugin.js and in other CKEditor plugins: https://github.com/ckeditor/ckeditor-dev/search?q=linkcommandintegrator&.... Let's stay consistent.
7. Good remark, but … this is exactly the same as drupalimagecaption/plugin.js. Let's stay consistent.
8. That'd be a nice minor improvement :) We should do the same for existing CKEditor plugins probably!

So I think I made points 1–5 and 8 actionable, I think points 6 and 7 do not require changes.

I've only started reviewing this, so this is incomplete, but here's what I found so far...

1. +++ b/core/modules/media/media.routing.yml
   @@ -39,3 +39,10 @@ media.settings:
   +  requirements:
   +    _entity_access: 'filter_format.use'
   

   Can we add an additional access requirement that the Media Embed filter is enabled for this format? E.g., if anonymous users only have access to formats that don't allow media embedding, then there's no reason to let them execute this controller.

2. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -0,0 +1,93 @@
   +    $text = $request->get('text');
   

   Can we change this to $request->query->get('text')? Or is there a reason that we need the flexibility of $request->get()?

1. +1 Question: it'd be easiest to check this in the controller, but in principle this should happen at route access checking time. That would require a new route access checker. Do you have a preference?
2. This also was raised in #15.2. My answer then: This is just following an existing pattern in core, see \Drupal\editor\EditorController::filterXss().

I don't see a reason an anonymous user should ever be able to access the media.filter.preview route. What about adding
_user_is_logged_in: 'TRUE'?

How about a new operation,

_entity_access: 'filter_format.preview'

Then update FilterFormatAccessControlHandler to specify for preview access you must be logged in and have the same access as the 'use' access.

I don't think a specific access check for whether the filter_ckeditor_media_embed filter is enabled is necessary, from a security perspective, as long as the route is protected. I think it's fine for it just to not render the embed if the filter isn't enabled, if only users with the appropriate permission can access the route.

If we add another entity operation, site builders won't have to set it every time they create a filter format.

Here are some options for protecting the route with a new access configuration.

I noticed that filter_format.use is normally not given to anonymous users, but it can be. By explicitly preventing anonymous users from hitting this route, we add additional security.

I'm leaving out the es6 changes for now, until those are fixed. Apologies to @phenaproxima. I want to test these access changes, and I don't know es6 yet.

#73:

The anonymous user may be able to use a text editor (for example for posting comments) and hence be able to use a format that includes media embeds. They may even be allowed to select media from the media library!

Adding a preview operation for filters may be possible too, but what does that mean? What would the semantical difference between preview and use be?

I don't think a specific access check for whether the filter_ckeditor_media_embed filter is enabled is necessary, from a security perspective, as long as the route is protected.

It isn't necessary strictly speaking, but we err on the side of caution and safety in Drupal core. Having one more route/controller be accessible to users that can use a text format (which could be the anonymous user!) means one extra potential attack vector. This is why @effulgentsia's proposal to neutralize this potential attack vector by denying access if the relevant filter isn't in use by the format makes sense.

I started reviewing the patch. Unfortunately, I got stuck reviewing the 9 lines of CSS on the patch. Given the complexity around that, I thought I'd post the problems I found here to see if anyone else have time to trial and error more on how to solve this. I'm planning to continue reviewing other parts of the patch next week.

The styles must work in all iterations of data-attribute, as well as without it. Currently, there's some margin set to a child element so that it's rendered inside the border. The margin should be instead outside of the border. Whatever styles we come up with, should presumably rely on the frontend theme styles so that the CKEditor content matches closely what can be seen on the page.

Here's how this looks currently:

There's one part of #76 that I'd like to get a bit more clarification on:

Whatever styles we come up with, should presumably rely on the frontend theme styles so that the CKEditor content matches closely what can be seen on the page.

I know that frontend theme styles can be provided to ckeditor via the theme's ckeditor_stylesheets: property in its info.yml file, but it seems like this approach would largely benefit Bartik (and Bartik-subtheme) users. Is there perhaps a different approach I'm overlooking that would more easily integrate this with a wider variety of themes?
If not, adding it to Bartik will still be beneficial even to non-bartik users, as they'll have something in the codebase to reference if they want to include this in their own themes. I'm interested in knowing if theres a more elegant contrib-friendly way to incorporate some of those FE styles in CKEditor.

FWIW: I had a call with @lauriii earlier today to walk him through what the challenges are. In a nutshell: #68.4 + we cannot change the DOM structure (which involves two wrappers) + CKEditor Widgets have a cke_widget_focused style that adds an outline that should work. The challenge is that due to the wrappers, the outline looks slightly weird, like @lauriii demonstrates in these screenshots.

#77: That's why this CSS does not live in a theme, but is loaded into CKEditor as part of the CKEditor plugin:

+++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css

+++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
@@ -0,0 +1,123 @@
+      $this->moduleList->getPath('media') . '/css/plugins/drupalmedia/ckeditor.drupalmedia.css',

I'm sure @lauriii meant that the CSS should not be coupled to Bartik, just like the current CSS is not. You can find inspiration in core/modules/media/css/filter.caption.css for CSS tweaks that are theme-agnostic yet are able to remove margins and padding that a particular theme is adding.

Reworking the preview route access. Adding custom access check to check if media_embed filter is enabled on the given filter format.

Adding one unrelated coding style fix. I don't think semicolons belong in the places I changed, and the coder linter was complaining these lines don't end in a period. Perhaps those should be colons, if the point is to indicate that the sentence refers to the example code below.

-    //   and therefore we have to do it explicitly here for the embedded entity;
+    //   and therefore we have to do it explicitly here for the embedded entity.
     $build['#access'] = $media->access('view', NULL, TRUE);

1. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -90,4 +92,23 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +   *   The currently logged in account.
   

   Nit: s/account/user/

2. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -90,4 +92,23 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +   *   The text format.
   

   Nit: The text format for which to check access.

3. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -90,4 +92,23 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +  public function checkAccess(AccountInterface $account, FilterFormatInterface $filter_format) {
   +
   +    $media_embed = $filter_format->filters('media_embed');
   

   Nit: Extraneous \n.

4. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -90,4 +92,23 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +    return AccessResult::allowedIf($filter_format->access('use', $account) &&  $media_embed->status === TRUE)
   +      ->cachePerPermissions()
   +      ->addCacheableDependency($filter_format);
   

   This should instead do:

   return $filter_format->access('use', $account, TRUE)
     ->andIf(AccessResult::allowedIf($media_embed->status === TRUE));
   

   Because that removes the need for the cachePerPermissions() and the addCacheableDependency() calls.

5. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -386,6 +387,66 @@ public function testLinkabilityWhenDrupalImageIsAbsent() {
   +   *   Whether to test with media_embed filter enabled on the filter_format.
   ...
   +   *   Whether the logged-in user has access to use the filter_format.
   

   Nits:

   • s/on the filter_format/on the text format/
   • s/logged-in/logged in/
   • s/has access to/is allowed to/

6. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -386,6 +387,66 @@ public function testLinkabilityWhenDrupalImageIsAbsent() {
   +    if (!$media_embed_enabled) {
   ...
   +    if (!$can_use_format) {
   

   In both cases, you're modifying the default situation. This requires the reader of the test to know about the default situation created in ::setUp().

   I think it'd be clearer if this always created a new text format and always created a new user, then there's no need for overrides and understanding things outside this test.

7. +++ b/core/modules/media/tests/src/FunctionalJavascript/CKEditorIntegrationTest.php
   @@ -386,6 +387,66 @@ public function testLinkabilityWhenDrupalImageIsAbsent() {
   +      $filter_format->setFilterConfig('media_embed', $media_embed->getConfiguration());
   

   Why is this line necessary?

1. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -0,0 +1,245 @@
   +      // Register drupal-media element as an allowed child in each tag that can
   +      // contain a div element.
   ...
   +      dtd['a']['drupal-media'] = 1;
   

   This comment doesn't take into account that we allow this to be placed inside a elements at all. Could be great if we could explain why we want this to behave like a div, with the exception that it is also allowed to be placed inside a elements.

2. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -0,0 +1,245 @@
   +          if (element.name !== 'drupal-media' || attributes['data-entity-type'] !== 'media' || attributes['data-entity-uuid'] === undefined) {
   

   Could we explain somewhere why we require drupal-media to include data-entity-type attribute if we only expect it to have a single value?

3. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -0,0 +1,245 @@
   +          // Allow ckeditor.drupalmedia.css to respond to changes (for example in alignment).
   +          this.element.setAttributes(this.data.attributes);
   

   For better consistency across Drupal, we should convert the current selectors used for targeting the drupal-media element into following BEM and CSS classes.

4. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -0,0 +1,245 @@
   +          // Track the previous state, to allow for smarter decisions.
   

   "to allow for smarter decisions" could use some extra clarity.

5. +++ b/core/modules/media/js/plugins/drupalmedia/plugin.es6.js
   @@ -0,0 +1,245 @@
   +        _loadPreview(callback) {
   ...
   +              callback(this);
   

   Should we handle scenarios where the HTTP request fails as well? Currently, if the request fails for some reason, there's no indication at all for the user that something is missing or that something went wrong.

This is a bit confusing because there's no explanation at all about what went wrong. Was there HTTP error or is there something wrong with the content?

Updating patch based on Wim's review in #80:
1) ✅
2) ✅
3) ✅
4) ✅Much nicer handing of the access results, and makes total sense.
5) ✅I checked before I added patch 79 and "logged in" and "logged-in" are both used as adjectival form in core. But changing, per your recommendation. Why are they called text formats if the machine name is FilterFormat? I guess, because that's how it's done?
6) ✅I reworked the test to use a new text format.
7) 👍You're right, that line wasn't necessary. It's been refactored away now. But I tested, and because the object is passed by reference, that line wasn't necessary.

@laurii, in regards to #82. The motivation for the placeholder was that it was invisible and therefore you couldn't focus on the broken embed to delete it. It has alt and title text to indicate something has gone wrong. Are you suggesting there should be some visible text as well, indicating what the issue is?

The motivation for the placeholder was that it was invisible and therefore you couldn't focus on the broken embed to delete it.

If users are unable to remove broken embeds, how are they supposed to deal with broken embeds? I'm also wondering if this is something that should already happen with the current patch? For some reason, I'm able to focus on broken media and delete it.

It has alt and title text to indicate something has gone wrong. Are you suggesting there should be some visible text as well, indicating what the issue is?

Having a title and alt attributes do help deliver the message. If we would make the same text visible also in the UI, it would be even clearer and easier for users to understand why the media isn't rendered as they would expect. 🙂

I guess I wasn't clear:

If users are unable to remove broken embeds, how are they supposed to deal with broken embeds?

The point of adding a broken image indicator was, before the current patch (back in the original version of the filter in entity_embed module), the embed just wouldn't render, so there was nothing in the editor to allow focus. The current patch has the image indicating that the embed is broken because the entity can't be loaded.

I'm able to focus on broken media and delete it.

That's what the placeholder image allows.

#81

1. 👍 +1 for an extra comment for the dtd['a']… line.
2. Because it matches the behavior of the filter. See #2940029: Add an input filter to display embedded Media entities. A similar question about the data-entity-type attribute was raised there. Would you like a comment such as // Match the behavior of the corresponding filter plugin.?
3. 🤔 Hm … I do not understand what this is asking.
4. 👍
5. 🤔 Yes … except pretty much nothing in Drupal core does this, so I chose to keep it simple and similar. The indication will be that it looks broken.

#82: this relates to #81.5 — I agree this could be improved. But let's be clear: this is an exceptional case. The alt and title do explain what is wrong. The content author must be able to go in and make their one change without having to click through multiple error notifications and without being forced to fix this. But they do need to be able to fix this.
#2982322: Mark missing embedded entities in WYSIWYG is the issue that did this. The rationale:

Embeds of missing entities are made accessible (and visible) in both CKEditor (for the content authors, so they can fix it) and the filtered text (for the readers, so they can still make sense of the text, and potentially report it to the site owner).

Perhaps we just need a better "missing media" image?

#83.5: because FilterFormat is the internal code-level name, "text format" is the name of the concept. Just like we say "content type" in the UI and "node type" in the code.

#87.3: we are currently using the element type and data attribute for attaching styles to the element. Instead, we should be using CSS classes.

#87.5 there's no indication that something went from. All the user will be able to see is white.

#87: I agree that we shouldn't have error notification or anything like that. I think it would be enough to just display the current title/alt text on the screen along with the image. Something like this:

+1 to #89. I didn't know about The ::after area just needs to be aligned correctly and displayed as a block-level element, is true — looking forward to seeing that 😀

we should sally forth with it.

TIL this is an idiom and not a typo: https://www.merriam-webster.com/dictionary/sally%20forth

Adding back phenaproxima's es6 changes from #69 (minus changes to drupallink, which was the source of the test failure).

This combines

• #83
• #69
• leaves out changes to drupallink - those changes need to be re-examined and reworked

Here are the changes removed from 69 that will need to be re-examined and reworked.

I'm still trying to remember when I make interdiffs manually not to put .patch extension.

Here's an experiment to add a message to the missing media indicator without having to use a creative solution (see #89 for suggested creative solution). Perhaps I'm missing something, but when testing locally this seemed to work fine.

Here are two screenshots of changes in patch #93:

With caption:

Without caption:

#91: I am very confused by that "69-91" interdiff. That includes most of the changes I made in #25, which were already in #69?

#92: again, the non-ES6 changes were made by me as early as #25.

#93 + #94: Hm … interesting. I'm not a fan because this generates different markup: it's no longer <img> but <div><img><p></div>. But perhaps this is what it takes. It looks acceptable to me as a visual user … but I think a screen reader user would not be happy to hear the exact same information twice. I think we could solve that by removing the <p> from the accessibility tree. Evidently I'm concerned we will need a new round of accessibility review, because the current approach already incorporated feedback from the accessibility maintainer in #3063705: Improve `alt` and `title` attributes for missing entity indicator after they gave feedback in the first core patch that went in, for the filter: #2940029: Add an input filter to display embedded Media entities.
Looking forward to @phenaproxima's feedback and especially @lauriii's.

That 69-91 interdiff must be off. I think because I removed the changes to the drupallink plugin.js, they showed up in the interdiff. Ignore that interdiff.

I tracked the failure to this bit of code:

diff --git a/core/modules/ckeditor/js/plugins/drupallink/plugin.js b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
index b313a25b91..786a18812a 100644
--- a/core/modules/ckeditor/js/plugins/drupallink/plugin.js
+++ b/core/modules/ckeditor/js/plugins/drupallink/plugin.js
@@ -50,6 +50,7 @@
   }
 
   var registeredLinkableWidgets = [];
+
   function registerLinkableWidget(widgetName) {
     registeredLinkableWidgets.push(widgetName);
   }
@@ -101,19 +102,20 @@
         modes: { wysiwyg: 1 },
         canUndo: true,
         exec: function exec(editor) {
-          var focusedLinkableWidget = getFocusedLinkableWidget(editor);
+          var drupalImageUtils = CKEDITOR.plugins.drupalimage;
+          var focusedImageWidget = drupalImageUtils && drupalImageUtils.getFocusedWidget(editor);
           var linkElement = getSelectedLink(editor);
 
           var existingValues = {};
           if (linkElement && linkElement.$) {

if you change

 var focusedImageWidget = drupalImageUtils && drupalImageUtils.getFocusedWidget(editor);

to

 var focusedImageWidget = getFocusedLinkableWidget(editor);

the error goes away.

After some sleuthing I found the issue:

The es6 in #25 doesn't transpile to the working js. Wim said as much in #25, and we have just missed it for several comments.

sigh, ignore this one, this has some changes from 69 on the 25 branch.

98 adds aria-hidden on the p tag. I tested adding on the image too, but decided to go with the p tag.
I also added some of the comments Lauriii recommended. #81.1 and #81.4.

in #25 Wim wrote:

This still needs to be converted to *.es6.js. Last time I installed the necessary tooling, my dev env stopped working . . . If anybody else would like to do that, that'd be splendid.

This lead to the drupalink javascript file in #69 to #98 to be either out of sync or wrong (failing). This patch fixes that, it changes the drupallink es6, so that when transpiled, it matches the js.

#97: Thanks for that separate interdiff, that makes it much easier for me to do a comprehensive review :)
#99 Thanks for figuring that out!

1. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -0,0 +1,112 @@
   +   *   The text format for which to check access.
   +   * @return \Drupal\Core\Access\AccessResultInterface
   

   Needs a blank line in between. (I'm surprised phpcs doesn't complain?!)

2. +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -0,0 +1,112 @@
   +  public function checkAccess(AccountInterface $account, FilterFormatInterface $filter_format) {
   +
   +    $media_embed = $filter_format->filters('media_embed');
   

   Needs blank line to be removed.

3. +++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
   --- a/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   +++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
   

   I'm not a fan of the fact that we're modifying the filter in this patch. #2940029: Add an input filter to display embedded Media entities just introduced it. But the only thing we're changing (besides punctuation nits in comments) is the missing media indicator.

   The importance of that indicator becomes more important in this issue, so refinements being made here is reasonable.

4. +++ b/core/modules/media/css/plugins/drupalmedia/ckeditor.drupalmedia.css
   @@ -0,0 +1,19 @@
   +.missing-media__message {
   +  text-align: center;
   +}
   

   Why would this only be relevant in CKEditor? This is also relevant when it's displayed to the end user. Because the filter is what generates this message, and hence this markup also is displayed to end users.

   So this should be moved to filter.caption.css.

5. +++ b/core/modules/media/media.routing.yml
   @@ -45,4 +45,4 @@ media.filter.preview:
   -    _entity_access: 'filter_format.use'
   +    _custom_access: '\Drupal\media\Controller\MediaFilterController::checkAccess'
   
   +++ b/core/modules/media/src/Controller/MediaFilterController.php
   @@ -90,4 +92,21 @@ public function preview(Request $request, FilterFormatInterface $filter_format)
   +  public function checkAccess(AccountInterface $account, FilterFormatInterface $filter_format) {
   +
   +    $media_embed = $filter_format->filters('media_embed');
   +    return $filter_format->access('use', $account, TRUE)
   +      ->andIf(AccessResult::allowedIf($media_embed->status === TRUE));
   +  }
   

   Actually … if we keep both _entity_access: … and _custom_access: …, then we can simplify the callback to just check if $media_embed->status === TRUE. And then the callback can be renamed to ::formatUsesMediaEmbedFilter().

   Because all requirements are ANDed.

Once my #100 remarks are addressed, the patch looks ready to me. Next steps:

1. @lauriii to confirm his concern from #88 has been addressed
2. if @lauriii confirms this, we need to get accessibility maintainer sign-off per #98
3. @effulgentsia to confirm his concern from #70.1 has been addressed and my answer to .2 was adequate

Addressing feedback from #100:

1 👍
2 👍
3 I'm not a big fan of changing the missing media indicator either. I think the ghostbusters logo is sufficient. We put a lot of work into making sure that was bug free for Entity Embed. To change it here last minute here is less-than-desirable.
4 Moved the css rule. UPDATE: This needs work now. It's no longer center-aligned, neither in the CKEditor, nor when the page is saved.
5 👍 Updated the access on the route to use two requirements.

Fixing the css for #100.4. I checked and filter.caption.css seems to be loaded even if filter caption is disabled for the format, because the stable theme is loading it.

I feel there's something wrong with the way we're loading the css rule for the missing media here. It should to be tied to the media embed filter, rather than the caption filter. That way, if a theme doesn't have filter.caption.css it still works.

UPDATE

I can confirm, if your theme doesn't override filter.caption.css and filter_caption is not enabled for a text format, the missing media embed's p tag is not text aligned centered. So we need a different way to attach it.

Continuing to work on the css for the revised missing media indicator. I found the center alignment no longer works if you turn off filter caption (and if you're not using a theme that includes it, such as stable). Also, the width of the missing-media div would go full screen without filter caption on and a caption present. So I borrowed some of the css from filter.caption.css and applied it to the .missing-media div. I added a new library for this.
While the filter caption library is tied to the rendered media embed, it is not tied to the missing media indicator. So we'd have to add the caption library to missing embed indicator as well. Which would work, but seems like not the quite right place for it. If you have disabled filter caption to require its css for a completely separate element that seems odd.

This is the same as the last patch (#105), except that I had forgotten to delete one css rule:

diff --git a/core/modules/media/css/filter.caption.css b/core/modules/media/css/filter.caption.css
index dc69ce8aa6..a92505c308 100644
--- a/core/modules/media/css/filter.caption.css
+++ b/core/modules/media/css/filter.caption.css
@@ -8,7 +8,3 @@
   float: none;
   margin: unset;
 }
-
-.missing-media__message {
-  text-align: center;
-}

I missed this interdiff on the last comment, accidentally uploaded the same interdiff twice.

I changed my mind about adding a new css library. When I saw the failure from #106 I realized I'd have to add it to the stable theme as well. And then I realized that I'm adding two css files right along media/filter.caption.css. So now I feel the redundancy is worse than the lack of accuracy of the name of the media/filter.caption.css (I was feeling it was inaccurate because it now affects more than just the caption). I do think we could rename media/filter.caption.css to media/filter.media_embed.css, I don't think it's absolutely necessary. And since this css file is already added to 8.8.x, I'd rather not start a discussion about renaming unnecessarily. It could turn into a red herring / bikeshed from the goal of getting this RTBC. So going back to Wim's original suggestion that I add it to filter.caption.css. It wasn't working, because I needed to add it to stable as well. Now that I am adding it to both the media/filter.caption.css in the media module and in the stable theme, the changes take affect properly. But we do need to add it to the wysiwyg.

diff --git a/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
index 470d879c1c..316da2ae2f 100644
--- a/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
+++ b/core/modules/media/src/Plugin/CKEditorPlugin/DrupalMedia.php
@@ -77,7 +77,7 @@ public function isEnabled(Editor $editor) {
   public function getCssFiles(Editor $editor) {
     return [
       drupal_get_path('module', 'media') . '/css/plugins/drupalmedia/ckeditor.drupalmedia.css',
+      drupal_get_path('module', 'media') . '/css/filter.caption.css',
       drupal_get_path('module', 'system') . '/css/components/hidden.module.css',
     ];
   }

as well as the render array for the missing media:

diff --git a/core/modules/media/src/Plugin/Filter/MediaEmbed.php b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
index f134691f98..1a957f1825 100644
--- a/core/modules/media/src/Plugin/Filter/MediaEmbed.php
+++ b/core/modules/media/src/Plugin/Filter/MediaEmbed.php
@@ -233,7 +233,7 @@ protected function renderMissingMedia() {
         ],
       ],
       '#attached' => [
+        'library' => ['media/filter.caption'],
       ],
     ];
   }

This takes care of this:

If your theme doesn't override filter.caption.css and filter_caption is not enabled for a text format, the missing media embed's p tag is not text aligned centered.

This allows it work both in the wysiwyg and when viewing the page after saving.

This still needs addressing:

#87.3: we are currently using the element type and data attribute for attaching styles to the element. Instead, we should be using CSS classes.

I started working on it. I think Lauriii wants these rules changed to use classes:

drupal-media[data-align=left],
drupal-media[data-align=right] {
  display: inline;
}
drupal-media[data-align=center] {
  display: flex;
}

This is a bit of a heavy lift, I believe, as the drupalmedia plugin.js has allowed attributes, the text format / editor form has validation of the allowed attributes on the drupal-media tag and CKEditor is manipulating the class tag to add "cke_widget_element".
I'm thinking we could change them to classes:
.drupal-media--align-left
.drupal-media--align-right
.drupal-media--align-center
but allowing those classes to persist on the drupal-media tag might require changing a lot of code.

After looking into this a bit more, I don't think we'll need to change the validation on the text format / editor form. Also, they don't need to persist as they are only in the wysiwyg and can be removed on downcasting.

So the plugin.js could add these classes to the widget, if the data properties are there. These rules don't persist out of the wysiwyg.

Addressing #87.3 (and #110). It would be good as a follow-up to this patch to add some simple test coverage that the css styles in this patch are applied.

This patch adds:

• test coverage for #111 (Addressing #87.3).
• some coding standard fixes.

• #104: RE It should to be tied to the media embed filter, rather than the caption filter. +1
• #109: Having granular asset libraries is good. I liked #106 better. It's fine to add it to stable as well.
• #110: Ah, so that's how #81.3 had to be interpreted! I strongly disagree with this. This is not about your typical styling, this is specifically something that is a CKEditor Widget implementation detail. I'd even prefer hardcoding setStyle() calls in the CKEditor Widget JS and deleting the CSS file altogether than adding additional classes for nicer selectors. That'd make it more explicit that this is an implementation detail.
• #113.13: 👎 This suggestion would result in a weird UX that we don't have anywhere else. See #87.5.
• #113.14: 🤔 This is follows the pattern established in \Drupal\media\Controller\OEmbedIframeController. If we're changing it here, then let's also update that one.
• #114++

This patch addresses feedback from #113. I'll address #118 separately.

#113.3:

In regards to this rule:

+.missing-media {
+  display: table;
+}

I'm not a FE dev. I pulled this from /core/modules/filter/css/filter.caption.css.

I tried changing it to other common display options and none allowed the element's width to shrink to fit its contents. I'll leave it until a FE dev comes up with something better.

This is how I was testing if you want to give it a look: Put a "sleep(500)" statement into CKEditorIntegrationTest::testLinkability() after the embed is rendered in the CKEditor, then press the "source" button and edit the uuid so it's invalid.

Here's a screenshot of #85.5. (#113.3)

There's some disagreement about whether we should do this. If for some reason the embed (or missing entity indicator) fails to render, such as a fatal error in a preprocess function or a server becoming unresponsive, this will give feedback to the user. I see it as a minor change and a net positive as it will handle an edge case we haven't handled.

There's no test coverage for this. This is how I was testing if you want to give it a look:

• Add throw new NotFoundHttpException(); at the beginning of MediaFilterController::preview()
• Put a "sleep(500)" statement into CKEditorIntegrationTest::testLinkability() after the embed is rendered in the CKEditor.

Adding back separate css file for the .missing-media css per #118.2.

For #81.3, which was implemented in #110 and disagreed with in #118.

In #118 Wim writes:

Ah, so that's how #81.3 had to be interpreted! I strongly disagree with this. This is not about your typical styling, this is specifically something that is a CKEditor Widget implementation detail.

I can see your point. They are single use rules, not to be re-used, so there's no point adding reusable classes, and it's somewhat clearer to have it in a css file, but it does give the impression that these are rules. I think more comments could allow us to remove the classes and go back to the original. I think before I change or revert this, Wim and Lauriii should come to agreement as to what to do here.

I'd even prefer hardcoding setStyle() calls in the CKEditor Widget JS and deleting the CSS file altogether than adding additional classes for nicer selectors. That'd make it more explicit that this is an implementation detail.

I like it in a css file. It's easier to see the rules laid out and separated from the data manipulation. So I'd rather not use setStyle calls(). I'd rather either keep it as is now, or revert it over that.

#118 related to #113.13: The problem I was trying to point out is that without explicitly handling the error scenario, there's no visual hint what so ever that something has gone wrong if the preview fails to load. Without explicitly handling errors, the UI would have a little bit of empty space instead of a preview whenever an error occurs. It's hard to predict how users would react to that, but it's hard to imagine that they would at least at first associate that to a system-level error. Instead, they might expect it to be something wrong with their content, and they would try to take action to solve that. This issue is related to this problem: #2987444: Ajax errors are not communicated through the UI.

I totally agree with @lauriii with the need of giving more information/feedback to the user to improve the usability of the content authoring experience.

So for #82, this icon is not enough feedback. First, because if it will show up when the referenced media has been deleted, it means the source is missing. And actually this icon usually stands for “forbidden”. Maybe we could try to use a “broken link” or similar representation. Apart from the icon itself, some more info should be given, specially because we have it and we can point out the user to solve the issue.
So I’d add an explanatory text similar to “The referenced media source has been deleted.”

#81.5 (and answering Wim’s #87.5)

🤔 Yes … except pretty much nothing in Drupal core does this, so I chose to keep it simple and similar. The indication will be that it looks broken.

That is exactly one of the most frustrating experiences when editing content: ghost errors that you don’t know how to solve. So we should try to clearly communicate:
a. That something went wrong.
b. How to solve the current situation.
c. Warnings about what can go wrong so they have a chance to decide before doing anything.
d. Ideally, what has gone wrong. Probably difficult to know here.

So I’d suggest, as mentioned in earlier comments, that we show a message as implemented in #120 adding some instructions:
“An error occurred while trying to preview(load?) the media source. Save you content to avoid data loss and reload this page.”

Addressing #118.3, I present two options, the third being no change.

• Option 1, use set style as proposed in #118.3.
• Option 2, revert changes made to address #81.3.
• Option 3: leave as is.

My concern with setStyle is that, if someone needs to override these settings for some reason, it is much harder to do that. My concern with reverting the changes is, Lauriii feedback that we should use classes instead of data attributes to attach css will not have been addressed.

Therefore my opinion is the best option here is option 3, followed by option 2.

The patch numbers are off due to another comment coming in as I was adding mine.

Updated patch for reverting BEM classes change, see #125. I had forgotten to remove the functional js test.

In regards to this in #124:

I’d add an explanatory text similar to “The referenced media source has been deleted.”

If the media to be embedded cannot be loaded based on the uuid, we can't tell if it's been deleted, or has never been added. Therefore, we have to use a more generic message "Missing media". I think the reason it's missing, while valuable, cannot be ascertained with confidence.

Addressing #118.5, updating @internal comment in OEmbedIframeController.

We still need to address #76.