Add title attribute to oEmbed iframe for accessibility

patch works well, tested with 8.6.x.

Added a test for 8.7 to the patch.

+1 for including the title.

The only question I have about the implementation is:

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
@@ -220,6 +220,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+            'title' => $resource->getTitle(),

The $resource->getTitle() call could return NULL. Is this implementation the best way to accomplish this, or something like:

// If the resource has a title, include it as an iframe attribute.
if ($resource_title = $resource->getTitle()) {
  $element[$delta]['#attributes']['title'] = $resource_title;
}

would be more appropriate? (Or in other words, is an empty title attribute any better/worse for a11y than no attribute, or is it the same?)

Empty title attribute will disable inheriting title for the element from its parent (more about title inheritance). I assume that in this case it shouldn't matter but would be great if we could get confirmation from the accessibility maintainers.

We use the resource title as a default for the title of the media item, but if users want/need to they can override that. Maybe the title of the media item would be better?

#4 @Martijn de Wit, did you forget to upload the new patch with the test?

@SeanB No I added a test manually to #2

I ummed and ahhed on requesting an automated test - imo the fact attributes are added to render elements doesn't need testing - however if we add logic to get different titles depending on what is available then that does need testing. Anyhow it looks like the solution needs a little bit more discussion as to what title to use.

+1 for this patch. But I agree with @lauriii that we need the opinion of the accessibility experts.

Discussed with @andrewmacpherson on Slack, so removing the accessibility review tag.

Here's what we came up with:

andrewmacpherson [10:34 AM]
what does `getTitle()` return... the media entity label?

or another field?

phenaproxima [10:34 AM]
Not specifically, no. It’s the “resource title”, which is returned by the third-party oEmbed provider. It is also entirely optional; some providers will return nothing for it.
In the case of a YouTube video, for example, it’ll certainly be the title of the video as known to YouTube. For a tweet, it probably will be null.

andrewmacpherson [10:35 AM]
Oh, not Drupal data :-)

phenaproxima [10:35 AM]
So that’s the question, I think — should we use the resource title if present, and fall back to Drupal data?
Or just use the Drupal data, ignoring the resource title?

andrewmacpherson [10:37 AM]
Unsure. Lemne think....

phenaproxima [10:37 AM]
No worries. At least now it’s on your radar.

andrewmacpherson [10:38 AM]
iFrames are a rare example where the title attribute IS actually useful, so I agree we should use one if we can figure out what to put in it.

phenaproxima [10:39 AM]
Yup, +1 on that. It’s just a question of _what_

andrewmacpherson [10:42 AM]
Re comment #6 - Omitting the title attribute completely is better than including an empty `title=""`. 
Re comnent #7 - I'm not sure the title inheritance actually nakes any difference at all to real assistive tech, I'd need to try it out. But still safer to avoid an empty title on the iframe like lauriii says.
If title available from the 3rd party, use that.
But falling back to Drupal data? Not sure.

phenaproxima [10:43 AM]
Thanks! I’ll relay this into the issue.
Should I remove the `needs a11y review` tag, in your opinion?

andrewmacpherson [10:47 AM]
Some authors are gonna the media entity label for an "administrative label", like blocks.  But that coukd be confusing as an iframe title for a website visitor. And if the author doesn't see the title, they may be unaware that Drupal has rendered an administrative entity label.
It might be something we can expose to an author, e.g. in a CKEditor oembed button, for override per use.
Yeah, remove the needs a11y review tag.

So basically: use the resource title, and do not fall back to the media entity label. If the resource title is null, omit the attribute entirely.

As per @alexpott's comment in #10, since the title attribute is going to be added conditionally, we are going to need tests.

Here we go.

DrupalCon Seattle 2019

Rerolled patch for 8.8.x after https://www.drupal.org/project/drupal/issues/2998091 landed.

Thanks, @Luke.Leber! Let's see if this passes tests...

Remove title attribute from 'img' in formatter test.

Image titles do not appear to be in scope for this change.

I believe that we'll still need a test case for ensuring that an empty title is not rendered if the resource title is empty.

Patch#20 (3021452-16.patch) tested and working fine. We can move this to RTBC.

