Make WordPress Core

Opened 8 years ago

Closed 6 years ago

#40245 closed enhancement (fixed)

Youtube embeds lack title attribute

Reported by: morriscountynj's profile morriscountynj Owned by: swissspidy's profile swissspidy
Milestone: 5.2 Priority: normal
Severity: normal Version: 2.9
Component: Embeds Keywords: has-patch has-unit-tests commit
Focuses: accessibility Cc:

Description

I use a service called Siteimprove to monitor for accessibility issues. They pointed out an issue -- when you embed a Youtube video via a link, the code comes out something like this

<iframe width="960" height="540" src="https://www.youtube.com/embed/iGddWyoWkyg?feature=oembed"
frameborder="0" allowfullscreen=""></iframe>

However, Siteimprove gives that an A-level error:

The iFrame is missing a description
The iFrame has no 'title' attribute or the 'title' attribute is empty.
Provide the frame with the attribute title=”” and add a description of the content in the title.

Is there any way to add a field to enter a title for the iframe?

See this page for an example: Example

Attachments (9)

for40245.diff (2.2 KB) - added by bamadesigner 7 years ago.
for40245updated.diff (2.2 KB) - added by bamadesigner 7 years ago.
Updated patch for 40245
40245_update.diff (3.1 KB) - added by TomHarrigan 6 years ago.
patch refresh
40245_update.2.diff (3.1 KB) - added by TomHarrigan 6 years ago.
patch refresh - comment fix
40245_update.3.diff (2.1 KB) - added by TomHarrigan 6 years ago.
patch refresh - proper diff
40245_update.4.diff (2.1 KB) - added by TomHarrigan 6 years ago.
patch refresh - proper diff for real this time…
40245.diff (5.1 KB) - added by swissspidy 6 years ago.
40245.2.diff (5.3 KB) - added by afercia 6 years ago.
40245.3.diff (5.9 KB) - added by swissspidy 6 years ago.

Download all attachments as: .zip

Change History (69)

#1 @swissspidy
8 years ago

  • Summary changed from Youtube embed -- need space to put title="" in iframe to Youtube embeds lack title attribute
  • Type changed from feature request to enhancement
  • Version changed from 4.7.3 to 2.9

Hey there,

Welcome to WordPress Trac and thanks for your report!

WordPress embeds have a title attribute for exact these reasons. Unfortunately, YouTube doesn't add a title attribute themselves. oEmbed fetches the <iframe> code directly from YouTube. See https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json for an example.

It seems like Drupal fixed this on their own by adding a custom title attribute, see https://www.drupal.org/node/2085749. Apparently they also added a name attribute: https://www.drupal.org/node/2129317. We could certainly do the same, although I'd love to see YouTube fixing this on their end.

#2 @afercia
8 years ago

  • Focuses accessibility added

