[media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`

What media modules are you using? What's in core now doesn't have its own upload widget as such and is hidden.

This is definitely not a core issue, I'm guessing it's more related to Entity Browser, but it could be Entity Embed instead, not sure.

I'd bet it is possible to achieve what you are looking for, but TBH I don't remember off the top of my head the steps to do that. My recommendation would be for you to research a bit the issue queue for Entity Browser and/or Entity Embed how to expose the Alt field when you are embedding a media entity.

If you can't find anything conclusive, try explaining here the steps to reproduce your scenario on a clean install, so hopefully someone can spot the part of the process that should be changed (or the bug, if it exists).

@if-jds Clean install refers to "no contributed modules" modules usually, as in "core only".

Forget that...not in this case.

This is not only an Entity Embed issue. It's also happening with Entity Browser. I have created a content type with a Media Image field. The Form Display I am using for that field is 'Entity Browser' using two widgets - 'Upload images as media items' and 'View'. For both of these options when I upload a new Media Image or Browse Existing Media Images, when I select the desired image, the image is placed in the Media Image field and displays only the file name and an Edit and Remove button. To enter the ALT text, I need to click on the Edit button after placing the image - which content editors are not going to do. More worryingly, I can save the node without entering the 'required' Alt text.

BTW, I'm not using Entity Embed. I have recreated this on a new install of Drupal 8.5.5 and 8.6.0-alpha-1 with only Media, Entity Browser and Chaos Tools enabled in addition to the Standard Install.

Anyone got any ideas? Also, where should this issue be posted?

F

Back to Entity Browser?

RE: comment #14 This is exactly my problem too.

We are experiencing this workflow problem also.

For now we will have to tell our clients to "be sure to click 'edit' so you can add alt text".

I may look at a quick form_alter hook to see if we can add a warning message when the (hidden) alt text is detected to be empty.

The alternative text is already associated with/stored in the image managed by Media. Why do you need to be able to override it?

Why do you need to be able to override it?

I can see the uses for this, for sure. It might have accessibility implications, too. The media item stores canonical alt text/title metadata, but that metadata might not make sense, or might otherwise lack some important context, when it's embedded in content. So I think that being able to override some metadata (alt text being the 80% use case, I'd hazard to guess) in a particular embed is quite useful. I believe Entity Embed already supports this, at least to some extent; I know that Lightning has code which will use the image embed display plugin by default when embedding an image media item, precisely to fulfill this case.

You already allude to the tricky problem that this comes with: it only makes sense for images!

It doesn’t make sense for *all* media.

Or am I seeing that wrong? :)

@phenaproxima, do you understand how this relates to #2834112: [file entities] @EntityEmbedDisplay=image plugin's `alt` and `title` attributes not saving in CKEditor?

@if-jds: Also: thank you very much for your swift response! I hope we can bring this issue to a close in the near future.

@phenaproxima, do you understand how this relates to #2834112: [file entities] @EntityEmbedDisplay=image plugin's `alt` and `title` attributes not saving in CKEditor?

Although the issue titles are similar, the actual problems are unrelated. That issue only refers to embedding file entities directly, whereas this is about embedding a media entity. The mechanisms for doing those two things are quite different, and the bug in the other issue is only affecting embedded files.

Aha, so:

1. #2834112 is specifically about it already being possible to specify alt and title for images when using the @EntityEmbedDisplay=image plugin and using File entities. The only remaining problem there is that some configuration is not yet updated automatically.
2. whereas this issue is about bringing the same ability to Media entities.

Is that a correct understanding?

That is exactly right.

FYI, in comment number #5, @if-jds is using Media Entity module and Media entity image. Media entity is now in core and includes an image bundle. To solve the issue, we should make sure it works for both the contrib module(s), used on older sites and the core version.

The sandbox module fixed the issue by using a new formatter based on Drupal\image\Plugin\Field\FieldFormatter\ImageFormatter.

Testing this locally I found that if I select "Thumbnail" as the display option, it's using core MediaThumbnailFormatter and was giving (in Drupal 8.6) alt="thumbnail" in the output.

But that's because Thumbnail isn't really intended for display to the end user.

So that may be a fix for those who selected "Thumbnail" as the display in their test entity embed button.

It doesn't fix the workflow issue where you can't edit the alt text and title text within the form produced by EntityEmbedDialog. I think that's worth pursuing.

alt="thumbnail" in the output.

That's what #2956368: MediaThumbnailFormatter produces unhelpful text alternative and title attributes for media thumbnails fixed indeed.

It doesn't fix the workflow issue where you can't edit the alt text and title text within the form produced by EntityEmbedDialog. I think that's worth pursuing.

Yep, that's what this issue is about :)

Here's a patch that adds access to the media image's alt and title fields within the dialog. Seems to work well with the view mode formatter.

It it seems like it wouldn't cause issues with any other embeds.

To do, it needs to be abstracted, so that we aren't hard coding the bundle and field name.

Here's a slightly more generic variation on #33 that ignores the entity type and bundle and just checks if it has a file field at $entity->field_image.

This will work for core's media image as well as others that follow this pattern.

the patch above won't work on the standard profile because the standard profile's media image doesn't have field_image, it has field_media_image.

I think we'd need some sort of indication of which field to use, or else we'd just be guessing.

Also, I don't think this patch works with translation.

redacted, was thinking we might need a new config or third party setting to specify the field, because I was ignorant of the media source plugin.

We can use that to determine if this is a media image and which field is the primary image field.

OK, I see the above is unnecessary.

When you go to create one there's a form for setting the source field, so we can use that to determine the correct field.

Here's a new patch based on #33 and #34 that uses the Source plugin to determine if it's an image and if it has a primary image field.

To do:

1) Alter the patch to make it work with translations of alt and title
2) add tests.

I'm looking at editing the alt and title in the correct language based on the parent entity's language. But it looks like EntityEmbedDialog has no knowledge of the parent entity's language. Created a separate issue for that. #3018485: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set

Another possible solution would be if we can get the correct translation from the selected entity's uuid. Is that possible?

It doesn't look like uuids are language specific, so that doesn't help. So I think we would need to pass the language code to the dialog to allow editing of the correct alt and title.

Correct. Thanks for opening #3018485: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set!

I have a patch for #3018485: Selected Entity should display in host entity's language in EntityEmbedDialog when data-langcode not set that allows patch #38 to work with content translation.

title field doesn't always exist on media image field_media_image field. Out of the box it isn't enabled:

So if we use the media image field's alt and title field as in patch 38, we'll need to account for that. Basically, we need to check if the field settings allow for it before displaying the form.

I tested it out when the title tag isn't enabled, and the code in patch 38 still manages to save the title property on field_media_image.

So that raises the question, should we ignore the default settings or hide the title field? I would think it should respect the field settings.

Adding code to check if the title and alt field are disabled on the field settings, and if so, do not show in the entity embed dialog.

I think that being able to override some metadata (alt text being the 80% use case, I'd hazard to guess) in a particular embed is quite useful.

I can also see the counterargument being made. What if you want to change the alt text on the original image each place it's used? Then it's no longer stored in one place. You'd have to find everywhere it's embedded and change it. I can understand wanting to change the caption, which is stored with the embed's drupal-entity data properties, but I think the alt and title should shouldn't be made data properties on the embed.

I could see another workflow where you use the media image field's alt and title as the defaults when embedding, but allow changing them, and once the embed is created, they are now decoupled from the original values. This would allow more granular control, but would also mean if you wanted to change the alt text on the original image, it wouldn't propagate to the embeds.

- I think we could have a setting whether to use (to be added properties) data-image-alt and data-image-title properties.
- The administrator should be able to choose if they want the edit form to update the original alt/title.
- If the administrator chooses unique alt and title per embed, the alt and title could be saved on data-image-alt and data-image-title properties on the drupal-entity.

If we added all of this, it might be over-engineered, but everyone could be happy.

One issue with adding this functionality is that it isn't relevant for certain display plugins, such as label or entity id.

So if someone is using an entity embed based on a media entity and selects two display plugins, "label" and one derived from a view mode, it should really appear with ajax within the embed dialog if the user selects the view mode display. If you always display it for media image, then it would show up in the dialog when someone is embedding with the "label" formatter.

I guess that's not a real world situation. Most of the time, if someone's embedding an image, they'll use a view mode display plugin. So maybe it's best to always have it to keep the code simpler. They could always hide the fields with an alter hook.

Here's a patch that addresses the issue mentioned in #48, that alt and title are irrelevant for "label" display plugins, et. al. This patch moves the code to the view mode formatter. This way, if you select "label" as well as a view mode display plugin, as you switch the select in the entity embed dialog, the alt text field goes away when you select label and returns when you select the view mode display.

Still to do:
- look into ImageFieldFormatter, which behaves differently. See if the functionality can be combined in some way.
- look into adding a checkbox or select to change the behavior so that if they want to save the alt and title per embed, rather than back to the image field on the media, user has an option to do that.
- add test coverage.

Here's a video file showing the alt text field showing only on the view mode formatters, patch #49.

Hmm. Patch #48 wouldn't add the option to other plugins other than the view mode. The current title of this ticket has this verbiage:

Regardless of @EntityEmbedDisplay plugin

But it isn't relevant for many plugins, so I think the title of the ticket could be revised. Only plugins that actually display an image should have the option.

Oops. I was testing the change in #49 and found it wasn't saving. This is why we need test coverage.

patch 50 had the same problem, was diffing the wrong branch. This should fix it.

I'm trying to test out the ImageFieldFormatter, and I ran into a bug which is preventing me from testing it. See #3022754: @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected.

I figured it out. See the issue for more details. The behavior is confusing when the image isn't valid.

I added a second issue related to ImageFieldFormatter: #3022768: Validate that `alt` and `title` are required attributes for `<drupal-entity>`, and ensure they're added by default.

You must manually add "alt" and "title" attributes to the allowed html tags in the editor, or else the alt and title won't save.

- Borrows some of the behavior from ImageFieldFormatter, such as allowing double quotes to represent an empty alt, even if required.
- Adds a select field under the alt textfield that allows choosing if you want to update the original entity or save to the embed!

Still to do:
- needs feedback / review
- needs test coverage.

I looked into whether we should apply the concept of saving to the embed vs saving to the entity to the ImageFieldFormatter display, and I believe file entity's only have the file_managed table and it doesn't support alt and title properties. So unless that changes, the ImageFieldFormatter can't support saving to the entity.

This patch moves the functionality to a separate class MediaViewModeFieldFormatter that extends ViewModeFieldFormatter. This new class is used for view mode display plugins when the entity type is "media". Since alt and title text is irrelevant for most entities, this is better organized.

I also tested with caching turned on and added a fix for caching. When you use the alt and title attributes on the drupal-entity to override the entity values, the entity view mode render array needs to have a different cache key.

$build['#cache']['keys'][] = 'embed';

Tagging this, to remind myself to try this out soon. There's a lot of discussion about the how various options (or override policies?) should work, and I'm not sure I'll understand what you're building until I try using it.

I'm skeptical of the idea in #44-45, that the embedded entity's image should follow the same policy for alt text as the original imagefield. It might make more sense to set the policy on the place where it's being embedded, if that's feasible. I'll ponder that a bit more, in the context of ATAG.

I think it would be perfectly reasonable for a site builder to set up an image media type without enabling the alt text field at all. Often the point of a media library is just to have a central place to collect assets, without caring about what purpose they will serve as content of an actual webpage. A default/canonical alt text may not be worth collecting in the library - only the alt text in context matters. A publisher may prefer to govern accessible content with house style guides and reviews, rather than entity schemas.

Conversely, alt text may be required by a image media type, but that doesn't mean an image can't serve a purely decorative purpose when embedded on a particular page.

#44:

So that raises the question, should we ignore the default settings or hide the title field? I would think it should respect the field settings.

+1. You did that in #45, great!

#46:

What if you want to change the alt text on the original image each place it's used? Then it's no longer stored in one place.

That would still work if no override was specified in the EntityEmbedDialog: if no override is specified, no alt attribute is injected, which should result in the embedded entity's alt attribute being used. Changing the "original" (i.e. the embedded entity) would result in that showing up everywhere where alt was not overridden.

I could see another workflow where you use the media image field's alt and title as the defaults when embedding, but allow changing them, and once the embed is created, they are now decoupled from the original values.

This is not how I think most Media Initiative people think about this.

#47: why data-image-alt and data-image-title and not just alt and title?

#48: correct. We should only show these fields if they also will show up in the chosen entity embed display. I see #49 already did that 👍 Thanks for the video in #50!

#56:

- Adds a select field under the alt textfield that allows choosing if you want to update the original entity or save to the embed!

This is cool … but I think this is dangerous to do; it can easily result in data loss. It's hard to communicate clearly to the end user. I'd like to remove this for usability reasons.

#60:

It might make more sense to set the policy on the place where it's being embedded, if that's feasible.

That's not feasible.

The only two choices are: A) respect the existing field settings (which is what #45 does), or B) always require alt and title.

I think it would be perfectly reasonable for a site builder to set up an image media type without enabling the alt text field at all. Often the point of a media library is just to have a central place to collect assets, without caring about what purpose they will serve as content of an actual webpage. A default/canonical alt text may not be worth collecting in the library - only the alt text in context matters.

I disagree. Every image needs some textual description. Otherwise searching the media library becomes impossible. Searching just by filename is a nightmare (IMG_*.JPG times five thousand, anyone? 😂). And … that's also what Drupal 8 core already does out of the box: look at /media/add/image 😊

At the very least: the default in Drupal 8 does have alt enabled for image media, and such defaults are rarely changed.

Conversely, alt text may be required by a image media type, but that doesn't mean an image can't serve a purely decorative purpose when embedded on a particular page.

This is true. But … how often to content authors insert purely decorative content in a text editor? Content authors shouldn't be dealing with pure decoration, but with content. Purely decorative images in text editors is generally a bad practice. And in any case, there already is an opt-in way out for this: the same one as Drupal core's image dialog has: entering "" as the alt text.

So as far as I'm concerned, the primary thing that should change is the removal of the "Save to embed" functionality.

for #47, I think the reason I was thinking #data-image-alt and #data-image-title was to be consistent with the other properties. This isn't an img tag, so to distinguish the properties, I was thinking giving them names that match the other attributes, such as data-entity-id. It's not important to me what they're called. I don't have a strong preference. If you like alt and title, I can change it back.

. . the primary thing that should change is the removal of the "Save to embed" functionality.

Sounds good, I can take a look in the next week, if no one else wants to jump in. Given the crickets I've been hearing around here, I doubt it.

I think alt and title are well-understood, well-defined and therefore appropriate. We're using them on a custom element, but it'll be instantly clear to anybody who seems them what their meaning is.

Sounds good, I can take a look in the next week, if no one else wants to jump in. Given the crickets I've been hearing around here, I doubt it.

I think you're right :( That'd be wonderful! 🥳

I thought of another good reason to use alt and title: it's already set up that way, so it's more backwards compatible.

#64: 👍

FYI: I marked #3052239: [media entities] Prepopulate caption with Media field data as related to this: what this issue/patch does for the alt and title attributes, that aims to do for the data-caption attribute. Let's keep that separate though, because it's more complicated.

Patch review before you start working on this again, so that you can tackle it all in one go:

1. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
   @@ -0,0 +1,200 @@
   +  public function build() {
   +    $build = parent::build();
   +
   +    if ($this->getConfigurationValue('alt_method') == 'embed') {
   +
   +      $entity = $this->getEntityFromContext();
   

   Can't we override ::getEntityFromContext() rather than overriding ::build()? That'd mean less duplicated code AFAICT :)

2. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
   @@ -0,0 +1,200 @@
   +      $build['#cache']['keys'][] = 'embed';
   

   Rather than doing this, I think we should unset($build['#cache']) in \Drupal\entity_embed\EntityEmbedDisplay\FieldFormatterEntityEmbedDisplayBase::build()? Because caching an embed doesn't really make sense anyway; it's being cached as part of the host entity anyway :)

   Also: how is it possible we've never encountered this caching problem before?! :O EDIT: oh well, apparently there's still a whole bunch of TODOs relating to this, dating back to years ago. I shouldn't have been surprised I guess.

3. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
   @@ -0,0 +1,200 @@
   +    if ($entity->getEntityTypeId() != 'media') {
   +      return NULL;
   +    }
   

   AFAICT this should be impossible, so if it is happening anyway, this should throw an exception.

Here's an updated patch. It drops the option to save to the entity, per #61. I still think saving to entity would be a great alternative workflow, and this is still an issue with some of the entity browser upload widgets. If someone uploads an image through one of these widgets, the alt and text won't have been set, so there's no path to add the alt text on the image field of the media entity in that case yet. But perhaps that should be handled separately in entity browser module.

Perhaps the media upload widget within the entity browser should have its own fields for adding alt and title within the selection phase of the entity embed dialog (and while within the entity browser upload widget), or it should signal to the entity embed dialog that those values need saving and a form alter hook (within the entity browser module) could be added to change the default behavior of the title and alt field added in this patch.

In response to #66.2. If you remove this:

+++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
@@ -0,0 +1,200 @@
+      $build['#cache']['keys'][] = 'embed';

and unset($build['#cache']) in FieldFormatterEntityEmbedDisplayBase::build(), it doesn't prevent the embed from getting cached, because it has a pre render hook EntityViewBuilder::build which adds back the cache tags.

Perhaps it could be done with changes to FieldFormatterEntityEmbedDisplayBase::build() You could pre-render the entity and then remove the cache tags within FieldFormatterEntityEmbedDisplayBase::build() but you might break someone's custom implementation where they alter the cache tags.

Unless we can think of a better way to do it, I think adding a cache key within MediaViewModeFieldFormatter::build is the way to go.

Also, I tried moving the alteration of the field's alt and title to getEntityFromContext, but it doesn't work, because the entity gets rebuilt. See ViewModeFieldFormatter::getFieldValue():

  public function getFieldValue() {
    return ['target_id' => $this->getContextValue('entity')->id()];
  }

Here's the context of the code above:

$items = $this->typedDataManager->create(
      $definition,
      $this->getFieldValue($definition),
      $definition->getName(),
      $node->getTypedData()
    );

    // Prepare, expects an array of items, keyed by parent entity ID.
    $formatter = $this->getFieldFormatter();
    $formatter->prepareView([$node->id() => $items]);
    $build = $formatter->viewElements($items, $this->getLangcode());

The build of the embed pretty much must go through this process, in order to capture alter hooks to the field formatter output. Therefore the overriding of the image field's alt and title needs to happen after FieldFormatterEntityEmbedDisplayBase::build(), which is why an override with the extra code works.

I also added some code because of a bug I encountered:

TypeError: Argument 2 passed to Drupal\Component\Plugin\PluginManagerBase::createInstance() must be of the type array, null given, called in /home/vagrant/docroot/web/modules/contrib/entity_embed/src/Form/EntityEmbedDialog.php on line 494 in Drupal\Component\Plugin\PluginManagerBase->createInstance() (line 71 of /home/vagrant/docroot/web/core/lib/Drupal/Component/Plugin/PluginManagerBase.php).

Here's the code I added:

diff --git a/src/Form/EntityEmbedDialog.php b/src/Form/EntityEmbedDialog.php
index f5f6964..7d991a5 100644
--- a/src/Form/EntityEmbedDialog.php
+++ b/src/Form/EntityEmbedDialog.php
@@ -491,6 +491,9 @@ class EntityEmbedDialog extends FormBase {
       if (is_string($entity_element['data-entity-embed-display-settings'])) {
         $entity_element['data-entity-embed-display-settings'] = Json::decode($entity_element['data-entity-embed-display-settings']);
       }
+      if (empty($entity_element['data-entity-embed-display-settings'])) {
+        $entity_element['data-entity-embed-display-settings'] = [];
+      }
       $display = $this->entityEmbedDisplayManager->createInstance($plugin_id, $entity_element['data-entity-embed-display-settings']);
       $display->setContextValue('entity', $entity);
       $display->setAttributes($entity_element);

Looking at this code now, it looks like maybe I should have used "elseif", since the conditional statement above would mean that $entity_element['data-entity-embed-display-settings'] is not empty, so you could skip checking it again.

Here's an interdiff for #59 to #67.

Addressing

Looking at this code now, it looks like maybe I should have used "elseif" . . .

there's no path to add the alt text on the image field of the media entity in that case yet. But perhaps that should be handled separately in entity browser module.

This is IMHO a problem with the defaults for when originally uploading media: it should not be allowed to upload those without specifying alt for each. That's also the approach Drupal 8.7's Media Library has taken. Perhaps Entity Browser ought to follow core's lead there.

In response to #66.2. If you remove this: […]

Argh, you're right. I was gonna say we should just make isViewModeCacheable() return FALSE i.e. it's just a configuration problem at that point, but Entity Embed allows any view mode to be selected, so that won't work either. Nor will hook_ENTITY_TYPE_ID_builde_defaults_alter().

Perhaps it's okay for us to cache these after all. But … just adding a embed part to the key still will yield incorrect results, because it assumes that there's only one variation ever: a single "embed" variation. But every host entity may specify a different alt override. Which is why I think it is far better to not cache embedded entities' representations separately at all.

So I did some more digging, and AFAICT this should work.

Thoughts?

Interesting. I hadn't thought of the edge case of embedding the same image with different alt text. Here's a test case with alt text of "cat 1". Testing with

• no alt tag on the drupal-entity, which should render with "cat 1"
• alt="cat 1 different alt text"
• alt="cat 1 different alt text 2"
• alt=""""

<drupal-entity data-embed-button="media_entity_embed" data-entity-embed-display="view_mode:media.full" data-entity-embed-display-settings="" data-entity-type="media" data-entity-uuid="a23b59dc-b39d-43b5-a356-41ff09e0cdda" data-langcode="en"></drupal-entity>

<drupal-entity alt="cat 1 different alt text" data-embed-button="media_entity_embed" data-entity-embed-display="view_mode:media.full" data-entity-embed-display-settings="" data-entity-type="media" data-entity-uuid="a23b59dc-b39d-43b5-a356-41ff09e0cdda" data-langcode="en"></drupal-entity>

<drupal-entity alt="cat 1 different alt text 2" data-embed-button="media_entity_embed" data-entity-embed-display="view_mode:media.full" data-entity-embed-display-settings="" data-entity-type="media" data-entity-uuid="a23b59dc-b39d-43b5-a356-41ff09e0cdda" data-langcode="en"></drupal-entity>

<drupal-entity alt="&quot;&quot;" data-embed-button="media_entity_embed" data-entity-embed-display="view_mode:media.full" data-entity-embed-display-settings="" data-entity-type="media" data-entity-uuid="a23b59dc-b39d-43b5-a356-41ff09e0cdda" data-langcode="en"></drupal-entity>

It totally doesn't work, not with #69 nor with #73. Marking back to "Needs Work".

You were on the right track. You just needed to remove the 'embed' cache key too. This works now with caching turned on and the test case in #74 embedded in one node's body.

Ah yes, I did make that change locally but hadn't included it in the patch 😁

I think the only thing that's missing is functional test coverage! :)

I'll work on the test coverage this week.

Thank you! 🙏 🥳

I started working on test coverage for this, but I realized the current patch doesn't add the alt and title "Regardless of @EntityEmbedDisplay plugin", for example media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php isn't affected. This is because the override I created only affects view mode derived display plugins, not field formatter derived plugins.
There are two derivers that ship with entity embed:

• /src/Plugin/Derivative/ViewModeDeriver.php
• /src/Plugin/Derivative/FieldFormatterDeriver.php

Here's initial test coverage.

There were some bugs I found.

1) A typo in the field settings:

diff --git a/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
index 0a70f8d..bd17afe 100644
--- a/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
+++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
@@ -33,7 +33,7 @@ class MediaViewModeFieldFormatter extends ViewModeFieldFormatter {
         $entity->{$image_field}->alt = $this->getAttributeValue('alt');
       }
 
-      if (!empty($settings['field_field']) && $this->hasAttribute('title')) {
+      if (!empty($settings['title_field']) && $this->hasAttribute('title')) {
         $entity->{$image_field}->title = $this->getAttributeValue('title');
       }
     }

2) #79, we need to get this working for displays using FieldFormatterDeriver.php
3) I found with the view mode deriver, if the thumbnail is displayed in the view mode, rather than the image, the alt and title text do not get overridden. I don't know why yet.

There were a few things I got stuck on, such as the @todo item here:

+    $field = FieldConfig::load('media.image.field_media_image');
+    $settings = $field->getSettings();
+    $settings['alt_field'] = FALSE;
+    $settings['title_field'] = TRUE;
+    $field->set('settings', $settings);
+    $field->save();
+
+    // @todo flush only what's necessary.
+    drupal_flush_all_caches();

We need to figure out what to clear after updating the FieldConfig, so that the changes are reflected in the dialog, so we don't have to clear everything.

One thing I noticed after posting this: There's an extraneous select on the first two lines here that should be removed:

+    $select->setValue('entity_reference:entity_reference_label');
+    $this->assertSession()->assertWaitOnAjaxRequest();
+
+    // @todo entity_reference:media_thumbnail should also have alt and title text.
+
+    $select->setValue('view_mode:media.embed');
+    $this->assertSession()->assertWaitOnAjaxRequest();

Here's an update, although it's still a work in progress, I just ran out of time tonight.

It fixes media/src/Plugin/Field/FieldFormatter/MediaThumbnailFormatter.php using a similar approach in the FieldFormatterDeriver that we used in the ViewModeDeriver:

      // @todo Don't hard code this.
      if ($formatter == 'media_thumbnail') {
        $this->derivatives[$formatter]['class'] = MediaReferenceFieldFormatter::class;
      }

Since there aren't entity types to check in the deriver as there were with the view mode deriver, this is going to be trickier, so I put in a hack (hard-coded media_thumbnail) for now. We need to find a solution either here or when the plugin is loaded to switch classes based on the entity type, or just include the media functionality in the base class and then remove the two classes MediaViewModeFieldFormatter and MediaReferenceFieldFormatter. Another idea would be to have entity specific loading, or plugin using a different class on createInstance in the plugin factory. The classes, if kept, have a lot of redundancy between them, so we'd want a solution for that.

For this:

I found with the view mode deriver, if the thumbnail is displayed in the view mode, rather than the image, the alt and title text do not get overridden. I don't know why yet.

I haven't tested, but I'm pretty sure, since I had to fix this for the MediaThumbnailFormatter, we have to just update the alt and title on the thumbnail as well:

diff --git a/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
index bd17afe..33baa98 100644
--- a/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
+++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaViewModeFieldFormatter.php
@@ -31,10 +31,12 @@ class MediaViewModeFieldFormatter extends ViewModeFieldFormatter {
 
       if (!empty($settings['alt_field']) && $this->hasAttribute('alt')) {
         $entity->{$image_field}->alt = $this->getAttributeValue('alt');
+        $entity->thumbnail->alt = $this->getAttributeValue('alt');
       }
 
       if (!empty($settings['title_field']) && $this->hasAttribute('title')) {
         $entity->{$image_field}->title = $this->getAttributeValue('title');
+        $entity->thumbnail->title = $this->getAttributeValue('title');
       }
     }
 

I started to change getImageSourceField, and I realized it doesn't even need entity interface, it should be MediaInterface. I think that's left over from an earlier iteration of this. So this function needs revision.

@@ -144,10 +146,6 @@ class MediaViewModeFieldFormatter extends ViewModeFieldFormatter {
    */
   protected function getImageSourceField(EntityInterface $entity) {
 
-    if (!$entity->getSource() instanceof Image) {
-      return NULL;
-    }
-
     $config = $entity->getSource()->getConfiguration();
 
     if (!empty($config['source_field'])) {

I discussed this with @oknate in Slack. Thanks to his patient explanation, I think I understand the problem we're facing here, and I propose a couple of potential solutions.

It seems to me that the issue here is we're trying to detect, at discovery time, which plugins will be capable of handling "images", or anything which could potentially have alt/title text. That seems a bit fragile to me; Entity Embed is designed to be extensible, so there's no real way for us to detect all possible ones which could need the capability. At least, not in an elegant fashion.

So, I see two possible options here:

1. We could abstract this stuff into a trait for handling alt and title attributes, which plugins could use as needed, and leave it at that. Make it an explicit, opt-in part of Entity Embed's API.
2. Instead of trying to figure out which plugins will need alt and title, we could assume any of them might, and automatically wrap all display plugins with a decorator which would detect the need for, and implement the handling of, alt and title text on a per-entity basis. I posted a gist with a quick sketch of this approach: https://gist.github.com/phenaproxima/319914a266d9663d93ad1aa86da47e3b

Thoughts?

I really like the gist that #83 proposes. 👍 Let's try to go that way :)

Two remarks about it:

1. I don't think $configuration['_entity_embed_image_decorator'] is necessary — if a decorator is indeed working transparently, then there should be no reason for particular decoratable things to opt in or out of it.
2. The ::isEmbeddingAnImage() sample code is talking about the MIME type and using that to determine whether alt and title override form items should be offered. Perhaps that makes sense in a future step, but I think this should be solely focused on Media entities representing images, since only there do we know how to retrieve the stored alt and title, that we allow the user to override.

And finally, the one thing that I did see in the Slack chat but not captured in #83 is that the problem that @oknate was running into in #81 with derivers is that the concrete entity is not available at plugin derivation time; the decorator solution that @phenaproxima proposes is executed at runtime, and at that point a concrete EntityInterface object is available for inspection, so the challenges @oknate was facing vanish.

I don't think $configuration['_entity_embed_image_decorator'] is necessary — if a decorator is indeed working transparently, then there should be no reason for particular decoratable things to opt in or out of it.

I'm not sure I agree with this. Although I do think it's an edge case, there could be plausible scenarios where a display plugin wants to handle an "alt textable" entity (i.e., something it considers to be an image), yet not expose the alt text and title controls to the author. In which case, it would want to opt out of the decorator.

That said, it is an edge case and I'd be fine deferring this opt-out stuff for later, until someone actually requests it.

@phenaproxima and I agreed in Slack that we'd prefer not adding $configuration['_entity_embed_image_decorator'] because it's extra API surface that we need to document and maintain and support; we agree it's better to only add that when the need arises.

So, I think we're all on the same page here :)

Here's a proof of concept for the decorator. It works, at least within the confines of the MediaImageTest functional javascript test the patch adds.

I need to add additional test coverage for the field formatter plugins and I need to add method comments within the decorator class, do some cleanup.

I think there are risks where if someone calls a custom method on their EntityEmbedDisplay, the decorator will not have implemented it. It's too bad there's not a generic way to pass methods on to the decorated class.

There's also a lot of jenky code like this:

  public function setContextValue($name, $value) {
    if (method_exists($this->decorated, 'setContextValue')) {
      $this->decorated->setContextValue($name, $value);
    }
  }

setContextValue() is in the EntityEmbedDisplayBase but not in EntityEmbedDisplayInterface.

There may be a few more methods we need to add. We need to look at all the EntityEmbedDisplay plugins we know about and implement all of the known methods.

It seems like a lot of unnecessary code, and perhaps we could find a different solution. But it does force every plugin to be considered a candidate for our alteration, so for now, it's the best known solution.

I do want to keep exploring other possibilities. I think there may be a less verbose way of doing this.

setContextValue() is in the EntityEmbedDisplayBase but not in EntityEmbedDisplayInterface.

Yeah, I noticed this when writing the gist. The answer is (and we could do this in another issue, and block this one on that one) to add ContextAwarePluginInterface to EntityEmbedDisplayInterface, and make EntityEmbedDisplayBase extend ContextAwarePluginBase. Entity Embed already assumes that stuff is there, but it never formally supported it by using the correct interfaces. This could even potentially block RC, in my opinion -- I'd like to get Wim's opinion, though.

It's too bad there's not a generic way to pass methods on to the decorated class.

There's __call() :) Example: \Drupal\Core\Plugin\Discovery\InfoHookDecorator::__call().

Cool I'll look into that. A magic method. Makes sense. That would let us remove some of the irrelevant methods.

Adds remaining test coverage and cleans things up. I don't know of any remaining issues.

I don't know of any remaining issues.

The failing testbot disagrees with you :D

TypeError: Argument 1 passed to Drupal\entity_embed\Plugin\entity_embed\EntityEmbedDisplay\MediaImageDecorator::getMediaImageSourceField() must be an instance of Drupal\media\MediaInterface, instance of Drupal\file\Entity\File given, called in /Users/oknate/dev/entity_browser_testing/web/modules/contrib/entity_embed/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php on line 177 in Drupal\entity_embed\Plugin\entity_embed\EntityEmbedDisplay\MediaImageDecorator->getMediaImageSourceField() (line 254 of /Users/oknate/dev/entity_browser_testing/web/modules/contrib/entity_embed/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php).

Hmm. I thought I checked the entity type before passing it to that method.

Here's a fix.

Thanks so much for all your work on this! 🙏 👏

1. +++ b/src/EntityEmbedDisplay/FieldFormatterEntityEmbedDisplayBase.php
   @@ -169,6 +169,11 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
   +    // Don't ever cache a representation of an embedded entity, since the host
   +    // entity may be overriding specific values (such as an `alt` attribute)
   +    // which means that this particular rendered representation is unique to the
   +    // host entity, and hence nonsensical to cache separately anyway.
   +    unset($build[0]['#cache']['keys']);
   

   This could land in a separate issue; this is a separate bug that merits separate test coverage.

2. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
   @@ -0,0 +1,269 @@
   +  /**
   +   * An array of EntityEmbedDisplay ids to exclude.
   +   *
   +   * @var array
   +   */
   +  private $excludeList = [
   +    'entity_reference:entity_reference_entity_id',
   +    'entity_reference:entity_reference_label',
   +  ];
   

   We still need to hardcode knowledge about specific entity embed display plugins. Not ideal, but better than the status quo. I think this is acceptable.

   OTOH, isn't this what $configuration['_entity_embed_image_decorator'] is for? If we need a reject list like this hardcoded, I'd much prefer to use $configuration['_entity_embed_image_decorator'] instead, because that's at least a pattern that third party entity embed display plugins can adopt themselves, without needing to also get added to this reject list.

3. +++ b/tests/modules/entity_embed_test/entity_embed_test.info.yml
   @@ -3,3 +3,13 @@ type: module
   + - drupal:file
   ...
   + - drupal:editor
   

   Nit: At least these two dependencies are unnecessary, perhaps more.

4. +++ b/tests/src/FunctionalJavascript/EntityEmbedTestBase.php
   @@ -42,4 +43,23 @@ JS;
   +   *   A string containing a drupal-entity dom element.
   

   Nit: s/dom/DOM/, s/drupal-entity//

I created a separate issue for #97.1

#3058872: Entity Embed render array shouldn't cache separately from host field

I'll work on the other three comments, marking back to Needs work.

• For #97.2, I decided to try the plugin definition instead. It's overridable, but much more static. The issue I was having with $configuration is that createInstance is called several places, and since we don't control the default configuration for those plugins, it would need to be set or altered at run time. If you think we need that granularity, we could set it up.

  

• For #97.3, I removed those two and the tests ran ok. When I get a chance, I'll see if there are other dependencies that are unnecessary.
• For #97.4: Updated

• Adding test coverage that node embed button doesn't show the alt and title.
• Adds an optional check for $configuration['_no_media_image_decorator'], although this isn't used:
  

  diff --git a/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php b/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php
  index 10f23ad..6c6ef5e 100644
  --- a/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php
  +++ b/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php
  @@ -184,7 +184,7 @@ class EntityEmbedDisplayManager extends DefaultPluginManager {
       $instance = parent::createInstance($plugin_id, $configuration);
       $definition = $instance->getPluginDefinition();
   
  -    if (!empty($definition['_no_media_image_decorator'])) {
  +    if (!empty($definition['_no_media_image_decorator']) || !empty($configuration['_no_media_image_decorator'])) {
         return $instance;
       }
       else {

• Changes the strings in the test to be more fun. I went with odd animal names.

#99: FYI: <ol start=2> makes the first <li> start at 2 :)

+++ b/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php
@@ -176,4 +177,21 @@ class EntityEmbedDisplayManager extends DefaultPluginManager {
+    if (!empty($definition['_no_media_image_decorator']) || !empty($configuration['_no_media_image_decorator'])) {

This combined with what you wrote in #99 made me realize that it's wrong to use $configuration['_entity_embed_image_decorator']. What we want is to add an optional plugin annotation key supports_image_alt_and_title to \Drupal\entity_embed\Annotation\EntityEmbedDisplay with a default value of FALSE, and then opt in just the (derived) plugins that support it. That means we'll only retain the if ($definition['…']) check.

Other than that, this is now looking close. Sorry it took us this long to get here :)

Updated. There were a few coding style changes too.

Corrected interdiff.

I like this much more!

1. +++ b/src/Annotation/EntityEmbedDisplay.php
   @@ -55,4 +55,14 @@ class EntityEmbedDisplay extends Plugin {
   +   * Shows alt and title fields in the dialog if applicable.
   

   Supports per-embed alt and title overrides for media entities with an image source.

   Happy to do this on commit.

2. +++ b/src/Annotation/EntityEmbedDisplay.php
   @@ -55,4 +55,14 @@ class EntityEmbedDisplay extends Plugin {
   +  public $supports_alt_and_title = FALSE;
   

   Perhaps supports_image_alt_and_title_override would be more descriptive?

3. +++ b/src/EntityEmbedDisplay/EntityEmbedDisplayManager.php
   @@ -184,7 +184,7 @@ class EntityEmbedDisplayManager extends DefaultPluginManager {
   +    if (empty($definition['supports_image_alt_and_title'])) {
   

   This does not match what's in the annotation :)

I like supports_image_alt_and_title_override, although it's not really an override as an addition.

Some options:

• supports_image_alt_and_title_override
• supports_image_alt_and_title
• display_alt_and_title_field
• alt_title
• supports_alt_and_title
• alt_title_enabled
• llama_alt_title

1. "Supports per-embed alt and title overrides for media entities with an image source." is 83 characters, so against the Drupal standard "All summaries (first lines of docblocks) must be under 80 characters".

Partial review. I didn't get to the tests, but I found only nitpicks in the stuff I did read -- except for one major, but not commit-blocking, thing.

1. +++ b/src/EntityEmbedDisplay/EntityEmbedDisplayBase.php
   @@ -324,6 +324,20 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
   +   * @return bool
   +   *   Returns true of value is set.
   

   Should be 'Returns TRUE if value is set.'

2. +++ b/src/EntityEmbedDisplay/EntityEmbedDisplayBase.php
   @@ -324,6 +324,20 @@ abstract class EntityEmbedDisplayBase extends PluginBase implements ContainerFac
   +    $attributes = $this->getAttributeValues();
   +    return array_key_exists($name, $attributes);
   

   Nit: This can be one line.

3. +++ b/src/EntityEmbedDisplay/FieldFormatterEntityEmbedDisplayBase.php
   @@ -169,6 +169,11 @@ abstract class FieldFormatterEntityEmbedDisplayBase extends EntityEmbedDisplayBa
   +    unset($build[0]['#cache']['keys']);
   

   Nit: Is there any particular reason not to simply unset($build[0]['#cache'])?

4. +++ b/src/Plugin/Derivative/FieldFormatterDeriver.php
   @@ -65,9 +65,21 @@ class FieldFormatterDeriver extends DeriverBase implements ContainerDeriverInter
   +      if (in_array($formatter, $no_media_image_decorator)) {
   

   Nit: TRUE should be passed as the third argument to in_array().

5. +++ b/src/Plugin/Derivative/ViewModeDeriver.php
   @@ -64,6 +64,9 @@ class ViewModeDeriver extends DeriverBase implements ContainerDeriverInterface {
   +        if ($definition['targetEntityType'] == 'media') {
   

   Nit: Should use ===

6. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/EntityReferenceFieldFormatter.php
   @@ -21,7 +21,8 @@ use Symfony\Component\DependencyInjection\ContainerInterface;
   + *   field_type = "entity_reference",
   + *   supports_image_alt_and_title = TRUE
   

   Nit: supports_image_alt_and_title = TRUE should have a trailing comma.

7. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
   @@ -0,0 +1,254 @@
   +  /**
   +   * Passes through all unknown calls to the decorated object.
   +   */
   +  public function __call($method, $args) {
   +    return call_user_func_array([$this->decorated, $method], $args);
   +  }
   

   Nice idea!

8. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
   @@ -0,0 +1,254 @@
   +  public function buildConfigurationForm(array $form, FormStateInterface $form_state) {
   +
   +    $form = $this->decorated->buildConfigurationForm($form, $form_state);
   

   Nit: There is a superfluous blank line here.

9. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
   @@ -0,0 +1,254 @@
   +    if ($image_field = $this->getMediaImageSourceField($entity)) {
   +
   +      $settings = $entity->{$image_field}->getItemDefinition()->getSettings();
   

   And here.

10. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +      // Setting empty alt to double quotes.  See ImageFieldFormatter.
    

    Supernit: There's an extra space before "See".

11. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +      if ($settings['alt_field_required'] && $alt == '') {
    

    This should probably use ===

12. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +        $alt = '""';
    

    Not a big deal, but should '""' be a constant on this class?

13. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +      if (!empty($settings['alt_field'])) {
    +
    +        // Add support for editing the alternate and title text attributes.
    

    Nit: Extra blank line!

14. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +  public function submitConfigurationForm(array &$form, FormStateInterface $form_state) {
    +
    +    /** @var \Drupal\Core\Entity\EntityInterface $entity */
    +    $entity = $this->decorated->getEntityFromContext();
    

    Nit: Blank line.

15. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +        if (trim($values['alt']) === '""') {
    +          $values['alt'] = '""';
    +        }
    +        // If the alt text is unchanged from the values set on the
    +        // field, there's no need for the alt property to be set.
    +        if ($values['alt'] === $entity->{$image_field}->alt) {
    +          $values['alt'] = '';
    +        }
    

    This seems like it should be an if/elseif?

16. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +        if (empty($values['title'])) {
    +          $values['title'] = '';
    +        }
    +        // If the title text is unchanged from the values set on the
    +        // field, there's no need for the title property to be set.
    +        if ($values['title'] === $entity->{$image_field}->title) {
    +          $values['title'] = '';
    +        }
    

    Same here?

17. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +  protected function getMediaImageSourceField(EntityInterface $entity) {
    +
    

    Nit: Blank line.

18. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +    if ($entity->getEntityTypeId() != 'media' || !$entity->getSource() instanceof Image) {
    +      return NULL;
    +    }
    

    Not commit blocking, but this is a bit faulty and inflexible. Rather than test the entity's declared entity type ID, just check if it implements Drupal\media\MediaInterface. And we shouldn't couple this to a specific source plugin class -- instead, we should retrieve the source field definition and determine if its item class is ImageItem. This means it will support core image fields and anything that derives from them. If this can't be fixed now, can we open a follow-up issue for this, and mention it in a @todo comment above this code?

19. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/MediaImageDecorator.php
    @@ -0,0 +1,254 @@
    +    if (!empty($config['source_field'])) {
    +      return $config['source_field'];
    +    }
    

    If we do as I mentioned above and check the source field definition ($entity->getSource()->getSourceFieldDefinition()), we don't need this check. Checking configuration values of the source plugin directly is, ultimately, violating encapsulation and therefore more brittle than it ideally should be.

    So overall, this method should look something like this:

    if (!($entity instanceof MediaInterface)) {
      return NULL;
    }
    
    // @todo Support field types that extend from image fields.
    $field_definition = $entity->getSource()->getSourceFieldDefinition($entity->bundle->entity);
    if ($field_definition->getType() === 'image') {
      return $field_definition->getName();
    }
    return NULL;
    

Thank you for your careful review, @phenaproxima. Lots of helpful feedback.

• 1. Updated
• 2. Updated.
• 3. Leaving cache key deletion as is for now. I think for bubbling up of cache tags we need to leave it the cache. I also vaguely remember when testing removing it didn't work for some reason. We have a separate ticket for this, so we can revisit it. #3058872: Entity Embed render array shouldn't cache separately from host field
• 4. Updated
• 5. Updated
• 6. I don't think the Drupal standard is to add a trailing comma on the last item of an annotation. I checked the standards page on Drupal.org and a bunch of examples in core. The annotation shows an example without it, and when looking at annotations in core, it seems that more often than not, the last item doesn't have a trailing comma. Since it's not explicitly addressed in the standards, I think it's a matter of preference, and I prefer to leave it off, although I don't feel strongly about it.
• 8. Updated
• 9. Updated
• 10. Updated
• 11. Updated
• 12. Updated. Using a constant is nice because we can add a description to it.
• 13. Updated
• 14. Updated
• 15. Updated.
• 16. Updated.
• 17. Updated.
• 18. Updated, checking instanceof to allow subclasses to work.
• 19. Updated, checking instanceof on field item to allow subclasses to work.

Test failure due to this error:

Error: Undefined class constant 'EMPTY_STRING' in Drupal\entity_embed\Plugin\entity_embed\EntityEmbedDisplay\ImageFieldFormatter->submitConfigurationForm() (line 214 of /Users/oknate/dev/entity_browser_testing/web/modules/contrib/entity_embed/src/Plugin/entity_embed/EntityEmbedDisplay/ImageFieldFormatter.php)

When I was cutting and pasting I failed to add the constant to this class too.

Coding standard fixes.

#107.3: that'd make us lose cacheability too. We just don't want to trigger render caching for embeds. We don't want to lose cache tags, contexts and max-age.

#108.3: Right, thanks for reminding me of #3058872: Entity Embed render array shouldn't cache separately from host field. That's technically a blocker to this issue.

#110: rather than duplicating the constant, I think it should just use the same constant.

1. I realized there were no screenshots yet! Added one.
2. Found & fixed a bunch of nits.
3. This had me confused the most:
   

   +++ b/src/Plugin/Derivative/FieldFormatterDeriver.php
   @@ -65,9 +65,21 @@ class FieldFormatterDeriver extends DeriverBase implements ContainerDeriverInter
   +    $no_media_image_decorator = [
   +      'entity_reference_entity_id',
   +      'entity_reference_label',
   +    ];
   +
        foreach ($this->formatterManager->getOptions($base_plugin_definition['field_type']) as $formatter => $label) {
          $this->derivatives[$formatter] = $base_plugin_definition;
          $this->derivatives[$formatter]['label'] = $label;
   +
   +      // Set plugins we know don't need alt and title to not use
   +      // MediaImageDecorator.
   +      if (in_array($formatter, $no_media_image_decorator, TRUE)) {
   ...
   +      }
   

   This should not be necessary since the default value for the annotation already is FALSE. Oh, but this is the (confusingly named, this is a pre-existing problem) deriver for \Drupal\entity_embed\Plugin\entity_embed\EntityEmbedDisplay\EntityReferenceFieldFormatter, which does have it set to TRUE. D'oh. Clarified the comment.

#108.3: Right, thanks for reminding me of #3058872: Entity Embed render array shouldn't cache separately from host field. That's technically a blocker to this issue.

Let's see if tests here pass without that change.

That failed, thanks to the explicit test coverage that this patch was adding.

Instead of doing that here, let's bring #113's interdiff and this comment's interdiff to #3058872: Entity Embed render array shouldn't cache separately from host field. The interdiff in this comment basically does exactly what I suggested over at #3058872-7: Entity Embed render array shouldn't cache separately from host field we do for test coverage.

By managing issue scope, we also are indirectly managing test scope. The test coverage that this interdiff removes also happens to not match the description (and hence scope) of the test method it's in! By moving that to a separate test, this becomes a lot simpler. Also pretty important: it doesn't need to be a functional JS test!

Fixed one remaining CS fail, #111 introduced it.

Fixed another CS Fail that I introduced in #114.

The needs accessibility review was added in #60 by accessibility maintainer @andrewmacpherson (yay, thanks for being present here!).

1. The most important thing he wrote:
   

   I'm skeptical of the idea in #44-45, that the embedded entity's image should follow the same policy for alt text as the original imagefield. It might make more sense to set the policy on the place where it's being embedded, if that's feasible. I'll ponder that a bit more, in the context of ATAG.

   I wrote in #61:

   The only two choices are: A) respect the existing field settings (which is what #45 does), or B) always require alt and title.

   But that was wrong.

   There is only one choice: A) respect the existing field settings. Because Entity Embed reuses the existing entity view modes/displays, and those can only display fields that are actually present in a particular entity.

2. The other thing he wrote:
   

   Conversely, alt text may be required by a image media type, but that doesn't mean an image can't serve a purely decorative purpose when embedded on a particular page.

   This is working, and now even has explicit test coverage 🥳

Given that, removing needs accessibility review.

Besides, this module is currently in beta, and has been for years. And this is definitely a step forward, because right now, you can only provide alt and title by manually modifying the HTML! We'll do an RC before tagging a stable release, at that point it'll be great to have a round of accessibility review :)

Before committing, I wanted to test this manually. Unsurprisingly, it works great :)

We're far beyond 100 comments. This is a huge step forward. I think @phenaproxima still wanted to review the tests, and they can probably be improved just like any test can be improved, but it's testing the obvious edge cases already. Let's get anything else that we believe could and should be done better in a separate issue.

Thanks @oknate for being the driving force on this!

I don't know why this was not marked critical, because it is! That's why it was listed in #2577891: Entity Embed 8.x-1.0.0-rc1 release, because being able to create accessible content is critical.

🚢

Now see you in #3058872: Entity Embed render array shouldn't cache separately from host field for sure and perhaps also #3052239: [media entities] Prepopulate caption with Media field data!

ARGH WTF! I've marked @oknate as the primary author on d.o, but it apparently reset that, now I've credited myself instead of @oknate 😩

Very sorry, @oknate, I'll credit the next patch that I'm the primary author of to you.

No worries. Thanks for hard work on this this morning, and getting it committed!