Looks right. Thanks!

+++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
@@ -220,6 +220,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
+            'title' => $resource->getTitle(),

There's comments about how we should avoid title="" - what happens if $resource->getTitle() returns an empty string. Does the resulting html have an empty title attribute? I.e. how come

I believe that we'll still need a test case for ensuring that an empty title is not rendered if the resource title is empty.

is crossed out?

It looks like if $resource->getTitle returns NULL, then no title attribute is rendered, however if an empty string is returned, then an empty title tag is rendered (at least on 8.8.x - the resource method documentation does not specify a behavior though, so this may be an implementation detail).

From what I can surmise, whether or not a particular OEmbed endpoint does not provide a title key (NULL) or provides an empty one (empty string) is also an implementation detail of each particular provider. The OEmbed spec does not seem to address exactly what should happen in the case of an empty title.

If we want to force all empty strings to omit the title tag, that responsibility will have to be managed either in the formatter, the resource, or the resource fetcher. I do not see any valid reason for a user to explicitly define an empty string for a title tag, so forcing it somewhere within Drupal seems to be a valid conclusion to come to (unless anyone disagrees, of course :) )

It seems reasonable to me to put the check on the formatter so that all legacy media items will be fixed as well when it rolls out. Does anyone else have input?

I had striked the part about having a test for an empty title tag out because line 80 in the formatter test does not specify a title. This was incorrect logic however, because that test doesn't actually work, so I was wrong in striking it out.

I am probably abusing the core test runner and for this I apologize, but I cannot get the test runner working locally.

For the purpose of code review, I am concerned about the changes to the test case and fixtures. I think there is probably a more elegant way of testing this change that I do not know of.

hmm, would love to see this implemented

I would like to request two fairly small, and basically stylistic, changes. Other than that, I really think this looks great.