Adding the accessibility focus, since iframe elements should really have a title attribute (and it's the only valid case of title attribute usage WordPress currently recommends, see all the work done so far about title attributes).

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#5 @afercia
7 years ago

  • Milestone changed from Awaiting Review to 4.9

We should try a workaround for 4.9.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#8 follow-up: @tfrommen
7 years ago

Even though noone responded to the Google ticket, this seems to have been fixed already, see the example embed link @joedolson posted: https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json

However, when I paste the URL (i.e., https://www.youtube.com/watch?v=M3r2XDceM6A) into the editor, the generated output still lacks the title attribute:
<iframe src="https://www.youtube.com/embed/M3r2XDceM6A?feature=oembed" allowfullscreen="" height="473" frameborder="0" width="840"></iframe>

Am I doing anything wrong, or did I misunderstand what the example link was about? Can you help, @swissspidy?

#9 in reply to: ↑ 8 @SergeyBiryukov
7 years ago

Replying to tfrommen:

this seems to have been fixed already, see the example embed link @joedolson posted: https://www.youtube.com/oembed?url=http%3A//youtube.com/watch%3Fv%3DM3r2XDceM6A&format=json

There's still no title on the <iframe> element in that response, only as a separate value in the array.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#11 @afercia
7 years ago

Seems a fix from upstream isn't going to happen. @swissspidy any chance to make a patch for this?

#12 @swissspidy
7 years ago

It would be great if we could first check other oEmbed providers to see whether this is a broader problem. If so, we could apply a more general solution, not just one tied to YouTube.

#13 @afercia
7 years ago

It would be great if we could first check other oEmbed providers to see whether this is a broader problem

I've made my part and checked Vimeo :) it adds a title attribute on the iframe with the video title. Just the title of the video. It doesn't say the iframe content is about a video though. So, for example, when embedding a video that ironically has a title like The Bigger Picture, the iframe title doesn't let me actually understand what the iframe content is about.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#15 @afercia
7 years ago

  • Milestone changed from 4.9 to Future Release

Beta 1 is in 2 days. Punting as per today's a11y bug scrub.

#16 @bamadesigner
7 years ago

I wrote some code to solve this issue for my websites. I don't mind submitting a patch to get things started, if it's still needed.

Here's how I solved with a filter. Basically it looks for the title in the embed data array and as a possible existing title attribute. If anything, even if the title does exist in the iframe markup, it allows for it to be filtered. If it doesn't exist, it allows for it to be created from the data array or via filter.

https://github.com/wpcampus/wpcampus-network-plugin/commit/a48e4c81b1233a7051139046a91b3a19777ea902

This ticket was mentioned in Slack in #accessibility by bamadesigner. View the logs.


7 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

#19 follow-up: @afercia
7 years ago

@bamadesigner please do feel free to go ahead :)

#20 in reply to: ↑ 19 @bamadesigner
7 years ago

On it!

Replying to afercia:

@bamadesigner please do feel free to go ahead :)

#21 @bamadesigner
7 years ago

Here you go @afercia! I apologize for the delay. #life

for40245.diff

#22 @afercia
7 years ago

  • Keywords has-patch added
  • Milestone changed from Future Release to 5.0

@bamadesigner thanks for the patch! Seems a very good start to me. Couple minor things: the keyword public should be removed and the title attribute value should be escaped with esc_attr().

Moving to 5.0 for consideration. Review and feedback from the Embeds component team would be greatly appreciated.

@bamadesigner
7 years ago

Updated patch for 40245

#23 @bamadesigner
7 years ago

Ah! Great catch. I've uploaded the new patch. Thanks!

for40245updated.diff

#24 @swissspidy
7 years ago

Why is an existing title attribute being replaced in that patch? Shouldn't this only add missing attributes instead of overriding the provided ones?

#25 @bamadesigner
7 years ago

It's being replaced because this patch also introduces a "oembed_title" filter.

#26 @swissspidy
7 years ago

If we‘re going to add that, I think that filter should be renamed to oembed_iframe_title_attribute or something. Also, it needs to be documented.

#27 @ocean90
7 years ago

#43647 was marked as a duplicate.

#28 @swissspidy
6 years ago

  • Keywords needs-refresh added

#29 @pento
6 years ago

  • Milestone changed from 5.0 to 5.1

@TomHarrigan
6 years ago

patch refresh

@TomHarrigan
6 years ago

patch refresh - comment fix

@TomHarrigan
6 years ago

patch refresh - proper diff

@TomHarrigan
6 years ago

patch refresh - proper diff for real this time...

#30 @afercia
6 years ago

@swissspidy mind owning this ticket? As I know you have some expertise with embeds :) Would be great to make it land in 5.1.

#31 @pento
6 years ago

  • Keywords needs-unit-tests added; needs-refresh removed
  • Milestone changed from 5.1 to 5.2

This could do with a bunch of unit tests.

@swissspidy
6 years ago

#32 @swissspidy
6 years ago

  • Keywords has-unit-tests added; needs-unit-tests removed

Added a new patch with unit tests.

I think $pattern needs to be improved so that only iframes will be touched.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#34 @swissspidy
6 years ago

  • Owner set to swissspidy
  • Status changed from new to assigned

