Problem/Motivation

The ImageFieldFormatter requires alt and title added to drupal-entity, but this is not documented in the README.md and those tags aren't included in the list of attributes added when adding an entity embed on the editor/filter form.

Out of the box, a person who creates a file embed button will not know why their "alt" and "title" attributes aren't saving.

This took me some digging to figure out. If you use a file entity embed with the "Image" ImageFieldFormatter, you must manually update the Allowed HTML tags to allow "alt" and "title" attributes on the drupal-entity tag.

Proposed resolution

- Documentation should be added to the README.md.
- "alt" and "title" should be added to list of allowed attributes added when an entity embed is created
- validation should be added to assure if someone manually removes the required tag and attribute from the allowed_html, the filter/editor form throws an error.

Remaining tasks

- reviews

User interface changes

- form validation added to filter_format_edit_form

API changes

- alt and title tags added to list of allowed tags for drupal-entity

Comments

oknate created an issue. See original summary.

oknate’s picture

Issue summary: View changes
oknate’s picture

Title: ImageFieldFormatter requires manually updating whitelisted attributes on drupal-entity » ImageFieldFormatter lacks documentation
Issue summary: View changes
oknate’s picture

Issue summary: View changes
oknate’s picture

StatusFileSize
new4.38 KB

Here's a patch that adds some documentation to the README.md and adds validation on the form "filter_format_edit_form".

oknate’s picture

To do:
- Since the patch is adding a check for the drupal-entity tag within allowed_html, I think it could be updated to check each required attribute of the drupal-entity tag.
- add test coverage

oknate’s picture

StatusFileSize
new833 bytes
new5.19 KB

Oh, there's a way to add the alt and title to drupal-entity automatically when a button is added to the editor. Adding to this patch.

See #2554687: <drupal-entity> tag not being automatically added when an Entity Embed button is added to active editor toolbar for reference.

This kind of makes the validation I'm adding less useful. But I suppose someone could manually remove those, and they should be warned that the button they have enabled has a dependency on those attributes.

oknate’s picture

StatusFileSize
new9.35 KB
new14.54 KB

Adding test coverage

oknate’s picture

Issue summary: View changes
oknate’s picture

Title: ImageFieldFormatter lacks documentation » ImageFieldFormatter alt and title attribute validation
oknate’s picture

Issue summary: View changes
oknate’s picture

StatusFileSize
new4.35 KB
new14.55 KB

Here's a updated patch that throws out the ImageFieldFormatter specific validation, and adds validation for a base set of attributes on the drupal-entity tag.

I'm working on an another issue that will add support for the alt and title tags on media entity of source type "image", see #2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`.

oknate’s picture

Issue summary: View changes
Status: Active » Needs review
andrewmacpherson’s picture

Issue tags: +accessiblity
andrewmacpherson’s picture

Issue tags: -accessiblity +Accessibility

cleaning up issues with this typo in the tag name

wim leers’s picture

Title: ImageFieldFormatter alt and title attribute validation » @EntityEmbedDisplay=image plugin: alt and title attribute validation

wim leers’s picture

I marked #3036302: Make Image Title Required as a duplicate of this, but I'd like it to be considered here too: @alienzed wrote:

Making the Image title mandatory (optionally) would be great!

oknate’s picture

Title: @EntityEmbedDisplay=image plugin: alt and title attribute validation » Validate that 'alt' and 'title' are required attributes for drupal-entity

I wouldn't say that this makes the title and alt mandatory. It validates that they are allowed attributes on the drupal-entity embed element. This is required for proper functioning of saving of alt and title when saving it to the entity embed.

This issue:
#2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`
is more about having the alt and title available in the dialog, and how the dialog should behave.

There are two different types of behavior could be implemented:

  • Saving alt and title to the embed, which is necessary for direct image embeds as the file_managed table doesn't save those properties.
  • Saving alt and title back to the media entity if the embed is media of type image.

If you're using media, in some cases you might want to update every instance's alt and title, in other use cases, you might want the alt and title to be overridable on per embed basis. It really depends on what the expected user wants. Patch #17 on issue 2924391 adds an option to either save it to the embed or update the original media image.

So you could say that 2924391 requires this patch (or requires someone to know they need to add alt and title to the allowed attributes on drupal-embed in the filter settings).

The latest patch here is just assuring that alt and title are properties that are available. If they aren't used, for example on a node embed, they won't be added to the embed. So again, I wouldn't say that this makes the title and alt mandatory. This is making them allowed out of the box.

oknate’s picture

Rerolling #12. The interdiff is a little messed up because of a merge conflict.

oknate’s picture

Because I keep stepping on my own toes, I'm creating a meta issue with some of my test updates:
#3055928: [META] 8.x-1.0-beta4 plan

wim leers’s picture

Title: Validate that 'alt' and 'title' are required attributes for drupal-entity » Validate that `alt` and `title` are required attributes for `<drupal-entity>`, and ensure they're added by default
Component: Documentation » Code
Status: Needs review » Reviewed & tested by the community
Issue tags: +Usability
StatusFileSize
new8.55 KB
new14.81 KB
  1. #20: sorry, you're right, I misunderstood. I was basing my comment too much off of the issue title instead of reading the entire issue. Thanks for explaining it so patiently :)
  2. I'm pleasantly surprised that you added validation logic for this!
  3. I'm even more pleasantly surprised you added test coverage for this!

Can we please clone you? 😀


I only found nits, which I all fixed:

  1. +++ b/entity_embed.module
    @@ -109,6 +109,88 @@ function entity_embed_filter_format_edit_form_validate($form, FormStateInterface
    +  $allowedHtmlPath = [
    

    Nit: camelCase when it shouldn't be.

  2. +++ b/tests/src/FunctionalJavascript/ImageFieldFormatterTest.php
    @@ -158,12 +155,18 @@ class ImageFieldFormatterTest extends WebDriverTestBase {
    +    // Currently there is a bug where you must enable filter_align and caption
    +    // and have them in a certain order.
    

    This comment is outdated. Removed.

  3. +++ b/tests/src/FunctionalJavascript/ImageFieldFormatterTest.php
    @@ -158,12 +155,18 @@ class ImageFieldFormatterTest extends WebDriverTestBase {
    +    $this->assertSession()->responseNotContains('The <em class="placeholder">Image Embed</em> button requires "alt" and "title" among the attributes of the "drupal-entity" tag within the allowed html tags.');
    

    Nit: s/html/HTML/.

  4. +++ b/tests/src/FunctionalJavascript/ImageFieldFormatterTest.php
    @@ -186,10 +190,70 @@ class ImageFieldFormatterTest extends WebDriverTestBase {
    +    // Verify error message when drupal-entity missing but entity embed button included.
    ...
    +    // Verify error message when drupal-entity present alt and title missing but image entity embed button included.
    

    Language nits and 80 cols violation. Fixed.

Excellent work! 👏

  • Wim Leers committed fc6ad10 on 8.x-1.x authored by oknate
    Issue #3022768 by oknate, Wim Leers, alienzed: Validate that `alt` and `...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

🚢

oknate’s picture

Sweet!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.