1. +++ b/core/modules/media/src/Plugin/Field/FieldFormatter/OEmbedFormatter.php
   @@ -220,6 +220,7 @@ public function viewElements(FieldItemListInterface $items, $langcode) {
   +            'title' => $resource->getTitle() ?: NULL,
   

   I think that, rather than hope that the rendering system will always ignore null attributes, we should be more explicit about adding the title if, and only if, it's present. So, I would change this to something like:

   $title = $resource->getTitle();
   if ($title) {
     $element['#attributes']['title'] = $title;
   }
   

2. +++ b/core/modules/media/tests/src/Functional/FieldFormatter/OEmbedFormatterTest.php
   @@ -172,7 +187,12 @@ public function testRender($url, $resource_url, array $formatter_settings, array
   +        if ($value !== NULL) {
   +          $assert->elementAttributeContains('css', $selector, $attribute, $value);
   +        }
   +        else {
   +          $assert->elementNotExists('css', $selector . '[' . $attribute . ']');
   +        }
   

   This is more of a style thing, but I think more explicit assertions would be useful for future readers of this test. So something like this:

   $element = $assert->elementExists('css', $selector);
   foreach ($attributes as $attribute => $value) {
     if (isset($value)) {
       $this->assertContains($value, $element->getAttribute($attribute));
     }
     else {
       $this->assertFalse($element->hasAttribute($attribute));
     }
   }
   

i agree with both 1 and 2 that @phenaproxima posted in #32

Here's a new patch.

As a side-effect of using the assertContains method, the data providers had to be changed slightly to provide string values. The width, height, max_width, and max_height inputs were failing because assertContains requires either strings or arrays for the first two arguments.

+++ b/core/modules/media/tests/src/Functional/FieldFormatter/OEmbedFormatterTest.php
@@ -172,9 +187,16 @@ public function testRender($url, $resource_url, array $formatter_settings, array
+        $element = $assert->elementExists('css', $selector);

One minor thing: we can put this line just before foreach ($attributes as $attribute => $value), so we don't need to keep asserting the element's presence for each attribute we're checking.

Once this change is made, this is done in my opinion. Feel free to set this issue's status directly to "Reviewed and tested by the community".

Wow, I can't believe that I missed that. Thanks!

I'll wait for the test runner to complete on the latest patch before updating the status.

Thanks, @Luke.Leber! All set. RTBC once testbot declares it to be passing.

Looks good. The only addition I'd suggest is a comment above the code to explain the bit about empty titles on iframes being no bueno, since that is useful contextual info. Can always be done on commit. (Basically, a paraphrase of #7.)

I was also confused about:

-            ‘width’ => 480,
-            ‘height’ => 360,
+            ‘width’ => ‘480’,
+            ‘height’ => ‘360’,

...this is because we switch from $assert->elementAttributeContains() to $this->assertContains() which needs everything to be strings.

Looks good here, and a nice a11y fix! Thanks! Waiting on testbot for the commit.

Saving issue credit.

Ok great, committed and pushed to 8.8.x!

For the comment, after the least contentious bikeshedding exercise ever ;) we came up with:

        // An empty title attribute will disable title inheritance, so only
        // add it if the resource has a title.

Thanks a lot, everyone!!

Thanks, @webchick!

Changing the status to "Patch (to be ported)" because I think that, although this is not a bugfix, it's a non-disruptive accessibility improvement that would benefit 8.7.x.

Backporting this seems fine because we're only adding title attribute which is fine since it doesn't affect how the page is rendered visually. Title is coming from content so we don't have to worry about untranslated string translations. This is also added in the render array so that if someone uses alter functions to add their own title, it would be unlikely to be affected by this change. The only risk that I can think of is that someone has set title attribute for the parent element, and this change would lead to overriding that. That seems unlikely, so I think it should be fine to backport this to 8.7.x.

just to clarify, will this be released in 8.7.8? Does the title get pulled automatically, or do we have to manually configure it somewhere? THANKS!

@xeM8VfDh

This has been backported to the 8.7.x branch, so it will appear in the next patch release for the 8.7 branch.

There is no configuration involved. Drupal will add a title attribute to the iframe automatically, if the oEmbed resource provides a title.

Awesome, perfect, thanks for the clarification @andrewmacpherson and everyone else who contributed to this.

I don't think this is fully fixed. I opened a separate issue: https://www.drupal.org/project/drupal/issues/3085545

Anyone available to review the new issues?

I don't believe this issue is fixed.

I am running Drupal 8.8.0. I originally had CKEditor Media Embed module enabled. I tested that out by creating a new page and pasting a YouTube url into the page body. The Embed module did it's thing and embedded the video automatically. I saved the page then inspected the source on the youtube video's iframe... no title.

So, I did as @Luke.Leber suggested in #9; I disabled CKEditor Media Embed module. Now my Editor only shows two buttons, the core Drupal Image button, and the "Insert Images using IMCE File Manager" image button.

CORE DRUPAL IMAGE BUTTON
This shows a popup (see attached core_button_popup.png) with a URL field that has a supplemental option to "open file browser". Instead of opening the file browser, I paste my YouTube url into the URL field. I add some alt text, then I save the settings. When I'm returned to the page body in the edit screen, I see a broken image icon (see attached core_button_url_resulting_error.png). I saved the page just for good measure, and nothing displays where the video should be embedded. This doesn't work.

IMCE IMAGE BUTTON
This just opens the IMCE File Browser, the same thing that would happen had I clicked "Open file browser" in the Drupal core image button's popup, explained above. There is no option here to enter a URL.

@Luke.Leber, (or anyone else who has this working) please walk me through how you are embedding your video url, step by step, in precise details please.

@pookmish confirmed here that the title attribute isn't working properly.

I think I am maybe talking about a different process than he is, which may be the source of my own confusion. He mentioned "video media type", by which I presume he meant this. I am simply wondering about embedding videos into a standard page, or on the test field of some other Content Type. We've historically used the CKEditor Media Embed Plugin to handle this process, and it has worked well for that purpose. But, it doesn't pull in the title attribute.

My understanding was that this issue fix somehow allowed us to embed videos into the body field of a page, or some text field of a given Content Type somehow via core functionality, in a way that would include the title attribute for accessibility purposes. Am I misunderstanding this--does the issue in question only address Media Types, and not page/text embeds?

See my comment below...

@pookmish created a great video demonstrating the exact problem here