This ticket was mentioned in Slack in #core-editor by brianpeat. View the logs.


6 years ago

#36 @afercia
6 years ago

@swissspidy when you have a chance: what's left here? Ten days left to 5.2 beta 1. I'd really appreciate some feedback to not punt this ticket again to next release. Thank you :)

#37 @swissspidy
6 years ago

I think wp_filter_oembed_title_attribute needs some adjustments to make sure it only targets iframes. Otherwise anyone filtering oembed_title_attribute would add title attributes to any element.

Maybe using a regex like this: <iframe[^>]*?title=["']([^"']*)["'][^>]*?>

I'm not a regex expert though :-)

#38 @afercia
6 years ago

@swissspidy thanks. Who's the best person who can help you moving this ticket on? :)

This ticket was mentioned in Slack in #core by afercia. View the logs.


6 years ago

#40 @jrf
6 years ago

Did I hear regex expert ? Want me to take a look ?

What are the specs and have we got test data for things which should and shouldn't match ?

#41 @afercia
6 years ago

@jrf thanks! I think @swissspidy has more context but I'll try to describe what I see in the patch the best I can.
We need to add a title attribute with the video title to the HTML returned from an oembed provider response. The patch already checks if the oembed is a video.
After that, it uses the following regex to search for an existing title attribute in the returned HTML:
$pattern = '/title\=[\"|\\\']{1}([^\"\\\']*)[\"|\\\']{1}/i';

that could match any title attribute in any element in the returned HTML. Instead, we need to search for a match only within the <iframe ...> HTML tag, if any.

#42 @jrf
6 years ago

@afercia Ok, I'll have a look. I presume you'd be happy to see other improvements to the regex ? It currently looks very unstable with regards to the quote matching to me.

#43 @afercia
6 years ago

@jrf please do feel free to make this regex yours :) I totally trust your judgment.

#44 @jrf
6 years ago

Improved regex line:

<?php
$pattern        = '`<iframe[^>]*?title=(\\\\'|\\\\"|['"])([^>]*?)\1`i';

Important: $matches[1] on line 802 and 803 will need to be changed to $matches[2] when using this regex.

Test data used:

<img src="..." title="title"/>
'<iframe src="..." title="title" width="324" height="auto"/>'
"<iframe src='...' width='324' height='auto' title='title' ></iframe>"
"<iframe src=\"...\" title=\"title\" width=\"324\" height=\"auto\"/>"
'<iframe src=\'...\' width=\'324\' height=\'auto\' title=\'title\' ></iframe>'
"<iframe src=\"...\" title=\"title's with apostrophe\" width=\"324\" height=\"auto\"/>"
"<iframe src='...' width='324' height='auto' title='title \"quoting\" something' ></iframe>"

Aside from the first one which shouldn't match, all the others now match the title correctly.
Just for fun, try using the old regex against this data set.

Let me know if there are any peculiarities in how the oembed data is received which should be taken into account additionally.

Version 0, edited 6 years ago by jrf (next)

#46 @afercia
6 years ago

Thanks @jrf! Then, I guess there's the need for a second (simpler) regex to use for the replacement at line 823. Would something like the following be OK or can we use something simpler?

<?php
'`title=(\\\\\'|\\\\"|[\'"]).*?\1`i'

#47 @afercia
6 years ago

@swissspidy I wonder if we should prepend something to the title attribute string. With the current patch the title attribute would be, for example:

<iframe title="House of Commons debate on extension of Article 50" ...

Users will be informed about the topic but still they don't know it's a video :) How about prepending Video:

<iframe title="Video: House of Commons debate on extension of Article 50" ...

#48 @jrf
6 years ago

@afercia Good question. To be fair, I would just simplify that code block like so:

Original code:

<?php
if ( $has_title_attr ) {
        $result = preg_replace( $pattern, 'title="' . esc_attr( $title ) . '"', $result );
} else {
        return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result );
}

return $result;

Suggested alternative:

<?php
if ( $has_title_attr ) {
        // Remove the old title.
        $result = preg_replace( $pattern, '', $result );
}

return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result );

#49 @afercia
6 years ago

@jrf to my understanding:

<?php
        // Remove the old title.
        $result = preg_replace( $pattern, '', $result );

would remove also the <iframe start tag :) the full match of $pattern includes anything starting with <iframe (which we need for the first check) and a title attribute.

Last edited 6 years ago by afercia (previous) (diff)

#50 @jrf
6 years ago

@afercia Shoot, yes, you're right. I shouldn't be doing twenty things at the same time.

Ok, what about this: (though you run the risk of replacing the title in other HTML elements too if it would be exactly the same)

<?php
if ( $has_title_attr ) {
        // Remove the old title.
        $result = str_replace( ' title=' . $matches[1] . $matches[2] . $matches[1], '', $result );
}

return str_ireplace( '<iframe ', sprintf( '<iframe title="%s" ', esc_attr( $title ) ), $result );

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


6 years ago

#52 @joedolson
6 years ago

Re: preprending "Video" - what about prepending "YouTube"? That would give users more definite context, and possibly tell users of YouTube how the controls will work, where 'Video' would still require them to discover that.

#53 @afercia
6 years ago

I shouldn't be doing twenty things at the same time.

😄 Thanks for your help @jrf ! Yep, I'd tend to think improving the conditional instead of using one more regex would be best.

#54 @swissspidy
6 years ago

Re: preprending "Video" - what about prepending "YouTube"?

Well, the patch currently applies to all iframe embeds, regardless of their source. It could be a YouTube video, Vimeo video, tweet, poll, etc. We could use the provider_name field in the oEmbed data if it's available and prepend that to the title. Would that work?

#55 @afercia
6 years ago

the patch currently applies to all iframe embeds, regardless of their source

The patch only tries to set a title if the embed type is of type rich or video: it returns early if ! in_array( $data->type, array( 'rich', 'video' )

Looks like Twitter is rich but doesn't use an iframe (any longer?) and uses the shadow DOM instead.

Regardless, this doesn't help us. We could certainly prepend Video: when the type is video, then the regex searches for an iframe but it's not possible to predict what a rich embed with iframe actually is: it could be a Tweet, a WordPress post, whatever.

Anyways, having an iframe title is better than not having it :)

#56 @afercia
6 years ago

40245.2.diff refreshes the patch:

  • uses the regex and final replacement suggested by @jrf
  • renames all the things to clarify it's about the iframe title attribute
  • running grunt precommit updated also the wp-api-generated.js file, adding the title attribute to "Una cancion para programadores". Didn't know about that song :)

Some testing and review would be appreciated. Reminder: the oembed responses get saved as post meta. When testing, either use a new video to embed each time or delete the _oembed ... entries from the database.

#57 @jrf
6 years ago

@afercia Looks like your new patch is missing the unit tests which were included in @swissspidy's patch....

@afercia
6 years ago

#58 @afercia
6 years ago

Uh thanks @jrf. I even spent some time to rename stuff in the tests... and forgot to svn add. #blamesvn.
Patch updated.

@swissspidy
6 years ago

#59 @swissspidy
6 years ago

  • Keywords commit added

Added a new test_filter_oembed_iframe_title_attribute_does_not_modify_other_tags test in 40245.3.diff.

#60 @afercia
6 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 44942:

Accessibility: Ensure embed iframes have a title attribute.

Screen reader users rely on the iframe title attribute to describe the contents of iframes. A meaningful title attribute allows to quickly identify the iframe content, so users can determine which iframe to enter and explore in detail or skip if desired.
Note: this is the only case where a title attribute is required for compliance with the W3C Web Content Accessibility Guidelines (WCAG).

  • checks for oEmbed response of type video or rich
  • checks if they use an iframe
  • fetches the title (if any) from the oEmbed response
  • adds the title to the embed iframe

Props bamadesigner, TomHarrigan, swissspidy, jrf, afercia.
Fixes #40245.

Note: See TracTickets for help on using tickets.